From 52163beb8f0f7047003508cafc2ab0b7796074ff Mon Sep 17 00:00:00 2001 From: Andrew Huth Date: Wed, 16 Oct 2024 11:43:46 -0400 Subject: [PATCH] Simplify promise queue implementation (#102) * Simplify promise queue implementation * Edit comment explaining the promise queue * Move reject to its own catch handler Just to make sure promise rejections are handled. * Add changelog entry * Move defaultDisabledRules out of the Result object And into the AxePage code, with the rest of the axe-core logic. * Move AxePage stuff out of the Result object Keep all Axe and playwright stuff in the "Browser" code. * Fix indentation * Add another changelog entry --- CHANGELOG.md | 3 ++ src/Result.ts | 35 ++---------- src/browser/AxePage.ts | 108 ++++++++++++------------------------- src/browser/TestBrowser.ts | 10 +++- 4 files changed, 48 insertions(+), 108 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f3909c9..29e0a09 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## Unreleased +- [fix] Simplify the promise queue implementation [#102](https://github.com/chanzuckerberg/axe-storybook-testing/pull/102) +- [fix] Minor code re-organizations [#102](https://github.com/chanzuckerberg/axe-storybook-testing/pull/102) + ## 8.2.0 - [new] Add parameter for Axe `context` [#98](https://github.com/chanzuckerberg/axe-storybook-testing/pull/98) diff --git a/src/Result.ts b/src/Result.ts index 54b4a40..9188f4e 100644 --- a/src/Result.ts +++ b/src/Result.ts @@ -1,40 +1,11 @@ import type {Result as AxeResult, NodeResult} from 'axe-core'; import indent from 'indent-string'; -import type {Page} from 'playwright'; import dedent from 'ts-dedent'; -import type ProcessedStory from './ProcessedStory'; -import {analyze} from './browser/AxePage'; - -/** - * These rules aren't useful/helpful in the context of Storybook stories, and we disable them when - * running Axe. - */ -const defaultDisabledRules = [ - 'bypass', - 'landmark-one-main', - 'page-has-heading-one', - 'region', -]; /** * Violations reported by Axe for a story. */ export default class Result { - /** - * Run Axe on a browser page that is displaying a story. - */ - static async fromPage(page: Page, story: ProcessedStory) { - const disabledRules = [...defaultDisabledRules, ...story.disabledRules]; - const axeResults = await analyze( - page, - disabledRules, - story.runOptions, - story.context, - story.config, - ); - return new Result(axeResults.violations); - } - violations: AxeResult[]; constructor(violations: AxeResult[]) { @@ -58,10 +29,10 @@ export default class Result { toString(): string { return dedent` - Detected the following accessibility violations! + Detected the following accessibility violations! - ${this.violations.map(formatViolation).join('\n\n')} - `; + ${this.violations.map(formatViolation).join('\n\n')} + `; } } diff --git a/src/browser/AxePage.ts b/src/browser/AxePage.ts index be0398c..099a7c9 100644 --- a/src/browser/AxePage.ts +++ b/src/browser/AxePage.ts @@ -8,11 +8,28 @@ import type { } from 'axe-core'; import type {Page} from 'playwright'; +// Functions we pass to `page.evaluate` execute in a browser environment, and can access window. +// eslint-disable-next-line no-var +declare var window: { + enqueuePromise: (createPromise: () => Promise) => Promise; +}; + export type Context = | SerialFrameSelector | SerialFrameSelector[] | SerialContextObject; +/** + * These rules aren't useful/helpful in the context of Storybook stories, and we disable them when + * running Axe. + */ +const defaultDisabledRules = [ + 'bypass', + 'landmark-one-main', + 'page-has-heading-one', + 'region', +]; + /** * Prepare a page for running axe on it. */ @@ -33,7 +50,10 @@ export function analyze( config?: Spec, ): Promise { return page.evaluate(runAxe, { - options: getRunOptions(runOptions, disabledRules), + options: getRunOptions(runOptions, [ + ...defaultDisabledRules, + ...disabledRules, + ]), config, context, }); @@ -50,7 +70,7 @@ function runAxe({ }): Promise { // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore This function executes in a browser context. - return window.axeQueue.add(() => { + return window.enqueuePromise(() => { // Always reset the axe config, so if one story sets its own config it doesn't affect the // others. // eslint-disable-next-line @typescript-eslint/ban-ts-comment @@ -86,82 +106,20 @@ export function getRunOptions( } /** - * Add a Queue implementation for promises, forcing a single promise to run at a time. + * Add a promise queue so we can ensure only one promise runs at a time. * - * This will be used to ensure that we only run one `axe.run` call at a time. We will never - * intentionally run multiple at a time. However, a component throwing an error during its - * lifecycle can result in a test finishing and proceeding to the next one, but the previous - * `axe.run` call still "running" when the next one starts. This results in an error (see - * https://github.com/dequelabs/axe-core/issues/1041). + * Used to prevent concurrent runs of `axe.run`, which breaks (see https://github.com/dequelabs/axe-core/issues/1041). + * This should never happen, but in the past errors during rendering at the right/wrong time has + * caused the next test to start before the previous has stopped. * - * We avoid that by forcing one `axe.run` call to finish before the next one can start. Got the - * idea from https://github.com/dequelabs/agnostic-axe/pull/6. + * Got the idea from https://github.com/dequelabs/agnostic-axe/pull/6. */ function addPromiseQueue() { - type QueuedPromise = { - promiseCreator: () => Promise; - resolve: (value: T | PromiseLike) => void; - reject: (reason?: unknown) => void; - }; - - /** - * Queue of promises, which forces any promises added to it to run one at a time. - * - * This was originally implemented as an ES6 class. But I ran into a lot of issues with how the - * class's properties were compiled not working in different environments and browsers, and even - * breaking when Babel was updated. - * - * To avoid that, I've instead implemented this as a function that maintains some state within a - * closure. Since there are no class properties (which can be compiled by Babel in different ways), - * there are no more problems. - */ - function PromiseQueue() { - const pending: QueuedPromise[] = []; - let working = false; - - /** - * Add a promise to the queue. Returns a promise that is resolved or rejected when the added - * promise eventually resolves or rejects. - */ - function add(promiseCreator: () => Promise): Promise { - return new Promise((resolve, reject) => { - pending.push({promiseCreator, resolve, reject}); - dequeue(); - }); - } - - /** - * Run the next promise in the queue. - */ - function dequeue() { - // If we're already working on a promise, do nothing. - if (working) { - return; - } - - const nextPromise = pending.shift(); + let queue = Promise.resolve(); - // If there are no promises to work on, do nothing. - if (!nextPromise) { - return; - } - - working = true; - - // Execute the promise. When it's done, start working on the next. - nextPromise - .promiseCreator() - .then(nextPromise.resolve, nextPromise.reject) - .finally(() => { - working = false; - dequeue(); - }); - } - - return {add}; - } - - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore This function executes in a browser context. - window.axeQueue = PromiseQueue(); + window.enqueuePromise = function (createPromise: () => Promise) { + return new Promise((resolve, reject) => { + queue = queue.then(createPromise).then(resolve).catch(reject); + }); + }; } diff --git a/src/browser/TestBrowser.ts b/src/browser/TestBrowser.ts index f86068f..6627d2d 100644 --- a/src/browser/TestBrowser.ts +++ b/src/browser/TestBrowser.ts @@ -80,7 +80,15 @@ export default class TestBrowser { }); } - return Result.fromPage(this.page, story); + const axeResults = await AxePage.analyze( + this.page, + story.disabledRules, + story.runOptions, + story.context, + story.config, + ); + + return new Result(axeResults.violations); } /**