fix(e2e): complete Cypress migration and stabilize make e2e for #523#774
fix(e2e): complete Cypress migration and stabilize make e2e for #523#774PRAteek-singHWY wants to merge 7 commits into
Conversation
|
Hi @northdpole picked this up based on your earlier guidance to finish the Cypress migration for e2e. PR #774 incorporates the prior review iterations from related e2e threads (#729, #732) and is scoped to resolve #523:
Validation: |
|
Tracking consolidated Cypress migration in #967. Please keep |
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. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThe e2e workflow now starts Flask with PID-tracked cleanup and readiness polling, and the test harness is switched to Cypress with new config, scripts, dependency, and a smoke spec. ChangesCypress e2e migration
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
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.
PRAteek-singHWY
force-pushed
the
fix/e2e-tests-523
branch
from
4faef58 to
56d4dad
Compare
![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 `@Makefile`:
- Around line 51-54: The Flask readiness loop in the Makefile silently continues
into yarn test:e2e even when the server never becomes reachable. Add a final
post-loop check after the existing curl retry loop, using the same
localhost:5000 readiness probe, and fail immediately with a clear error message
if Flask is still unavailable. Keep the fix localized to the wait logic around
the loop so the test target stops early on startup failures.
🪄 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: ffbffddf-f7fc-4320-97bb-5c5154c9f6c3
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
Makefileapplication/frontend/src/test/basic-e2e.test.tscypress.config.jscypress.jsoncypress/e2e/smoke.cy.jspackage.json
💤 Files with no reviewable changes (2)
- application/frontend/src/test/basic-e2e.test.ts
- cypress.json
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
50-61: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd a fail-fast check after the Flask readiness loop. If Flask never starts, the target still falls through to
yarn test:e2e, hiding the startup failure.🤖 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 `@Makefile` around lines 50 - 61, The Flask startup sequence in the Makefile target should fail fast if the readiness loop never sees the server respond, because the current flow can continue into yarn test:e2e even when flask run never became healthy. Update the target around the readiness loop and FLASK_PID handling so it verifies Flask came up successfully after the curl retry loop and exits with an error before running the E2E tests if it did not; keep the fix localized to this startup block using the existing flask run, FLASK_PID, trap, and curl loop logic.
🧹 Nitpick comments (1)
Makefile (1)
90-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare
installas a PHONY target.Without
.PHONY: install, if a file or directory namedinstallever exists in the project root,make installwill silently skip execution because make considers the target up-to-date.♻️ Proposed fix
install: install-typescript install-python migrate-upgradeAdd near other
.PHONYdeclarations (or at the top of the file):+.PHONY: install🤖 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 `@Makefile` at line 90, Declare the Makefile’s install target as phony so make always runs install: add install to the existing .PHONY declaration near the other targets, alongside install-typescript, install-python, and migrate-upgrade. This ensures the install rule is not skipped if a file or directory named install exists in the project root.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In `@Makefile`:
- Around line 50-61: The Flask startup sequence in the Makefile target should
fail fast if the readiness loop never sees the server respond, because the
current flow can continue into yarn test:e2e even when flask run never became
healthy. Update the target around the readiness loop and FLASK_PID handling so
it verifies Flask came up successfully after the curl retry loop and exits with
an error before running the E2E tests if it did not; keep the fix localized to
this startup block using the existing flask run, FLASK_PID, trap, and curl loop
logic.
---
Nitpick comments:
In `@Makefile`:
- Line 90: Declare the Makefile’s install target as phony so make always runs
install: add install to the existing .PHONY declaration near the other targets,
alongside install-typescript, install-python, and migrate-upgrade. This ensures
the install rule is not skipped if a file or directory named install exists in
the project root.
|
Hi @northdpole, |
# Conflicts: # yarn.lock
PRAteek-singHWY
force-pushed
the
fix/e2e-tests-523
branch
from
c88b73c to
07066de
Compare
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.
2 participants
Fixes #523
Summary
This PR completes the E2E migration path to Cypress and restores a working smoke test flow.
What changed
test:e2efromjesttocypress runinpackage.jsoncypress:opencypress:runcypress.config.jscypress/e2e/smoke.cy.jscypress.jsonapplication/frontend/src/test/basic-e2e.test.ts(commented Jest/Puppeteer test)make e2einMakefile:127.0.0.1:5000env -u ELECTRON_RUN_AS_NODE yarn test:e2efor stable Cypress executionplaywright installstep frominstall-python(Makefile), consistent with Cypress migrationValidation
make e2e3 passing, 0 failingScope notes