[PHP Tests] Increase parity with WordPress Core

hellofromtonya commented 1 year ago

This is an epic ticket to track ongoing roadmap to increase the PHP test suite parity with WordPress Core.

Updates:

17 Aug 2022: Initially created.

Scope

WordPress Core' PHP tests:

Run against all supported PHP versions (currently 5.6 to 8.1)
Use PHPUnit Polyfills to match the PHPUnit and PHP versions
Use the WordPress Core test coding standards including:
- the standards for file structures, file naming, and test class naming
- preferred assertions such as assertSame() instead of assertEquals()
- renaming of the shared fixtures
- use of data providers
- annotations including @covers for code coverage
With PHP notices and deprecations turned on for awareness and testing

The scope of this roadmap is to upgrade the Gutenberg PHP unit tests for parity to WordPress Core.

Why? Benefits

Reduce backporting effort by getting tests Core-ready long before backporting starts.
To promote discovery of potential issues and incompatibilities earlier and continuously through the development and testing cycles.
To increase commonality for Core contributors to more easily contribute to Gutenberg.

Ongoing Work

[x] https://github.com/WordPress/gutenberg/pull/43102
[x] https://github.com/WordPress/gutenberg/pull/43334
[x] https://github.com/WordPress/gutenberg/pull/46510
[ ] https://github.com/WordPress/gutenberg/pull/48603
[ ] Reorganize and rename test files and classes to Core's test coding standards
[ ] Audit shared fixtures and resolve any mismatches
[ ] Audit tests and convert into data providers where appropriate
[ ] Add failure messages when a test method has more than 1 assertion or expectation
[x] https://github.com/WordPress/gutenberg/pull/52696
[x] https://github.com/WordPress/gutenberg/pull/53777
[x] https://github.com/WordPress/gutenberg/pull/53438
[ ] Refactor unit tests in the gutenberg/gutenberg-coding-standards package

ockham commented 1 year ago

Run test CI jobs across multiple PHP versions

I wonder if we can leverage reusable workflows to basically, well, re-use wordpress-develop's phpunit-tests.yml 🤔

cc/ @desrosj 👋

desrosj commented 1 year ago

I wonder if we can leverage reusable workflows to basically, well, re-use wordpress-develop's phpunit-tests.yml 🤔

I have been thinking about this for a while, but I'm not sure the best way to approach it. Callable workflows execute as a job, and as far as I know, you can't attach additional steps in the calling workflow. However, the callable workflow could potentially be dynamic and look for the presence of a JSON file, or something similar to provide test conditions.

For example, instead of manually defining the matrix of values in the phpunit-tests.yml file, it would look for a JSON file with the desired matrix instead. This should allow Gutenberg to have a workflow file that calls the one in Core@trunk while still defining its own desired test conditions.

Where this potentially breaks down is when multiple versions of WordPress are tested. As the workflow file within trunk is updated over time, it's likely it will break for older branches at some point.

ockham commented 1 year ago

I have been thinking about this for a while, but I'm not sure the best way to approach it. Callable workflows execute as a job, and as far as I know, you can't attach additional steps in the calling workflow.

If I'm reading the real-world example correctly, then it should be possible to run the re-usable workflow at job level 🤔

But even if that wasn't the case: I know GB currently has one workflow for unit tests that includes both JS and PHP ones, but I don't think that's a hard requirement -- we could create a separate workflow file for PHP unit tests if that's required to re-use wordpress-develop's PHPUnit test workflow...

However, the callable workflow could potentially be dynamic and look for the presence of a JSON file, or something similar to provide test conditions.

Yeah, TBH, I haven't worked with re-usable workflows myself yet, so I'm still not totally sure about how they relate to their respective repositories -- i.e. if a re-usable workflow could be configured through any files in the "consuming" repository... Anyway, it seems that re-usable workflows should allow for some degree of parametrization, so maybe we can leverage that.

For example, instead of manually defining the matrix of values in the phpunit-tests.yml file, it would look for a JSON file with the desired matrix instead. This should allow Gutenberg to have a workflow file that calls the one in Core@trunk while still defining its own desired test conditions.

TBH I wouldn't mind avoiding this degree of configurability -- at least for starters (since it introduces some extra complexity) 😅

Maybe I'll try filing a PR against wordpress-develop to add the workflow_call trigger.

If this isn't going to work, we might consider composite actions; or maybe just exporting a PHPUnit test action from wordpress-develop...

jrfnl commented 1 year ago

I fully support the efforts being made here.

The Gutenberg ports to WP Core have in recent merges introduced quite some new PHP 8.1 and 8.2 incompatibilities, which could have been avoided if the tests were run on all appropriate PHP versions.

These kind of new incompatibilities being introduced via new code is highly discouraging for the people trying to make WordPress Core compatible with more recent PHP versions.

Also see #44536, #44537 and #44538

While I understand that it would be a "nice-to-have" to use some form of re-usable workflow, getting the tests running on multiple PHP versions shouldn't need to wait on that and is actionable now.

If anyone is working on this and needs guidance, give me a shout. If nobody is working on this, let me know and I'll get this sorted ASAP.

talldan commented 1 year ago

Run test CI jobs across multiple PHP versions

I had a look at this in #44566. Have left some comments about the issues. I welcome commits to the PR if anyone has ideas for improving things.

One thing I'm unsure about is how many jobs we can add or if there's any kind of limit to our github actions. (I'm guessing there is)

desrosj commented 1 year ago

One thing I'm unsure about is how many jobs we can add or if there's any kind of limit to our github actions. (I'm guessing there is)

Wanted to provide clarity here. GitHub Actions are not limited by the volume of jobs, but rather how they run, timing, and concurrency.

You can schedule as many workflows as you'd like, with these exceptions:

A job matrix can generate a maximum of 256 jobs per workflow run (source)
No more than 500 workflow runs can be queued in a 10 second interval per repository (source)
Some miscellaneous, one off exceptions. For example, if more than 3 tags are pushed at the same time, the push events required to trigger the appropriate workflows will not be fired (source)

The WordPress organization recently upgraded from Team to Enterprise plans. Previously, we were limited to 60 total concurrent jobs at any point in time (maximum of 5 MacOS at once), but now the limit is 180 total concurrent jobs (maximum of 50 MacOS at once). Since this change, I have not noticed a time where there was a significant queue built up.

Jobs are run on a first come first serve basis in the order they are requested. So if workflow A and workflow B both have job 1 and job 2 (depends on 1), the workflow who's job 1 completes first will see their job 2 have priority. This can be a bit weird where when many workflows are created in a small period of time, it seems like its taking a long time, but really it's just waiting in line for their turn.

At this time, I'm not concerned about adding more jobs or workflows. And if we do notice this, I would advise we look more closely at what we are running, when, and tuning the performance within the workflows. For example, the performance workflow runs on every commit, even if only GitHub Action workflow files are modified. We should probably add some include/exclude path rules for this scenario. That workflow is one of our longest running ones, so seems wasteful to run when the performance will not actually be impacted (when only .yml, or .md files are edited, for example).

I've created a POC for using a single location to define the PHP versions to test for both Core and Gutenberg in #44579. It by no means accounts for everything (Core has includes that may not apply to Gutenberg), but using a data file instead of a shared workflow may be a quick way to bring testing parity. Here are the relevant workflows for the PR as I experimented: https://github.com/WordPress/gutenberg/actions/workflows/unit-test.yml?query=branch%3Aexperiment%2Fshared-php-testing-matrix

desrosj commented 1 year ago

To expand on the idea of the JSON file, external projects could also include use this to construct their testing matrix to match the one Core uses instead of a reusable workflow.

jrfnl commented 1 year ago

Thanks @talldan and @desrosj for your efforts.

I have always wondered about why the PHP tests here are being run via NPM and Docker instead of having a plain PHP based workflow.

A plain PHP workflow gives much more flexibility and also allows for testing "not ideal" environments, which the current setup really doesn't allow for.

An example workflow for a plain PHP based setup for integration tests for a WP plugin can be seen here: https://github.com/WordPress/wordpress-importer/blob/master/.github/workflows/test.yml

Also note: the php:lint command should IMO be run in a separate job as it is unrelated to the unit tests and only needs to be run on one PHP version. While I understand the desire to "NPM everything", again running this in plain PHP will have advantages in how the errors can be displayed in both the logs as well as in PRs.

desrosj commented 1 year ago

Testing outside of Docker would certainly simplify what is needed to share testing configurations across Core and Gutenberg (and the wider ecosystem) since the Core Docker implementation is different than the wp-env one.

With GitHub Actions, having a consistent environment is not as problematic as it was on Travis-CI, which I believe, was one argument for using the Docker environment to begin with.

Definitely open to exploring it, but won't have the time until after 6.1.

I also agree the about the linting job. I'd argue it belongs in a different workflow entirely with any other linting related checks spread out across workflows/jobs.

jrfnl commented 1 year ago

What about if I set up and pull a PHP-based test workflow + PHPCS workflow for the Gutenberg repo now to at least prevent more issues being introduced into WP Core ? And then after WP 6.1 has been released, we can revisit this for WP Core and explore re-usability of (parts) of the workflows.

What do you think @desrosj @talldan ?

jrfnl commented 1 year ago

@talldan @desrosj ☝🏻

anton-vlasenko commented 1 year ago

I've just submitted a PR that runs PHPUnit CI jobs on all supported PHP versions. I'd appreciate your review, @desrosj @jrfnl.

WordPress / gutenberg