Make WordPress Core

Opened 5 weeks ago

Last modified 4 weeks ago

#65770 new defect (bug)

PHPUnit tests: Build once and reuse.

Reported by: peterwilsoncc Owned by:
Priority: normal Milestone: Awaiting Review
Component: Build/Test Tools Version:
Severity: normal Keywords: has-patch
Cc: Focuses:

Description

The PHPUnit tests currently run npm run build:dev in each of the tests included in the matrix, currently this is over 80 times.

As the same version of NPM is used in each of the test runs, this is a lot of repetition for the same result.

I propose the WordPress-Develop build be created in the recently created prepare-gutenberg and saved as an artifact. The results of the build can then be applied in the reusable PHP workflow.

This will save about 30 seconds for each test, or 40-odd minutes of runtime.

Change History (5)

This ticket was mentioned in PR #12779 on WordPress/wordpress-develop by @peterwilsoncc.


5 weeks ago
#1

  • Keywords has-patch added

Runs npm run build:dev in the newly created prepare-gutenberg reusable workflow for the PHPUnit tests.

This allows the step to be bypassed in each test, saving about 30 seconds of runtime.

As the local environment uses NPM for the docker pull and configuration, the npm ci step is still required. That can be a project for another time.

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

## Use of AI Tools

AI assistance: Yes
Tool(s): GitHub Copilot
Used for: Initial drafting of this in another PR that also included some docker changes. The creation of this PR built upon portions of that but is much reduced in scope. AI was not used for the generation of this PR per se.

@peterwilsoncc commented on PR #12779:


5 weeks ago
#2

@lancewillett I wanted to see what happened to the tests if they were re-run after the artifacts expired and did so on PHP 7.4 / MySQL 9.7.

The test fails at the step attempting to download the Gutenberg artifact but the same would occur in the equivalent step for applying the WP-Dev build artifact.

  • Do you think it's worth handling this and attempting to download/build in the reusable workflow?
  • If so, are you able to suggest how?

@lancewillett commented on PR #12779:


5 weeks ago
#3

Yes, and it is cheaper than it looks, because the Gutenberg fallback already exists. npm run build:dev runs gutenberg:verify, which downloads when the directory or hash file is missing and hard-fails if the hash still mismatches afterwards. The pin holds as well: GUTENBERG_EXPECTED_SHA is a prepare-gutenberg job output, and outputs survive a partial re-run. On your attempt 2 the producer kept its attempt-1 start time and was not re-executed.

Suggested shape, tolerating the miss only on re-runs so attempt 1 still fails loudly on a real digest-mismatch:

- name: Download WordPress build
        id: download-wordpress
        if: inputs.wordpress-build-artifact != ''
        continue-on-error: ${{ github.run_attempt != '1' }}
        uses: actions/download-artifact@... # unchanged

      - name: Apply WordPress build
        if: steps.download-wordpress.outcome == 'success'
        run: tar -xzf ... # unchanged

      - name: Build WordPress
        if: steps.download-wordpress.outcome != 'success'
        run: npm run build:dev
        env:
          GUTENBERG_EXPECTED_SHA: ${{ inputs.gutenberg-sha }}

Same treatment on the Gutenberg download. A skipped step reports outcome: skipped, so != 'success' also covers callers that pass no artifact. It replaces the == '' check rather than adding to it.

Raising retention is the wrong lever: 35 MB of Gutenberg plus 15 MB of build, on every push.

One aside. The Gutenberg download may be unnecessary in matrix jobs once the build artifact is applied, since I found no runtime reference to gutenberg/ in tests/phpunit/, tools/local-env/, the bootstrap, or the built src/ tree. That would remove one expiry failure mode outright. I measured the download at about 3 seconds though, so it is simplification rather than speed.

@desrosj commented on PR #12779:


5 weeks ago
#4

I haven't gotten to review this in depth just yet (which I would love a chance to do), but I wanted to note that the PHPUnit test jobs previously had a prepare step prior to the strategy.matrix expanding into parallel jobs.

It was removed in r50441 when switching back to running the PHPUnit test suite against the src instead of build, but I seem to recall at the time that it was actually _slower_ over many runs to prepare the codebase once for use by the actual test jobs. That was 5+ years ago now so it's likely my memory is failing me, and a lot h as improved in both our workflows and GitHub Actions (the artifacts API was refactored at around a ~98% performance gain a few years ago). But I just wanted to make sure that we are mindful of the possible timing variation.

@peterwilsoncc commented on PR #12779:


4 weeks ago
#5

The build step was added back in to phpunit in https://github.com/WordPress/wordpress-develop/commit/22294af4ed3b46f577f42590ba4cc19ca0a93f3f / r61438 as some PHP files are coming from the Gutenberg directory now.

I think this is taking a few minutes longer for human minutes but dropping the run time minutes. Which is preferable to optimize for?

Note: See TracTickets for help on using tickets.