Skip to content

[test] Remove maxRetries and hardError parameters #85536

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 30, 2025
+35 −33
Merged

[test] Remove maxRetries and hardError parameters #85536

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 24 additions & 7 deletions test/e2e/app-dir/actions/app-action.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { FileRef, nextTestSetup } from 'e2e-utils'
import {
assertHasRedbox,
retry,
check,
waitFor,
getRedboxSource,
getDistDir,
Expand Down Expand Up @@ -527,17 +526,29 @@ describe('app-dir action handling', () => {

// navigate to server
await browser.elementByCss('#navigate-server').click()
// intentionally bailing after 2 retries so we don't retry to the point where the async function resolves
await check(() => browser.url(), `${next.url}/server`, true, 2)
// intentionally bailing after 2s so we don't retry to the point where the async function resolves
await retry(
async () => {
expect(await browser.url()).toEqual(`${next.url}/server`)
},
2000,
1000
)

browser = await next.browser('/server')

await browser.elementByCss('#slow-inc').click()

// navigate to client
await browser.elementByCss('#navigate-client').click()
// intentionally bailing after 2 retries so we don't retry to the point where the async function resolves
await check(() => browser.url(), `${next.url}/client`, true, 2)
// intentionally bailing after 2s so we don't retry to the point where the async function resolves
await retry(
async () => {
expect(await browser.url()).toEqual(`${next.url}/client`)
},
2000,
1000
)
})

it('should not block router.back() while a server action is in flight', async () => {
Expand All @@ -549,8 +560,14 @@ describe('app-dir action handling', () => {

await browser.back()

// intentionally bailing after 2 retries so we don't retry to the point where the async function resolves
await check(() => browser.url(), `${next.url}/`, true, 2)
// intentionally bailing after 2s so we don't retry to the point where the async function resolves
await retry(
async () => {
expect(await browser.url()).toEqual(`${next.url}/`)
},
2000,
1000
)
})

it('should trigger a refresh for a server action that also dispatches a navigation event', async () => {
Expand Down
10 changes: 3 additions & 7 deletions test/e2e/getserversideprops/test/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,13 +598,9 @@ const runTests = (isDev = false, isDeploy = false) => {

it('should load a fast refresh page', async () => {
const browser = await webdriver(next.url, '/refresh')
expect(
await check(
() => browser.elementByCss('p').text(),
/client loaded/,
false
)
).toBe(true)
await retry(async () => {
expect(await browser.elementByCss('p').text()).toMatch(/client loaded/)
})
Comment on lines +601 to +603
Copy link
Contributor

@vercel vercel bot Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
await retry(async () => {
expect(await browser.elementByCss('p').text()).toMatch(/client loaded/)
})
await retry(
async () => {
expect(await browser.elementByCss('p').text()).toMatch(/client loaded/)
},
30000,
1000
)

The test timeout has been significantly reduced from ~30 seconds (original check() default) to 3 seconds (default retry() duration), which could cause the test to flake if the page takes longer than 3 seconds to render the "client loaded" text.

View Details

Analysis

Test timeout reduced 10x from 30 seconds to 3 seconds during refactoring

What fails: The "should load a fast refresh page" test in test/e2e/getserversideprops/test/index.test.ts uses retry() with default parameters (3 second timeout) instead of maintaining the original 30 second timeout from the deprecated check() function.

How to reproduce:

# In a slower CI environment or under system load, the test can timeout:
cd test/e2e/getserversideprops
npm test -- --testNamePattern="should load a fast refresh page"

What happens: The retry timeout fails after 3 seconds if the page hasn't fully rendered the "client loaded" text. The original check() function with its default maxRetries=30 parameter allowed up to 30 seconds for client-side JavaScript to execute.

Expected behavior: The test should maintain the original ~30 second timeout window. This is confirmed by comparing the refactoring in the same commit (a362dcb) to test/e2e/app-dir/actions/app-action.test.ts, where similar conversions from check() to retry() explicitly specified 30000, 1000 parameters to maintain the original timeout behavior.

Root cause: When refactoring from check(..., false) (using default maxRetries=30) to retry(), the explicit duration and interval parameters were not specified, causing the function to use defaults of 3000ms duration instead of ~30000ms.

Fix: Specify explicit duration and interval parameters: retry(fn, 30000, 1000) to maintain 30-second timeout with 1-second intervals, matching the original behavior.

Copy link
Member Author

@eps1lon eps1lon Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intended. We want to reduce the default timeout while migrating. By doing it while changing tests, we ensure flake detection runs on the PRs reducing the timeout.

})

it('should provide correct query value for dynamic page', async () => {
Expand Down
27 changes: 8 additions & 19 deletions test/lib/next-test-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,25 +697,18 @@ export async function startCleanStaticServer(dir: string) {
/**
* Check for content in 1 second intervals timing out after 30 seconds.
* @deprecated use retry + expect instead
* @param {() => Promise<unknown> | unknown} contentFn
* @param {RegExp | string | number} regex
* @param {boolean} hardError
* @param {number} maxRetries
* @returns {Promise<boolean>}
*/
export async function check(
contentFn: () => any | Promise<any>,
regex: any,
hardError = true,
maxRetries = 30
) {
let content
let lastErr
contentFn: () => unknown | Promise<unknown>,
regex: any
): Promise<boolean> {
let content: unknown
let lastErr: unknown

for (let tries = 0; tries < maxRetries; tries++) {
for (let tries = 0; tries < 30; tries++) {
try {
content = await contentFn()
if (typeof regex !== typeof /regex/) {
if (typeof regex !== 'object') {
if (regex === content) {
return true
}
Expand All @@ -730,11 +723,7 @@ export async function check(
}
}
console.error('TIMED OUT CHECK: ', { regex, content, lastErr })

if (hardError) {
throw new Error('TIMED OUT: ' + regex + '\n\n' + content + '\n\n' + lastErr)
}
return false
throw new Error('TIMED OUT: ' + regex + '\n\n' + content + '\n\n' + lastErr)
}

export class File {
Expand Down
Loading