From 1abe62ba2ac40755eec66ce201fd52dce4223cd0 Mon Sep 17 00:00:00 2001 From: Andrew Huth Date: Fri, 11 Oct 2024 15:50:55 -0400 Subject: [PATCH 1/8] Simplify promise queue implementation --- src/browser/AxePage.ts | 79 +++++++----------------------------------- 1 file changed, 13 insertions(+), 66 deletions(-) diff --git a/src/browser/AxePage.ts b/src/browser/AxePage.ts index be0398c..0c20abb 100644 --- a/src/browser/AxePage.ts +++ b/src/browser/AxePage.ts @@ -8,6 +8,12 @@ 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[] @@ -50,7 +56,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 @@ -98,70 +104,11 @@ export function getRunOptions( * 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(); - - // If there are no promises to work on, do nothing. - if (!nextPromise) { - return; - } + let queue = Promise.resolve(); - 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, reject); + }); + }; } From cfa8804a9bcfd145e606f7bb89ff371c109b3ac9 Mon Sep 17 00:00:00 2001 From: Andrew Huth Date: Fri, 11 Oct 2024 19:03:42 -0400 Subject: [PATCH 2/8] Edit comment explaining the promise queue --- src/browser/AxePage.ts | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/browser/AxePage.ts b/src/browser/AxePage.ts index 0c20abb..676460b 100644 --- a/src/browser/AxePage.ts +++ b/src/browser/AxePage.ts @@ -92,16 +92,13 @@ 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() { let queue = Promise.resolve(); From c17b492cadd98c55a9db3fd03ef76b8ec5f71e7c Mon Sep 17 00:00:00 2001 From: Andrew Huth Date: Fri, 11 Oct 2024 19:18:07 -0400 Subject: [PATCH 3/8] Move reject to its own catch handler Just to make sure promise rejections are handled. --- src/browser/AxePage.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/browser/AxePage.ts b/src/browser/AxePage.ts index 676460b..b44dcc9 100644 --- a/src/browser/AxePage.ts +++ b/src/browser/AxePage.ts @@ -105,7 +105,7 @@ function addPromiseQueue() { window.enqueuePromise = function (createPromise: () => Promise) { return new Promise((resolve, reject) => { - queue = queue.then(createPromise).then(resolve, reject); + queue = queue.then(createPromise).then(resolve).catch(reject); }); }; } From f93bd8420ed8d1d5020aea568d56b9c7a78fe5e9 Mon Sep 17 00:00:00 2001 From: Andrew Huth Date: Sat, 12 Oct 2024 07:53:42 -0400 Subject: [PATCH 4/8] Add changelog entry --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f3909c9..eb2281f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- [fix] Simplify the promise queue implementation [#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) From 381dffeeafa698df120664e8daa3a07f9dd685b7 Mon Sep 17 00:00:00 2001 From: Andrew Huth Date: Sat, 12 Oct 2024 08:29:04 -0400 Subject: [PATCH 5/8] Move defaultDisabledRules out of the Result object And into the AxePage code, with the rest of the axe-core logic. --- src/Result.ts | 14 +------------- src/browser/AxePage.ts | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/src/Result.ts b/src/Result.ts index 54b4a40..99d3b27 100644 --- a/src/Result.ts +++ b/src/Result.ts @@ -5,17 +5,6 @@ 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. */ @@ -24,10 +13,9 @@ 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.disabledRules, story.runOptions, story.context, story.config, diff --git a/src/browser/AxePage.ts b/src/browser/AxePage.ts index b44dcc9..099a7c9 100644 --- a/src/browser/AxePage.ts +++ b/src/browser/AxePage.ts @@ -19,6 +19,17 @@ export type Context = | 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. */ @@ -39,7 +50,10 @@ export function analyze( config?: Spec, ): Promise { return page.evaluate(runAxe, { - options: getRunOptions(runOptions, disabledRules), + options: getRunOptions(runOptions, [ + ...defaultDisabledRules, + ...disabledRules, + ]), config, context, }); From 3652ca26b2e68dd192d759988c7f88cba8d36f9b Mon Sep 17 00:00:00 2001 From: Andrew Huth Date: Sat, 12 Oct 2024 08:41:41 -0400 Subject: [PATCH 6/8] Move AxePage stuff out of the Result object Keep all Axe and playwright stuff in the "Browser" code. --- src/Result.ts | 17 ----------------- src/browser/TestBrowser.ts | 10 +++++++++- 2 files changed, 9 insertions(+), 18 deletions(-) diff --git a/src/Result.ts b/src/Result.ts index 99d3b27..1e5d201 100644 --- a/src/Result.ts +++ b/src/Result.ts @@ -1,28 +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'; /** * 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 axeResults = await analyze( - page, - story.disabledRules, - story.runOptions, - story.context, - story.config, - ); - return new Result(axeResults.violations); - } - violations: AxeResult[]; constructor(violations: AxeResult[]) { 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); } /** From 1984a78aea2dbf1339a2fb001b39f2cbd1e36e91 Mon Sep 17 00:00:00 2001 From: Andrew Huth Date: Sat, 12 Oct 2024 08:42:03 -0400 Subject: [PATCH 7/8] Fix indentation --- src/Result.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Result.ts b/src/Result.ts index 1e5d201..9188f4e 100644 --- a/src/Result.ts +++ b/src/Result.ts @@ -29,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')} + `; } } From 150e2eb7cb2eea2794380c155a6d1b3f95b901bb Mon Sep 17 00:00:00 2001 From: Andrew Huth Date: Mon, 14 Oct 2024 10:31:47 -0400 Subject: [PATCH 8/8] Add another changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index eb2281f..29e0a09 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## 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