diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 337e0e0d..9fd3db4f 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -165,9 +165,9 @@ jobs: - name: Setup Node.js and install dependencies uses: ./.github/setup-node - - name: Npm build - run: npm run build - + # No `npm run build` here. `lint:js` is `biome check`, and the shared + # Biome config excludes `**/build`; `lint:css` globs the `assets` sources + # directly. Neither reads a single file the build produces. - name: Running the lint run: npm run lint diff --git a/package-lock.json b/package-lock.json index 1bacb9f0..f22d64e0 100644 --- a/package-lock.json +++ b/package-lock.json @@ -45,7 +45,7 @@ "@babel/preset-env": "^7.23.9", "@babel/register": "^7.23.7", "@biomejs/biome": "2.5.7", - "@nk-crew/plugin-toolkit": "^0.5.1", + "@nk-crew/plugin-toolkit": "^0.6.0", "@playwright/test": "^1.45.3", "@wordpress/e2e-test-utils-playwright": "^1.4.0", "@wordpress/env": "^10.4.0", @@ -3566,9 +3566,9 @@ } }, "node_modules/@nk-crew/plugin-toolkit": { - "version": "0.5.1", - "resolved": "https://registry.npmjs.org/@nk-crew/plugin-toolkit/-/plugin-toolkit-0.5.1.tgz", - "integrity": "sha512-NMNVIZKcwQ6oRu6mSEjpOcme72s6n7f+nzRk9c+rinfjFnr1khB9tZJmLNrjBR1kE1l6FpArXLOWm4nvtZ6Ivg==", + "version": "0.6.0", + "resolved": "https://registry.npmjs.org/@nk-crew/plugin-toolkit/-/plugin-toolkit-0.6.0.tgz", + "integrity": "sha512-KU9IwgzP8JP9Ra4rlE9UI4OOn8JhHdmyMHhmWDq3DjfuFbIio+STRBlbmxVe/jLEsycESWBoHt1vdR1AH9b5mw==", "dev": true, "license": "MIT", "dependencies": { diff --git a/package.json b/package.json index 465da057..5f5278bb 100644 --- a/package.json +++ b/package.json @@ -45,7 +45,7 @@ "@babel/preset-env": "^7.23.9", "@babel/register": "^7.23.7", "@biomejs/biome": "2.5.7", - "@nk-crew/plugin-toolkit": "^0.5.1", + "@nk-crew/plugin-toolkit": "^0.6.0", "@playwright/test": "^1.45.3", "@wordpress/e2e-test-utils-playwright": "^1.4.0", "@wordpress/env": "^10.4.0", diff --git a/tests/e2e/specs/gist-block.spec.js b/tests/e2e/specs/gist-block.spec.js index f6014f29..637a345e 100644 --- a/tests/e2e/specs/gist-block.spec.js +++ b/tests/e2e/specs/gist-block.spec.js @@ -73,7 +73,22 @@ async function mockGistResponses(target, stylesheetUrl) { return stats; } -async function publishAndGetFrontendPage(page, editor) { +/** + * Publishes the post and opens its frontend. + * + * `onPageReady` runs against the new page *before* it navigates. That ordering + * is the whole point: Playwright does not buffer `console` or `pageerror` for + * listeners attached later, and `goto` resolves on `load`, which already waited + * for the async plugin scripts to execute. A listener attached after this + * function returned could never see an error thrown during script evaluation -- + * which is exactly the error these specs exist to catch. + * + * @param {Object} page Playwright page holding the editor. + * @param {Object} editor Block editor utils. + * @param {Function} onPageReady Called with the frontend page before it loads. + * @return {Object} The frontend page. + */ +async function publishAndGetFrontendPage(page, editor, onPageReady) { await editor.publishPost(); const viewPageButton = page @@ -86,14 +101,17 @@ async function publishAndGetFrontendPage(page, editor) { if (href) { const frontendPage = await page.context().newPage(); + onPageReady?.(frontendPage); + // No `waitForLoadState('domcontentloaded')` after this: `goto` resolves + // on `load`, which is strictly later, so the wait was a no-op. await frontendPage.goto(href); - await frontendPage.waitForLoadState('domcontentloaded'); return frontendPage; } const popupPromise = page.waitForEvent('popup', { timeout: 3000 }); await viewPageButton.click(); const popupPage = await popupPromise; + onPageReady?.(popupPage); await popupPage.waitForLoadState('domcontentloaded'); return popupPage; @@ -143,13 +161,15 @@ test.describe('gist block', () => { }, }); - await page.waitForTimeout(1000); - await expect( editor.canvas.locator('.ghostkit-gist').first() ).toBeVisible(); - await expect(mockStats.jsonRequests).toBeGreaterThan(0); - await expect(mockStats.cssRequests).toBeGreaterThan(0); + // `expect.poll`, not `expect`: `mockStats` is a plain object the route + // handlers mutate, so a bare `expect` reads it once with no retry. That + // is why a flat 1s sleep used to sit above this -- polling ends as soon + // as the mock has fired instead. + await expect.poll(() => mockStats.jsonRequests).toBeGreaterThan(0); + await expect.poll(() => mockStats.cssRequests).toBeGreaterThan(0); const relevantErrors = runtimeErrors.filter( (message) => @@ -191,16 +211,28 @@ test.describe('gist block', () => { }, }); - const frontendPage = await publishAndGetFrontendPage(page, editor); - const runtimeErrors = trackRuntimeErrors(frontendPage); - - await frontendPage.waitForTimeout(1000); + // Attached through the callback so the listeners exist before the page + // navigates. Attaching them to the returned page, as this did, meant the + // script had already run and any error was long gone -- `runtimeErrors` + // was always empty and the assertion below could not fail. + let runtimeErrors = []; + const frontendPage = await publishAndGetFrontendPage( + page, + editor, + (frontend) => { + runtimeErrors = trackRuntimeErrors(frontend); + } + ); await expect( frontendPage.locator('.ghostkit-gist').first() ).toBeVisible(); - await expect(mockStats.jsonRequests).toBeGreaterThan(0); - await expect(mockStats.cssRequests).toBeGreaterThan(0); + // `expect.poll`, not `expect`: `mockStats` is a plain object the route + // handlers mutate, so a bare `expect` reads it once with no retry. That + // is why a flat 1s sleep used to sit above this -- polling ends as soon + // as the mock has fired instead. + await expect.poll(() => mockStats.jsonRequests).toBeGreaterThan(0); + await expect.poll(() => mockStats.cssRequests).toBeGreaterThan(0); const relevantErrors = runtimeErrors.filter( (message) => diff --git a/tests/e2e/specs/lottie-block.spec.js b/tests/e2e/specs/lottie-block.spec.js index ffadd3ed..00854d62 100644 --- a/tests/e2e/specs/lottie-block.spec.js +++ b/tests/e2e/specs/lottie-block.spec.js @@ -61,7 +61,22 @@ async function mockLottieResponse(target) { }); } -async function publishAndGetFrontendPage(page, editor) { +/** + * Publishes the post and opens its frontend. + * + * `onPageReady` runs against the new page *before* it navigates. That ordering + * is the whole point: Playwright does not buffer `console` or `pageerror` for + * listeners attached later, and `goto` resolves on `load`, which already waited + * for the async plugin scripts to execute. A listener attached after this + * function returned could never see an error thrown during script evaluation -- + * which is exactly the error these specs exist to catch. + * + * @param {Object} page Playwright page holding the editor. + * @param {Object} editor Block editor utils. + * @param {Function} onPageReady Called with the frontend page before it loads. + * @return {Object} The frontend page. + */ +async function publishAndGetFrontendPage(page, editor, onPageReady) { await editor.publishPost(); const viewPageButton = page @@ -74,14 +89,17 @@ async function publishAndGetFrontendPage(page, editor) { if (href) { const frontendPage = await page.context().newPage(); + onPageReady?.(frontendPage); + // No `waitForLoadState('domcontentloaded')` after this: `goto` resolves + // on `load`, which is strictly later, so the wait was a no-op. await frontendPage.goto(href); - await frontendPage.waitForLoadState('domcontentloaded'); return frontendPage; } const popupPromise = page.waitForEvent('popup', { timeout: 3000 }); await viewPageButton.click(); const popupPage = await popupPromise; + onPageReady?.(popupPage); await popupPage.waitForLoadState('domcontentloaded'); return popupPage; @@ -164,8 +182,18 @@ test.describe('lottie block', () => { }, }); - const frontendPage = await publishAndGetFrontendPage(page, editor); - const runtimeErrors = trackRuntimeErrors(frontendPage); + // Attached through the callback so the listeners exist before the page + // navigates. Attaching them to the returned page, as this did, meant the + // script had already run and any error was long gone -- `runtimeErrors` + // was always empty and the assertion below could not fail. + let runtimeErrors = []; + const frontendPage = await publishAndGetFrontendPage( + page, + editor, + (frontend) => { + runtimeErrors = trackRuntimeErrors(frontend); + } + ); await expect( frontendPage.locator('lottie-player').first() diff --git a/tests/e2e/specs/typography.spec.js b/tests/e2e/specs/typography.spec.js index 3e7789c5..93aef477 100644 --- a/tests/e2e/specs/typography.spec.js +++ b/tests/e2e/specs/typography.spec.js @@ -56,10 +56,24 @@ test.describe('typography', () => { await fontInput.fill(font); + // Wait for the save, not for a guess at how long it takes. The settings + // page debounces by exactly 1000ms and then fires the POST without + // awaiting it, so `waitForTimeout(1000)` expired as the request was + // being issued -- and the `page` fixture closes the page at teardown, + // discarding anything still in flight. When it lost that race the + // symptom appeared in a later test, as a font mismatch. + // + // Registered before the keypress: the response is a second away, but the + // listener still has to exist before the thing that triggers it. + const saved = page.waitForResponse( + (response) => + response.url().includes('update_custom_typography') && + response.request().method() === 'POST' + ); + await fontInput.press('Enter'); - // Just wait 1 second for the selected font to be saved in the database after selection. - await page.waitForTimeout(1000); + await saved; return wrapper; }