WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#40531 closed task (blessed) (fixed)

Avoid test skipping for multisite only or single site only tests

Reported by: johnbillion Owned by: johnbillion
Milestone: 4.8 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: needs-patch
Focuses: multisite Cc:

Description

When running the core test suite with PHPUnit you'll never see a nice green OK result, because there are always skipped tests for multisite specific and single site specific tests.

Example:

if ( ! is_multisite() ) {
	$this->markTestSkipped( 'This test requires multisite.' );
}

The consequence is that it's difficult to spot unexpectedly skipped tests or incomplete tests, because there are always skipped tests polluting the results.

Instead of using the is_multisite() -> markTestSkipped pattern, all these tests should be put into ms-required and ms-only groups, and then the phpunit.xml/multisite.xml configuration files should be updated to control whether the corresponding groups are skipped.

The is_multisite() -> markTestSkipped code should remain (at least for now, maybe permanently) so the tests in those groups don't cause failures if they are run when they shouldn't be.

The aim is to attempt to remove all skipped tests when running the test suite, at least on Travis and ideally locally too. TDB whether or not this is achievable.

Change History (10)

#1 @johnjamesjacoby
4 years ago

I am +1 on this. We have a bunch of skipped tests in the bbPress suite, and yellow has become our new green, which is obviously not ideal.

#2 @johnbillion
4 years ago

  • Owner set to johnbillion
  • Status changed from new to accepted

#3 @johnbillion
4 years ago

In 40520:

Build/Test Tools: Introduce ms-required and ms-excluded groups for tests.

Tests in the ms-excluded group are now excluded when running tests with multisite enabled, and tests in the ms-required group are excluded when running tests without multisite enabled. The end result is a significantly reduced number of skipped tests polluting PHPUnit's output, which means verbose mode can be used to more easily see which tests are skipped or incomplete, and why.

See #40531

#4 @johnbillion
4 years ago

In 40522:

Build/Test Tools: Add some more tests to the ms-required and ms-excluded groups.

See #40531

#5 @johnbillion
4 years ago

In 40527:

Build/Test Tools: Enable verbose mode in PHPUnit so we can see which tests are being skipped, and now that the number of skipped tests has been lowered.

See #40533, #40531

#6 @johnbillion
4 years ago

In 40530:

Build/Test Tools: Correct an incorrect ms- group name.

See #40531

#7 @johnbillion
4 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 40543:

Build/Test Tools: Introduce skipWithoutMultisite() and skipWithMultisite() methods into the test suite.

This brings much needed uniformity to test skipping when a test requires Multisite or when a test should be excluded from running when Multisite is enabled.

Used in conjunction with the @group ms-required and @group ms-excluded notation, this removes a significant number of skipped tests from the default test suite run.

Fixes #40531

#8 follow-up: @jdgrimes
4 years ago

Instead of the new skip methods, you could do something like I did for my plugin tests by creating a custom `@requires` annotation. You could probably make it so that the @group annotations automatically triggered these methods, so that there'd be no need to call them manually. Just a thought.

#9 in reply to: ↑ 8 @johnbillion
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to jdgrimes:

You could probably make it so that the @group annotations automatically triggered these methods, so that there'd be no need to call them manually.

I did consider this but for some reason I thought that custom annotations required PHPUnit 4.x+, which only works on PHP 5.3+. It would be preferable to only have to specify once that the test requires or excludes multisite.

Last edited 4 years ago by johnbillion (previous) (diff)

#10 @johnbillion
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 40564:

Build/Test Tools: Automatically skip tests in the ms-required and ms-excluded groups.

This removes the need to manually call $this->skipWithMultisite() and $this->skipWithoutMultisite() from within the test when the test only runs without Multisite or only runs on Multisite, respectively.

Props jdgrimes for the suggestion.

Fixes #40531

Note: See TracTickets for help on using tickets.