WordPress.org

Make WordPress Core

Opened 5 weeks ago

Last modified 10 days ago

#53009 assigned task (blessed)

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

Reported by: jrf Owned by: hellofromTonya
Milestone: Awaiting Review 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 (10)

#1 @johnbillion
5 weeks ago

  • Version trunk deleted

#2 @jrf
5 weeks 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.


5 weeks ago

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


2 weeks ago

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


2 weeks ago

  • 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
2 weeks 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
2 weeks ago

Just FYI: as additional output of this review:

#9 @prbot
12 days ago

jrfnl commented on PR #1218:

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

#10 @hellofromTonya
10 days ago

  • Keywords commit added
  • Owner set to hellofromTonya
  • Status changed from new to assigned
Note: See TracTickets for help on using tickets.