Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
195 changes: 195 additions & 0 deletions .github/workflows/html5lib-tests.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,195 @@
name: HTML API html5lib Tests

on:
push:
branches:
- trunk
# The html5lib tests were introduced in WordPress 6.6.
- '6.[6-9]'
- '[7-9].[0-9]'
- '[1-9][0-9]+.[0-9]'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

tags:
- '6.[6-9]'
- '6.[6-9].[0-9]+'
- '[7-9].[0-9]'
- '[7-9].[0-9].[0-9]+'
- '[1-9][0-9]+.[0-9]'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

- '[1-9][0-9]+.[0-9].[0-9]+'
pull_request:
branches:
- trunk
- '6.[6-9]'
- '[7-9].[0-9]'
- '[1-9][0-9]+.[0-9]'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here as well.

paths:
# 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'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

workflow_dispatch:

# Cancels all previous workflow runs for pull requests that have not completed.
concurrency:
# The concurrency group contains the workflow name and the branch name for pull requests
# or the commit hash for any other events.
group: ${{ github.workflow }}-${{ github.event_name == 'pull_request' && github.head_ref || github.sha }}
cancel-in-progress: true

# Disable permissions for all available scopes by default.
# Any needed permissions should be configured at the job level.
permissions: {}

jobs:
# Runs the HTML API html5lib PHPUnit tests for WordPress.
#
# Performs the following steps:
# - Sets environment variables.
# - Checks out the repository.
# - Sets up Node.js.
# - Sets up PHP.
# - Installs Composer dependencies.
# - Installs npm dependencies
# - Logs general debug information about the runner.
# - Logs Docker debug information (about the Docker installation within the runner).
# - Starts the WordPress Docker container.
# - Logs the running Docker containers.
# - Logs debug information about what's installed within the WordPress Docker containers.
# - Install WordPress within the Docker container.
# - Run the HTML API html5lib PHPUnit tests.
# - Ensures version-controlled files are not modified or deleted.
run-html-api-html5lib-tests:
name: PHP ${{ matrix.php }}
runs-on: ubuntu-latest
permissions:
contents: read
if: ${{ github.repository == 'WordPress/wordpress-develop' || github.event_name == 'pull_request' }}
strategy:
fail-fast: false
matrix:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

php: [ '7.2', '7.3', '7.4', '8.0', '8.1', '8.2', '8.3' ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


steps:
- name: Configure environment variables
run: |
echo "PHP_FPM_UID=$(id -u)" >> $GITHUB_ENV
echo "PHP_FPM_GID=$(id -g)" >> $GITHUB_ENV

- name: Checkout repository
uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
with:
show-progress: ${{ runner.debug == '1' && 'true' || 'false' }}

- name: Set up Node.js
uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2
with:
node-version-file: '.nvmrc'
cache: npm
Comment on lines +82 to +86
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could probably be skipped.

Copy link
Contributor Author

@costdev costdev May 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


##
# This allows Composer dependencies to be installed using a single step.
#
# Since the tests are currently run within the Docker containers where the PHP version varies,
# the same PHP version needs to be configured for the action runner machine so that the correct
# dependency versions are installed and cached.
##
- name: Set up PHP
uses: shivammathur/setup-php@a4e22b60bbb9c1021113f2860347b0759f66fe5d # v2.30.0
with:
php-version: '${{ matrix.php }}'
coverage: none

# Since Composer dependencies are installed using `composer update` and no lock file is in version control,
# passing a custom cache suffix ensures that the cache is flushed at least once per week.
- name: Install Composer dependencies
uses: ramsey/composer-install@57532f8be5bda426838819c5ee9afb8af389d51a # v3.0.0
with:
custom-cache-suffix: $(/bin/date -u --date='last Mon' "+%F")

- name: Install npm dependencies
run: npm ci
Comment on lines +108 to +109
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could probably be skipped.


- name: General debug information
run: |
npm --version
node --version
curl --version
git --version
composer --version
locale -a

- name: Docker debug information
run: |
docker -v

- name: Start Docker environment
run: |
npm run env:start

- name: Log running Docker containers
run: docker ps -a

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

docker compose run --rm php php --version
docker compose run --rm php php -m
docker compose run --rm php php -i
docker compose run --rm php locale -a

- name: Install WordPress
run: npm run env:install

- name: Run HTML API html5lib PHPUnit tests
run: node ./tools/local-env/scripts/docker.js run php ./vendor/bin/phpunit --verbose -c phpunit.xml.dist --group html-api-html5lib-tests

- name: Ensure version-controlled files are not modified or deleted
run: git diff --exit-code

slack-notifications:
name: Slack Notifications
uses: WordPress/wordpress-develop/.github/workflows/slack-notifications.yml@trunk
permissions:
actions: read
contents: read
needs: [ run-html-api-html5lib-tests ]
if: ${{ github.repository == 'WordPress/wordpress-develop' && github.event_name != 'pull_request' && always() }}
with:
calling_status: ${{ contains( needs.*.result, 'cancelled' ) && 'cancelled' || contains( needs.*.result, 'failure' ) && 'failure' || 'success' }}
secrets:
SLACK_GHA_SUCCESS_WEBHOOK: ${{ secrets.SLACK_GHA_SUCCESS_WEBHOOK }}
SLACK_GHA_CANCELLED_WEBHOOK: ${{ secrets.SLACK_GHA_CANCELLED_WEBHOOK }}
SLACK_GHA_FIXED_WEBHOOK: ${{ secrets.SLACK_GHA_FIXED_WEBHOOK }}
SLACK_GHA_FAILURE_WEBHOOK: ${{ secrets.SLACK_GHA_FAILURE_WEBHOOK }}

failed-workflow:
name: Failed workflow tasks
runs-on: ubuntu-latest
permissions:
actions: write
needs: [ slack-notifications ]
if: |
always() &&
github.repository == 'WordPress/wordpress-develop' &&
github.event_name != 'pull_request' &&
github.run_attempt < 2 &&
(
contains( needs.*.result, 'cancelled' ) ||
contains( needs.*.result, 'failure' )
)

steps:
- name: Dispatch workflow run
uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1
with:
retries: 2
retry-exempt-status-codes: 418
script: |
github.rest.actions.createWorkflowDispatch({
owner: context.repo.owner,
repo: context.repo.repo,
workflow_id: 'failed-workflow.yml',
ref: 'trunk',
inputs: {
run_id: '${{ github.run_id }}'
}
});
2 changes: 2 additions & 0 deletions tests/phpunit/tests/html-api/wpHtmlProcessorHtml5lib.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
*
* See the README file at DIR_TESTDATA / html5lib-tests for details on the third-party suite.
*
* A change to test that the workflow triggers.
*
Comment on lines +15 to +16
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep 🙂

* @package WordPress
* @subpackage HTML-API
*
Expand Down
Loading