Make WordPress Core

Opened 3 years ago

Last modified 3 years ago

#52610 new task (blessed)

Consider removing many of the default test group exclusions

Reported by: johnbillion's profile johnbillion Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: needs-patch dev-feedback
Focuses: multisite Cc:

Description (last modified by johnjamesjacoby)

When the tests are run with composer test, the following groups are excluded:

  • ajax
  • ms-files
  • ms-required
  • external-http

When the tests are run with Multisite enabled with composer test -- -c tests/phpunit/multisite.xml, the following groups are excluded:

  • ajax
  • ms-files
  • ms-excluded
  • external-http
  • oembed-headers

The ms-required and ms-excluded group exclusions are needed so that the Multisite-specific tests and single-site-specific tests don't run when they don't need to.

It's less clear why the other groups in these lists are excluded by default.

The ajax and ms-files groups are not slow, so excluding them for performance reasons doesn't make sense. I think the ajax exclusion should be removed from both the single site and Multisite configuration. The ms-files exclusion should be removed too because the tests in the ms-files group don't get registered on a non-Multisite test run so the exclusion is redundant.

The external-http tests are excluded because they are somewhat slow, taking around 10-15 seconds on GitHub Actions and around 40 seconds on my local, highly dependent on network connection speed. Let's keep these excluded by default.

The oembed-headers group is excluded by default because it requires Xdebug, however this is already covered by the @requires function xdebug_get_headers tag on the only test in this group, along with being in the debug group which runs separately on GitHub Actions. The oembed-headers group exclusion can be removed as it's redundant.

Here's my proposed new config for phpunit.xml.dist:

<exclude>
    <group>ms-required</group>
    <group>external-http</group>
</exclude>

and for multisite.xml:

<exclude>
    <group>ms-excluded</group>
    <group>external-http</group>
</exclude>

Change History (6)

#1 @johnjamesjacoby
3 years ago

  • Description modified (diff)

#2 @jeremyfelt
3 years ago

The ms-files exclusion should be removed too because the tests in the ms-files` group don't get registered on a non-Multisite test run so the exclusion is redundant.

Removing the exclusion for ms-files from the single site config makes sense.

Via [30286]:

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.

I think this remains a reason to continue excluding ms-files in the multisite test config.

#3 @SergeyBiryukov
3 years ago

Some history here:

  • [924/tests] / #tests49 #tests42 excluded the ajax group for performance reasons.
  • [30298] / #30304 excluded the external-http group for performance reasons.
  • [30286] / #30256 excluded the ms-files group from multisite config for a more consistent environment.
  • [37269] / #36566 excluded the ms-files group from single site config to avoid pollution when mixed with the rest of the tests.
  • [40520] / #40531 introduced the ms-required and ms-excluded groups to reduce the number of skipped tests polluting PHPUnit's output.
Last edited 3 years ago by SergeyBiryukov (previous) (diff)

#4 @johnbillion
3 years ago

Ah yes, the ms-files tests needs to remain separate because the constants in ms_upload_constants() are set based on the value of the ms_files_rewriting option, which is itself set in the @group ms-files tests. Removing ms-files from the excluded groups causes many failures.

#5 @johnbillion
3 years ago

The ajax group exclusion hasn't been needed since [41293]. It was introduced in [924/tests] because of the reliance on DOING_AJAX which is no longer the case.

#6 @jrf
3 years ago

Ah yes, the ms-files tests needs to remain separate because the constants in ms_upload_constants() are set based on the value of the ms_files_rewriting option, which is itself set in the @group ms-files tests. Removing ms-files from the excluded groups causes many failures.

Just a heads-up: you can declare multiple testsuite entries in <testsuites>. If the ms-files group gets set as a separate testsuite, it will probably not need a separate run and prevent the errors you are seeing. At least worth giving it a try. I remember fixing a similar situation not too long ago this way.

Last edited 3 years ago by jrf (previous) (diff)
Note: See TracTickets for help on using tickets.