Build/Test Tools: Add a workflow for the HTML API's html5lib tests. #6546

costdev · 2024-05-13T22:40:49Z

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.

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

github-actions · 2024-05-13T22:41:28Z

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 costdev, jonsurrell.

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

costdev · 2024-05-13T22:42:25Z

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

.github/workflows/html5lib-tests.yml

github-actions · 2024-05-13T23:26:38Z

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.

sirreal

Thanks for doing this, I've left some notes but it looks good in general.

sirreal · 2024-05-14T10:02:28Z

tests/phpunit/tests/html-api/wpHtmlProcessorHtml5lib.php

+ * A change to test that the workflow triggers.
+ *


Flagging this, I expect the plan is to remove this before final commit.

.github/workflows/html5lib-tests.yml

sirreal

There may be a couple of candidates to simplify and speed up the job. HTML API is relatively isolated and I'm pretty sure it could run without doing any Node.js or npm installation/setup.

sirreal · 2024-05-14T10:09:30Z

.github/workflows/html5lib-tests.yml

+      - name: Set up Node.js
+        uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2
+        with:
+          node-version-file: '.nvmrc'
+          cache: npm


This could probably be skipped.

Tried removing Docker, Node and NPM in 36e9f49, but an error was thrown due to wp-tests-config.php being missing. The tests are run in Docker via Node. I've reverted the removal, at least for now.

sirreal · 2024-05-14T10:09:52Z

.github/workflows/html5lib-tests.yml

+      - name: Install npm dependencies
+        run: npm ci


This could probably be skipped.

This reverts commit 36e9f49.

sirreal

I don't have anything else to add at this point 👍

desrosj

I started a review, but then started questioning whether a separate workflow is really necessary for this. There were a lot of small details missing here that are in the PHPUnit workflow, and it seems like all of the same conditions and steps are just repeated here.

The PHPUnit workflow already runs specific groups (ajax, external-http, xdebug), has more testing configurations, and is already maintained. Is there a reason I'm missing as to why this needs to be an entirely separate workflow? There's currently only 3 test methods with 504 test assertions for the html-api-html5lib-tests group. It seems reasonable to include them as an additional step in phpunit-tests-run.yml.

I also noticed that 86% of the test assertions are currently skipped. Is there any way to improve this? While some tests are better than no tests at all, that's a pretty high percentage of assertions not being performed.

desrosj · 2024-05-14T12:51:20Z

.github/workflows/html5lib-tests.yml

+      # The html5lib tests were introduced in WordPress 6.6.
+      - '6.[6-9]'
+      - '[7-9].[0-9]'
+      - '[1-9][0-9]+.[0-9]'


Let's remove this range. At our current 3 major versions a year pace, we won't reach 10.0 for 11 more years. 🙈

All other workflows will need to be updated as well, and I think it's preferable to just do them all at once.

desrosj · 2024-05-14T12:51:40Z

.github/workflows/html5lib-tests.yml

+      - '6.[6-9].[0-9]+'
+      - '[7-9].[0-9]'
+      - '[7-9].[0-9].[0-9]+'
+      - '[1-9][0-9]+.[0-9]'


Same here, let's stop with the 7-9 pattern.

desrosj · 2024-05-14T12:51:46Z

.github/workflows/html5lib-tests.yml

+      - trunk
+      - '6.[6-9]'
+      - '[7-9].[0-9]'
+      - '[1-9][0-9]+.[0-9]'


Here as well.

desrosj · 2024-05-14T12:56:24Z

.github/workflows/html5lib-tests.yml

+    strategy:
+      fail-fast: false
+      matrix:
+        php: [ '7.2', '7.3', '7.4', '8.0', '8.1', '8.2', '8.3' ]


Are there reasons not to run against multisite? Or different database types?

I don't think either would be valuable here since these tests are extremely close to actual unit tests. None of the HTML API code interacts with the database nor does it rely on any code which is different for multisite.

desrosj · 2024-05-14T12:59:59Z

.github/workflows/html5lib-tests.yml

+    if: ${{ github.repository == 'WordPress/wordpress-develop' || github.event_name == 'pull_request' }}
+    strategy:
+      fail-fast: false
+      matrix:


The PHP version is not being applied properly to the test environment. Every job in the workflow is running on the default version of PHP, which is currently 8.2.

LOCAL_PHP will need to be set to ${{ matrix.php }} so that the dotenv package can have the right version to use.

desrosj · 2024-05-14T13:00:21Z

.github/workflows/html5lib-tests.yml

+
+      - name: WordPress Docker container debug information
+        run: |
+          docker compose run --rm mysql ${{ env.LOCAL_DB_TYPE }} --version


This environment variable is not set within this workflow. The command (docker compose run --rm mysql --version) still works and outputs debug info. But there's no database type being configured here.

aaronjorbin · 2024-05-14T15:42:01Z

.github/workflows/html5lib-tests.yml

+      # Any change to a PHP file in the HTML API should run checks.
+      - 'src/wp-includes/html-api/**.php'
+      - 'tests/phpunit/tests/html-api/wpHtmlProcessorHtml5lib.php'
+      - 'tests/phpunit/data/html5lib-tests/**.dat'


Changes to this file should also trigger it.

Suggested change

- 'tests/phpunit/data/html5lib-tests/**.dat'

- 'tests/phpunit/data/html5lib-tests/**.dat'

# Changes to this file should run the tests to ensure they still run correctly.

- '.github/workflows/html5lib-tests.yml'

aaronjorbin · 2024-05-14T15:45:36Z

.github/workflows/html5lib-tests.yml

+    strategy:
+      fail-fast: false
+      matrix:
+        php: [ '7.2', '7.3', '7.4', '8.0', '8.1', '8.2', '8.3' ]


I don't think either would be valuable here since these tests are extremely close to actual unit tests. None of the HTML API code interacts with the database nor does it rely on any code which is different for multisite.

costdev marked this pull request as ready for review May 13, 2024 22:41

costdev force-pushed the html-api/add-html5libtests-workflow branch 10 times, most recently from 727acd1 to 746646f Compare May 13, 2024 23:11

costdev commented May 13, 2024

View reviewed changes

.github/workflows/html5lib-tests.yml Outdated Show resolved Hide resolved

costdev added 2 commits May 14, 2024 00:31

Add html5lib-tests workflow.

86ca144

Test workflow trigger.

5cdf152

costdev force-pushed the html-api/add-html5libtests-workflow branch from 746646f to 5cdf152 Compare May 13, 2024 23:31

sirreal approved these changes May 14, 2024

View reviewed changes

sirreal reviewed May 14, 2024

View reviewed changes

costdev added 5 commits May 14, 2024 11:43

Remove multisite.

2b71fbd

Try without Docker, Node, or NPM.

36e9f49

Revert "Try without Docker, Node, or NPM."

a8f0aa8

This reverts commit 36e9f49.

Clean up docs.

9d7f20b

Update branch and tag globs.

1bd3a1b

sirreal approved these changes May 14, 2024

View reviewed changes

desrosj requested changes May 14, 2024

View reviewed changes

aaronjorbin reviewed May 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build/Test Tools: Add a workflow for the HTML API's html5lib tests. #6546

Build/Test Tools: Add a workflow for the HTML API's html5lib tests. #6546

costdev commented May 13, 2024 •

edited

github-actions bot commented May 13, 2024 •

edited

costdev commented May 13, 2024

github-actions bot commented May 13, 2024

sirreal left a comment

sirreal May 14, 2024

costdev May 14, 2024

sirreal left a comment

sirreal May 14, 2024

costdev May 14, 2024 •

edited

sirreal May 14, 2024

sirreal left a comment

desrosj left a comment

desrosj May 14, 2024

desrosj May 14, 2024

desrosj May 14, 2024

desrosj May 14, 2024

aaronjorbin May 14, 2024

desrosj May 14, 2024

desrosj May 14, 2024

aaronjorbin May 14, 2024

aaronjorbin May 14, 2024

Build/Test Tools: Add a workflow for the HTML API's html5lib tests. #6546

Are you sure you want to change the base?

Build/Test Tools: Add a workflow for the HTML API's html5lib tests. #6546

Conversation

costdev commented May 13, 2024 • edited

github-actions bot commented May 13, 2024 • edited

costdev commented May 13, 2024

github-actions bot commented May 13, 2024

Test using WordPress Playground

Some things to be aware of

sirreal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirreal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costdev May 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirreal left a comment

Choose a reason for hiding this comment

desrosj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costdev commented May 13, 2024 •

edited

github-actions bot commented May 13, 2024 •

edited

costdev May 14, 2024 •

edited