-
Notifications
You must be signed in to change notification settings - Fork 0
Fix production smoke health configuration #103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,3 +18,4 @@ terraform.tfvars | |
| terraform.tfvars.json | ||
| tfplan | ||
| tfplan.* | ||
| .vercel | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -347,3 +347,9 @@ export const capturedRoutes: CapturedRoute[] = [ | |
| expectPage: expectEventPage, | ||
| }, | ||
| ]; | ||
|
|
||
| export const productionSmokeRoutes: CapturedRoute[] = capturedRoutes.filter((route) => | ||
| ["submit", "sign-in", "privacy-suppression", "event-new-signed-out", "server-status", "deployment"].includes( | ||
| route.name, | ||
| ), | ||
| ); | ||
|
Comment on lines
+351
to
+355
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/web/e2e/public-routes.ts
Line: 351-355
Comment:
**Silent empty suite if names drift**
`productionSmokeRoutes` is built by filtering `capturedRoutes` on a hard-coded list of string names. If any of the six names is later renamed (or mistyped here), the filter silently drops it — and if they all disappear, the test suite passes with zero tests, giving false green on production smoke runs. There is no assertion that the filtered list is non-empty or that every listed name resolved to a route.
How can I resolve this? If you propose a fix, please make it concise. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PLAYWRIGHT_BASE_URLas production signal narrows staging smoke coveragePLAYWRIGHT_BASE_URLis also set when running the mutation-backed hosted flow against a dev/staging target (per the docs:set PLAYWRIGHT_BASE_URL and VRDEX_E2E_BROWSER_TOKEN). If anyone runs this smoke spec against a dev/staging URL that does have fixture data, they'll silently exercise only the 6 production-safe routes rather than the fullcapturedRoutesset — potentially missing regressions that only surface through fixture-backed routes. A separate env var (e.g.PLAYWRIGHT_PRODUCTION_SMOKE=true) would make the intent explicit.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!