Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#53009 closed task (blessed) (fixed)

Tests: review all calls to markTestSkipped() and @requires annotations

Reported by: jrf's profile jrf Owned by: hellofromtonya's profile hellofromTonya
Milestone: 5.9 Priority: normal
Severity: minor Version:
Component: Build/Test Tools Keywords: has-patch has-unit-tests commit
Focuses: docs, performance Cc:

Description

PHPUnit allow for skipping tests when certain conditions are not met.

There are multiple ways in which test skipping can be done:

  • Via an annotation at test class level.
  • Via an annotation at test function level.
  • Via an inline condition in a test or fixture function with a call to markTestSkipped().

The annotations can mostly be used when a particular PHP version / extension / PHPUnit version / Operating System etc is needed and will result in a descriptive "test skip" notice in the verbose test output.
See: https://phpunit.readthedocs.io/en/9.5/incomplete-and-skipped-tests.html#incomplete-and-skipped-tests-requires-tables-api

Inline conditions with a call to markTestSkipped() should be used for any situation for which annotations can not be used.

While reviewing some tests, @hellofromtonya and me noticed situations where the test skipping can be improved.
To that end, we are recommending a full review of all test skipping being done in the complete WP Core test suite.

Such a review should:

  • Check whether an annotation uses the right condition and format.
  • Check whether annotations are at the right level - class vs function.
  • Check whether an inline condition can be replaced by an annotation.
  • Check whether an inline condition is at the right level - class vs fixture vs function
  • Check that all calls to markTestSkipped() contain a verbose explanation of why the test is being skipped.

This list should function as a starting point and should be enhanced with any additional checks which may be needed as discovered once the review is started.

Also see the following section of the PHPUnit manual: https://phpunit.readthedocs.io/en/9.5/incomplete-and-skipped-tests.html#skipping-tests

Change History (15)

#1 @johnbillion
4 years ago

  • Version trunk deleted

#2 @jrf
4 years ago

The intention is to try and address this ticket in a pair programming session on Friday April 30th 2021, starting at 13:00 UTC.

Anyone who wants to join in is very welcome. Please leave a message here or ping @hellofromTonya or me on the WP Core Slack and we'll send you a ping on Slack with the details of where to join for this session.

This ticket was mentioned in Slack in #core by ayesh. View the logs.


4 years ago

This ticket was mentioned in Slack in #core-php by jrf. View the logs.


4 years ago

This ticket was mentioned in PR #1218 on WordPress/wordpress-develop by jrfnl.


4 years ago
#6

  • Keywords has-patch has-unit-tests added

Trac ticket: https://core.trac.wordpress.org/ticket/53009

See the individual commits for full details.

This is the result of a full review of the PHPUnit test suite for test requirements.

#7 @jrf
4 years ago

Between @hellofromTonya and me, we finished the complete action list in a marathon pair programming session yesterday.

On top of that, I've done some runs of the complete test suite with select extensions/functions disabled to discover missing @requires tags and those have been added as well.

The linked PR contains the outcome of this collaboration, a full patch to address this ticket completely.

I'd recommend that this type of review should be scheduled as a recurring task to be executed once a year or so.

#8 @jrf
4 years ago

Just FYI: as additional output of this review:

jrfnl commented on PR #1218:


4 years ago
#9

FYI: CS issue is unrelated to this PR, but caused by a CS issue introduced in commit 955df634d1b4899008bd5900d99c02f773d76e66

#10 @hellofromTonya
4 years ago

  • Keywords commit added
  • Owner set to hellofromTonya
  • Status changed from new to assigned

#11 @hellofromTonya
4 years ago

  • Milestone changed from Awaiting Review to 5.9

Moving into the milestone as PR 1218 is ready for commit consideration.

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


4 years ago

#13 @SergeyBiryukov
4 years ago

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

In 51415:

Tests: Clean up skipping conditions and requirements for various tests.

This improves the consistency of test skipping and ensures that:

  • The @requires annotations use the right condition and format, and are on the right level (class vs. function).
  • Inline conditions with a markTestSkipped() call are only used when annotations cannot be used.
  • All markTestSkipped() calls contain a verbose explanation of why the test is being skipped.

Props jrf, hellofromTonya.
Fixes #53009.

Note: See TracTickets for help on using tickets.