From 6e7e6e1948f1c79896007fa0be0915f62accdfb8 Mon Sep 17 00:00:00 2001 From: eps1lon Date: Fri, 19 Jul 2024 20:35:18 +0200 Subject: [PATCH 1/4] Ensure "[Fast Refresh] rebuilding" logs have a matching "[Fast Refresh] done" log --- .../app/hot-reloader-client.tsx | 3 +- test/development/app-hmr/hmr.test.ts | 129 +++++++++++------- 2 files changed, 81 insertions(+), 51 deletions(-) diff --git a/packages/next/src/client/components/react-dev-overlay/app/hot-reloader-client.tsx b/packages/next/src/client/components/react-dev-overlay/app/hot-reloader-client.tsx index 72c2c006d9453..9c7a6333a1389 100644 --- a/packages/next/src/client/components/react-dev-overlay/app/hot-reloader-client.tsx +++ b/packages/next/src/client/components/react-dev-overlay/app/hot-reloader-client.tsx @@ -59,7 +59,6 @@ function onFastRefresh( updatedModules: ReadonlyArray ) { dispatcher.onBuildOk() - reportHmrLatency(sendMessage, updatedModules) dispatcher.onRefresh() @@ -161,6 +160,7 @@ function tryApplyUpdates( ) { if (!isUpdateAvailable() || !canApplyUpdates()) { dispatcher.onBuildOk() + reportHmrLatency(sendMessage, []) return } @@ -430,7 +430,6 @@ function processMessage( router.hmrRefresh() dispatcher.onRefresh() }) - reportHmrLatency(sendMessage, []) if (process.env.__NEXT_TEST_MODE) { if (self.__NEXT_HMR_CB) { diff --git a/test/development/app-hmr/hmr.test.ts b/test/development/app-hmr/hmr.test.ts index c89a4e3eb47a3..8fcd585a4e064 100644 --- a/test/development/app-hmr/hmr.test.ts +++ b/test/development/app-hmr/hmr.test.ts @@ -3,8 +3,6 @@ import { retry, waitFor } from 'next-test-utils' const envFile = '.env.development.local' -const isPPREnabledByDefault = process.env.__NEXT_EXPERIMENTAL_PPR === 'true' - describe(`app-dir-hmr`, () => { const { next } = nextTestSetup({ files: __dirname, @@ -73,7 +71,7 @@ describe(`app-dir-hmr`, () => { const fastRefreshLogs = logs.filter((log) => { return log.message.startsWith('[Fast Refresh]') }) - // FIXME: 3+ "rebuilding" but single "done" is confusing. + // FIXME: 3+ "rebuilding" but no "done" is confusing. // There may actually be more "rebuilding" but not reliably. // To ignore this flakiness, we just assert on subset matches. // Once the bug is fixed, each "rebuilding" should be paired with a "done in" exactly. @@ -81,50 +79,59 @@ describe(`app-dir-hmr`, () => { expect.arrayContaining([ { source: 'log', message: '[Fast Refresh] rebuilding' }, { source: 'log', message: '[Fast Refresh] rebuilding' }, - { - source: 'log', - message: expect.stringContaining('[Fast Refresh] done in '), - }, { source: 'log', message: '[Fast Refresh] rebuilding' }, ]) ) + // FIXME: Turbopack should have matching "done in" for each "rebuilding" + expect(logs).not.toEqual( + expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('[Fast Refresh] done in'), + source: 'log', + }), + ]) + ) }) } else { await retry( async () => { - const fastRefreshLogs = logs.filter((log) => { - return log.message.startsWith('[Fast Refresh]') + const envValue = await browser.elementByCss('p').text() + const mpa = await browser.eval( + 'window.__TEST_NO_RELOAD === undefined' + ) + // Used to be flaky but presumably no longer is. + // If this flakes again, please add the received value as a commnet. + expect({ envValue, mpa }).toEqual({ + envValue: 'ipad', + mpa: false, }) - // FIXME: Should be either a single "rebuilding"+"done" or the last "rebuilding" should be followed by "done" - expect(fastRefreshLogs).toEqual([ - { source: 'log', message: '[Fast Refresh] rebuilding' }, - { source: 'log', message: '[Fast Refresh] rebuilding' }, - { - source: 'log', - message: expect.stringContaining('[Fast Refresh] done in '), - }, - { source: 'log', message: '[Fast Refresh] rebuilding' }, - ]) }, // Very slow Hot Update for some reason. // May be related to receiving 3 rebuild events but only one finish event 5000 ) + + const fastRefreshLogs = logs.filter((log) => { + return log.message.startsWith('[Fast Refresh]') + }) + expect(fastRefreshLogs).toEqual([ + { source: 'log', message: '[Fast Refresh] rebuilding' }, + { + source: 'log', + message: expect.stringContaining('[Fast Refresh] done in '), + }, + { source: 'log', message: '[Fast Refresh] rebuilding' }, + { source: 'log', message: '[Fast Refresh] rebuilding' }, + { + source: 'log', + message: expect.stringContaining('[Fast Refresh] done in '), + }, + { + source: 'log', + message: expect.stringContaining('[Fast Refresh] done in '), + }, + ]) } - const envValue = await browser.elementByCss('p').text() - const mpa = await browser.eval('window.__TEST_NO_RELOAD === undefined') - // Flaky sometimes in Webpack: - // A. misses update and just receives `{ envValue: 'mac', mpa: false }` - // B. triggers error on server resulting in MPA: `{ envValue: 'ipad', mpa: true }` and server logs: ⨯ [TypeError: Cannot read properties of undefined (reading 'polyfillFiles')] ⨯ [TypeError: Cannot read properties of null (reading 'default')] - // A is more common than B. - expect({ envValue, mpa }).toEqual({ - envValue: - isPPREnabledByDefault && !process.env.TURBOPACK - ? // FIXME: Should be 'ipad' but PPR+Webpack swallows the update reliably - 'mac' - : 'ipad', - mpa: false, - }) } finally { await next.patchFile(envFile, envContent) } @@ -158,14 +165,26 @@ describe(`app-dir-hmr`, () => { expect(await browser.elementByCss('p').text()).toBe('ipad') }) - expect(logs).toEqual( - expect.arrayContaining([ - expect.objectContaining({ - message: expect.stringContaining('[Fast Refresh] done in'), - source: 'log', - }), - ]) - ) + if (process.env.TURBOPACK) { + // FIXME: Turbopack should have matching "done in" for each "rebuilding" + expect(logs).not.toEqual( + expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('[Fast Refresh] done in'), + source: 'log', + }), + ]) + ) + } else { + expect(logs).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('[Fast Refresh] done in'), + source: 'log', + }), + ]) + ) + } } finally { await next.patchFile(envFile, envContent) } @@ -198,14 +217,26 @@ describe(`app-dir-hmr`, () => { expect(await browser.elementByCss('p').text()).toBe('ipad') }) - expect(logs).toEqual( - expect.arrayContaining([ - expect.objectContaining({ - message: expect.stringContaining('[Fast Refresh] done in'), - source: 'log', - }), - ]) - ) + if (process.env.TURBOPACK) { + // FIXME: Turbopack should have matching "done in" for each "rebuilding" + expect(logs).not.toEqual( + expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('[Fast Refresh] done in'), + source: 'log', + }), + ]) + ) + } else { + expect(logs).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('[Fast Refresh] done in'), + source: 'log', + }), + ]) + ) + } } finally { await next.patchFile(envFile, envContent) } From 2906145ccf3d0371febb80d8748a5888732f77b6 Mon Sep 17 00:00:00 2001 From: eps1lon Date: Fri, 19 Jul 2024 21:23:58 +0200 Subject: [PATCH 2/4] Remove dev server restart workaround since flakiness is presumably fixed --- test/development/app-hmr/hmr.test.ts | 9 --------- 1 file changed, 9 deletions(-) diff --git a/test/development/app-hmr/hmr.test.ts b/test/development/app-hmr/hmr.test.ts index 8fcd585a4e064..f5f393ccc27b9 100644 --- a/test/development/app-hmr/hmr.test.ts +++ b/test/development/app-hmr/hmr.test.ts @@ -138,11 +138,6 @@ describe(`app-dir-hmr`, () => { }) it('should update server components pages when env files is changed (nodejs)', async () => { - // If "should update server components after navigating to a page with a different runtime" failed, the dev server is in a corrupted state. - // Restart fixes this. - await next.stop() - await next.start() - const envContent = await next.readFile(envFile) const browser = await next.browser('/env/node') expect(await browser.elementByCss('p').text()).toBe('mac') @@ -191,10 +186,6 @@ describe(`app-dir-hmr`, () => { }) it('should update server components pages when env files is changed (edge)', async () => { - // Restart to work around a bug highlighted in the flakiness of "should update server components after navigating to a page with a different runtime" - await next.stop() - await next.start() - const envContent = await next.readFile(envFile) const browser = await next.browser('/env/edge') expect(await browser.elementByCss('p').text()).toBe('mac') From 5192a9421c6f75815601798ceb95ff90452a1059 Mon Sep 17 00:00:00 2001 From: eps1lon Date: Fri, 19 Jul 2024 21:50:54 +0200 Subject: [PATCH 3/4] Debug why some tests start with "mac" --- test/development/app-hmr/hmr.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/test/development/app-hmr/hmr.test.ts b/test/development/app-hmr/hmr.test.ts index f5f393ccc27b9..3b100e8ecc27b 100644 --- a/test/development/app-hmr/hmr.test.ts +++ b/test/development/app-hmr/hmr.test.ts @@ -61,6 +61,7 @@ describe(`app-dir-hmr`, () => { await browser.loadPage(`${next.url}/env/edge`) await browser.eval('window.__TEST_NO_RELOAD = true') + expect(await browser.elementByCss('p').text()).toBe('mac') await next.patchFile(envFile, 'MY_DEVICE="ipad"') try { From 7c68fa57f44c0a82c71a69164466c9100683820e Mon Sep 17 00:00:00 2001 From: eps1lon Date: Fri, 19 Jul 2024 22:08:17 +0200 Subject: [PATCH 4/4] Ensure we only start next test until current one is cleaned up --- test/development/app-hmr/hmr.test.ts | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/test/development/app-hmr/hmr.test.ts b/test/development/app-hmr/hmr.test.ts index 3b100e8ecc27b..ebe2d56a934c7 100644 --- a/test/development/app-hmr/hmr.test.ts +++ b/test/development/app-hmr/hmr.test.ts @@ -55,8 +55,6 @@ describe(`app-dir-hmr`, () => { }) it('should update server components after navigating to a page with a different runtime', async () => { - const envContent = await next.readFile(envFile) - const browser = await next.browser('/env/node') await browser.loadPage(`${next.url}/env/edge`) await browser.eval('window.__TEST_NO_RELOAD = true') @@ -134,12 +132,15 @@ describe(`app-dir-hmr`, () => { ]) } } finally { - await next.patchFile(envFile, envContent) + // TOOD: use sandbox instead + await next.patchFile(envFile, 'MY_DEVICE="mac"') + await retry(async () => { + expect(await browser.elementByCss('p').text()).toBe('mac') + }) } }) it('should update server components pages when env files is changed (nodejs)', async () => { - const envContent = await next.readFile(envFile) const browser = await next.browser('/env/node') expect(await browser.elementByCss('p').text()).toBe('mac') await next.patchFile(envFile, 'MY_DEVICE="ipad"') @@ -182,12 +183,15 @@ describe(`app-dir-hmr`, () => { ) } } finally { - await next.patchFile(envFile, envContent) + // TOOD: use sandbox instead + await next.patchFile(envFile, 'MY_DEVICE="mac"') + await retry(async () => { + expect(await browser.elementByCss('p').text()).toBe('mac') + }) } }) it('should update server components pages when env files is changed (edge)', async () => { - const envContent = await next.readFile(envFile) const browser = await next.browser('/env/edge') expect(await browser.elementByCss('p').text()).toBe('mac') await next.patchFile(envFile, 'MY_DEVICE="ipad"') @@ -230,7 +234,11 @@ describe(`app-dir-hmr`, () => { ) } } finally { - await next.patchFile(envFile, envContent) + // TOOD: use sandbox instead + await next.patchFile(envFile, 'MY_DEVICE="mac"') + await retry(async () => { + expect(await browser.elementByCss('p').text()).toBe('mac') + }) } })