From 2d8208e007a4a6b42015d98db1625b29cd100e27 Mon Sep 17 00:00:00 2001 From: Timmy Willison Date: Mon, 11 Mar 2024 10:45:17 -0400 Subject: [PATCH] Tests: add --hard-retries option to test runner - Add the ability to retry by restarting the worker and getting a different browser instance, after all normal retries have been exhausted. This can sometimes be successful when a refresh is not. Close gh-5439 --- .github/workflows/browserstack.yml | 2 +- test/runner/browserstack/browsers.js | 38 ++++++++++++++++------------ test/runner/browserstack/queue.js | 33 ++++++++++++++++++++++-- test/runner/command.js | 20 ++++++++++----- test/runner/createTestServer.js | 30 +++++++++++++--------- test/runner/run.js | 9 ++++++- 6 files changed, 94 insertions(+), 38 deletions(-) diff --git a/.github/workflows/browserstack.yml b/.github/workflows/browserstack.yml index 8e2d91fa4..0f434dd8e 100644 --- a/.github/workflows/browserstack.yml +++ b/.github/workflows/browserstack.yml @@ -77,4 +77,4 @@ jobs: run: npm run pretest - name: Run tests - run: npm run test:unit -- -v --browserstack "${{ matrix.BROWSER }}" --run-id ${{ github.run_id }} --isolate --retries 3 + run: npm run test:unit -- -v --browserstack "${{ matrix.BROWSER }}" --run-id ${{ github.run_id }} --isolate --retries 3 --hard-retries 1 diff --git a/test/runner/browserstack/browsers.js b/test/runner/browserstack/browsers.js index 3a7da4fc9..218d13fe7 100644 --- a/test/runner/browserstack/browsers.js +++ b/test/runner/browserstack/browsers.js @@ -66,7 +66,25 @@ async function waitForAck( worker, { fullBrowser, verbose } ) { } ); } -async function ensureAcknowledged( worker, restarts ) { +async function restartWorker( worker ) { + await cleanupWorker( worker, worker.options ); + await createBrowserWorker( + worker.url, + worker.browser, + worker.options, + worker.restarts + 1 + ); +} + +export async function restartBrowser( browser ) { + const fullBrowser = getBrowserString( browser ); + const worker = workers[ fullBrowser ]; + if ( worker ) { + await restartWorker( worker ); + } +} + +async function ensureAcknowledged( worker ) { const fullBrowser = getBrowserString( worker.browser ); const verbose = worker.options.verbose; try { @@ -74,13 +92,7 @@ async function ensureAcknowledged( worker, restarts ) { return worker; } catch ( error ) { console.error( error.message ); - await cleanupWorker( worker, { verbose } ); - await createBrowserWorker( - worker.url, - worker.browser, - worker.options, - restarts + 1 - ); + await restartWorker( worker.browser ); } } @@ -132,7 +144,7 @@ export async function createBrowserWorker( url, browser, options, restarts = 0 ) // Wait for the worker to show up in the list // before returning it. - return ensureAcknowledged( worker, restarts ); + return ensureAcknowledged( worker ); } export async function setBrowserWorkerUrl( browser, url ) { @@ -159,13 +171,7 @@ export async function checkLastTouches() { }min.` ); } - await cleanupWorker( worker, options ); - await createBrowserWorker( - worker.url, - worker.browser, - options, - worker.restarts - ); + await restartWorker( worker ); } } } diff --git a/test/runner/browserstack/queue.js b/test/runner/browserstack/queue.js index c948f29bf..6d1c8d51f 100644 --- a/test/runner/browserstack/queue.js +++ b/test/runner/browserstack/queue.js @@ -1,6 +1,11 @@ import chalk from "chalk"; import { getBrowserString } from "../lib/getBrowserString.js"; -import { checkLastTouches, createBrowserWorker, setBrowserWorkerUrl } from "./browsers.js"; +import { + checkLastTouches, + createBrowserWorker, + restartBrowser, + setBrowserWorkerUrl +} from "./browsers.js"; const TEST_POLL_TIMEOUT = 1000; @@ -32,6 +37,9 @@ export function getNextBrowserTest( reportId ) { } export function retryTest( reportId, maxRetries ) { + if ( !maxRetries ) { + return; + } const test = queue.find( ( test ) => test.id === reportId ); if ( test ) { test.retries++; @@ -46,10 +54,31 @@ export function retryTest( reportId, maxRetries ) { } } +export async function hardRetryTest( reportId, maxHardRetries ) { + if ( !maxHardRetries ) { + return false; + } + const test = queue.find( ( test ) => test.id === reportId ); + if ( test ) { + test.hardRetries++; + if ( test.hardRetries <= maxHardRetries ) { + console.log( + `Hard retrying test ${ reportId } for ${ chalk.yellow( + test.options.modules.join( ", " ) + ) }...${ test.hardRetries }` + ); + await restartBrowser( test.browser ); + return true; + } + } + return false; +} + export function addBrowserStackRun( url, browser, options ) { queue.push( { browser, fullBrowser: getBrowserString( browser ), + hardRetries: 0, id: options.reportId, url, options, @@ -59,7 +88,7 @@ export function addBrowserStackRun( url, browser, options ) { } export async function runAllBrowserStack() { - return new Promise( async( resolve, reject )=> { + return new Promise( async( resolve, reject ) => { while ( queue.length ) { try { await checkLastTouches(); diff --git a/test/runner/command.js b/test/runner/command.js index 0f06a5dff..fb1d16883 100644 --- a/test/runner/command.js +++ b/test/runner/command.js @@ -63,12 +63,6 @@ const argv = yargs( process.argv.slice( 2 ) ) type: "boolean", description: "Log additional information." } ) - .option( "retries", { - alias: "r", - type: "number", - description: "Number of times to retry failed tests in BrowserStack.", - implies: [ "browserstack" ] - } ) .option( "run-id", { type: "string", description: "A unique identifier for this run." @@ -89,6 +83,20 @@ const argv = yargs( process.argv.slice( 2 ) ) "Otherwise, the --browser option will be used, " + "with the latest version/device for that browser, on a matching OS." } ) + .option( "retries", { + alias: "r", + type: "number", + description: "Number of times to retry failed tests in BrowserStack.", + implies: [ "browserstack" ] + } ) + .option( "hard-retries", { + type: "number", + description: + "Number of times to retry failed tests in BrowserStack " + + "by restarting the worker. This is in addition to the normal retries " + + "and are only used when the normal retries are exhausted.", + implies: [ "browserstack" ] + } ) .option( "list-browsers", { type: "string", description: diff --git a/test/runner/createTestServer.js b/test/runner/createTestServer.js index 81eb1bd6a..cd82844d5 100644 --- a/test/runner/createTestServer.js +++ b/test/runner/createTestServer.js @@ -26,23 +26,29 @@ export async function createTestServer( report ) { // Add a script tag to the index.html to load the QUnit listeners app.use( /\/test(?:\/index.html)?\//, ( _req, res ) => { - res.send( indexHTML.replace( - "", - "" - ) ); + res.send( + indexHTML.replace( + "", + "" + ) + ); } ); // Bind the reporter - app.post( "/api/report", bodyParser.json( { limit: "50mb" } ), ( req, res ) => { - if ( report ) { - const response = report( req.body ); - if ( response ) { - res.json( response ); - return; + app.post( + "/api/report", + bodyParser.json( { limit: "50mb" } ), + async( req, res ) => { + if ( report ) { + const response = await report( req.body ); + if ( response ) { + res.json( response ); + return; + } } + res.sendStatus( 204 ); } - res.sendStatus( 204 ); - } ); + ); // Handle errors from the body parser app.use( bodyParserErrorHandler() ); diff --git a/test/runner/run.js b/test/runner/run.js index 0de46f3bb..0b3ffec30 100644 --- a/test/runner/run.js +++ b/test/runner/run.js @@ -14,6 +14,7 @@ import { cleanupAllBrowsers, touchBrowser } from "./browserstack/browsers.js"; import { addBrowserStackRun, getNextBrowserTest, + hardRetryTest, retryTest, runAllBrowserStack } from "./browserstack/queue.js"; @@ -30,6 +31,7 @@ export async function run( { browserstack, concurrency, debug, + hardRetries, headless, isolate, modules = [], @@ -72,7 +74,7 @@ export async function run( { // Create the test app and // hook it up to the reporter const reports = Object.create( null ); - const app = await createTestServer( ( message ) => { + const app = await createTestServer( async( message ) => { switch ( message.type ) { case "testEnd": { const reportId = message.id; @@ -120,6 +122,11 @@ export async function run( { if ( retry ) { return retry; } + + // Return early if hardRetryTest returns true + if ( await hardRetryTest( reportId, hardRetries ) ) { + return; + } errorMessages.push( ...Object.values( pendingErrors[ reportId ] ) ); } -- 2.39.5