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 27 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. |
Addresses northdpole's review on OWASP#984: CI shouldn't hit opencre.org every PR, and the specs' hardcoded ids/terms should be an explicit fixture contract rather than accidents of prod data. - scripts/seed_e2e_fixtures.py builds a minimal graph (CRE 558-807 linked to ASVS/V13.2.5 + NIST/AC-1, a "Cryptography" CRE, and enough filler ASVS sections to trigger pagination) via the same add_cre/add_node/ add_link calls the app already uses, no network I/O. - make e2e-db now runs the seeder instead of `cre.py --upstream_sync`; make e2e fails fast with a clear message if the DB wasn't seeded. - e2e.yml timeout drops from 45m (sized for the network sync) to 15m, and uploads Cypress screenshots as an artifact on failure. - Cypress specs now assert on actual fixture content (filtered links, search result rows, exact smartlink redirect, pagination + external reference + linked-CRE follow-through) instead of chrome-only checks. - package.json: drop the duplicate cypress:run script. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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. |
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).