Add E2E happy-path Playwright suite#239
Conversation
The `wp-env:cli` script runs lifecycle steps on the tests-cli container (wpTestsPort, 8801), but `@gravitykit/e2e-bootstrap`'s default baseURL is wpPort (8800). Global setup cleanup, login, and tests were hitting the empty dev instance and getting HTML login pages where JSON was expected, producing `SyntaxError: Unexpected token '<', "<!DOCTYPE "...`. Override `use.baseURL` and `webServer.url` with the tests port so cleanup, login, and page navigations resolve against the env where plugins and REST routes actually live.
11 new specs covering the Lite-side behavior of GravityExport: - P0 download URL: enable, .xlsx default, .csv extension swap, regenerate/disable - P1 admin config: disable field, sort order - P2 access control: logged-in-only restriction - P3 data shaping: transpose mode, entry notes column - P5 notifications: attach single entry to a notification (CSV and XLSX) Supporting infrastructure: - tests/E2E/helpers/test-helpers.js wraps @gravitykit/e2e-bootstrap, fixes the fixtures baseURL inside Playwright workers (api.initFromEnv needs both WP_ENV_URL and WP_ENV_PORT and only the latter is in the env), and adds Lite-specific helpers for form-settings submission, feed-meta patching, notification seeding, entry submission, mail-capture access, attachment reads, and CSV parsing. - tests/E2E/setup/mu-plugins/e2e-mail-capture.php short-circuits wp_mail, copies attachments to uploads/e2e-mail-capture/attachments so they survive gform_after_email cleanup, and exposes GET/DELETE /wp-json/gk-e2e/v1/mail. - tests/E2E/setup/wp-env.config.js maps the mu-plugin into the tests container via additionalMappings. - tests/E2E/setup/playwright.config.js pins workers: 1 because several specs mutate global state (rewrite rules, gf_addon_feed, shared mail inbox) and the bootstrap default '50%' caused intermittent contention. The discovery doc (.claude/gravityexport-lite-happy-paths.md) records the full Phase 1 list, what shipped, what was dropped and why (custom labels, multi-form report, URL search filters), and the gotchas resolved along the way (form data-js page-loader stripping submit button name/value, anonymous request isolation, atomic-write breaking the wp-env bind mount). Full suite runs serially in roughly 90s and was verified green three times in a row.
Playwright writes its compiled-spec cache to playwright-transform-cache-<uid> at the project root. The existing *cache rule only catches names that end in "cache", so the suffixed dir was being reported as untracked after every local run.
The attach-csv and attach-xlsx specs intermittently failed with "apiRequestContext.get: socket hang up" on the /wp-json/gk-e2e/v1/mail endpoint. Apache in the wp-env container has a 5s KeepAlive idle timeout; Playwright's APIRequestContext reuses HTTP connections across specs, so the next reader can land on a socket the server has already closed. - Replace Playwright APIRequestContext usage with Node's global fetch in getCapturedEmails / clearCapturedEmails. The helper retries on transient connection-level errors (socket hang up, ECONNRESET, EPIPE, fetch failed) with exponential backoff. - Add waitForCapturedEmail(predicate) that polls until the matching message appears, absorbing both connection blips and any sub-second race between the wp-cli notification send and the file landing on disk. Notification specs now use this instead of a single GET. - Have the mu-plugin DELETE also wipe captured attachments so the uploads dir doesn't grow unbounded across long-running suites. - Drop the workers: 1 cap from playwright.config.js. The original cap was a workaround for what is actually the same connection-level flakiness — with retries in place, the bootstrap default (50%, i.e. 5 workers on a 10-core box) runs clean. Verified: full suite 3× back-to-back at the bootstrap default (5 workers), full suite at workers=2 and workers=4, and attach-csv x10 via --repeat-each — all green.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Playwright-based E2E test suite: test-plan docs, Playwright/wp-env setup, mail-capture mu-plugin, shared test helpers, CI tweak, and 11 new specs validating download URLs, CSV/XLSX outputs, field shaping, data shaping, permissions, and notification attachments. ChangesGravityExport Lite E2E Test Suite
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
tests/E2E/helpers/test-helpers.js (1)
459-491: 💤 Low value
patchExportFeedMeta/addNotification/submitEntryTriggeringNotificationshave inconsistent async signatures.All three call into the synchronous
wpEval(which usesexecFileSync) and do no real awaiting:
addNotificationandsubmitEntryTriggeringNotificationsare declaredasync(wrapping the return in a Promise).patchExportFeedMeta(line 528) is declared sync and is invoked withoutawaitacross the specs.Both styles "work", but the inconsistency leaks into callers — e.g.,
logged-in-required.spec.jsline 53 callspatchExportFeedMeta(...)withoutawait, whiletranspose.spec.js/disable-field.spec.jsdo the same, yetaddNotificationmust be awaited. If any of these ever become genuinely async (e.g., switched toexecFileor to a REST call), the un-awaited call sites will silently break ordering.Either make all three
async(and await everywhere), or keep them all sync. Aligning them now prevents a footgun later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/E2E/helpers/test-helpers.js` around lines 459 - 491, The three helper functions addNotification, submitEntryTriggeringNotifications, and patchExportFeedMeta have inconsistent async signatures—two are declared async while patchExportFeedMeta is synchronous—causing some call sites to omit await and risking ordering bugs if these become truly async; pick one approach and apply it consistently: either (A) make patchExportFeedMeta async (returning a Promise) and update all call sites (e.g., in logged-in-required.spec.js, transpose.spec.js, disable-field.spec.js and any others) to await patchExportFeedMeta, or (B) remove async from addNotification and submitEntryTriggeringNotifications and return values synchronously so callers need not await; ensure you modify the function declarations (addNotification, submitEntryTriggeringNotifications, patchExportFeedMeta) and all callers to match the chosen synchronous or asynchronous pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/gravityexport-lite-happy-paths.md:
- Around line 182-216: The doc incorrectly states that playwright.config.js
"pins workers: 1" — update .claude/gravityexport-lite-happy-paths.md to remove
or revise the two references (around the text currently at lines 183 and 216)
and instead describe the current workers configuration in
tests/E2E/setup/playwright.config.js (mention whether it is absent or set to a
non-1 value), and confirm or correct the suite runtime claim (~90s) after
running the full suite across the actual worker configuration; reference the
symbol "workers: 1" and the file tests/E2E/setup/playwright.config.js so
reviewers can locate and verify the change.
In `@tests/E2E/helpers/test-helpers.js`:
- Around line 370-415: findTestsContainer is passing a plain object literal {
value: cachedWpContainer } so updates inside findContainer don't persist back to
the module-scoped cachedWpContainer, causing repeated docker ps calls; fix by
making the cache proxy like findCliContainer (provide an object with get value()
and set value(v) that reads/writes cachedWpContainer) or simplify both helpers
to use a single memo pattern (have findTestsContainer check cachedWpContainer
and set it directly after findContainer returns) so the cachedWpContainer is
actually mutated and subsequent calls avoid shelling out.
- Around line 110-124: submitSettingsForm currently calls
page.waitForLoadState('load') before triggering form.requestSubmit which can
resolve immediately and miss the subsequent navigation; change the flow to wait
for a navigation event that must occur as a result of the submit: inside
submitSettingsForm use Promise.all to run page.waitForNavigation({ waitUntil:
'load' }) (or page.waitForNavigation({ waitUntil: 'domcontentloaded' }) if
preferred) in parallel with page.evaluate(...) that locates the button and calls
form.requestSubmit(btn) so the function only returns after the resulting
navigation completes; keep the existing checks (document.querySelector and
getElementById) and the function name submitSettingsForm unchanged.
In `@tests/E2E/setup/mu-plugins/e2e-mail-capture.php`:
- Around line 93-95: The current write uses file_put_contents($filename,
wp_json_encode($record, JSON_UNESCAPED_SLASHES)) which can produce
partially-written JSON visible to parallel readers; change to an atomic write:
serialize with wp_json_encode into a temp file (e.g. $filename . '.tmp' or using
tempnam), write the full payload to that temp file (ensuring write error
handling), fsync/close it, then rename/move it to $filename (atomic on most
filesystems) and handle errors from rename; update the code around the filename,
$record and file_put_contents references to use this temp-write-and-rename
approach so readers only ever see fully-written JSON files.
- Around line 70-80: The current code always appends an attachment entry to
$attachment_records even when the `@copy`($path, $copy) fails, resulting in
records pointing to non-existent files; update the flow around the copy() call
in the block that builds the attachment record (variables: $path, $copy, $base,
$size, $mime, $sha1, $attachment_records) to only append the record if the
destination file actually exists (e.g., check the boolean return of copy() or
re-check file_exists($copy)) and skip logging or adding the attachment when the
copy did not succeed.
In `@tests/E2E/setup/playwright.config.js`:
- Line 9: The testsBaseURL construction appends :${ports.wpTestsPort}
unconditionally which produces invalid URLs when process.env.WP_ENV_URL already
contains a port (e.g., http://localhost:8888); update the logic that builds
testsBaseURL (referencing testsBaseURL, process.env.WP_ENV_URL and
ports.wpTestsPort) to detect whether the provided WP_ENV_URL includes a port
(use the URL constructor or a regex) and only append :ports.wpTestsPort when no
port exists, otherwise use the WP_ENV_URL as-is (and still ensure a default
scheme/host when WP_ENV_URL is undefined).
In `@tests/E2E/tests/download-url/regenerate-disable.spec.js`:
- Around line 64-79: Remove the duplicate visibility assertion for the
activation button: after calling submitSettingsForm(page,
'#download-url-disable') keep a single expect for the activation button locator
('button[name="gform-settings-save"][value="download_url_enable"]') and the
expect that the URL input ('input[name="_gform_setting_hash"]') has count 0;
delete the repeated expect block that calls toBeVisible() a second time so only
one visibility check plus the toHaveCount(0) remain.
---
Nitpick comments:
In `@tests/E2E/helpers/test-helpers.js`:
- Around line 459-491: The three helper functions addNotification,
submitEntryTriggeringNotifications, and patchExportFeedMeta have inconsistent
async signatures—two are declared async while patchExportFeedMeta is
synchronous—causing some call sites to omit await and risking ordering bugs if
these become truly async; pick one approach and apply it consistently: either
(A) make patchExportFeedMeta async (returning a Promise) and update all call
sites (e.g., in logged-in-required.spec.js, transpose.spec.js,
disable-field.spec.js and any others) to await patchExportFeedMeta, or (B)
remove async from addNotification and submitEntryTriggeringNotifications and
return values synchronously so callers need not await; ensure you modify the
function declarations (addNotification, submitEntryTriggeringNotifications,
patchExportFeedMeta) and all callers to match the chosen synchronous or
asynchronous pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f265fe5-33c0-4d61-8f13-ac5f23f769e5
📒 Files selected for processing (17)
.claude/gravityexport-lite-happy-paths.md.gitignoretests/E2E/helpers/test-helpers.jstests/E2E/setup/mu-plugins/e2e-mail-capture.phptests/E2E/setup/playwright.config.jstests/E2E/setup/wp-env.config.jstests/E2E/tests/data-shaping/entry-notes.spec.jstests/E2E/tests/data-shaping/transpose.spec.jstests/E2E/tests/download-url/download-csv.spec.jstests/E2E/tests/download-url/download-xlsx.spec.jstests/E2E/tests/download-url/enable-download-url.spec.jstests/E2E/tests/download-url/regenerate-disable.spec.jstests/E2E/tests/fields/disable-field.spec.jstests/E2E/tests/fields/sort-order.spec.jstests/E2E/tests/notifications/attach-csv.spec.jstests/E2E/tests/notifications/attach-xlsx.spec.jstests/E2E/tests/permissions/logged-in-required.spec.js
…aits
CI failed both notification specs with "waitForCapturedEmail: no message
matched predicate within 10000ms. Last seen subjects: []". The endpoint
was healthy and returning valid JSON, but the inbox was always empty —
pre_wp_mail was firing yet the JSON record never landed on disk.
Root cause: wp-content/uploads/ is created by Apache (www-data, UID 33)
inside the tests-wordpress container. wp-env's tests-cli runs commands
as the host user (UID 501 locally, UID 3434 on CircleCI), so on CI the
host user cannot write into a www-data-owned uploads dir, and
file_put_contents fails silently inside the pre_wp_mail handler.
- Mu-plugin now writes to WP_CONTENT_DIR/e2e-mail-capture/ instead.
- wp-env.config.js adds an additionalMappings bind-mount from
tests/E2E/setup/mail-capture/ on the host (created mode 0777 before
wp-env starts) to wp-content/e2e-mail-capture/ in the containers.
Both containers see the same dir, host-owned, world-writable; the
cli writes and Apache reads cleanly regardless of host UID.
- gitignore the new host capture dir and a stray playwright chromium
profile dir.
While verifying the fix locally a separate intermittent surfaced in
goToFormSettings / submitSettingsForm — the title-only and
waitForLoadState('load') waits could both resolve against the wrong
page state:
- goToFormSettings now waits for form#gform-settings to be visible, not
just the page title (Gravity Forms renders the title before the
panel body in some interleavings).
- submitSettingsForm now waits for the actual POST to the
subview=gravityexport-lite URL rather than a load event that may
belong to the previous page, then waits for domcontentloaded on the
response.
CI reproduced two notification spec failures with "waitForCapturedEmail: no message matched predicate within 10000ms. Last seen subjects: []". SSH'd into the CI container and traced: - The bind-mounted capture dir works (cli writes, apache reads). - pre_wp_mail has exactly one callback registered (our closure). - send_notifications returns ["debug_notif"] but our pre_wp_mail filter never fires, and only gform_disable_notification (not gform_notification) fires inside GF. Root cause: GFAPI::send_notifications routes through the GF_Notifications background processor (Async\GF_Background_Process service provider). The processor pushes the work to a queue and dispatches on PHP shutdown via WP cron — in a one-shot wp-cli context that shutdown handler never delivers a worker request, so wp_mail itself is never called. Bypass the async path by calling GFCommon::send_notifications directly with the explicit list of notification ids matching the event. That's the synchronous code path GFAPI falls back to when async is disabled, and the only one that invokes wp_mail from within our wp eval process. Verified on the failing CI host: attach-csv and attach-xlsx pass in isolation; full 14-spec suite passes in 14s.
Six valid findings:
- helpers: findTestsContainer was passing { value: cachedWpContainer }
as a fresh object each call, so writes inside findContainer never
mutated the module-scoped cache and every call shelled out to
'docker ps'. Replaced both helpers with a single keyed-by-filter
containerCache map.
- mail capture mu-plugin: only append an attachment record when @copy
actually succeeds, otherwise we end up with records pointing at
files tests will fail to read.
- mail capture mu-plugin: write the JSON capture atomically (write to
.tmp then rename) so a parallel GET /mail can't observe a partially
flushed file.
- playwright.config: build testsBaseURL via URL() so a user-provided
WP_ENV_URL that already includes a port no longer produces invalid
results like http://localhost:8888:8801.
- regenerate-disable spec: removed the duplicate visibility assertion
after the disable click — one toBeVisible + one toHaveCount(0) is
enough.
- happy-paths doc: removed the stale workers: 1 references; the cap
was lifted in a prior commit once the connection-pool retry layer
was in.
One finding skipped:
- submitSettingsForm waitForLoadState('load') race — already fixed in
a prior commit; current code uses waitForRequest + waitForLoadState
('domcontentloaded').
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/gravityexport-lite-happy-paths.md:
- Around line 183-190: Update the conflicting runtime claims by choosing one
baseline and clarifying contexts: in the "Suite parallelism" paragraph replace
"the suite runs in roughly 35–50s on a typical dev box" with a clear label
(e.g., "35–50s on a warm local dev machine with parallel workers") or change the
"Final delivered suite" statement to specify its environment (e.g., "~90s on CI
or when run cold / 3× consecutively"). Edit the two phrases "Suite parallelism"
and "Final delivered suite (2026-05-11)" so they reference the same measured
scenario (local vs CI, warm vs cold) and include the chosen baseline next to the
runtime to remove ambiguity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba1da75a-e05c-4c44-82fa-f8edf1ca6a33
📒 Files selected for processing (5)
.claude/gravityexport-lite-happy-paths.mdtests/E2E/helpers/test-helpers.jstests/E2E/setup/mu-plugins/e2e-mail-capture.phptests/E2E/setup/playwright.config.jstests/E2E/tests/download-url/regenerate-disable.spec.js
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/E2E/tests/download-url/regenerate-disable.spec.js
- tests/E2E/setup/playwright.config.js
- tests/E2E/setup/mu-plugins/e2e-mail-capture.php
- tests/E2E/helpers/test-helpers.js
| 9. **Suite parallelism** — Several specs mutate global WordPress state (rewrite rules, `gf_addon_feed`, the shared capture inbox). An earlier iteration pinned `workers: 1` to mask intermittent failures, but the underlying flake turned out to be a connection-pool race (Apache KeepAlive idle timeout vs. Playwright APIRequestContext reuse) that was fixed by `fetchWithRetry` + `waitForCapturedEmail`. With the retry layer in place, `playwright.config.js` leaves the bootstrap default (`workers: '50%'`) in place — the suite runs in roughly 35–50s on a typical dev box. | ||
|
|
||
| --- | ||
|
|
||
| ## Final delivered suite (2026-05-11) | ||
|
|
||
| **11 happy-path specs + the existing `activation.spec.js` = 14 specs total. Full suite runs 3× consecutively green in ~90s each.** | ||
|
|
There was a problem hiding this comment.
Align the two runtime claims to avoid conflicting expectations.
Line 183 says the suite runs in roughly 35–50s, while Line 189 says full suite runs in ~90s each. Please reconcile these into one clear baseline (for example: local dev vs CI, cold vs warm containers).
Suggested doc edit
-9. **Suite parallelism** — Several specs mutate global WordPress state (rewrite rules, `gf_addon_feed`, the shared capture inbox). An earlier iteration pinned `workers: 1` to mask intermittent failures, but the underlying flake turned out to be a connection-pool race (Apache KeepAlive idle timeout vs. Playwright APIRequestContext reuse) that was fixed by `fetchWithRetry` + `waitForCapturedEmail`. With the retry layer in place, `playwright.config.js` leaves the bootstrap default (`workers: '50%'`) in place — the suite runs in roughly 35–50s on a typical dev box.
+9. **Suite parallelism** — Several specs mutate global WordPress state (rewrite rules, `gf_addon_feed`, the shared capture inbox). An earlier iteration pinned `workers: 1` to mask intermittent failures, but the underlying flake turned out to be a connection-pool race (Apache KeepAlive idle timeout vs. Playwright APIRequestContext reuse) that was fixed by `fetchWithRetry` + `waitForCapturedEmail`. With the retry layer in place, `playwright.config.js` leaves the bootstrap default (`workers: '50%'`) in place. Documented full-suite timing is ~90s per run (3 consecutive green runs); if you want, add a separate local warm-run range here explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/gravityexport-lite-happy-paths.md around lines 183 - 190, Update the
conflicting runtime claims by choosing one baseline and clarifying contexts: in
the "Suite parallelism" paragraph replace "the suite runs in roughly 35–50s on a
typical dev box" with a clear label (e.g., "35–50s on a warm local dev machine
with parallel workers") or change the "Final delivered suite" statement to
specify its environment (e.g., "~90s on CI or when run cold / 3×
consecutively"). Edit the two phrases "Suite parallelism" and "Final delivered
suite (2026-05-11)" so they reference the same measured scenario (local vs CI,
warm vs cold) and include the chosen baseline next to the runtime to remove
ambiguity.
The Run E2E tests step has a 3-attempt retry around npm run tests:e2e:setup.
Each retry runs 'rm -rf /home/circleci/.wp-env' to drop wp-env state before
trying again. That cleanup runs as the circleci user, but the bind-mounted
mu-plugin files inside the state dir
(.wp-env/<hash>/wp-content/mu-plugins/{e2e-fixtures,e2e-mail-capture}.php)
are created by Apache inside the container as uid 0, so the rm fails with
"Permission denied". Once that happens the retry can't reset state and
every attempt fails the same way — which is what we hit on pipeline 324
(transient mysql-readiness flake on the first try, then nothing else can
recover).
Stop wp-env before the cleanup so containers release the bind-mount, then
sudo rm the state dir so we can remove root-owned files. sudo is available
on CircleCI Linux machine runners.
Verified on the failing CI host: plain rm reproduces the original
permission-denied error; the new sequence (wp-env stop, sudo rm -rf, fresh
setup) restores wp-env cleanly and the full 14-spec suite passes in ~16s.
Summary
activation.spec.jsthe suite is 14 specs.tests/E2E/helpers/test-helpers.jswrapper around@gravitykit/e2e-bootstrapwith GravityExport-specific helpers, and a smalle2e-mail-capturemu-plugin that short-circuitswp_mailand exposes the captured payloads + attachments over REST. Discovery doc and rationale live at.claude/gravityexport-lite-happy-paths.md.Test plan
npm run tests:e2e:setup(only on a fresh clone — re-uses an existingwp-envinstance otherwise)npm run tests:e2e:run— full suite greennpx playwright test --config=tests/E2E/setup/playwright.config.js tests/E2E/tests/notifications --repeat-each 5to confirm the notification specs are stablerun_e2e_testsjob green on this branch💾 Build file (dfd260a).
Summary by CodeRabbit
Tests
Chores
Documentation