fix(e2e): consolidate frontend E2E to Cypress and restore CI (#967) - #984
fix(e2e): consolidate frontend E2E to Cypress and restore CI (#967)#984DevHusnainAi wants to merge 34 commits into
Conversation
Playwright here backs Python embeddings (prompt_client), not frontend e2e. The Cypress migration replaced frontend e2e but this line was removed by mistake, breaking `make install-python`. Restore it per reviewer feedback on OWASP#774.
# Conflicts: # yarn.lock
The readiness loop broke on success but fell through to yarn test:e2e even when Flask never came up, causing confusing Cypress failures. Add a post-loop probe that exits with a clear error if the server is still unreachable. Addresses CodeRabbit review on OWASP#774.
…ings Clarify Makefile and AGENTS.md so Cypress e2e migration PRs do not drop playwright install from install-python (embeddings/scraping in prompt_client). Co-authored-by: Cursor <cursoragent@cursor.com>
Build the e2e SQLite schema from the ORM models (create_all) instead of migrate-upgrade: migrations are the Postgres path and omit columns the models added without a migration (e.g. cre.document_metadata), so a migrate-built cache is incomplete. Add a make e2e-db target and switch the workflow to install-deps + e2e-db, dropping migrate-upgrade and the unused playwright install (Cypress ships its own browser). Raise the job timeout to cover the full upstream_sync graph download. Refs OWASP#967
Add Cypress specs covering the historical Jest/Puppeteer coverage: no-results and free-text search, ASVS standard browse, CRE lookup by id and case-insensitive filter, and smartlink internal/external redirects. Each spec asserts on real seeded data so it fails on an empty database. Refs OWASP#967
Frontend e2e now runs on Cypress; drop the unused puppeteer and jest-puppeteer devDependencies and their transitive lockfile entries. Refs OWASP#967
Note how to seed the local database (make e2e-db) and run the Cypress suite (make e2e, yarn cypress:open) in README and AGENTS.md. Refs OWASP#967
|
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:
Summary by CodeRabbit
WalkthroughThe PR migrates frontend E2E testing from Puppeteer/Jest to Cypress. It adds deterministic fixtures, Cypress coverage, robust Flask and database orchestration, updated CI execution, Python Playwright setup, and local development instructions. ChangesCypress E2E migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 14-15: Update the checkout step using actions/checkout@v4 to set
persist-credentials to false, preventing the GitHub token from being stored in
local Git configuration while leaving the rest of the workflow unchanged.
In `@Makefile`:
- Around line 89-101: Update the Makefile recipe containing the Flask startup
and e2e test commands so all setup, environment exports, virtualenv activation,
background PID capture, trap registration, readiness polling, and test execution
run in one shell session. Add recipe line continuations and appropriate command
separators while preserving the existing startup, failure, and cleanup behavior.
In `@README.md`:
- Around line 362-363: Update the README comment for the make e2e-db command to
state that it rebuilds the local SQLite cache on every invocation and discards
the existing cache; remove the misleading “one-time” wording while leaving the
make e2e instruction unchanged.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: f9880549-6dce-4a44-b898-62a462cdd4cc
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
.github/workflows/e2e.ymlAGENTS.mdMakefileREADME.mdapplication/frontend/src/test/basic-e2e.test.tscypress.config.jscypress.jsoncypress/e2e/cre.cy.jscypress/e2e/search.cy.jscypress/e2e/smartlink.cy.jscypress/e2e/smoke.cy.jscypress/e2e/standard.cy.jspackage.json
💤 Files with no reviewable changes (2)
- application/frontend/src/test/basic-e2e.test.ts
- cypress.json
Set persist-credentials: false on the e2e checkout (the job only reads, never pushes) and correct the README: make e2e-db rebuilds and replaces the local SQLite cache on every run, it is not a one-time step. Refs OWASP#967
|
Hey team! @northdpole @robvanderveer @Pa04rth @paoga87 Just a quick heads-up that this migration is ready for your review. All 5 CI/CD pipelines are passing completely green, the legacy test suite has been successfully ported over to Cypress, and I've addressed the initial CodeRabbit feedback regarding workflow credentials and documentation updates.Let me know if you need any adjustments or if this is good to land! Thanks. |
northdpole
left a comment
northdpole
left a comment
There was a problem hiding this comment.
Thanks for consolidating the Cypress migration and for the careful create_all vs migrate-upgrade diagnosis — that SQLite/CI schema fix is the right call.
Accepting your offer on a lighter seed (see inline on e2e-db / upstream_sync). Once that lands and CI is green on this PR, this looks mergeable. Please also close #774 / #523 as superseded when we merge.
|
Ok thanks. |
SearchName.tsx was the only page-level route missing the window.scrollTo(0, 0) on mount that every sibling page (Standard, BrowseRootCres, CRE, ...) already has. Under the minimal e2e fixture, the leftover scroll from reaching the homepage search bar (below a sticky header) carried into the client-side route change, clipping the "Results matching" heading inside #mount's overflow-y: auto and failing smoke.cy.js in CI. standard.cy.js asserted the ASVS/V13.2.5 external reference link was visible before expanding its accordion, but DocumentNode's NestedView keeps that content in a display: none div until expanded — reorder the click before the visibility assertion. Verified with a real local Cypress run (yarn build + make e2e-db + make e2e): all 5 specs / 11 tests pass.
|
Hey @northdpole — thanks for the detailed review, I think this addresses everything you flagged: Biggest ask — no more live upstream_sync in CI CI timeout: dropped from 45m (sized for the network sync) to 15m now that there's no network dependency. Stronger assertions: cre.cy.js, search.cy.js, smartlink.cy.js, and standard.cy.js were all updated to check actual rendered data (link containers, filter results, exact smartlink redirect target, expanded-accordion content) instead of just headings/chrome that would pass against an empty page. Small cleanups: removed the duplicate cypress:run script in package.json, added a Cypress screenshot-on-failure artifact upload to the e2e workflow, and added a fail-fast check in make e2e if standards_cache.sqlite wasn't seeded first. Two real bugs the fixture surfaced, also fixed: running this against the minimal fixture (rather than the full prod dataset) exposed two genuine issues, not just test flakiness:
Verification: ran the full thing for real locally — yarn build + make e2e-db + make e2e against actual headless Cypress/Electron, not just reasoning from source. All 5 specs / 11 tests pass. This is a fork PR, so CI is sitting at action_required on the latest commit — whenever you get a chance to approve the workflow run, it should come back green. Let me know if anything else needs adjusting. |
|
@northdpole Branch Updated so you can check and test. |
|
@northdpole Branch is updated again so you can test. |
|
Maintainer note: if this PR is still blocked by end of August 2026, we will pick up the remaining review items ourselves (see #). Still needed for merge: rebase onto |
|
Correction on the fallback link: tracking issue is #1007 (finish Cypress e2e ourselves by 2026-08-31 if this PR has not landed). |
|
@northdpole — rebased onto latest Blocking items (2026-07-22 review + #1007)
Soft items also done
Rebase / conflict
Local verification (real run, not just reasoning)Full No network. Since this is a fork PR, all four checks ( |
northdpole
left a comment
northdpole
left a comment
There was a problem hiding this comment.
Re-review (post Actions approval) — 2026-08-12
CI surface: Test / Lint / Frontend Lint / CodeQL green; Test-e2e job marked green with 11/11 Cypress specs passing.
That green e2e job is misleading. Digging the log:
Blocking
1. yarn build fails hard inside make e2e, but the recipe still succeeds
From the Test-e2e log (Run Cypress e2e):
webpack … compiled with 34 errorserror Command failed with exit code 1(yarn build)- then Flask readiness curls / Cypress still run
- job exits success because
.ONESHELLis on and the recipe has noset -e, so Make uses the last command’s status (yarn test:e2e), notyarn build’s.
Please add set -euo pipefail (or at least set -e) at the top of the e2e (and ideally e2e-db) recipe so a failed frontend build fails the target/CI.
2. Cypress types are breaking the production webpack build
The 34 errors are TS2339 on Jest matchers (toBeTruthy / toBeNull) in ResourceSelector.test.tsx — classic Cypress global Assertion vs Jest/Chai clash after adding "cypress": "^13.17.0".
Cypress must not leak into the app/webpack TS program. Typical fixes (pick one, keep scope tight):
- exclude
**/*.test.tsx/cypress/from the webpack TS program, and/or - isolate Cypress types (
tsconfigfor Cypress only; avoid triple-slash / global Cypress types in the app build)
Until yarn build is green on its own, e2e is not trustworthy even when specs pass.
Prior CHANGES_REQUESTED — addressed
| Ask | Status |
|---|---|
Fixture seed instead of upstream_sync |
Done — scripts/seed_e2e_fixtures.py + make e2e-db (seed step succeeded in CI) |
| Drop e2e timeout to ~15m | Done |
Keep create_all for SQLite cache |
Done + comment |
| Stronger CRE filter / search / standard / smartlink / smoke assertions | Done |
| Fail-fast if DB missing | Done |
Drop duplicate cypress:run |
Done |
| Screenshots artifact on failure | Done |
Keep playwright install comment |
Done |
Nits (non-blocking once 1–2 fixed)
- Docs drift: README +
AGENTS.mdstill saye2e-dbloads the graph from upstream /upstream_sync. Seeder is fixture-only — please fix the wording. SearchName.tsxwindow.scrollTo(0, 0): harmless (mirrorsStandard.tsx) but unrelated to Cypress — prefer drop or split.seed_e2e_fixtures.py:with_graph()isn’t needed foradd_cre/add_node/add_link; considerNO_LOAD_GRAPH_DB=1on the seed invocation too (create_all already sets it) so a future Neo4j touch can’t flake CI..PHONY: adde2e/e2e-dbwhen you’re there.
Verdict
Not merge-ready until make e2e fails on yarn build failure and yarn build is actually green (Cypress type isolation). Happy to re-approve quickly after that.
…ebpack TS program - make e2e: add set -euo pipefail so a failing `yarn build` fails the target instead of being masked by the later `yarn test:e2e` exit code (.ONESHELL only checks the recipe's last command). - tsconfig.json: exclude cypress/, cypress.config.js, and *.test.ts(x) from the app's TS program — Cypress's global Assertion type was clashing with Jest matchers in ResourceSelector.test.tsx, breaking yarn build with 34 TS2339 errors. - README/AGENTS: fix stale "loads from upstream" wording for e2e-db, which seeds from the checked-in fixture graph, not upstream_sync. Addresses northdpole's blocking review comments on PR OWASP#984.
…ith_graph() seed_e2e_fixtures.py called Node_collection().with_graph(), which loads the full in-memory CRE graph — unnecessary since add_cre/add_node/add_link all guard graph writes with `if self.graph:`. Worse, the Makefile's seed invocation was missing NO_LOAD_GRAPH_DB=1 (only the create_all step had it), so Node_collection.__init__ would unconditionally try NEO_DB.instance() during seeding — a latent Neo4j-touch flake risk in CI. Verified with a real make e2e-db + make e2e run: 11/11 Cypress specs pass. Also confirmed the earlier `set -euo pipefail` fix actually catches a broken build (make e2e correctly fails instead of going green). Addresses northdpole's remaining nit on PR OWASP#984.
|
@northdpole Addressed the remaining nits from the 2026-08-12 review:
Also re-verified the two blocking items end-to-end rather than just trusting the code change:
Commits: |
Closes #967
Consolidates the frontend E2E migration to Cypress and restores the
Test-e2eCI job, which has been disabled onmain. Builds on #774 andsupersedes #729 / #732 (both now closed). Bundles the Playwright-install
guardrail.
Python Playwright is untouched
This PR does not touch Python Playwright (embeddings/scraping in
application/prompt_client/). The diff changes zero Playwright orprompt_clientfiles, andplaywright installinmake install-pythonis unchanged. The Cypress e2e job simply doesn't invoke that target
(Cypress ships its own browser), so nothing Playwright is removed or
modified. The Python
Testworkflow (which exercises the Playwright/embedding tests) stays green.
Why CI was red, and the fix
Re-enabling
e2e.ymlas-is failed because the e2e job built the SQLitecache the wrong way:
flask db upgrade(migrations) is the Postgres path and omitscolumns the models added without a migration (e.g.
cre.document_metadata),so
upstream_syncfailed withno such column.main, which left two Alembic heads —flask db upgradeaborted with "Multiple head revisions".Fix:
upstream/main→ single Alembic head (upstream's merge migration).make e2e-dbtarget seeds the schema from the ORM models viacreate_all(the documented SQLite/CI path) then loads data withupstream_sync.e2e.ymlnow runsinstall-deps+make e2e-db+make e2e, droppingmigrate-upgradeandplaywright installfrom the frontend job.Test coverage (ported from the legacy Jest/Puppeteer suite)
smoke.cy.js/search/{term}, browse/root_cressearch.cy.jsasdf), free-text results (crypto)standard.cy.js/node/standard/ASVSheading, sections, paginationcre.cy.js558-807), case-insensitive filtersmartlink.cy.jsEach spec asserts on real seeded data, so it fails on an empty DB
rather than passing against a broken backend.
Two notes on approach
chore/playwright-install-guardrailchange (69049e36)is already present in this branch as a commit with a byte-identical
diff, so the Makefile + AGENTS.md guardrail docs land here. Cherry-picking
the original SHA would just duplicate it.
upstream_syncseeds only the CRE-linkedsubset, which doesn't include
CWE/1002. I ported the same flow usingseeded fixtures —
ASVS/V13.2.5(internal redirect) and an unmapped CWE id(external Mitre fallback) — so the test is deterministic.
Cleanup
puppeteer/jest-puppeteerdevDependencies (+ lockfile).cypress.jsonand the commentedbasic-e2e.test.ts(via fix(e2e): complete Cypress migration and stabilize make e2e for #523 #774).make e2e-db,make e2e,yarn cypress:open)in README and AGENTS.md.
Verification
All green on my fork (run):
All specs passed! 11/11(~10 min incl. fullupstream_sync)make lint,yarn build,make e2e(3× no flakes).