#53009 closed task (blessed) (fixed)
Tests: review all calls to markTestSkipped() and @requires annotations
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
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
#5
@
4 years ago
Action list we'll be working from today: https://docs.google.com/spreadsheets/d/1UqXgrE0kZlX5GlnAjNBa17oLlYtpx_76sXI71TCNSgk/edit?usp=sharing
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
@
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
@
4 years ago
Just FYI: as additional output of this review:
- A number of other, follow-up Trac tickets have been opened for further test reviews which were out of scope for this ticket - #53118, #53119, #53123
- An opportunity for code simplification was discovered and a ticket opened to discuss that - #53121
- A PR was opened to clarify the PHPUnit documentation, which has since been merged - https://github.com/sebastianbergmann/phpunit-documentation-english/pull/232
4 years ago
#9
FYI: CS issue is unrelated to this PR, but caused by a CS issue introduced in commit 955df634d1b4899008bd5900d99c02f773d76e66
#10
@
4 years ago
- Keywords commit added
- Owner set to hellofromTonya
- Status changed from new to assigned
#11
@
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
4 years ago
#14
Closing as committed via https://core.trac.wordpress.org/changeset/51415
4 years ago
#15
Closing as committed via https://core.trac.wordpress.org/changeset/51415
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.