Try running html5lib tests efficiently.

[desrosj]

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

---

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.

[github-actions]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• The Plugin and Theme Directories cannot be accessed within Playground.
• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[dmsnell]

@sirreal check out this awesome work @desrosj is doing - runs the html5lib tests in a separate job and only once, instead of on every version of PHP and MySQL. should make it nicer to have the full test suite run on every change without holding up the CI process.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props desrosj, jonsurrell, dmsnell.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[sirreal]

This is great! At the moment, it appears to be running the tests on this branch, is that expected?

I expected it not to run because there are no changes to the relevant files.

[desrosj]

It's running for this PR because I included the two relevant GitHub Action workflow files in the list that should run the tests when updated. If there are changes to the PHPUnit workflows, it's possible they could change how the html5lib tests are run, so we should run them just to confirm.

[sirreal]

This seems good, thanks!

[sirreal]

This needs to be updated before landing.

[sirreal]

This needs to be updated before landing.

[dmsnell]

Can we not go ahead and run the tests on changes to every file in Core? there are connections through the WP_Token_Map and kses.php. The test suite is quite fast, executing in 1.44s on my laptop, including the PHPUnit setup time.

Seems like this wouldn't be much of a cost on the CI runners or delay PRs but could provide important early warnings that something unintentionally changed the parser.

[desrosj]

an we not go ahead and run the tests on changes to every file in Core?

The test group should only run if the following files are changed or patterns matched:

• src/wp-includes/html-api/**.php
• tests/phpunit/tests/html-api/wpHtmlProcessorHtml5lib.php
• tests/phpunit/data/html5lib-tests/**.dat
• .github/workflows/phpunit-tests.yml
• .github/workflows/reusable-phpunit-tests-v3.yml

Any other time the workflow runs the test group will not be run. They are running for this PR because the last two files in that list are being updated to support testing that specific group separately. Are you seeing something indicating differently @dmsnell?

[dmsnell]

@desrosj exactly. my point is that I don't understand why we're limiting it to running only when files in the wp-includes/html-api/ directory change. For one, other files in Core can affect the parser behavior, for two, the tests are really cheap to run. Is there a compelling reason to avoid running them on changes to any Core files?

[desrosj]

Ah! Apologies, I misread your comment. This was based on the description on the Trac ticket and the original attempt to solve this in #6546.

@jorbin do you recall any context to that detail?

[sirreal]

The reason these tests were excluded from the main test run is mentioned on the ticket:

The html-api-html5lib-tests group of tests is excluded by default since it contains many intentionally and dynamically skipped tests (see discussion in #60227 for why this is the best route for these tests).

These tests currently skip 568 tests:

OK, but incomplete, skipped, or risky tests!
Tests: 1498, Assertions: 930, Skipped: 568.

The amount of skipped tests were some of the main concerns in the original work: https://core.trac.wordpress.org/ticket/60227

The skipped tests may come down a bit more and at some point we may be able to hardcode lists of tests that we do not plan to support to omit them entirely (without skipping).

We can always revisit the decision and consider running these tests along with the rest of the tests.

[sirreal]

I'd be very happy to see this move ahead, is there any way I can help?

Anecdotally, some (minor) failures in the html5lib tests slipped into trunk recently. It wasn't a big problem and has already been fixed, but this work would have prevented that from happening.

[desrosj]

It looks like today there are 421 skipped tests. Great work getting that number way down, everyone! The total counts for reference are Tests: 1508, Assertions: 1087, Skipped: 421..

I've gone and opened #7602, which is an alternative approach. Core-59251 added the ability to run only a specific test group, so the new PR takes advantage of that. I think I like the simplicity of that approach instead with the trade off of running the tests more frequently. With the numbers above, the group is now at a ~70% run rate.

[desrosj]

Going to close this out in favor of #7602.