[Website] Add E2E coverage for playground-zip import regressions - #3874
Conversation
github-actions
Bot
added
[Package][@wp-playground] Website
[Aspect] Browser
[Aspect] Website
and removed
[Package][@wp-playground] Website
labels
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR improves reliability of ZIP imports into OPFS by ensuring imported files are flushed before reporting success, and adds an E2E regression test to verify imported sites persist across site switches and reloads.
Changes:
- Document why OPFS flush is required after ZIP import before surfacing success.
- Add Playwright E2E coverage for ZIP import persistence across switching sites and direct reload by
site-slug. - Add small E2E helpers for getting/setting the active site and navigating within the Playground.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/playground/website/src/components/saved-playgrounds-overlay/index.tsx | Adds an inline comment clarifying the OPFS flush rationale during ZIP import flow. |
| packages/playground/website/playwright/e2e/opfs.spec.ts | Adds a regression E2E test plus helpers to validate ZIP import persistence across navigation and site switching. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@brandonpayton Can you take a look at this too? This is awaiting your review based on your feedback in #3847 |
|
It looks like the dock UI change probably broke this PR, and I'm having an agent look at that as part of reviewing this PR. |
…esting-follow-up-2knrg # Conflicts: # packages/playground/website/src/components/saved-playgrounds-overlay/index.tsx
The persistence test previously requested the imported marker from the active import runtime before switching sites. Completing that request emits request.end and can trigger background OPFS flushing, masking writes that were not durable when import success was reported. Boot the imported site directly from its saved slug before issuing the first marker request. Keep the existing failure UI checks, switch-away and switch-back coverage, final direct reload, and exact success-dialog assertion unchanged. This remains a black-box durability check rather than an assertion about a specific flush RPC; automatic filesystem-write flushing may still settle before the success dialog.
…esting-follow-up-2knrg
The persistence test fresh-boots the imported saved-site slug so that its first test-controlled request cannot run in the runtime that performed the ZIP import. Keep the marker navigation and content assertion immediately after that top-level reload. This makes the durability boundary explicit: the old runtime has been discarded, and the marker must be restored from OPFS before the fresh runtime can serve it. Checking the marker before the reload could emit request.end and allow automatic background flushing to mask a missing import-time flush. Move the existing Save failed and Site failed checks after the marker assertion, while retaining the later switch-away, switch-back, direct-reload, and exact success-dialog coverage. This remains a black-box check of the user-visible contract. It verifies that an import reported as successful is durable at the next boot without coupling the test to Comlink or asserting which OPFS flush path completed.
|
I pushed the conflict resolution plus one small test change. Why: the test now reloads the imported site before requesting its marker file. Reloading discards the runtime that performed the import. If the test requested the file first, that request could trigger a background save and hide the bug. After a fresh boot, the file can appear only if the import was already saved to OPFS. The later site-switch and reload checks are unchanged. |
|
I removed the checks that searched for the exact text “Save failed” and “Site failed.” Those checks could pass even when something was broken if the UI wording changed, because the old text would simply no longer be found. The test already checks the behavior we actually care about: the imported marker file must load after a fresh boot, after switching away and back, and after another reload. Those checks fail when the import is not persisted and do not depend on UI copy. |
|
Thank you @brandonpayton |
What changed
E2E test coverage
?site-slug=<slug>, discarding the runtime that performed the import.Why
The previous assertion order requested the imported marker from the active import runtime before switching sites. Completing that request emits
request.end, which can trigger background OPFS flushing and mask files that were not durable when import success was reported.Fresh-booting first checks the user-visible contract: once import reports success, the imported files can be restored from saved storage. This intentionally remains a black-box durability check; it does not couple the test to Comlink or assert which OPFS flush path completed.
Follow-up to review feedback on #3847.