Fix production smoke health configuration#103
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Playwright Data-Flow PreviewOutcome: success Captured flow:
Artifacts include screenshots, traces, and recorded video for the flow run. |
Playwright Image DiffOutcome: success Changed screenshot baselines: none in this PR. This check compares public route screenshots against committed baselines. Inline images show only added or modified baseline PNGs. |
Playwright Public Screenshot PreviewOutcome: success Screenshots: all public route checks passed on desktop and mobile. Full screenshot set is available in the artifact. Pixel diff baselines are handled by the separate Playwright Image Diff check. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Greptile SummaryThis PR narrows the hosted production smoke suite to six fixture-free public routes, documents the expected environment variable posture for E2E helpers across environments, and formalises the IaC-first rule in
Confidence Score: 4/5Safe to merge; the production smoke narrowing is intentional and correct today, but the string-name filter in productionSmokeRoutes has no guard against silent zero-test runs if routes are renamed. The string-name filter that builds productionSmokeRoutes will silently yield an empty list if any of the six hardcoded names is later renamed, causing the entire hosted smoke suite to pass with zero tests and provide false confidence about production health. apps/web/e2e/public-routes.ts and apps/web/e2e/public-routes.smoke.spec.ts warrant a second look for the empty-filter and broad-signal concerns. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[pnpm test:e2e:hosted:smoke] --> B{PLAYWRIGHT_BASE_URL set?}
B -- Yes --> C[productionSmokeRoutes\n6 fixture-free routes]
B -- No --> D[capturedRoutes\nfull local set]
C --> E[filter capturedRoutes by string name]
E --> F{all 6 names resolve?}
F -- Yes --> G[run 6 smoke tests against hosted URL]
F -- No / empty --> H[⚠️ suite passes vacuously\nwith 0 tests]
D --> I[run full smoke suite locally]
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/web/e2e/public-routes.ts:351-355
**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.
### Issue 2 of 2
apps/web/e2e/public-routes.smoke.spec.ts:5
**`PLAYWRIGHT_BASE_URL` as production signal narrows staging smoke coverage**
`PLAYWRIGHT_BASE_URL` is 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 full `capturedRoutes` set — 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.
Reviews (1): Last reviewed commit: "Document deployed health config" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export const productionSmokeRoutes: CapturedRoute[] = capturedRoutes.filter((route) => | ||
| ["submit", "sign-in", "privacy-suppression", "event-new-signed-out", "server-status", "deployment"].includes( | ||
| route.name, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
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.
Prompt To Fix With AI
This 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.| import { capturedRoutes, productionSmokeRoutes } from "./public-routes"; | ||
|
|
||
| for (const route of capturedRoutes) { | ||
| const routes = process.env.PLAYWRIGHT_BASE_URL ? productionSmokeRoutes : capturedRoutes; |
There was a problem hiding this comment.
PLAYWRIGHT_BASE_URL as production signal narrows staging smoke coverage
PLAYWRIGHT_BASE_URL is 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 full capturedRoutes set — 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
This is a comment left during a code review.
Path: apps/web/e2e/public-routes.smoke.spec.ts
Line: 5
Comment:
**`PLAYWRIGHT_BASE_URL` as production signal narrows staging smoke coverage**
`PLAYWRIGHT_BASE_URL` is 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 full `capturedRoutes` set — 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.
How can I resolve this? If you propose a fix, please make it concise.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!
1 participant
What changed
Why
The manual production smoke health workflow was now wired, but failed because hosted smoke still expected fixture-backed profile/search data that production intentionally does not expose.
Testing
Risk
Production smoke coverage is intentionally narrower in hosted mode; full fixture-backed route checks still run locally/CI where Playwright fixtures are enabled.
Continues #100.