WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#30256 closed defect (bug) (fixed)

Create and exclude an area for ms-files.php specific unit tests

Reported by: jeremyfelt Owned by: jeremyfelt
Milestone: 4.1 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch
Focuses: multisite Cc:

Description

In our current test_switch_upload_dir(), we turn ms_files_rewriting on half way through and then fire ms_upload_constants(). At the end of the test, we turn ms_files_rewriting off, but there is no way to reset the constants that are now there.

If in another test we try to do the same thing, unpredictable results appear. Or predictable to be bad.

I think we can treat tests that require ms_files_rewriting similar to our ajax tests and exclude them by default with the option to run them exclusively.

Attachments (2)

30256.diff (5.1 KB) - added by jeremyfelt 5 years ago.
30256.2.diff (7.1 KB) - added by jeremyfelt 5 years ago.

Download all attachments as: .zip

Change History (6)

@jeremyfelt
5 years ago

#1 @jeremyfelt
5 years ago

  • Keywords has-patch added

In 30256.diff:

  • Mimic the current ajax test setup by excluding ms-files and adding a message.
  • Add a ms-files-rewriting.php test file, include the ms_files_rewriting flag in setUp().
  • Move over a portion of an existing test_switch_upload_dir() test.

I've also tested with a patch for #30121 that requires ms_files_rewriting to be completely effective.

@jeremyfelt
5 years ago

#2 @jeremyfelt
5 years ago

After moving the ms_files_rewriting flag out of the main multisite tests, another test started failing because one of the images was incrementing its filename in the uploads directory due to a conflict. My guess is that the ms-files flag was causing a change in the upload directory which helped avoid a conflict.

I went down the rabbit hole for a minute in 30256.2.diff, which set a prefix on uploaded files in the failing test, but I think the problem more likely lies in us not using remove_added_uploads() on every tearDown() that should have it.

This will be tomorrow's rabbit hole.

#3 @jeremyfelt
5 years ago

  • Owner set to jeremyfelt
  • Resolution set to fixed
  • Status changed from new to closed

In 30286:

Move tests for ms_files_rewriting to separate group, ms-files

When the ms_files_rewriting flag is enabled, ms_upload_constants() is required to properly set upload directory constants. Once this fires, it is impossible to clean up for a non ms_files_rewriting test by turning the option back off.

Excluding these tests by default offer a more consistent environment overall. Any tests written for uploaded files in multisite should ideally have a correspondign test in this area.

This commit also moves existing ms_files_rewriting tests for test_switch_upload_dir().

Fixes #30256

#4 @jeremyfelt
5 years ago

I went down the road of @runInSeparateProcess for a while in an attempt to continue including these tests as part of the main suite. Unfortunately, it seems that the separate processes spawn off with a default phpunit and miss the arguments originally passed in phpunit -c tests/phpunit/multisite.xml. Even though the group of tests ends up being processed, they immediately run into an issue where multisite tables are not available.

I dug for a long while and could not figure out how to get the parent process to pass arguments to the spawned process. We should continue to revisit this, as there may be a workaround. When we find an answer, we can merge the ms-files stuff back in.

Note: See TracTickets for help on using tickets.