Skip to content

Add E2E happy-path Playwright suite#239

Merged
Mwalek merged 8 commits into
developfrom
tests/happy-path
May 12, 2026
Merged

Add E2E happy-path Playwright suite#239
Mwalek merged 8 commits into
developfrom
tests/happy-path

Conversation

@Mwalek
Copy link
Copy Markdown
Contributor

@Mwalek Mwalek commented May 11, 2026

Summary

  • Adds 11 Playwright happy-path specs covering the Lite-side behavior: download URL lifecycle (enable, .xlsx default, .csv extension, regenerate/disable), admin configuration (disable field, sort order, logged-in restriction), data shaping (transpose, entry notes), and notification attachment (CSV + XLSX). With the existing activation.spec.js the suite is 14 specs.
  • Adds the supporting infrastructure: a tests/E2E/helpers/test-helpers.js wrapper around @gravitykit/e2e-bootstrap with GravityExport-specific helpers, and a small e2e-mail-capture mu-plugin that short-circuits wp_mail and exposes the captured payloads + attachments over REST. Discovery doc and rationale live at .claude/gravityexport-lite-happy-paths.md.
  • Out of scope (and documented in the discovery doc): per-field custom column labels (Pro-only UI), combined multi-form report (no Lite admin UI), URL-search filters (param syntax not ground-truthed).

Test plan

  • npm run tests:e2e:setup (only on a fresh clone — re-uses an existing wp-env instance otherwise)
  • npm run tests:e2e:run — full suite green
  • Spot-check npx playwright test --config=tests/E2E/setup/playwright.config.js tests/E2E/tests/notifications --repeat-each 5 to confirm the notification specs are stable
  • CircleCI run_e2e_tests job green on this branch

💾 Build file (dfd260a).

Summary by CodeRabbit

  • Tests

    • Added a 14-spec E2E happy-path suite for GravityExport Lite: download URLs (XLSX/CSV), URL regen/disable, CSV extension handling, transpose mode, field enable/disable and ordering, entry notes, notification attachments (CSV/XLSX), and permission gating.
  • Chores

    • Added mail-capture test support, Playwright/wp-env setup improvements, host mail-capture mapping, and more robust CI cleanup.
  • Documentation

    • Added a detailed "Happy Paths" blueprint describing Lite behaviors and prioritized E2E coverage.

Review Change Stack

Mwalek added 4 commits May 11, 2026 13:31
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.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 11, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Adds 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.

Changes

GravityExport Lite E2E Test Suite

Layer / File(s) Summary
Test Planning & Documentation
.claude/gravityexport-lite-happy-paths.md
Comprehensive blueprint defining Lite-only scope (P0–P6), mapped specs, Phase 2 decisions (dropped multi-form report, URL filters, per-field labels), implementation choices, and final spec list (11 new + activation = 14).
Project Configuration
.gitignore
Adds ignores for E2E artifacts: tests/E2E/setup/mail-capture/, .playwright-mcp/, playwright-transform-*, and playwright_chromiumdev_profile-*.
Mail Capture Infrastructure
tests/E2E/setup/mu-plugins/e2e-mail-capture.php
mu-plugin intercepts wp_mail, copies readable attachments to stable attachments/ paths, writes per-message JSON captures, and exposes GET /gk-e2e/v1/mail and DELETE /gk-e2e/v1/mail guarded by X-E2E-TEST-TOKEN.
E2E Setup Configuration
tests/E2E/setup/wp-env.config.js, tests/E2E/setup/playwright.config.js, .circleci/config.yml
Creates host mail-capture directory and bind-mount mapping into wp-env; Playwright config derives use.baseURL and webServer.url from wp-env ports so tests target the test instance; CI cleanup made more robust for wp-env artifacts.
E2E Test Helpers
tests/E2E/helpers/test-helpers.js
Exports browser helpers (navigate, enable/read/save download URL, reliable form submit), HTTP utilities (fetchDownload returns status/headers/body), CSV parser, mail-capture client with retry/polling, attachment byte reading via container exec, container discovery, wpEval, notification/entry creators, and patchExportFeedMeta.
Core Download URL Tests
tests/E2E/tests/download-url/enable-download-url.spec.js, download-xlsx.spec.js, download-csv.spec.js, regenerate-disable.spec.js
Validates enabling download URLs, default XLSX and CSV (extension swap) downloads with correct headers/body, regenerate/disable lifecycle, and old-URL invalidation.
Field Configuration Tests
tests/E2E/tests/fields/disable-field.spec.js, sort-order.spec.js
Verifies disabling a field removes its CSV column while preserving rows, and feed-meta enabled-field ordering controls CSV column order.
Permission Control Test
tests/E2E/tests/permissions/logged-in-required.spec.js
Validates "logged-in required" denies anonymous downloads and still allows authenticated admin downloads.
Data Shaping Tests
tests/E2E/tests/data-shaping/transpose.spec.js, entry-notes.spec.js
Tests transpose mode (entries⇄fields dimension swap) and entry notes column inclusion with injected note assertions.
Notification Attachment Tests
tests/E2E/tests/notifications/attach-csv.spec.js, attach-xlsx.spec.js
Verifies single-entry CSV/XLSX attachments are produced for notifications, asserting attachment count, filename extension, size, and binary magic bytes where applicable.

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly Related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'Add E2E happy-path Playwright suite' directly and clearly summarizes the main change: introduction of a new Playwright E2E test suite with happy-path specs and supporting infrastructure.
Docstring Coverage ✅ Passed Docstring coverage is 95.65% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch tests/happy-path

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.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 7

🧹 Nitpick comments (1)
tests/E2E/helpers/test-helpers.js (1)

459-491: 💤 Low value

patchExportFeedMeta / addNotification / submitEntryTriggeringNotifications have inconsistent async signatures.

All three call into the synchronous wpEval (which uses execFileSync) and do no real awaiting:

  • addNotification and submitEntryTriggeringNotifications are declared async (wrapping the return in a Promise).
  • patchExportFeedMeta (line 528) is declared sync and is invoked without await across the specs.

Both styles "work", but the inconsistency leaks into callers — e.g., logged-in-required.spec.js line 53 calls patchExportFeedMeta(...) without await, while transpose.spec.js / disable-field.spec.js do the same, yet addNotification must be awaited. If any of these ever become genuinely async (e.g., switched to execFile or 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

📥 Commits

Reviewing files that changed from the base of the PR and between cfa3155 and 6ceaf4f.

📒 Files selected for processing (17)
  • .claude/gravityexport-lite-happy-paths.md
  • .gitignore
  • tests/E2E/helpers/test-helpers.js
  • tests/E2E/setup/mu-plugins/e2e-mail-capture.php
  • tests/E2E/setup/playwright.config.js
  • tests/E2E/setup/wp-env.config.js
  • tests/E2E/tests/data-shaping/entry-notes.spec.js
  • tests/E2E/tests/data-shaping/transpose.spec.js
  • tests/E2E/tests/download-url/download-csv.spec.js
  • tests/E2E/tests/download-url/download-xlsx.spec.js
  • tests/E2E/tests/download-url/enable-download-url.spec.js
  • tests/E2E/tests/download-url/regenerate-disable.spec.js
  • tests/E2E/tests/fields/disable-field.spec.js
  • tests/E2E/tests/fields/sort-order.spec.js
  • tests/E2E/tests/notifications/attach-csv.spec.js
  • tests/E2E/tests/notifications/attach-xlsx.spec.js
  • tests/E2E/tests/permissions/logged-in-required.spec.js

Comment thread .claude/gravityexport-lite-happy-paths.md Outdated
Comment thread tests/E2E/helpers/test-helpers.js
Comment thread tests/E2E/helpers/test-helpers.js Outdated
Comment thread tests/E2E/setup/mu-plugins/e2e-mail-capture.php Outdated
Comment thread tests/E2E/setup/mu-plugins/e2e-mail-capture.php
Comment thread tests/E2E/setup/playwright.config.js Outdated
Comment thread tests/E2E/tests/download-url/regenerate-disable.spec.js
Mwalek added 3 commits May 11, 2026 16:32
…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').
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between e2f09bb and 4019c7e.

📒 Files selected for processing (5)
  • .claude/gravityexport-lite-happy-paths.md
  • tests/E2E/helpers/test-helpers.js
  • tests/E2E/setup/mu-plugins/e2e-mail-capture.php
  • tests/E2E/setup/playwright.config.js
  • tests/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

Comment on lines +183 to +190
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.**

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.
@Mwalek Mwalek merged commit 11e672a into develop May 12, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant