ci(e2e): run the 9 cypress specs that existed but were never invoked#1280
Conversation
The e2e workflow listed only 6 of the 16 spec files in
tests/e2e/cypress/integration/. The other 10 were committed and
syntactically valid but never executed in CI, so any regression they
guarded against could slip through unnoticed.
Audit findings (16 specs total):
Already in CI (6, unchanged):
- 000-setup.spec.js
- 010-manual-checkout-flow.spec.js
- 020-free-trial-flow.spec.js
- 030-stripe-checkout-flow.spec.js (gated, STRIPE_TEST_SK_KEY)
- 040-stripe-renewal-flow.spec.js (gated, STRIPE_TEST_SK_KEY)
- 065-sso-redirect-loop.spec.js
Added to CI in this change (9):
- login.spec.js (sanity)
- mail.spec.js (sanity)
- wizard.spec.js (drives Setup Wizard UI)
- 011-password-reset-subsite-domain.spec.js (regression: PR #1169)
- 030-modal-form-error-handling.spec.js (UI: AJAX error handling)
- 050-password-strength-enforcement.spec.js (UI: zxcvbn scoring)
- 060-sso-cross-domain.spec.js (SSO: domain mapping)
- 066-sso-bootstrap-race.spec.js (regression: PR #1185)
- 035-paypal-checkout-flow.spec.js (gated, new
PAYPAL_SANDBOX_*)
Intentionally excluded (1):
- installation.spec.js — single ‘cy.visit(/wp-admin/)’ call with
no assertions; doesn’t actually test anything. Recommend deletion
in a follow-up; not removed here to keep this change ci-only.
Why wizard.spec.js matters specifically: 000-setup.spec.js bypasses
the Setup Wizard UI by calling the installer + setup-finished flag
directly. The path Setup_Wizard_Admin_Page::handle_save_settings ->
Settings::save_settings -> Field::set_value -> validate_textarea_field
was therefore never exercised in CI. The recent
fix/textarea-array-coercion fix added unit-test coverage for the
validator itself; running wizard.spec.js now covers the integration
path.
Step organisation (after the existing ‘Run Setup Test’):
1. Sanity Tests — login, mail (smoke)
2. Wizard Test — wizard
3. Regression & UI — 011, 030, 050, 066
4. Checkout Tests — 010, 020 (existing)
5. SSO Tests — 060 (new), 065 (was: only 065)
6. Stripe Tests — 030, 040 (existing, gated)
7. PayPal Tests — 035 (new, gated on PAYPAL_SANDBOX_*)
All new groups follow the existing pattern: ‘set +e’ + per-spec loop +
aggregated failure count + final exit 1 if any failed, so a single
flake doesn’t mask other failures.
PayPal step mirrors the Stripe gating: if PAYPAL_SANDBOX_CLIENT_SECRET
is unavailable (typical for fork PRs) the step prints the same
diagnostic message Stripe uses and exits 0. The PAYPAL_SANDBOX_* repo
secrets are not currently configured upstream, so PayPal will skip
until they are added; Stripe continues to run.
Verification performed in this change:
- YAML parse: python3 -c 'import yaml; yaml.safe_load(open(path))' OK
- Per-spec node --check on all 9 added specs OK
- All referenced fixture .php files exist in tests/e2e/cypress/fixtures
- Inventory diff: workflow now references 15/16 specs
Local Cypress execution is not attempted in this commit because each
matrix variant requires a fresh wp-env (Docker download + WP install
+ plugin activation + multi-spec run) and would take longer than the
CI run that gates the merge. CI is the canonical verification surface
for this change.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR expands the E2E Cypress test pipeline by adding six new test execution steps organized in two sequential phases: foundation tests (sanity, wizard UI, post-wizard cleanup) that establish expected application state, followed by extended test coverage (regression/UI, SSO, and conditional PayPal tests) that all use consistent failure-aggregation patterns via Cypress exit codes. ChangesE2E Cypress Test Pipeline Expansion
Sequence Diagram(s)sequenceDiagram
participant Workflow as E2E Workflow
participant Sanity as Sanity Tests
participant Wizard as Wizard UI Test
participant Cleanup as Cleanup Fixtures
participant Regression as Regression Tests
participant SSO as SSO Tests
participant PayPal as PayPal Tests
Workflow->>Sanity: Run login + mail specs (set +e)
activate Sanity
Sanity-->>Workflow: aggregate exit code, fail if any spec fails
deactivate Sanity
Workflow->>Wizard: Run wizard UI spec
activate Wizard
Wizard-->>Workflow: exit code
deactivate Wizard
Workflow->>Cleanup: Run WP eval-file fixtures
activate Cleanup
Cleanup-->>Cleanup: Disable custom login page
Cleanup-->>Workflow: exit code
deactivate Cleanup
Workflow->>Regression: Loop multiple regression/UI specs
activate Regression
Regression->>Regression: Tally Cypress exit codes
Regression-->>Workflow: exit non-zero if any fail
deactivate Regression
Workflow->>SSO: Loop cross-domain + redirect-loop specs
activate SSO
SSO->>SSO: Aggregate Cypress exit codes
SSO-->>Workflow: exit non-zero if any fail
deactivate SSO
Workflow->>PayPal: Check PAYPAL_SANDBOX_CLIENT_SECRET exists
alt Secrets Present
PayPal->>PayPal: Inject sandbox credentials via --env
PayPal->>PayPal: Run PayPal checkout spec
PayPal-->>Workflow: exit non-zero if spec fails
else Secrets Missing
PayPal-->>Workflow: exit 0 (skip)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 @.github/workflows/e2e.yml:
- Around line 437-452: The conditional that gates PayPal tests only checks
PAYPAL_SANDBOX_CLIENT_SECRET; change the if-condition to test both
PAYPAL_SANDBOX_CLIENT_ID and PAYPAL_SANDBOX_CLIENT_SECRET (e.g., test -z
"$PAYPAL_SANDBOX_CLIENT_ID" || -z "$PAYPAL_SANDBOX_CLIENT_SECRET" or a combined
-z check) so the job skips when either secret is missing, and update the echo
messages inside that block to mention both PAYPAL_SANDBOX_CLIENT_ID and
PAYPAL_SANDBOX_CLIENT_SECRET to reflect the new gating logic.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce75ec2a-1d6d-4f67-8384-e7eb37cbeb4e
📒 Files selected for processing (3)
.github/workflows/e2e.ymlTODO.mdtodo/PLANS.md
| if [ -z "$PAYPAL_SANDBOX_CLIENT_SECRET" ]; then | ||
| echo "⏭️ Skipping PayPal tests: PAYPAL_SANDBOX_CLIENT_SECRET not available in this run." | ||
| echo "" | ||
| echo " The repo secrets PAYPAL_SANDBOX_CLIENT_ID and PAYPAL_SANDBOX_CLIENT_SECRET" | ||
| echo " must be configured for these tests to run. GitHub Actions does NOT expose" | ||
| echo " secrets to workflows triggered by pull_request events from forked" | ||
| echo " repositories — this is a security feature, not a misconfiguration." | ||
| echo "" | ||
| echo " PayPal tests will run automatically when:" | ||
| echo " • this branch is merged to main (push event), or" | ||
| echo " • the PR is opened from a branch in the upstream Ultimate-Multisite repo" | ||
| echo " AND the PAYPAL_SANDBOX_* secrets have been configured at the repo level." | ||
| echo "" | ||
| echo " To verify locally, run: gh secret list --repo Ultimate-Multisite/ultimate-multisite" | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/e2e.yml"
rg -n 'if \[ -z "\$PAYPAL_SANDBOX_CLIENT_ID" \] \|\| \[ -z "\$PAYPAL_SANDBOX_CLIENT_SECRET" \ ]; then|if \[ -z "\$PAYPAL_SANDBOX_CLIENT_SECRET" \ ]; then' "$FILE"Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/e2e.yml"
# Show the exact gate block around the referenced line range
nl -ba "$FILE" | sed -n '420,470p'
# Find all occurrences of PayPal secret variables
rg -n 'PAYPAL_SANDBOX_CLIENT_(ID|SECRET)' "$FILE" || true
# Look for the specific conditional check patterns (simple, no regex escapes)
rg -n 'if \[ -z "\$PAYPAL_SANDBOX_CLIENT_' "$FILE" || trueRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 124
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/e2e.yml"
echo "=== lines 420-470 (with numbers) ==="
awk 'NR>=420 && NR<=470 {printf "%d\t%s\n", NR, $0}' "$FILE"
echo
echo "=== occurrences of both secret names ==="
rg -n 'PAYPAL_SANDBOX_CLIENT_(ID|SECRET)' "$FILE" || true
echo
echo "=== occurrences of the specific skip gate (secret missing) ==="
rg -n 'Skipping PayPal tests|PAYPAL_SANDBOX_CLIENT_SECRET not available|PAYPAL_SANDBOX_CLIENT_ID.*not available|if \[ -z "\$PAYPAL_SANDBOX_CLIENT_' "$FILE" || trueRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 3368
Gate PayPal tests on both required secrets
.github/workflows/e2e.yml only skips when PAYPAL_SANDBOX_CLIENT_SECRET is empty; if PAYPAL_SANDBOX_CLIENT_ID is missing (empty) but the secret is present, the job will proceed and fail the tests instead of skipping.
Suggested fix
- if [ -z "$PAYPAL_SANDBOX_CLIENT_SECRET" ]; then
+ if [ -z "$PAYPAL_SANDBOX_CLIENT_ID" ] || [ -z "$PAYPAL_SANDBOX_CLIENT_SECRET" ]; then
echo "⏭️ Skipping PayPal tests: PAYPAL_SANDBOX_CLIENT_SECRET not available in this run."(Also update the skip message to reference both PAYPAL_SANDBOX_CLIENT_ID and PAYPAL_SANDBOX_CLIENT_SECRET so it matches the new condition.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ -z "$PAYPAL_SANDBOX_CLIENT_SECRET" ]; then | |
| echo "⏭️ Skipping PayPal tests: PAYPAL_SANDBOX_CLIENT_SECRET not available in this run." | |
| echo "" | |
| echo " The repo secrets PAYPAL_SANDBOX_CLIENT_ID and PAYPAL_SANDBOX_CLIENT_SECRET" | |
| echo " must be configured for these tests to run. GitHub Actions does NOT expose" | |
| echo " secrets to workflows triggered by pull_request events from forked" | |
| echo " repositories — this is a security feature, not a misconfiguration." | |
| echo "" | |
| echo " PayPal tests will run automatically when:" | |
| echo " • this branch is merged to main (push event), or" | |
| echo " • the PR is opened from a branch in the upstream Ultimate-Multisite repo" | |
| echo " AND the PAYPAL_SANDBOX_* secrets have been configured at the repo level." | |
| echo "" | |
| echo " To verify locally, run: gh secret list --repo Ultimate-Multisite/ultimate-multisite" | |
| exit 0 | |
| fi | |
| if [ -z "$PAYPAL_SANDBOX_CLIENT_ID" ] || [ -z "$PAYPAL_SANDBOX_CLIENT_SECRET" ]; then | |
| echo "⏭️ Skipping PayPal tests: PAYPAL_SANDBOX_CLIENT_SECRET not available in this run." | |
| echo "" | |
| echo " The repo secrets PAYPAL_SANDBOX_CLIENT_ID and PAYPAL_SANDBOX_CLIENT_SECRET" | |
| echo " must be configured for these tests to run. GitHub Actions does NOT expose" | |
| echo " secrets to workflows triggered by pull_request events from forked" | |
| echo " repositories — this is a security feature, not a misconfiguration." | |
| echo "" | |
| echo " PayPal tests will run automatically when:" | |
| echo " • this branch is merged to main (push event), or" | |
| echo " • the PR is opened from a branch in the upstream Ultimate-Multisite repo" | |
| echo " AND the PAYPAL_SANDBOX_* secrets have been configured at the repo level." | |
| echo "" | |
| echo " To verify locally, run: gh secret list --repo Ultimate-Multisite/ultimate-multisite" | |
| exit 0 | |
| fi |
🤖 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 @.github/workflows/e2e.yml around lines 437 - 452, The conditional that gates
PayPal tests only checks PAYPAL_SANDBOX_CLIENT_SECRET; change the if-condition
to test both PAYPAL_SANDBOX_CLIENT_ID and PAYPAL_SANDBOX_CLIENT_SECRET (e.g.,
test -z "$PAYPAL_SANDBOX_CLIENT_ID" || -z "$PAYPAL_SANDBOX_CLIENT_SECRET" or a
combined -z check) so the job skips when either secret is missing, and update
the echo messages inside that block to mention both PAYPAL_SANDBOX_CLIENT_ID and
PAYPAL_SANDBOX_CLIENT_SECRET to reflect the new gating logic.
|
Performance Test Results Performance test results for 9d04976 are in 🛎️! Note: the numbers in parentheses show the difference to the previous (baseline) test run. Differences below 2% or 0.5 in absolute values are not shown. URL:
|
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
The wizard's Default Content step creates a Login page and points Ultimate Multisite's custom-login feature at it, which redirects /wp-login.php -> /login/. The custom page has no #rememberme checkbox, so cy.loginByForm() fails in every subsequent spec's session-setup hook. 000-setup.spec.js already calls setup-disable-custom-login.php defensively in its before() hook for the same reason; this CI step runs the same fixture right after wizard.spec.js so the regression, checkout, SSO, and gated gateway groups inherit a clean login state. Observed on PR #1280 cypress matrix run 26456938227 where 030-modal-form-error-handling failed in its session-setup hook with 'Expected to find element: #rememberme, but never found it', cascading into every subsequent step being skipped.
SummaryThe Audit of all 16 specs
Why
|
| Check | Result |
|---|---|
python3 -c "import yaml; yaml.safe_load(open('.github/workflows/e2e.yml'))" |
✅ valid YAML |
node --check <spec> for each of the 9 added specs |
✅ all parse |
Fixture file existence: setup-password-reset-subsite.php, setup-sso-bootstrap-race.php, setup-sso-test.php, set-password-strength.php, setup-paypal-gateway.php |
✅ all present |
| Inventory grep: specs referenced in workflow vs specs in repo | 15/16 (only installation.spec.js excluded, intentionally) |
Existing CI step pattern matched (set +e + per-spec loop + aggregated exit) |
✅ |
What is not verified
Local Cypress execution was not attempted for this change. Each matrix variant requires a fresh npm run env:start:test (Docker image download + WP install + plugin activation), which takes longer than the CI run that gates this merge. Running it locally would also use a different PHP version and OS than the matrix in CI. CI is the canonical verification surface for this change — please review the CI run on this PR.
If any spec fails in CI, the per-spec loop will report exactly which one(s); the existing Dump container logs on setup failure step will also catch wp-env startup issues. I'll iterate as needed.
Risk
- Test runtime: the matrix
(php: 8.1, 8.2)× the new specs adds runtime. Per-speccypress runcalls each pay the cypress-startup cost, so total runtime grows by roughly the per-spec count, not by the actual test work. If the 45-minutetimeout-minutesbecomes tight, the obvious next step is to consolidate into fewercypress runinvocations (single--specglob). - Test interaction:
wizard.spec.jsclearsNETWORK_OPTION_SETUP_FINISHEDand re-marks it at the end. It runs after000-setupand before the regression / checkout groups; the wizard's "Default Content" step is designed to skip already-created items, so it should not conflict with the test product000-setupcreated. If it does conflict, the failure will surface in010-manual-checkout-flow.spec.jsand we can reorder. - No behaviour change to any test spec or plugin code — workflow-only.
Follow-ups (not in this PR)
- Delete
tests/e2e/cypress/integration/installation.spec.js(or add real assertions to it). - Consider adding a CI step (or a lint) that fails when a
*.spec.jsexists undertests/e2e/cypress/integration/but isn't referenced ine2e.yml— would have caught this gap automatically.
Merged via PR #1280 to main.
Merged by deterministic merge pass (pulse-wrapper.sh).
aidevops.sh v3.19.5 spent 47s on this as a headless bash routine.
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
superdav42
added
the
review-feedback-scanned
1 participant
Summary
The
.github/workflows/e2e.ymlworkflow listed only 6 of the 16 spec files intests/e2e/cypress/integration/. The other 10 were committed, syntactically valid, and well-written — but never executed in CI, so any regression they guarded against could slip through unnoticed.This change adds 9 of those 10 specs to the workflow. The 10th (
installation.spec.js) is intentionally excluded because it has no assertions — see "Excluded spec" below.Audit of all 16 specs
000-setup.spec.js010-manual-checkout-flow.spec.js011-password-reset-subsite-domain.spec.js020-free-trial-flow.spec.js030-modal-form-error-handling.spec.js030-stripe-checkout-flow.spec.js035-paypal-checkout-flow.spec.jsPAYPAL_SANDBOX_*secrets040-stripe-renewal-flow.spec.js050-password-strength-enforcement.spec.js060-sso-cross-domain.spec.js065-sso-redirect-loop.spec.js066-sso-bootstrap-race.spec.jsinstallation.spec.jslogin.spec.jsmail.spec.jswizard.spec.jsWhy
wizard.spec.jsmatters specifically000-setup.spec.jsbypasses the Setup Wizard UI entirely — it calls the installer directly viaCore_Installer::_install_database_tables()and flipsNETWORK_OPTION_SETUP_FINISHED, then uses fixture scripts to configure the test product, checkout form, gateway, and email settings.The path actually exercised by a real user —
Setup_Wizard_Admin_Page::handle_save_settings→Settings::save_settings→Field::set_value→validate_textarea_field— was therefore never exercised in CI. That's exactly the path that produces theaddslashes()/TypeErrorfatal when a textarea field receives a non-string value (e.g. a previously-corrupted DB value falling through as$existing_value).Running
wizard.spec.jsnow covers the wizard form integration path. A unit-level guard for the validator itself is in branchfix/textarea-array-coercion.Step organisation
After the existing "Run Setup Test (Must Run First)" step, the workflow now runs:
login,mail(smoke)wizard011,030-modal-form-error-handling,050,066010,020(existing, unchanged)060(new),065(was: only065)030-stripe-checkout-flow,040(existing, gated)035(new, gated onPAYPAL_SANDBOX_*)All new groups follow the existing pattern:
set +e+ per-spec loop + aggregated failure count + finalexit 1if any failed, so a single flake doesn't mask other failures in the same group.PayPal gating
The PayPal step mirrors the Stripe gating exactly: if
PAYPAL_SANDBOX_CLIENT_SECRETis empty (which is the case for fork PRs, and currently also for upstream because the secrets are not configured yet), the step prints a diagnostic message and exits 0.gh secret list --repo Ultimate-Multisite/ultimate-multisiteconfirmsPAYPAL_SANDBOX_*are not yet present. The PayPal tests will skip until they are added; Stripe continues to run on push to main.Excluded spec
tests/e2e/cypress/integration/installation.spec.jsis not added to CI:The body is a single
cy.visit("/wp-admin/")with no assertions. Itsit()title claims to check an error message that never gets verified. Adding it to CI would consume runtime without testing anything. Recommend deleting the file in a follow-up; not removed in this PR to keep the change CI-only.Verification performed
python3 -c "import yaml; yaml.safe_load(open('.github/workflows/e2e.yml'))"node --check <spec>for each of the 9 added specssetup-password-reset-subsite.php,setup-sso-bootstrap-race.php,setup-sso-test.php,set-password-strength.php,setup-paypal-gateway.phpinstallation.spec.jsexcluded, intentionally)set +e+ per-spec loop + aggregated exit)What is not verified
Local Cypress execution was not attempted for this change. Each matrix variant requires a fresh
npm run env:start:test(Docker image download + WP install + plugin activation), which takes longer than the CI run that gates this merge. Running it locally would also use a different PHP version and OS than the matrix in CI. CI is the canonical verification surface for this change — please review the CI run on this PR.If any spec fails in CI, the per-spec loop will report exactly which one(s); the existing
Dump container logs on setup failurestep will also catch wp-env startup issues. I'll iterate as needed.Risk
(php: 8.1, 8.2)× the new specs adds runtime. Per-speccypress runcalls each pay the cypress-startup cost, so total runtime grows by roughly the per-spec count, not by the actual test work. If the 45-minutetimeout-minutesbecomes tight, the obvious next step is to consolidate into fewercypress runinvocations (single--specglob).wizard.spec.jsclearsNETWORK_OPTION_SETUP_FINISHEDand re-marks it at the end. It runs after000-setupand before the regression / checkout groups; the wizard's "Default Content" step is designed to skip already-created items, so it should not conflict with the test product000-setupcreated. If it does conflict, the failure will surface in010-manual-checkout-flow.spec.jsand we can reorder.Follow-ups (not in this PR)
tests/e2e/cypress/integration/installation.spec.js(or add real assertions to it).*.spec.jsexists undertests/e2e/cypress/integration/but isn't referenced ine2e.yml— would have caught this gap automatically.Summary by CodeRabbit