Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify promise queue implementation #102

Merged
merged 8 commits into from
Oct 16, 2024
Merged

Simplify promise queue implementation #102

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1abe62b
Simplify promise queue implementation
ahuth Oct 11, 2024
cfa8804
Edit comment explaining the promise queue
ahuth Oct 11, 2024
c17b492
Move reject to its own catch handler
ahuth Oct 11, 2024
f93bd84
Add changelog entry
ahuth Oct 12, 2024
381dffe
Move defaultDisabledRules out of the Result object
ahuth Oct 12, 2024
3652ca2
Move AxePage stuff out of the Result object
ahuth Oct 12, 2024
1984a78
Fix indentation
ahuth Oct 12, 2024
150e2eb
Add another changelog entry
ahuth Oct 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
35 changes: 3 additions & 32 deletions src/Result.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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',
];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the default disabled rules into the AxePage file, so that Axe stuff is more centralized there.


/**
* 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);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this function into TestBrowser, so that Playwright code is centralized there.


violations: AxeResult[];

constructor(violations: AxeResult[]) {
Expand All @@ -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')}
`;
}
}

Expand Down
108 changes: 33 additions & 75 deletions src/browser/AxePage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: <T>(createPromise: () => Promise<T>) => Promise<T>;
};

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.
*/
Expand All @@ -33,7 +50,10 @@ export function analyze(
config?: Spec,
): Promise<AxeResults> {
return page.evaluate(runAxe, {
options: getRunOptions(runOptions, disabledRules),
options: getRunOptions(runOptions, [
...defaultDisabledRules,
...disabledRules,
]),
config,
context,
});
Expand All @@ -50,7 +70,7 @@ function runAxe({
}): Promise<AxeResults> {
// 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
Expand Down Expand Up @@ -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<T> = {
promiseCreator: () => Promise<T>;
resolve: (value: T | PromiseLike<T>) => 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<T>() {
const pending: QueuedPromise<T>[] = [];
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<T>): Promise<T> {
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<AxeResults>();
window.enqueuePromise = function <T>(createPromise: () => Promise<T>) {
return new Promise<T>((resolve, reject) => {
queue = queue.then(createPromise).then(resolve).catch(reject);
Comment on lines +121 to +122
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I understanding correctly that the first .then invokes the promise at the end of the queue, and then createPromise is invoked, adding on to the queue?

Does the second .then then resolve the one just created by createPromise?

Sorry a lot of promises going around 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort of!

.then creates a new promise off of the current queue (which of course is then set as the current queue). It's resolved with the value the created promise is resolved with...

Might be more clear if I wrote it out as

queue = queue.then(() => {
  return createPromise();
})

of if we made it less generic and inlined the promise creation (for illustration purposes), it would look something like

// For each story
queue = queue.then(() => {
  return axe.run(...);
})

Going even deeper, if we unrolled the loop it would be like

Promise.resolve()
  .then(() => axe.run(...)) // Story 1
  .then(() => axe.run(...)) // Story 2
  .then(() => axe.run(...)) // Story 3
  // etc.

Hope that helps!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the other way to say it is that .then creates a promise, and createPromise creates a different promise, but they are linked

});
};
Copy link
Contributor Author

@ahuth ahuth Oct 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A change with this new implementation is the it'll stop processing subsequent promises if one is rejected (unless we add rejection handing somewhere, which I might do at some point).

Regardless, I think this is fine, because:

  • The axe.run promise will not normally be rejected (even if there are axe violations). So something has gone very wrong with the axe run if it is.
  • This whole queue mechanism is a fallback that might not even be necessary. It shouldn't be possible to end up with multiple axe.run's at a time. This queue is probably overly paranoid.

}
10 changes: 9 additions & 1 deletion src/browser/TestBrowser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted elsewhere, I moved this code here. That way the Result object doesn't need to contain any Playwright stuff.


return new Result(axeResults.violations);
}

/**
Expand Down