run html-api-html5lib-tests grouped tests for changes to html api

[jorbin]

Follow up to #60227 and [58010].

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 should be run when there is a change to the HTML API related files though so that it's not up to maintainers to remember to manually run them.

This adds a GitHub workflow to run the HTML API's html5lib tests when any changes are made to the HTML API, the html5lib tests, or the html5lib data files. Runs on multiple versions of PHP.

@desrosj Do you have time to take a look at this in case there's anything I missed?

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

[desrosj]

I just realized that I had a Slack DM with @costdev (and I think @jorbin at some point) and never followed up with a summary here. Apologies for that!

I am strongly of the opinion that this should not have a separate workflow, and instead somehow integrated into the regular PHPUnit one. Since the idea is to only run these when the related files are updated, I suggested we add a flag to run this group of tests to a preexisting job and use something like tj-actions/changed-files to detect changes.

I've opened a PR that is an extension of @costdev's initial one and our discussion, but still is not quite detecting changes correctly, but is along what I had in mind.

For now I am punting this to 6.7.

@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.

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.

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.

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.

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?

@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?

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?

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.

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.

[chaion07]

Thanks @jorbin for reporting this. We reviewed this Ticket during a recent bug-scrub session. With RC1 just a day away we are updating the milestone. We would hope that this Ticket can progress with the help from @desrosj and @jonsurrel who offered their assistance. Additionally if anyone wants to work on this and take the Ticket move forward is welcome to do so before the 6.7 milestone is over.

Thanks.

Props to @engahmeds3ed

Cheers!

Alternative approach to #6963.

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

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.

@sirreal @aaronjorbin What do you think about this approach?

Is there anything I can do to help this land?

I think #6963 can be closed with this work superseding it.

There's an alternative approach in ​https://github.com/WordPress/wordpress-develop/pull/7602 leveraging the reusable phpunit tests.

Going to close this out in favor of #7602.

Let's close out in favor of #7602.

@sirreal I think this is good to go! Do you mind giving it a final look?

[desrosj]

In 59528:

Build/Test Tools: Regularly run the html-api-html5lib-tests.

In [58010], the external test suite from html5lib was imported to validate the tree-construction steps in the HTML Processor to ensure that they are behaving according to the HTML specification.

The test group was excluded by default because there are a high number of skipped tests. The number of skipped tests has come down, but the group does not need to be run on every job.

This introduces a new job in the PHPUnit workflow for regularly running these tests on their own using the changes in [59251[.

Props desrosj, jonsurrell, jorbin, dmsnell, costdev, chaion07, engahmeds3ed.
Fixes #61209.

Thanks all! Merged in https://core.trac.wordpress.org/changeset/59528.