From b48c205701cdc4338756b4e68ab143bf72cd0319 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Mon, 14 Aug 2023 14:01:56 -0700 Subject: [PATCH 1/5] WIP: upload unit test failure artifacts --- .github/workflows/unit.yml | 7 +++ core/test/scenarios/pptr-test-utils.js | 63 ++++++++++++++++++-------- 2 files changed, 51 insertions(+), 19 deletions(-) diff --git a/.github/workflows/unit.yml b/.github/workflows/unit.yml index 9f07711e16ff..80a45b24926d 100644 --- a/.github/workflows/unit.yml +++ b/.github/workflows/unit.yml @@ -84,6 +84,13 @@ jobs: - name: yarn test-viewer run: yarn build-viewer && xvfb-run --auto-servernum bash $GITHUB_WORKSPACE/.github/scripts/test-retry.sh yarn test-viewer + - name: Upload failures + if: failure() + uses: actions/upload-artifact@v1 + with: + name: Unit (ubuntu; node ${{ matrix.node }}) + path: .tmp/unit-failures/ + # For windows, just test the potentially platform-specific code. unit-windows: runs-on: windows-latest diff --git a/core/test/scenarios/pptr-test-utils.js b/core/test/scenarios/pptr-test-utils.js index 5bfe6e681653..885dd4611c6a 100644 --- a/core/test/scenarios/pptr-test-utils.js +++ b/core/test/scenarios/pptr-test-utils.js @@ -4,11 +4,14 @@ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. */ +import fs from 'fs'; + import {before, beforeEach, after, afterEach} from 'mocha'; import * as puppeteer from 'puppeteer-core'; import {getChromePath} from 'chrome-launcher'; import {Server} from '../../../cli/test/fixtures/static-server.js'; +import {LH_ROOT} from '../../../root.js'; /** @typedef {InstanceType} StaticServer */ @@ -22,19 +25,32 @@ const FLAKY_AUDIT_IDS_APPLICABILITY = new Set([ 'layout-shift-elements', // Depends on if the JS takes too long after input to be ignored for layout shift. ]); +const UNIT_OUTPUT_DIR = `${LH_ROOT}/.tmp/unit-failures`; + function createTestState() { /** @param {string} name @return {any} */ const any = name => new Proxy({}, {get: () => { throw new Error(`${name} used without invoking \`state.before\``); }}); + /** @type {puppeteer.Browser} */ + let browser = any('browser'); + /** @type {puppeteer.Page} */ + let page = any('page'); + /** @type {StaticServer} */ + let server = any('server'); + /** @type {StaticServer} */ + let secondaryServer = any('server'); + let serverBaseUrl = ''; + let secondaryServerBaseUrl = ''; + return { - browser: /** @type {puppeteer.Browser} */ (any('browser')), - page: /** @type {puppeteer.Page} */ (any('page')), - server: /** @type {StaticServer} */ (any('server')), - secondaryServer: /** @type {StaticServer} */ (any('server')), - serverBaseUrl: '', - secondaryServerBaseUrl: '', + browser, + page, + server, + secondaryServer, + serverBaseUrl, + secondaryServerBaseUrl, /** * @param {number=} port @@ -42,17 +58,17 @@ function createTestState() { */ installServerHooks(port = 10200, secondaryPort = 10503) { before(async () => { - this.server = new Server(port); - this.secondaryServer = new Server(secondaryPort); - await this.server.listen(port, '127.0.0.1'); - await this.secondaryServer.listen(secondaryPort, '127.0.0.1'); - this.serverBaseUrl = `http://localhost:${this.server.getPort()}`; - this.secondaryServerBaseUrl = `http://localhost:${this.secondaryServer.getPort()}`; + server = new Server(port); + secondaryServer = new Server(secondaryPort); + await server.listen(port, '127.0.0.1'); + await secondaryServer.listen(secondaryPort, '127.0.0.1'); + serverBaseUrl = `http://localhost:${this.server.getPort()}`; + secondaryServerBaseUrl = `http://localhost:${this.secondaryServer.getPort()}`; }); after(async () => { - await this.server.close(); - await this.secondaryServer.close(); + await server.close(); + await secondaryServer.close(); }); }, @@ -60,7 +76,7 @@ function createTestState() { this.installServerHooks(); before(async () => { - this.browser = await puppeteer.launch({ + browser = await puppeteer.launch({ headless: true, executablePath: getChromePath(), ignoreDefaultArgs: ['--enable-automation'], @@ -68,15 +84,24 @@ function createTestState() { }); beforeEach(async () => { - this.page = await this.browser.newPage(); + page = await browser.newPage(); + await page.tracing.start(); }); - afterEach(async () => { - await this.page.close(); + afterEach(async function() { + await page.close(); + const traceData = await page.tracing.stop(); + // eslint-disable-next-line no-invalid-this + const currentTest = this.currentTest; + if (currentTest?.state === 'failed' && traceData) { + const testOutputDir = `${UNIT_OUTPUT_DIR}/${currentTest.fullTitle()}`; + fs.mkdirSync(testOutputDir, {recursive: true}); + fs.writeFileSync(`${testOutputDir}/trace.json`, traceData); + } }); after(async () => { - await this.browser.close(); + await browser.close(); }); }, }; From 120dd290ad2c7f86c5350d05d728092fb09e09ac Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Mon, 14 Aug 2023 15:49:40 -0700 Subject: [PATCH 2/5] fix --- core/test/scenarios/api-test-pptr.js | 7 ++ core/test/scenarios/pptr-test-utils.js | 70 ++++++++++--------- .../start-end-navigation-test-pptr.js | 2 + 3 files changed, 45 insertions(+), 34 deletions(-) diff --git a/core/test/scenarios/api-test-pptr.js b/core/test/scenarios/api-test-pptr.js index b293ee81d286..811141e86467 100644 --- a/core/test/scenarios/api-test-pptr.js +++ b/core/test/scenarios/api-test-pptr.js @@ -75,6 +75,7 @@ describe('Individual modes API', function() { if (!result) throw new Error('Lighthouse failed to produce a result'); const {lhr, artifacts} = result; + state.saveTrace(artifacts.Trace); expect(artifacts.URL).toEqual({ finalDisplayedUrl: `${state.serverBaseUrl}/onclick.html#done`, }); @@ -133,6 +134,8 @@ describe('Individual modes API', function() { if (!result) throw new Error('Lighthouse failed to produce a result'); + state.saveTrace(result.artifacts.Trace); + expect(result.artifacts.URL).toEqual({ finalDisplayedUrl: `${serverBaseUrl}/onclick.html#done`, }); @@ -165,6 +168,8 @@ describe('Individual modes API', function() { if (!result) throw new Error('Lighthouse failed to produce a result'); + state.saveTrace(result.artifacts.Trace); + const networkRequestsDetails = /** @type {LH.Audit.Details.Table} */ ( result.lhr.audits['network-requests'].details); const networkRequests = networkRequestsDetails?.items @@ -222,6 +227,7 @@ Array [ if (!result) throw new Error('Lighthouse failed to produce a result'); const {lhr, artifacts} = result; + state.saveTrace(artifacts.Trace); expect(artifacts.URL).toEqual({ requestedUrl: url, mainDocumentUrl: url, @@ -263,6 +269,7 @@ Array [ expect(requestor).toHaveBeenCalled(); const {lhr, artifacts} = result; + state.saveTrace(artifacts.Trace); expect(lhr.requestedUrl).toEqual(requestedUrl); expect(lhr.finalDisplayedUrl).toEqual(mainDocumentUrl); expect(artifacts.URL).toEqual({ diff --git a/core/test/scenarios/pptr-test-utils.js b/core/test/scenarios/pptr-test-utils.js index 885dd4611c6a..93278c12eb2b 100644 --- a/core/test/scenarios/pptr-test-utils.js +++ b/core/test/scenarios/pptr-test-utils.js @@ -33,24 +33,16 @@ function createTestState() { throw new Error(`${name} used without invoking \`state.before\``); }}); - /** @type {puppeteer.Browser} */ - let browser = any('browser'); - /** @type {puppeteer.Page} */ - let page = any('page'); - /** @type {StaticServer} */ - let server = any('server'); - /** @type {StaticServer} */ - let secondaryServer = any('server'); - let serverBaseUrl = ''; - let secondaryServerBaseUrl = ''; + /** @type {LH.Trace|undefined} */ + let trace; return { - browser, - page, - server, - secondaryServer, - serverBaseUrl, - secondaryServerBaseUrl, + browser: /** @type {puppeteer.Browser} */ (any('browser')), + page: /** @type {puppeteer.Page} */ (any('page')), + server: /** @type {StaticServer} */ (any('server')), + secondaryServer: /** @type {StaticServer} */ (any('server')), + serverBaseUrl: '', + secondaryServerBaseUrl: '', /** * @param {number=} port @@ -58,17 +50,17 @@ function createTestState() { */ installServerHooks(port = 10200, secondaryPort = 10503) { before(async () => { - server = new Server(port); - secondaryServer = new Server(secondaryPort); - await server.listen(port, '127.0.0.1'); - await secondaryServer.listen(secondaryPort, '127.0.0.1'); - serverBaseUrl = `http://localhost:${this.server.getPort()}`; - secondaryServerBaseUrl = `http://localhost:${this.secondaryServer.getPort()}`; + this.server = new Server(port); + this.secondaryServer = new Server(secondaryPort); + await this.server.listen(port, '127.0.0.1'); + await this.secondaryServer.listen(secondaryPort, '127.0.0.1'); + this.serverBaseUrl = `http://localhost:${this.server.getPort()}`; + this.secondaryServerBaseUrl = `http://localhost:${this.secondaryServer.getPort()}`; }); after(async () => { - await server.close(); - await secondaryServer.close(); + await this.server.close(); + await this.secondaryServer.close(); }); }, @@ -76,7 +68,7 @@ function createTestState() { this.installServerHooks(); before(async () => { - browser = await puppeteer.launch({ + this.browser = await puppeteer.launch({ headless: true, executablePath: getChromePath(), ignoreDefaultArgs: ['--enable-automation'], @@ -84,26 +76,36 @@ function createTestState() { }); beforeEach(async () => { - page = await browser.newPage(); - await page.tracing.start(); + trace = undefined; + this.page = await this.browser.newPage(); }); - afterEach(async function() { - await page.close(); - const traceData = await page.tracing.stop(); + afterEach(async () => { + await this.page.close(); + }); + + afterEach(function() { // eslint-disable-next-line no-invalid-this const currentTest = this.currentTest; - if (currentTest?.state === 'failed' && traceData) { - const testOutputDir = `${UNIT_OUTPUT_DIR}/${currentTest.fullTitle()}`; + if (currentTest?.state === 'failed' && trace) { + const dirname = currentTest.fullTitle().replace(/[^a-z0-9]/gi, '_').toLowerCase(); + const testOutputDir = `${UNIT_OUTPUT_DIR}/${dirname}`; fs.mkdirSync(testOutputDir, {recursive: true}); - fs.writeFileSync(`${testOutputDir}/trace.json`, traceData); + fs.writeFileSync(`${testOutputDir}/trace.json`, JSON.stringify(trace, null, 2)); } }); after(async () => { - await browser.close(); + await this.browser.close(); }); }, + + /** + * @param {LH.Trace} testTrace + */ + saveTrace(testTrace) { + trace = testTrace; + }, }; } diff --git a/core/test/scenarios/start-end-navigation-test-pptr.js b/core/test/scenarios/start-end-navigation-test-pptr.js index 294489bf5a9a..efa50d1e1b2e 100644 --- a/core/test/scenarios/start-end-navigation-test-pptr.js +++ b/core/test/scenarios/start-end-navigation-test-pptr.js @@ -37,6 +37,8 @@ describe('Start/End navigation', function() { const lhr = flowResult.steps[0].lhr; const artifacts = flowArtifacts.gatherSteps[0].artifacts; + state.saveTrace(artifacts.Trace); + expect(artifacts.URL).toEqual({ requestedUrl: `${state.serverBaseUrl}/?redirect=/index.html`, mainDocumentUrl: `${state.serverBaseUrl}/index.html`, From a98ba71299e837a2a988f187be9900c89ee65079 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Tue, 15 Aug 2023 11:52:49 -0700 Subject: [PATCH 3/5] retries again --- core/test/scripts/run-mocha-tests.js | 6 ++++++ package.json | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/core/test/scripts/run-mocha-tests.js b/core/test/scripts/run-mocha-tests.js index eb237a07567f..09af9b8c9232 100644 --- a/core/test/scripts/run-mocha-tests.js +++ b/core/test/scripts/run-mocha-tests.js @@ -146,6 +146,9 @@ const rawArgv = y 'require': { type: 'string', }, + 'retries': { + type: 'number', + }, }) .wrap(y.terminalWidth()) .argv; @@ -259,6 +262,7 @@ function exit({numberFailures, numberMochaInvocations}) { * @property {boolean} bail * @property {boolean} parallel * @property {string | undefined} require + * @property {number | undefined} retries */ /** @@ -281,6 +285,7 @@ async function runMocha(tests, mochaArgs, invocationNumber) { // TODO: not working // parallel: tests.length > 1 && mochaArgs.parallel, parallel: false, + retries: mochaArgs.retries, }); // @ts-expect-error - not in types. @@ -325,6 +330,7 @@ async function main() { bail: argv.bail, parallel: argv.parallel, require: argv.require, + retries: argv.retries, }; mochaGlobalSetup(); diff --git a/package.json b/package.json index ba80efafba03..5135f65742ce 100644 --- a/package.json +++ b/package.json @@ -56,7 +56,7 @@ "unit-viewer": "yarn mocha --testMatch viewer/**/*-test.js", "unit-flow": "bash flow-report/test/run-flow-report-tests.sh", "unit": "yarn unit-flow && yarn mocha", - "unit:ci": "NODE_OPTIONS=--max-old-space-size=8192 npm run unit --forbid-only", + "unit:ci": "NODE_OPTIONS=--max-old-space-size=8192 npm run unit --forbid-only --retries=5", "core-unit": "yarn unit-core", "cli-unit": "yarn unit-cli", "viewer-unit": "yarn unit-viewer", From f54b888a3efd5d566167619eebda6a8c0432d99c Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Tue, 15 Aug 2023 12:44:58 -0700 Subject: [PATCH 4/5] log tries --- core/test/scenarios/pptr-test-utils.js | 1 + 1 file changed, 1 insertion(+) diff --git a/core/test/scenarios/pptr-test-utils.js b/core/test/scenarios/pptr-test-utils.js index 93278c12eb2b..dd8a4beff855 100644 --- a/core/test/scenarios/pptr-test-utils.js +++ b/core/test/scenarios/pptr-test-utils.js @@ -77,6 +77,7 @@ function createTestState() { beforeEach(async () => { trace = undefined; + console.log('########'); this.page = await this.browser.newPage(); }); From 6577d34c08284736e1b3355a4b75238e25101fea Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Tue, 15 Aug 2023 14:03:42 -0700 Subject: [PATCH 5/5] fix --- core/test/scripts/run-mocha-tests.js | 8 ++++++++ package.json | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/core/test/scripts/run-mocha-tests.js b/core/test/scripts/run-mocha-tests.js index 09af9b8c9232..d1a872ca710c 100644 --- a/core/test/scripts/run-mocha-tests.js +++ b/core/test/scripts/run-mocha-tests.js @@ -148,6 +148,11 @@ const rawArgv = y }, 'retries': { type: 'number', + default: process.env.CI ? 5 : undefined, + }, + 'forbidOnly': { + type: 'boolean', + default: Boolean(process.env.CI), }, }) .wrap(y.terminalWidth()) @@ -260,6 +265,7 @@ function exit({numberFailures, numberMochaInvocations}) { * @typedef OurMochaArgs * @property {RegExp | string | undefined} grep * @property {boolean} bail + * @property {boolean} forbidOnly * @property {boolean} parallel * @property {string | undefined} require * @property {number | undefined} retries @@ -282,6 +288,7 @@ async function runMocha(tests, mochaArgs, invocationNumber) { timeout: 20_000, bail: mochaArgs.bail, grep: mochaArgs.grep, + forbidOnly: mochaArgs.forbidOnly, // TODO: not working // parallel: tests.length > 1 && mochaArgs.parallel, parallel: false, @@ -331,6 +338,7 @@ async function main() { parallel: argv.parallel, require: argv.require, retries: argv.retries, + forbidOnly: argv.forbidOnly, }; mochaGlobalSetup(); diff --git a/package.json b/package.json index 5135f65742ce..f33e29a57ebb 100644 --- a/package.json +++ b/package.json @@ -56,7 +56,7 @@ "unit-viewer": "yarn mocha --testMatch viewer/**/*-test.js", "unit-flow": "bash flow-report/test/run-flow-report-tests.sh", "unit": "yarn unit-flow && yarn mocha", - "unit:ci": "NODE_OPTIONS=--max-old-space-size=8192 npm run unit --forbid-only --retries=5", + "unit:ci": "NODE_OPTIONS=--max-old-space-size=8192 npm run unit", "core-unit": "yarn unit-core", "cli-unit": "yarn unit-cli", "viewer-unit": "yarn unit-viewer",