WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 5 months ago

Last modified 5 months ago

#53891 closed task (blessed) (fixed)

GH Actions: enable running the tests on PHP 8.1

Reported by: jrf Owned by: jrf
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch php81 early
Focuses: Cc:

Description (last modified by jrf)

PHP 8.1 is expected to be released end of November 2021.
Enabling the tests to run in CI on PHP 8.1 will allow us to get WordPress ready in time.

To allow for enabling the tests to run on PHP 8.1, quite some prep work needs/needed to be done ahead of the actual CI change.

Preparation work already committed:

  • #47381 - removing of composer.lock file and switching to running the tests via Composer.
  • #46149 - making the test suite cross-version compatible with PHPUnit 5.x to 9.x
  • #53635 - [51517] patch JsonSerializable_Object::jsonSerialize() for compatibility with PHP 8.1

Preparation work in progress:

  • #52825 - fix mysqli error handling for PHP 8.1.
  • #53858 - fix parse error in PHP 8.1 due to readonly

Patches for these last two tickets need to be committed before PHP 8.1 in CI can be turned on as these are both causing fatal errors in the "Install WordPress" step before the actual test run.

I'll be attaching a patch to this ticket for the CI change, but this patch can/should only be committed once the patches for the above two tickets are in.

Once the tests are running, the application of fixes for PHP 8.1 will be handled in #53635 and related tickets.

Attachments (1)

53891-ghactions-change.patch (2.0 KB) - added by jrf 5 months ago.

Download all attachments as: .zip

Change History (20)

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


5 months ago

PHP 8.1 is expected to be released end of November. Enabling the tests to run in CI on PHP 8.1 will allow us to get ready in time.

These builds are, for now, still allowed to fail.

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

#2 @jrf
5 months ago

  • Description modified (diff)

I've opened PR 1553 with the patch as uploaded and (partial) patches from the two tickets which still need to be committed to demonstrate/verify the CI on PHP 8.1 will work with those two changes.

The fact that the tests fail is only to be expected for now.

#3 @prbot
5 months ago

jrfnl commented on PR #1553:

Once the patch is committed, all builds will show as "failed" until all PHP 8.1 issues have been fixed, which has the risk of people not noticing new failures on PHP < 8.1.

As an interim measure, while working through the PHP 8.1 issues, we could add separate conditional steps to run the tests on PHP 8.1 and add continue-on-error to those. That would allow the test builds to show as "successful" if all non-PHP 8.1 test runs pass.

Opinions ? /cc @desrosj

#4 @prbot
5 months ago

SergeyBiryukov commented on PR #1553:

As an interim measure, while working through the PHP 8.1 issues, we could add separate conditional steps to run the tests on PHP 8.1 and add continue-on-error to those. That would allow the test builds to show as "successful" if all non-PHP 8.1 test runs pass.

That makes sense to me 👍

#5 @prbot
5 months ago

SergeyBiryukov commented on PR #1553:

As an interim measure, while working through the PHP 8.1 issues, we could add separate conditional steps to run the tests on PHP 8.1 and add continue-on-error to those. That would allow the test builds to show as "successful" if all non-PHP 8.1 test runs pass.

One other consideration I've just learned from @aristath is that if WordPress core tests fail, Gutenberg contributors cannot merge any PRs, so definitely let's do that :)

#6 @prbot
5 months ago

jrfnl commented on PR #1553:

  • Rebased after the patches for the blockers had been merged.
  • Updated to apply my earlier suggestion.

You can see that the builds against PHP 8.1 now show as 🟢 , even though when you open them up, you will see that all test runs have failures.
The failures are also listed on the CI "Summary" overview of the "PHPUnit Tests" workflow.
See: https://github.com/WordPress/wordpress-develop/actions/runs/1113845520

https://i0.wp.com/user-images.githubusercontent.com/663378/128755722-498bf641-3098-4ada-8bc9-0d4502e9b089.png

#7 @SergeyBiryukov
5 months ago

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

In 51588:

Build/Test Tools: Enable running the tests on PHP 8.1.

PHP 8.1 is expected to be released at the end of November 2021.

Enabling the tests to run in CI on PHP 8.1 allows us to get WordPress ready in time.

As an interim measure, while working through the PHP 8.1 issues, separate conditional steps are added to run the tests on PHP 8.1 with the continue-on-error option. That allows the test builds to show as "successful" if all non-PHP 8.1 test runs pass.

Follow-up to [51517], [51543], [51545], [51574], [51582], [51586].

Props jrf.
Fixes #53891. See #53635.

#8 @prbot
5 months ago

jrfnl commented on PR #1553:

Closing as committed in 51588

#9 follow-ups: @swissspidy
5 months ago

Regarding r51588, couldn't the test workflow be simplified by just using continue-on-error: ${{ matrix.php == '8.1' }} instead of duplicating all steps?

#10 in reply to: ↑ 9 @jrf
5 months ago

Replying to swissspidy:

Regarding r51588, couldn't the test workflow be simplified by just using continue-on-error: ${{ matrix.php == '8.1' }} instead of duplicating all steps?

@swissspidy Please see the above discussion. With that (which was exactly what the initial commit on GH did), builds/PRs would still show as failed, even though it is only the PHP 8.1 builds which are failing.

This ticket was mentioned in PR #1564 on WordPress/wordpress-develop by desrosj.


5 months ago

Trac ticket:

#12 in reply to: ↑ 9 @desrosj
5 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to swissspidy:

Regarding r51588, couldn't the test workflow be simplified by just using continue-on-error: ${{ matrix.php == '8.1' }} instead of duplicating all steps?

I also came here to suggest this.

I did some testing, and I was able to get this to work. It looks like 53891-ghactions-change.patch added continue-on-error at the job level. I did not retry this approach, though it is documented as supported. Instead I included continue-on-error inline for each step that should be allowed to fail for PHP 8.1. This makes what can fail more specific, and could help us debug other parts of the workflow on PHP 8.1.

#13 @prbot
5 months ago

desrosj commented on PR #1564:

Thanks @jrfnl! I removed the PHP 8.1 check, but I did not add the continue-on-error for that step.

Slow tests are currently only split out for PHP 5.6. If PHP 8.1 started running slow tests, that is something that should get flagged.

#14 @prbot
5 months ago

jrfnl commented on PR #1564:

@desrosj Understood, though as split_slow is a matrix condition which _could_ be added anywhere, that means there's an undocumented assumption in the workflow that it will not be added for the PHP 8.1 workflows. So, in that sense, as that step does not have a PHP version based condition, it would still be _safer_ to add the continue-on-error to it, just in case.

Either way, with a bit of luck, these conditions will only be in the workflow for a month or two at the most, so no biggie.

#15 @prbot
5 months ago

desrosj commented on PR #1564:

I don't have a strong preference. @SergeyBiryukov care to be the tie breaker on this if you're able to commit?

#16 @prbot
5 months ago

SergeyBiryukov commented on PR #1564:

I don't have a strong preference either, but as the split_slow condition is specific to PHP 5.6 per [50444], it seems like a safe assumption that it will not be added for the PHP 8.1 workflows. So the PR as is seems good to me :)

#17 @SergeyBiryukov
5 months ago

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

In 51604:

Build/Test Tools: Simplify the PHPUnit test workflow.

This removes the previously duplicated set of test runs specifically for PHP 8.1 in favor of combining if conditions for individual test runs with the continue-on-error option.

Follow-up to [51588].

Props desrosj, swissspidy, jrf.
Fixes #53891.

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


5 months ago

Note: See TracTickets for help on using tickets.