WS-1767: Cypress Visit Retry Override#13830
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a Cypress cy.visit() override in the Next.js app’s E2E support code to reduce flakiness from transient non-200 responses (e.g. cold starts) during before() hooks.
Changes:
- Overwrites Cypress
visitto perform a pre-flight status check with retries before navigating (whenfailOnStatusCodeis true). - Removes the previous “ensure 200 + retry” logic from
runTestsForPage(now handled by thevisitoverwrite). - Adds temporary local API behaviour to simulate initial 500 responses for a specific Live asset.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| ws-nextjs-app/pages/api/local/[service]/[pageType]/[id]/[[...optionalParams]]/index.api.ts | Adds simulated 500 responses for a specific local API asset (test-only behaviour). |
| ws-nextjs-app/cypress/support/helpers/runTestsForPage.ts | Removes pre-visit response check/retry from the shared page test runner. |
| ws-nextjs-app/cypress/support/commands.ts | Overwrites cy.visit() to add a pre-check + retry before visiting and retains “Continue Reading” handling. |
ws-nextjs-app/pages/api/local/[service]/[pageType]/[id]/[[...optionalParams]]/index.api.ts
Outdated
Show resolved
Hide resolved
ws-nextjs-app/pages/api/local/[service]/[pageType]/[id]/[[...optionalParams]]/index.api.ts
Outdated
Show resolved
Hide resolved
| const runVisit = (): Cypress.Chainable => { | ||
| const visit = originalFn as Cypress.CommandOriginalFn<'visit'> as ( | ||
| urlOrOptionsParam: | ||
| | string | ||
| | (Partial<Cypress.VisitOptions> & { url: string }), | ||
| optionsParam?: Partial<Cypress.VisitOptions>, | ||
| ) => Cypress.Chainable; | ||
|
|
There was a problem hiding this comment.
Maybe nitpicky, but do we really need to be explicitily typing everything like this ( here and in this file in general) ? Does it throw errors without them? If it doesn't my preference would be to not have them in as i think it hurts readability - I know AI really likes to type things explicitly once you tell it it's a typescript file so just thought i'd double check we really need them lol. All good if it actually does throw errors!
There was a problem hiding this comment.
Yeah I don't like this. I think it will throw and error due to this being a TS file. We could ignore the error. Although I've asked copilot again, and it produced this, which makes it more readable:
const [options] = rest as [Partial<Cypress.VisitOptions>?];
const visitUrl =
typeof urlOrOptions === 'string' ? urlOrOptions : urlOrOptions.url;
const visitOptionsObj =
typeof urlOrOptions === 'string' ? (options ?? {}) : urlOrOptions;
const { failOnStatusCode = true, headers } = visitOptionsObj;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const originalFnAsAny = originalFn as (...args: any[]) => Cypress.Chainable;
const runVisit = () => {
if (typeof urlOrOptions === 'string') {
return originalFnAsAny(urlOrOptions, options);
}
return originalFnAsAny(urlOrOptions);
};
Resolves JIRA: WS-1767
Summary
Overrides the original Visit cypress command, to allow a retry mechanism to not fail tests in Before hook calls.
Note: First PR I've tried to be mostly Copilot led, I initially used Copilot Planning mode and fed in the ticket, and provided context via fetching relevant Cypress docs. Then I fed the generated markdown plan into Agent mode to generate the code. I've also tweaked parts of the generated code to make it more readable, and to follow our coding standards.
Code changes
Testing
I've prompted Copilot to add temporary code to 500 from the local Next.js API for live asset c7p765ynk9qt, which currently returns two 500 errors before 200'ing, this number is configurable. This change will be removed when testing is complete
cd ws-nextjs-appyarn test:e2e:interactiveUseful Links