From a7a7870a27638fb5ae1233c3cf7b1fd10a6ba342 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Fri, 9 Jun 2023 10:42:40 +0100 Subject: [PATCH 1/3] Wait for Percy to idle before screenshot task finishes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This attempts to solve a race condition where `percy exec` starts “finalizing” the Percy build early Even after the `percySnapshot()` promise resolves, resource and snapshot uploads are still in progress See: https://github.com/percy/cli/discussions/1137#discussioncomment-6110919 This commit also uses a tighter `peerDependencies` range to bump the minimum Percy SDK utils version from 1.24.0 to 1.25.0, maybe this will help too --- package-lock.json | 16 ++++++++++++---- shared/tasks/browser.mjs | 6 +++++- shared/tasks/package.json | 8 ++++++++ 3 files changed, 25 insertions(+), 5 deletions(-) diff --git a/package-lock.json b/package-lock.json index e7f5b32407..35a3ab3c4e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -3973,10 +3973,10 @@ } }, "node_modules/@percy/sdk-utils": { - "version": "1.24.0", - "resolved": "https://registry.npmjs.org/@percy/sdk-utils/-/sdk-utils-1.24.0.tgz", - "integrity": "sha512-kfYxX0rHP5N2Da6HyfjRCVaeNahAO9XV5WD4SKWKKjdKVkV/Z5/XjVgSKlTBLSYxnWDzYJJ4UHZV43Mw+facMA==", - "dev": true, + "version": "1.25.0", + "resolved": "https://registry.npmjs.org/@percy/sdk-utils/-/sdk-utils-1.25.0.tgz", + "integrity": "sha512-jv4/jb3Neh4IgHUOuuB11XbC0Ho7dLH9plRM1JcBwxaoaTeGL9Ftau3OC126320K37yRTO4KZ+sW3jxQk/Ghsw==", + "devOptional": true, "engines": { "node": ">=14" } @@ -27653,6 +27653,14 @@ "engines": { "node": "^18.12.0", "npm": "^8.1.0 || ^9.1.0" + }, + "peerDependencies": { + "@percy/sdk-utils": ">=1.25" + }, + "peerDependenciesMeta": { + "@percy/sdk-utils": { + "optional": true + } } } } diff --git a/shared/tasks/browser.mjs b/shared/tasks/browser.mjs index 8ce26c7416..a3ff509612 100644 --- a/shared/tasks/browser.mjs +++ b/shared/tasks/browser.mjs @@ -1,4 +1,5 @@ import percySnapshot from '@percy/puppeteer' +import { waitForPercyIdle } from '@percy/sdk-utils' import { download } from 'govuk-frontend-helpers/jest/browser/download.mjs' import { goToComponent, goToExample } from 'govuk-frontend-helpers/puppeteer' import { filterPath, getComponentFiles, getComponentNames } from 'govuk-frontend-lib/files' @@ -51,7 +52,10 @@ export async function screenshots () { } // Close browser - return browser.close() + await browser.close() + + // Wait for Percy to finish + return waitForPercyIdle() } /** diff --git a/shared/tasks/package.json b/shared/tasks/package.json index 4a159c3358..53d6b50f77 100644 --- a/shared/tasks/package.json +++ b/shared/tasks/package.json @@ -29,5 +29,13 @@ "sass-embedded": "^1.62.0", "slash": "^5.1.0", "yargs-parser": "^21.1.1" + }, + "peerDependencies": { + "@percy/sdk-utils": ">=1.25" + }, + "peerDependenciesMeta": { + "@percy/sdk-utils": { + "optional": true + } } } From 3f93850cf75edc89f88ed14a5d18a88f6d3fc028 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Wed, 7 Jun 2023 16:39:44 +0100 Subject: [PATCH 2/3] Run Percy asset discovery concurrently (again) This restores the default 5x concurrent browsers for asset discovery after an attempted fix in: https://github.com/alphagov/govuk-frontend/commit/75d047d46e721ee34e05a7b90d9bed3fa424c59d --- packages/govuk-frontend-review/percy.config.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/govuk-frontend-review/percy.config.js b/packages/govuk-frontend-review/percy.config.js index 00b212409d..dc2bd2aef4 100644 --- a/packages/govuk-frontend-review/percy.config.js +++ b/packages/govuk-frontend-review/percy.config.js @@ -7,7 +7,6 @@ const { executablePath } = require('puppeteer') */ module.exports = { discovery: { - concurrency: 1, launchOptions: { executable: executablePath() } From c599ed5dee6984afc91cf956d25d6c41e0aa8f98 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Wed, 7 Jun 2023 15:56:12 +0100 Subject: [PATCH 3/3] Take Percy snapshots sequentially Percy runs healthchecks in each `percySnapshot()` call and might cause a potential race condition --- shared/tasks/browser.mjs | 27 +++++++-------------------- 1 file changed, 7 insertions(+), 20 deletions(-) diff --git a/shared/tasks/browser.mjs b/shared/tasks/browser.mjs index a3ff509612..f363d2fe2b 100644 --- a/shared/tasks/browser.mjs +++ b/shared/tasks/browser.mjs @@ -28,27 +28,14 @@ export async function screenshots () { const componentNames = await getComponentNames() const exampleNames = ['text-alignment', 'typography'] - // Screenshot stack - const input = [] - - // Add components to screenshot - input.push(...componentNames.map((screenshotName) => - /** @type {const} */ ([screenshotComponent, screenshotName]))) - - // Add examples to screenshot - input.push(...exampleNames.map((screenshotName) => - /** @type {const} */ ([screenshotExample, screenshotName]))) - - // Batch 4x concurrent screenshots - while (input.length) { - const batch = input.splice(0, 4) - - // Take screenshots - const screenshotTasks = batch.map(async ([screenshotFn, screenshotName]) => - screenshotFn(await browser.newPage(), screenshotName)) + // Screenshot components + for (const componentName of componentNames) { + await screenshotComponent(await browser.newPage(), componentName) + } - // Wait until batch finishes - await Promise.all(screenshotTasks) + // Screenshot examples + for (const exampleName of exampleNames) { + await screenshotExample(await browser.newPage(), exampleName) } // Close browser