GH Actions/Tests: no longer allow test runs on PHP 8.1 + 8.2 to fail #3173
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jrfnl
force-pushed
the
WIP/trac-55652/ghactions-get-passing-build-php-81
branch
4 times, most recently
from
5b3a3e4
to
9a1eac5
Compare
jrfnl
force-pushed
the
WIP/trac-55652/ghactions-get-passing-build-php-81
branch
from
9a1eac5
to
c91471a
Compare
jrfnl
force-pushed
the
WIP/trac-55652/ghactions-get-passing-build-php-81
branch
3 times, most recently
from
8640297
to
bfd40a9
Compare
jrfnl
force-pushed
the
WIP/trac-55652/ghactions-get-passing-build-php-81
branch
6 times, most recently
from
a6ac0c1
to
00207df
Compare
jrfnl
changed the title
jrfnl
changed the title
This test was brought to my attention due to it failing on PHP 8.1. I have investigated the test failure and the failure is a little more problematic than a "plain" PHP compatibility issue. * The test sets a `$post_content` of `'Hello World'`, then does some things and then verifies that the subsequent REST API response does _not_ contain `'Hello World'` via a `assertStringNotContainsString()`. * The problem is that the original post which was created with the `'Hello World'` content, was created as a _password protected post_. * I couldn't reproduce the test failure locally on PHP 8.1 and when I dug deeper, it turned out that it was because the REST API response returned contained `'This content is password protected.'` instead of `'Hello World'`. * As the tests uses the _negative_ assertion `assertStringNotContainsString()`, it would pass, but as far as I can see, **it doesn't actually test what it should be testing**. My interim conclusion is that this needs further investigation on what the _intention_ was behind the test and the test probably needs to be rewritten to make sure the test tests what it is supposed to test. I'm proposing to skip that test for now (on all PHP versions) until it has been fixed, as, as it is, the test has no value.
These three tests would run into a PHP 8.1 "passing null to non-nullable" deprecation for the call to `trim()` when the result of `get_edit_post_link()` is passed to `esc_url()`. Setting a deprecation expectation would not solve this as the returned value would still not match the expected value(s). By setting a a current user, the prerequisites for the `wp_dashboard_recent_drafts()` are met, which means the deprecation notice is avoided and the assertions will succeed. With this change included, the `continue-on-error` for PHP 8.1 and 8.2 can be removed, which will prevent new PHP 8.1 and 8.2 issues from being introduced. Co-authored-by: Sergey Biryukov <sergeybiryukov@git.wordpress.org>
jrfnl
force-pushed
the
WIP/trac-55652/ghactions-get-passing-build-php-81
branch
from
00207df
to
4cfa3c9
Compare
|
I have rebased the PR after [54364] was committed, which means the first two commits are now gone. The last commit skipping three tests for PHP 8.1 has been replaced with a commit applying @SergeyBiryukov's suggestion. |
|
Thanks for the PR! Merged in r54365 and r54369. The REST API test issue was further investigated and resolved in r54368. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With one prepared PHP 8.1 patch still to be committed (first two commits in this PR),we're down to 3 errors and 1 test failure on both PHP 8.1 + 8.2.The test failure is being addressed in Trac ticket 56681.
The last remaining errors need further investigation, but in the mean time, we should actively prevent new PHP issues from being introduced in WP Core.
To that end, I'm proposing to skip those last four tests, either on all PHP versions (the failure, see the explanation in the ticket mentioned above) or on PHP 8.1+ (the three test errors).
With those test skips in place, we now have a 🟢 build for both PHP 8.1 + 8.2 and can remove the
continue-on-error.Trac ticket: https://core.trac.wordpress.org/ticket/55652
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.