Skip to content

Build/Test Tools: Run the Add New Media E2E spec in parallel. - #15896

Closed
adimoldovan wants to merge 4 commits into
WordPress:trunkfrom
adimoldovan:e2e/attachment-fixture
Closed

adimoldovan wants to merge 4 commits into
WordPress:trunkfrom
adimoldovan:e2e/attachment-fixture

Conversation

@adimoldovan

@adimoldovan adimoldovan commented Nov 11, 2026 •

Copy link
Copy Markdown

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

media-new-client-side-upload.test.js deleted all media and all posts after each test, so it had to run in the serial project. In parallel, that cleanup would delete other workers' content. Three of its tests also read the newest attachment on the site, which in parallel can come from another spec.

The new attachments fixture deletes only the attachments that a test uploads or tracks, when the test ends, also when the test fails:

import { test, expect } from '../fixtures';

test( 'example', async ( { attachments } ) => {
	// Upload through REST.
	const media = await attachments.upload( '/absolute/path/to/image.jpg' );

	// Or record an attachment that the browser uploaded, by the id
	// from the upload response.
	attachments.track( 123 );
} );

The spec now records each attachment that its page uploads, through REST or async-upload.php. It reads each attachment by that id, not the newest one, and creates its post with postFactory. The spec moves to parallel.

The README documents attachments and adds a rule: a fixture goes in fixtures/ only when 2 or more spec files use it. Two more client-side upload specs will use attachments in a follow-up PR.

Testing Instructions

  1. Run npm run env:cli -- post list --post_type=attachment --format=count and note the number.
  2. Run npm run test:e2e -- media-new-client-side-upload. All 18 tests pass.
  3. Repeat step 1. The number is the same.
  4. Run npm run test:e2e -- --project=parallel --shuffle --repeat-each=2. Every run of media-new-client-side-upload.test.js passes.

Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5.5
Used for: Code change, test runs and this description.


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

`media-new-client-side-upload.test.js` deleted all media and all posts after each test, so it had to run in the serial project. The new `attachments` fixture deletes only the attachments that a test uploads or tracks. The spec now reads each attachment by its own id, not the newest one, and creates its post with `postFactory`.

See #66392.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The upload test can leave persisted attachments uncleaned when metadata generation returns HTTP 500 because it does not capture the attachment ID header before checking response success.

1 open issue
What changed in this PR

This PR moves the Add New Media client-side upload spec into the parallel E2E project by replacing site-wide cleanup and newest-attachment queries with test-owned data tracking.

Changes:

  • Adds an attachment fixture for uploads, tracking, and scoped cleanup.
  • Updates the spec to query attachments by ID and create its parent post through postFactory.
  • Enables parallel execution and documents attachment usage and shared-fixture placement.
File Description
tests/​e2e/​specs/​media-new-client-side-upload.test.js Tracks browser uploads and uses test-owned attachments and posts.
tests/​e2e/​README.md Documents the attachment fixture and shared-fixture placement rule.
tests/​e2e/​playwright.config.js Removes the spec from the serial-only list.
tests/​e2e/​fixtures/​index.js Registers the attachment fixture.
tests/​e2e/​fixtures/​attachment.js Adds attachment upload, tracking, and teardown cleanup.

Review effort: Balanced

Core sends `X-WP-Upload-Attachment-ID` after it saves the attachment and before it makes the sub-sizes. Reading it tracks an upload that fails with a 500 after the save, which the old check skipped. The header arrives before the body, so the id is tracked before teardown can run.

See #66392.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The REST upload path can leak an attachment when metadata generation fails because it does not track the attachment ID from the response header before uploadMedia() rejects, leaving teardown unable to delete it.

1 open issue
1 resolved since last review

Review effort: Balanced

Comment thread tests/e2e/fixtures/attachment.js Resolved
`requestUtils.uploadMedia()` throws away the response on an error. If the server saves the attachment and then fails while it makes the sub-sizes, the fixture never learned the id, and the attachment stayed. `upload()` now posts the file itself and reads the id from `X-WP-Upload-Attachment-ID` before it checks the status.

See #66392.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

No active review issues were identified for the changes that isolate media-upload test data for parallel execution.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

@adimoldovan
adimoldovan marked this pull request as ready for review November 11, 2026 14:14
@github-actions

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs.

Core Committers: Use this line as a base for the props when committing in SVN:

Props adrianmoldovanwp.

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


Props Bot commands

You can trigger Props Bot actions by commenting on this PR:

  • @propsbot add <username> to give props to a GitHub user. Requires write access.
  • @propsbot remove <username> to leave a user out of the props. Requires write access.
  • @propsbot refresh to refresh this list. Adding the props-bot label works too.

@lancewillett lancewillett left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The scoped cleanup and attachment-ID reads make these existing tests suitable for parallel execution without deleting another worker’s media or selecting its latest upload.

The synchronous X-WP-Upload-Attachment-ID tracking also addresses the two cleanup misses: an HTTP error after attachment creation and teardown before response-body parsing completes. Focused Playwright diagnostics passed for both, plus the upload helper’s success/error paths. These were controlled local diagnostics, not a full local WordPress E2E run.

All four E2E CI jobs and JavaScript type checking passed on this revision. No blocking findings remain.

AI review · gpt-6

@lancewillett lancewillett moved this from Backlog to Ready in WordPress Project Build Tooling Nov 11, 2026
wporg-sync pushed a commit that referenced this pull request Nov 11, 2026
…E2E tests.

Track and clean up each test's attachments, and run the Add New Media spec in parallel. Give each application-password test its own user.

Developed in: #15896, #15902

Props adrianmoldovanwp.
See #66392.


git-svn-id: https://develop.svn.wordpress.org/trunk@65402 602fd350-edb4-49c9-b593-d223f7449a82
wporg-sync pushed a commit to WordPress/WordPress that referenced this pull request Nov 11, 2026
…E2E tests.

Track and clean up each test's attachments, and run the Add New Media spec in parallel. Give each application-password test its own user.

Developed in: WordPress/wordpress-develop#15896, WordPress/wordpress-develop#15902

Props adrianmoldovanwp.
See #66392.

Built from https://develop.svn.wordpress.org/trunk@65402


git-svn-id: http://core.svn.wordpress.org/trunk@64541 1a063a9b-81f0-0310-95a4-ce76da25c4cd
@lancewillett

Copy link
Copy Markdown
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

3 participants