-
Notifications
You must be signed in to change notification settings - Fork 6
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.
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
Allow matcher to accept Axe results object #23
Conversation
🦋 Changeset detectedLatest commit: 1a0f2ef The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done self-reviewing
src/utils/axe.ts
Outdated
@@ -30,7 +30,7 @@ export async function injectAxe(locator: Locator) { | |||
* Runs axe on an element handle. The script must already be injected | |||
* using `injectAxe`. | |||
*/ | |||
export function runAxe(locator: Locator, options: RunOptions = {}) { | |||
export function runAxeAfterInject(locator: Locator, options: RunOptions = {}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't think of a better name, sorry.
src/matchers/toBeAccessible/index.ts
Outdated
|
||
// If there are violations, attach an HTML report to the test for additional | ||
// visibility into the issue. | ||
if (!ok) { | ||
const html = await createHTMLReport(results) | ||
const filename = opts.filename || 'axe-report.html' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this is incorrect as it breaks the ability to define the filename at the project level.
src/axe/index.ts
Outdated
const opts = merge(info.project.use.axeOptions, options) | ||
const locator = resolveLocator(handle) | ||
await injectAxe(locator) | ||
return await poll(locator, timeout, async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The polling behavior feels like something that should be part of the matcher, not this low level utility.
src/axe/index.ts
Outdated
import { Handle, resolveLocator } from '../utils/matcher' | ||
import { poll } from '../utils/poll' | ||
|
||
export async function runAxe( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be defined inside of utils/axe
rather than the index file.
src/utils/axe.ts
Outdated
@@ -30,7 +30,7 @@ export async function injectAxe(locator: Locator) { | |||
* Runs axe on an element handle. The script must already be injected | |||
* using `injectAxe`. | |||
*/ | |||
export function runAxe(locator: Locator, options: RunOptions = {}) { | |||
export function runAxeAfterInject(locator: Locator, options: RunOptions = {}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checkLocator
perhaps?
src/matchers/toBeAccessible/index.ts
Outdated
let ok: boolean | ||
let results: AxeResults | ||
if ((handleOrResults as AxeResults).violations) { | ||
results = handleOrResults as AxeResults | ||
ok = !!results.violations.length | ||
} else { | ||
({ ok, results } = await runAxe(handleOrResults as Handle, { timeout, ...options })) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code feels a bit messy. A helper function that does the conditional and returns separate objects would be cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take 2! :)
I tried to implement all review suggestions.
src/waitForAxeResults.spec.ts
Outdated
const { ok } = await waitForAxeResults(page) | ||
await expect(ok).toBeTruthy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always prefer toBe(true)
as it tests that an actual boolean was returned.
const { ok } = await waitForAxeResults(page) | |
await expect(ok).toBeTruthy() | |
const { ok } = await waitForAxeResults(page) | |
await expect(ok).toBe(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, could we test the results object that is returned?
src/waitForAxeResults.spec.ts
Outdated
const content = await readFile("inaccessible.html") | ||
await page.setContent(content) | ||
const { ok } = await waitForAxeResults(page) | ||
await expect(ok).toBeFalsy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same about testing the results object and using toBe(false)
Co-authored-by: Mark Skelton <mdskelton99@gmail.com>
Thanks for the code review! How does it look now? |
Looks like there are some ESLint and Prettier issues. Once those are fixed, looks good. |
Forgot to mention, this needs a changeset with a version bump (minor) and change notes. |
CHANGELOG.md
Outdated
|
||
### Minor Changes | ||
|
||
- 4276db6: Allow matcher to accept Axe results object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Should I squash the commits and force push on this PR before referencing the commit SHA here?
I added this commit SHA manually, but it refers to only one of several commits on this PR branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changelog should not be manually edited. Please add a changeset with the change notes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, second attempt :)
I read those instructions yesterday, but I've never used this tool before, so when I ran yarn changeset
yesterday and it added a file to the .changesets/
directory, and I saw that there were no other files in that directory, I thought that meant that I did something wrong or that there were more steps I needed to complete. So then I ran yarn changeset version
, which added the note to the change log and the version bump.
To satisfy my own curiosity, I'm trying to wrap my mind around how this works. If you have the time, could you read what I've written below and see if I'm on the right track? No worries if not :)
Looking at the git log of the .changesets
directory, it looks to me that something like the following happens:
- Create a PR, push commits
- Add a changeset file to the
.changesets
directory. (Viayarn changeset
and committing and pushing the result). - Squash-merge the commits on the PR. (I assume this is how Changeset knows which SHA to use in the change log. Although I suppose it's probably smart enough to use the SHA of a merge commit.)
- The GitHub action you've configured (on push to
main
) uses the changesets/action to create a pull request that is essentially the output ofyarn changeset version
. - When you merge that PR into main, the changesets/action gets triggered a second time, but this time it runs the
yarn release
command?
Out of curiosity, how do the GitHub releases get added and/or how are the commits tagged with the version number on GitHub? I'm guessing the Changesets Release Action takes care of all that? Since it has an input called createGithubReleases
, defined as:
- createGithubReleases - A boolean value to indicate whether to create Github releases after publish or not. Default to true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, you're right on track with how changesets works. And yes, the action creates the GitHub release as well. It's honestly such an awesome tool for versioning, way superior to the old Lerna flow we used to use for our monorepos before. The other awesome part is it's all PR based, so you can have strict controls on push access, everything including versioning goes through a pull request, so no one needs direct push access when updating versions and publishing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks really neat!
Okay so if I may bother you with another question... 😁
One thing I wonder, since the changeset file only records the delta of the semantic versioning, meaning it only records whether the set of changes represent major, minor, or patch version upgrades, it seems that the order in which PRs get merged matters.
Scenario 1:
- PR Allow matcher to accept Axe results object #23 is merged, package goes from version 2.2.1 to 2.3.0
- PR Rename
toBeAccessible
totoPassAxe
#24 is merged, version 2.3.0 to 3.0.0
Scenario 2:
- PR Rename
toBeAccessible
totoPassAxe
#24 is merged, package goes from version 2.2.1 to 3.0.0 - PR Allow matcher to accept Axe results object #23 is merged, version 3.0.0 to 3.0.1
In my particular case with these two PRs, I think either order is fine, but it seems like a pretty heavy assumption that this is generally the case, especially where major changes are concerned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Order doesn't matter, all changesets will be merged to create the final version using the highest version bump. This allows notes in the changelog to still be separated by major/minor/patch, but the version bump will be correct. In this case, regardless of the merge order the new version will be 3.0.0.
Of course, that's assuming I wait to release until this PR is merged, which I'm planning on doing 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Neat!
Thank you for answering my questions. 😃
This reverts commit 59b10cd.
src/matchers/toBeAccessible/index.ts
Outdated
const info = test.info() | ||
const opts = merge(info.project.use.axeOptions, options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This merge needs to happen before calling getResults
and the opts
object needs to be passed to getResults
. This allows for defining config at the project level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, looks like getResults
is doing that already. Interesting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, there's some repetition, but I'm not sure how to avoid it.
Do you want me to try to refactor this in a way that avoids the repetition? Or do you want some time to think about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, it's fine. You could make a function that does it so the test.info
and merge
calls are consistent if you want.
Co-authored-by: Mark Skelton <mdskelton99@gmail.com>
Looks like there are merge conflicts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
Current behavior before this pull request
You cannot pass anything other than a Playwright handle (page, locator, etc.) to the Expect.js matcher:
Behavior after this pull request
After this pull request, you should be able to pass either a Playwright handle or an Axe results object:
Background / motivation
Refer to discussion in issue #21