Changed the E2E isolation model defaults#26761
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis pull request introduces a refined test isolation model for end-to-end tests. The changes encompass an updated Playwright fixture system that supports both per-file and per-test isolation modes, with environment caching and reuse capabilities. Configuration scripts for environment mode resolution are added alongside ESLint enforcement rules for safe environment reset patterns. Documentation is expanded to clarify isolation behavior, environment identity semantics, and lifecycle management. Several test files are updated to adopt the new per-test isolation configuration, and Playwright configuration timeouts are adjusted. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Velo CI Failure AnalysisClassification: 🟠 SOFT FAIL
|
|
Planning to look at this tomorrow morning! |
|
Ok, I pushed some changes - we can revert if desired, it just felt easier to push them in for now. The main gripe was the monkey patching. Even if valid, it felt fragile and, well, icky. Afaict the main reason to do this was to avoid multiple commands for the test-writer to avoid needing to call The other function of monkey patching was avoiding the deprecated Overall, I'd still like to maintain full isolation, but agree it's:
I think the next point of interest would be not needing to fully restart the Ghost container after db reset, which might buy us back some of the gap here with many restarts per file, or just be additional gains. |
9larsons
left a comment
9larsons
left a comment
There was a problem hiding this comment.
Approved as long as you're ok with the changes I pushed - just re-request review if you'd like to discuss more.
ref https://linear.app/ghost/issue/BER-3428/rework-e2e-isolation-model Defaulted E2E isolation to per-file, added root-level per-test opt-in, and updated the fixture and runner behavior to support mixed local workflows.
ref https://linear.app/ghost/issue/BER-3428/rework-e2e-isolation-model Moved mode sniffing into a shared shell helper and removed the conflicting default-dev assumptions so the E2E entrypoints resolve dev vs build consistently.
ref https://linear.app/ghost/issue/BER-3428/rework-e2e-isolation-model The default GitHub-hosted runner shape leaves limited CPU headroom for E2E, so keeping worker count at 1 and tightening timeouts gives us faster failure signal without increasing runner contention.
ref https://linear.app/ghost/issue/BER-3428/rework-e2e-isolation-model Removed ~120 lines of monkey-patching (stack-trace parsing, describe wrapping, runtime mode guards) in favour of a single helper that composes two standard Playwright APIs: test.describe.configure() and test.use(). The fixture now reads an explicit `isolation` option instead of intercepting configure() calls via a side-channel registry.
ref https://linear.app/ghost/issue/BER-3428/rework-e2e-isolation-model Catches test.describe.parallel() and test.describe.serial() at lint time with actionable error messages pointing to the correct replacement. This replaces the runtime monkey-patching that was removed in the previous commit.
9larsons
force-pushed
the
ber-3428-rework-e2e-isolation-model
branch
from
b712c1b to
7ff0af7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
e2e/eslint.config.js (1)
109-116: Add ESLint rule to forbid directtest.describe.configure({mode: 'parallel'})calls.Lines 109-116 restrict the deprecated sugar syntax but miss the supported Playwright API. Direct
test.describe.configure({mode: 'parallel'})bypasses the lint rules, allowing concurrent tests to share the per-file environment. Verification confirms no current direct uses exist ine2e/tests, but the gap should be closed to prevent future issues.Add a rule to catch
MemberExpression[object.name="test"][property.name="describe"][property.callee]patterns with{mode: 'parallel'}in the argument object.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/eslint.config.js` around lines 109 - 116, Add an ESLint rule in e2e/eslint.config.js to forbid direct calls to test.describe.configure({mode: 'parallel'}) by matching the AST CallExpression where the callee is the MemberExpression test.describe.configure and the first argument is an ObjectExpression with a property mode whose value is 'parallel'; emit a message instructing to use usePerTestIsolation() from `@/helpers/playwright/isolation` instead. Target the existing rule array near the test.describe.parallel/serial entries and add a rule that references the call form (test.describe.configure) and the offending object argument to prevent bypassing the per-test isolation linting.e2e/scripts/resolve-e2e-mode.sh (1)
12-14: Consider slightly relaxing the dev-server probe timeout.
curl --max-time 1can mis-detect a warming local dev server and unexpectedly fall back tobuild. A small increase (or split connect/read timeouts) would make auto-detection less brittle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/scripts/resolve-e2e-mode.sh` around lines 12 - 14, The dev-server probe in resolve-e2e-mode.sh uses curl with --max-time 1 which is too short; update the curl invocation that checks "$LOCAL_ADMIN_DEV_SERVER_URL" to relax the timeout (for example increase --max-time to 2 or use separate timeouts like --connect-timeout 1 --max-time 3) so the probe is less likely to mis-detect a warming server; ensure the modified curl still uses --silent --fail and redirects output to /dev/null as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/.claude/E2E_TEST_WRITING_GUIDE.md`:
- Around line 223-242: The docs incorrectly imply resolvedIsolation and
resetEnvironment() are properties of ghostInstance; instead state they are
standalone fixtures exported from fixture.ts and used like any other fixture
(e.g., resolvedIsolation, resetEnvironment). Update the example and surrounding
text to show resolvedIsolation and resetEnvironment() being injected as separate
parameters, note that resetEnvironment() should be called as the final step in a
test unless the test re-establishes auth/page state, and reconcile the “every
test gets a fresh Ghost instance” wording by clarifying that per-file mode
preserves instance across tests while usePerTestIsolation() (imported from
`@/helpers/playwright/isolation`) switches to per-test isolation which forces new
Ghost instances for each test.
In `@e2e/helpers/environment/service-managers/ghost-manager.ts`:
- Line 287: The dev-mode container override currently sets Cmd to ['node',
'--import=tsx', 'index.js'] which bypasses the image's yarn dev entrypoint and
hot-reload tooling; modify the logic around Cmd (the line setting Cmd when mode
=== 'dev') to leave Cmd unset (or remove the override) for dev mode so the
Dockerfile default CMD ["yarn","dev"] is used, ensuring nodemon/hot-reload is
preserved in the e2e dev workflow.
In `@e2e/helpers/playwright/fixture.ts`:
- Around line 76-78: getConfigSignature currently ignores the `labs` test option
so per-file environment reuse (the logic that compares configSignature to decide
sharing) can leak lab flags; update getConfigSignature to include the `labs`
field from the incoming GhostConfig (e.g., serialize { ...config, labs:
config?.labs ?? null } or explicitly include config.labs) so the returned
signature reflects lab overrides and prevents reuse when labs differ; ensure the
same signature is used where configSignature is computed so the per-file cache
key includes labs.
In `@e2e/scripts/resolve-e2e-mode.sh`:
- Around line 7-10: The script resolve-e2e-mode.sh currently returns any
non-empty GHOST_E2E_MODE; change it to validate that GHOST_E2E_MODE is one of
the allowed modes (e.g., "ci", "local", "staging" — use the actual valid set
your project expects) before printing and returning it; if the value is invalid,
print a clear error and exit with a non-zero status (or fall back to a safe
default) so downstream consumers of the resolved mode (the logic around the
printf in resolve-e2e-mode.sh) cannot receive typos like "buid".
---
Nitpick comments:
In `@e2e/eslint.config.js`:
- Around line 109-116: Add an ESLint rule in e2e/eslint.config.js to forbid
direct calls to test.describe.configure({mode: 'parallel'}) by matching the AST
CallExpression where the callee is the MemberExpression test.describe.configure
and the first argument is an ObjectExpression with a property mode whose value
is 'parallel'; emit a message instructing to use usePerTestIsolation() from
`@/helpers/playwright/isolation` instead. Target the existing rule array near the
test.describe.parallel/serial entries and add a rule that references the call
form (test.describe.configure) and the offending object argument to prevent
bypassing the per-test isolation linting.
In `@e2e/scripts/resolve-e2e-mode.sh`:
- Around line 12-14: The dev-server probe in resolve-e2e-mode.sh uses curl with
--max-time 1 which is too short; update the curl invocation that checks
"$LOCAL_ADMIN_DEV_SERVER_URL" to relax the timeout (for example increase
--max-time to 2 or use separate timeouts like --connect-timeout 1 --max-time 3)
so the probe is less likely to mis-detect a warming server; ensure the modified
curl still uses --silent --fail and redirects output to /dev/null as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 76d6b8dc-7ecc-45d3-8e47-e6356bf65856
📒 Files selected for processing (16)
e2e/.claude/E2E_TEST_WRITING_GUIDE.mde2e/AGENTS.mde2e/README.mde2e/eslint.config.jse2e/helpers/environment/environment-manager.tse2e/helpers/environment/service-managers/ghost-manager.tse2e/helpers/playwright/fixture.tse2e/helpers/playwright/isolation.tse2e/playwright.config.mjse2e/scripts/infra-up.she2e/scripts/resolve-e2e-mode.she2e/scripts/run-playwright-host.she2e/tests/admin/billing/force-upgrade.test.tse2e/tests/admin/members/export.test.tse2e/tests/admin/members/filter-actions.test.tse2e/tests/admin/members/members.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
e2e/eslint.config.js (1)
7-7: Consider includingghostInstancein the stale fixtures list.Based on context snippet 1 (fixture.ts:392-417),
ghostInstanceis also affected bycycle()since the holder object is mutated viaObject.assign(holder, nextInstance). While the current fixtures (baseURL,ghostAccountOwner,page,pageWithAuthenticatedUser) cover the most common cases,ghostInstanceitself would also become stale after a recycle if directly destructured alongsideresetEnvironment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/eslint.config.js` at line 7, The resetEnvironmentStaleFixtures array currently lists ['baseURL','ghostAccountOwner','page','pageWithAuthenticatedUser'] but omits ghostInstance, which becomes stale after cycle() mutates the holder via Object.assign(holder, nextInstance); add 'ghostInstance' to resetEnvironmentStaleFixtures so the fixture cleanup includes it and prevents stale destructured values when resetEnvironment is used.e2e/helpers/playwright/fixture.ts (1)
155-162: Consider handling session expiry gracefully.If the cached
storageStateis stale (e.g., session expired), neither the analytics header nor billing iframe may appear, causing the test to hang until Playwright's timeout kicks in. While the timeout will eventually fail the test, a more descriptive error could help debugging.♻️ Optional: Add a timeout with a clearer error message
- await Promise.race([ - analyticsPage.header.waitFor({state: 'visible'}), - billingIframe.waitFor({state: 'visible'}) - ]); + const loaded = await Promise.race([ + analyticsPage.header.waitFor({state: 'visible'}).then(() => true), + billingIframe.waitFor({state: 'visible'}).then(() => true) + ]).catch(() => false); + + if (!loaded) { + throw new Error('Cached storage state may be stale: unable to load authenticated page'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/playwright/fixture.ts` around lines 155 - 162, The Promise.race waiting for analyticsPage.header or billingIframe can hang if cached storageState is expired; update the wait to include an explicit timeout promise that rejects with a clear error (e.g., "session expired or storageState stale: neither AnalyticsOverviewPage.header nor Billing iframe became visible") so failures are descriptive; modify the block around page.goto('/ghost/#/'), AnalyticsOverviewPage(page) and billingIframe.waitFor to race those waits against a timed reject and optionally check for redirect to sign-in before throwing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/AGENTS.md`:
- Line 128: The list item containing "networkidle in waits** (`networkidle`)"
has stray asterisks breaking markdown formatting; edit that line in AGENTS.md
(the text "networkidle in waits** (`networkidle`)") to remove the extra
asterisks so it reads e.g. "networkidle in waits (`networkidle`)" or otherwise
valid markdown list syntax, preserving the inline code token.
---
Nitpick comments:
In `@e2e/eslint.config.js`:
- Line 7: The resetEnvironmentStaleFixtures array currently lists
['baseURL','ghostAccountOwner','page','pageWithAuthenticatedUser'] but omits
ghostInstance, which becomes stale after cycle() mutates the holder via
Object.assign(holder, nextInstance); add 'ghostInstance' to
resetEnvironmentStaleFixtures so the fixture cleanup includes it and prevents
stale destructured values when resetEnvironment is used.
In `@e2e/helpers/playwright/fixture.ts`:
- Around line 155-162: The Promise.race waiting for analyticsPage.header or
billingIframe can hang if cached storageState is expired; update the wait to
include an explicit timeout promise that rejects with a clear error (e.g.,
"session expired or storageState stale: neither AnalyticsOverviewPage.header nor
Billing iframe became visible") so failures are descriptive; modify the block
around page.goto('/ghost/#/'), AnalyticsOverviewPage(page) and
billingIframe.waitFor to race those waits against a timed reject and optionally
check for redirect to sign-in before throwing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c6140bfa-93de-4009-af59-dd17021b8974
📒 Files selected for processing (4)
e2e/AGENTS.mde2e/README.mde2e/eslint.config.jse2e/helpers/playwright/fixture.ts
3 participants
Summary
test.describe.configure({mode: 'parallel'})opt-in for per-test isolationresolvedIsolation, andresetEnvironment()to support mixed isolation safelyPerformance
Over the last 3 days, Velo reports Ghost CI averaging 29.7 minutes per successful run. The latest successful run on this branch completed in 22.6 minutes, a 7.1 minute improvement (23.8%).
Looking specifically at the E2E matrix, the last 10 successful CI runs averaged a 17.8 minute critical path across the 4 E2E shards; this branch completed that path in 13.1 minutes, a 4.7 minute improvement (26.2%).
Total E2E shard compute time also dropped from 66.9 minutes on average to 45.6 minutes, reducing runner time by 21.3 minutes (31.9%).
ref https://linear.app/ghost/issue/BER-3428/rework-e2e-isolation-model