feat(playwright): stop tracing the happy path, report slow tests - #2
Conversation
`trace: 'retain-on-failure'` records a trace for every test and throws it away when the test passes, so a fully green suite pays the tracing cost in full to keep artefacts for the tests that fail. `on-first-retry` moves that onto the retry, which `retries: 2` already guarantees under CI -- the same artefacts where they are useful, none of the overhead where they are not. `reportSlowTests: null` silenced Playwright's slow-test summary. That report is how you notice a suite going wrong: visual-portfolio has been running 56 tests at 10.4s each against 4.2-4.7s for every sibling suite, because a `beforeEach` loads the whole of wp-admin before each test to read one string. Nothing surfaced it for months. Restored at Playwright's default shape with a 30s threshold. Adds a CI workflow too. Only npm-publish existed, and it runs on tags, so until now nothing checked a pull request -- a change could only fail once it was already on its way to npm. Same lint + test pair, run before the merge. Minor rather than patch: both settings change behaviour for every consumer, and consumers pin ^0.5.1, so the bump has to be deliberate on their side.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a017f7636
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| contents: read | ||
|
|
||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.event_name == 'pull_request' && github.head_ref || github.sha }} |
There was a problem hiding this comment.
For pull requests from forks, github.head_ref contains only the source branch name, so unrelated contributors using a common name such as main or fix receive the same concurrency group. Starting CI for one of those PRs will therefore cancel the in-progress verify job for the other; use the PR number, github.ref, or include the head repository owner to make the group unique per PR.
Useful? React with 👍 / 👎.
1 participant
Why
Two settings in the shared Playwright preset were costing every consuming plugin time, and a third gap meant nothing checked this repo before publish.
trace: 'retain-on-failure'→'on-first-retry'retain-on-failurerecords a trace for every test and deletes it when the test passes. A green suite therefore pays the full tracing cost — DOM snapshots around every action — to keep artefacts for the tests that fail. Sinceretries: process.env.CI ? 2 : 0already retries anything that fails under CI,on-first-retryproduces the same artefacts where they are useful and none of the overhead where they are not.reportSlowTests: null→{ max: 5, threshold: 30_000 }This is the report that tells you a suite has gone wrong, and it was switched off. Concretely, measured across the plugin family last week:
The visual-portfolio core suite is ~2.4x slower per test than every sibling — including a 131-test suite on the same runner in the same job. The cause turned out to be a
beforeEachdoing a fullpage.goto('/wp-admin/')before each of 28 tests purely to readwindow.VPAdminVariables.nonce, while the tests themselves only issuepage.request.post. A slow-test report would have named that file the first time it ran.CI on pull requests
npm-publish.ymlruns onv*tags andworkflow_dispatchonly, so no workflow ran on a pull request. A regression could only be caught once it was already on its way to npm. The newci.ymlruns the samelint+testpair the publish job gates on, before the merge instead of after the tag. It stays onubuntu-latest— this repo is public, so those minutes are free.Verification
Both new settings are covered by
scripts/self-test.js, matching hownavigationTimeoutis guarded:tracing is not paid for on the happy path— assertstrace === 'on-first-retry'and thatretries > 0, since the setting only produces artefacts if failures are actually retried.slow tests are reported— assertsreportSlowTestsis notnulland that the threshold is non-zero.npm run lintandnpm testpass locally (12 checks).Version
0.6.0, not a patch. Both changes alter behaviour for every consumer, and consumers pin^0.5.1, so picking this up should be a deliberate bump on their side rather than something that arrives silently.