Skip to content

Commit

Permalink
Attempt to fix flakiness of prerender e2e tests
Browse files Browse the repository at this point in the history 
x-ref: [Flakiness Metrics](https://app.datadoghq.com/ci/test-runs?query=test_level%3Atest%20env%3Aci%20%40git.repository.id%3Agh.neting.cc%2Fvercel%2Fnext.js%20%40test.service%3Anextjs%20%40test.suite%3A%2APrerender%2A%20%40test.status%3Afail&agg_m=count&agg_m_source=base&agg_t=count&currentTab=overview&eventStack=&fromUser=false&index=citest&start=1720809000422&end=1721413800422&paused=false)

As part of this PR I'm also changing the new `patchFile` option to not
automatically retry. This should be handled by the call site.
  • Loading branch information
@unstubbable
unstubbable committed Jul 19, 2024
1 parent 9c3ed17 commit 46c9d31
Show file tree
Hide file tree
Showing 4 changed files with 114 additions and 129 deletions.
13 changes: 7 additions & 6 deletions test/e2e/app-dir/logging/fetch-logging.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,12 +220,13 @@ describe('app-dir - logging', () => {
await next.patchFile(
'app/default-cache/page.js',
(content) => content.replace('Default Cache', 'Hello!'),
async () => {
headline = await browser.waitForElementByCss('h1').text()
expect(headline).toBe('Hello!')
const logs = stripAnsi(next.cliOutput.slice(outputIndex))
expect(logs).not.toInclude(` │ GET `)
}
async () =>
retry(async () => {
headline = await browser.waitForElementByCss('h1').text()
expect(headline).toBe('Hello!')
const logs = stripAnsi(next.cliOutput.slice(outputIndex))
expect(logs).not.toInclude(` │ GET `)
})
)
})
}
Expand Down
216 changes: 100 additions & 116 deletions test/e2e/prerender.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1008,9 +1008,8 @@ describe('Prerender', () => {

if ((global as any).isNextDev) {
it('should not show warning from url prop being returned', async () => {
const urlPropPage = 'pages/url-prop.js'
await next.patchFile(
urlPropPage,
'pages/url-prop.js',
`
export async function getStaticProps() {
return {
Expand All @@ -1021,15 +1020,16 @@ describe('Prerender', () => {
}
export default ({ url }) => <p>url: {url}</p>
`
)

const html = await renderViaHTTP(next.url, '/url-prop')
await next.deleteFile(urlPropPage)
expect(next.cliOutput).not.toMatch(
/The prop `url` is a reserved prop in Next.js for legacy reasons and will be overridden on page \/url-prop/
`,
async () =>
retry(async () => {
const html = await renderViaHTTP(next.url, '/url-prop')
expect(next.cliOutput).not.toMatch(
/The prop `url` is a reserved prop in Next.js for legacy reasons and will be overridden on page \/url-prop/
)
expect(html).toMatch(/url:.*?something/)
})
)
expect(html).toMatch(/url:.*?something/)
})

it('should always show fallback for page not in getStaticPaths', async () => {
Expand Down Expand Up @@ -1083,33 +1083,30 @@ describe('Prerender', () => {
})

it('should log error in console and browser in development mode', async () => {
const indexPage = 'pages/index.js'
const origContent = await next.readFile(indexPage)

const browser = await webdriver(next.url, '/')
expect(await browser.elementByCss('p').text()).toMatch(/hello.*?world/)

await next.patchFile(
indexPage,
origContent
.replace('// throw new', 'throw new')
.replace('{/* <div', '<div')
.replace('</div> */}', '</div>')
'pages/index.js',
(content) =>
content
.replace('// throw new', 'throw new')
.replace('{/* <div', '<div')
.replace('</div> */}', '</div>'),
async () => {
await browser.waitForElementByCss('#after-change')
// we need to reload the page to trigger getStaticProps
await browser.refresh()

return retry(async () => {
await assertHasRedbox(browser)
const errOverlayContent = await getRedboxHeader(browser)
const errorMsg = /oops from getStaticProps/
expect(next.cliOutput).toMatch(errorMsg)
expect(errOverlayContent).toMatch(errorMsg)
})
}
)

try {
await browser.waitForElementByCss('#after-change')
// we need to reload the page to trigger getStaticProps
await browser.refresh()

await assertHasRedbox(browser)
const errOverlayContent = await getRedboxHeader(browser)
const errorMsg = /oops from getStaticProps/
expect(next.cliOutput).toMatch(errorMsg)
expect(errOverlayContent).toMatch(errorMsg)
} finally {
await next.patchFile(indexPage, origContent)
}
})

it('should always call getStaticProps without caching in dev', async () => {
Expand All @@ -1136,25 +1133,20 @@ describe('Prerender', () => {
})

it('should error on bad object from getStaticProps', async () => {
const indexPage = 'pages/index.js'
const origContent = await next.readFile(indexPage)
await next.patchFile(
indexPage,
origContent.replace(/\/\/ bad-prop/, 'another: true,')
'pages/index.js',
(content) => content.replace(/\/\/ bad-prop/, 'another: true,'),
async () =>
retry(async () => {
const html = await renderViaHTTP(next.url, '/')
expect(html).toMatch(/Additional keys were returned/)
})
)
await waitFor(1000)
try {
const html = await renderViaHTTP(next.url, '/')
expect(html).toMatch(/Additional keys were returned/)
} finally {
await next.patchFile(indexPage, origContent)
}
})

it('should error on dynamic page without getStaticPaths', async () => {
const curPage = 'pages/temp/[slug].js'
await next.patchFile(
curPage,
'pages/temp/[slug].js',
`
export async function getStaticProps() {
return {
Expand All @@ -1164,23 +1156,20 @@ describe('Prerender', () => {
}
}
export default () => 'oops'
`
`,
async () =>
retry(async () => {
const html = await renderViaHTTP(next.url, '/temp/hello')
expect(html).toMatch(
/getStaticPaths is required for dynamic SSG pages and is missing for/
)
})
)
await waitFor(1000)
try {
const html = await renderViaHTTP(next.url, '/temp/hello')
expect(html).toMatch(
/getStaticPaths is required for dynamic SSG pages and is missing for/
)
} finally {
await next.deleteFile(curPage)
}
})

it('should error on dynamic page without getStaticPaths returning fallback property', async () => {
const curPage = 'pages/temp2/[slug].js'
await next.patchFile(
curPage,
'pages/temp2/[slug].js',
`
export async function getStaticPaths() {
return {
Expand All @@ -1195,15 +1184,13 @@ describe('Prerender', () => {
}
}
export default () => 'oops'
`
`,
async () =>
retry(async () => {
const html = await renderViaHTTP(next.url, '/temp2/hello')
expect(html).toMatch(/`fallback` key must be returned from/)
})
)
await waitFor(1000)
try {
const html = await renderViaHTTP(next.url, '/temp2/hello')
expect(html).toMatch(/`fallback` key must be returned from/)
} finally {
await next.deleteFile(curPage)
}
})

it('should not re-call getStaticProps when updating query', async () => {
Expand Down Expand Up @@ -2199,26 +2186,23 @@ describe('Prerender', () => {

if (!isDev && !isDeploy) {
it('should automatically reset cache TTL when an error occurs and build cache was available', async () => {
await next.patchFile('error.txt', 'yes')
await waitFor(2000)
await next.patchFile('error.txt', 'yes', async () => {
await waitFor(2000)

for (let i = 0; i < 5; i++) {
const res = await fetchViaHTTP(
next.url,
'/blocking-fallback/test-errors-1'
)
expect(res.status).toBe(200)
}
await next.deleteFile('error.txt')
await check(
() =>
next.cliOutput.match(
for (let i = 0; i < 5; i++) {
const res = await fetchViaHTTP(
next.url,
'/blocking-fallback/test-errors-1'
)
expect(res.status).toBe(200)
}

return retry(async () => {
expect(next.cliOutput).toMatch(
/throwing error for \/blocking-fallback\/test-errors-1/
).length === 1
? 'success'
: next.cliOutput,
'success'
)
)
})
})
})

it('should automatically reset cache TTL when an error occurs and runtime cache was available', async () => {
Expand All @@ -2229,26 +2213,22 @@ describe('Prerender', () => {

expect(res.status).toBe(200)
await waitFor(2000)
await next.patchFile('error.txt', 'yes')

for (let i = 0; i < 5; i++) {
const res = await fetchViaHTTP(
next.url,
'/blocking-fallback/test-errors-2'
)
expect(res.status).toBe(200)
}
await next.deleteFile('error.txt')
await next.patchFile('error.txt', 'yes', async () => {
for (let i = 0; i < 5; i++) {
const res = await fetchViaHTTP(
next.url,
'/blocking-fallback/test-errors-2'
)
expect(res.status).toBe(200)
}

await check(
() =>
next.cliOutput.match(
return retry(async () => {
expect(next.cliOutput).toMatch(
/throwing error for \/blocking-fallback\/test-errors-2/
).length === 1
? 'success'
: next.cliOutput,
'success'
)
)
})
})
})

it('should not on-demand revalidate for fallback: blocking with onlyGenerated if not generated', async () => {
Expand Down Expand Up @@ -2294,15 +2274,17 @@ describe('Prerender', () => {
expect($('p').text()).toMatch(/Post:.*?test-if-generated-2/)
expect(res.headers.get('x-nextjs-cache')).toMatch(/MISS/)

const res2 = await fetchViaHTTP(
next.url,
'/blocking-fallback/test-if-generated-2'
)
const html2 = await res2.text()
const $2 = cheerio.load(html2)
await retry(async () => {
const res2 = await fetchViaHTTP(
next.url,
'/blocking-fallback/test-if-generated-2'
)
const html2 = await res2.text()
const $2 = cheerio.load(html2)

expect(initialTime).toBe($2('#time').text())
expect(res2.headers.get('x-nextjs-cache')).toMatch(/(HIT|STALE)/)
expect(initialTime).toBe($2('#time').text())
expect(res2.headers.get('x-nextjs-cache')).toMatch(/(HIT|STALE)/)
})

const res3 = await fetchViaHTTP(
next.url,
Expand All @@ -2318,14 +2300,16 @@ describe('Prerender', () => {
const revalidateData = await res3.json()
expect(revalidateData.revalidated).toBe(true)

const res4 = await fetchViaHTTP(
next.url,
'/blocking-fallback/test-if-generated-2'
)
const html4 = await res4.text()
const $4 = cheerio.load(html4)
expect($4('#time').text()).not.toBe(initialTime)
expect(res4.headers.get('x-nextjs-cache')).toMatch(/(HIT|STALE)/)
await retry(async () => {
const res4 = await fetchViaHTTP(
next.url,
'/blocking-fallback/test-if-generated-2'
)
const html4 = await res4.text()
const $4 = cheerio.load(html4)
expect($4('#time').text()).not.toBe(initialTime)
expect(res4.headers.get('x-nextjs-cache')).toMatch(/(HIT|STALE)/)
})
})

it('should on-demand revalidate for revalidate: false', async () => {
Expand Down
8 changes: 4 additions & 4 deletions test/lib/next-modes/base.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { ChildProcess } from 'child_process'
import { createNextInstall } from '../create-next-install'
import { Span } from 'next/dist/trace'
import webdriver from '../next-webdriver'
import { renderViaHTTP, fetchViaHTTP, findPort, retry } from 'next-test-utils'
import { renderViaHTTP, fetchViaHTTP, findPort } from 'next-test-utils'
import cheerio from 'cheerio'
import { once } from 'events'
import { BrowserInterface } from '../browsers/base'
Expand Down Expand Up @@ -491,7 +491,7 @@ export class NextInstance {
public async patchFile(
filename: string,
content: string | ((content: string) => string),
retryWithTempContent?: (context: { newFile: boolean }) => Promise<void>
runWithTempContent?: (context: { newFile: boolean }) => Promise<void>
): Promise<{ newFile: boolean }> {
const outputPath = path.join(this.testDir, filename)
const newFile = !existsSync(outputPath)
Expand All @@ -503,9 +503,9 @@ export class NextInstance {
typeof content === 'function' ? content(previousContent) : content
)

if (retryWithTempContent) {
if (runWithTempContent) {
try {
await retry(() => retryWithTempContent({ newFile }))
await runWithTempContent({ newFile })
} finally {
if (previousContent === undefined) {
await fs.rm(outputPath)
Expand Down
6 changes: 3 additions & 3 deletions test/lib/next-modes/next-dev.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ export class NextDevInstance extends NextInstance {
public override async patchFile(
filename: string,
content: string | ((contents: string) => string),
retryWithTempContent?: (context: { newFile: boolean }) => Promise<void>
runWithTempContent?: (context: { newFile: boolean }) => Promise<void>
) {
const isServerRunning = this.childProcess && !this.isStopping
const cliOutputLength = this.cliOutput.length
Expand All @@ -174,10 +174,10 @@ export class NextDevInstance extends NextInstance {
}
}

if (retryWithTempContent) {
if (runWithTempContent) {
return super.patchFile(filename, content, async ({ newFile }) => {
await waitForChanges({ newFile })
await retryWithTempContent({ newFile })
await runWithTempContent({ newFile })
})
}

Expand Down

0 comments on commit 46c9d31

Please sign in to comment.