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

Jump to bottom

Bump puppeteer from 19.3.0 to 19.4.0 #3084

Merged
merged 3 commits into from Dec 12, 2022
Merged

Bump puppeteer from 19.3.0 to 19.4.0 #3084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f2d3eac
Bump puppeteer from 19.3.0 to 19.4.0
dependabot[bot] Dec 8, 2022
68e7715
Fix race condition in Accordion accessible name test
colinrotherham Dec 5, 2022
af91aae
Add delay for accessibility node list to be populated
colinrotherham Dec 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/puppeteer-helpers.js
View file
Open in desktop
Expand Up @@ -126,11 +126,12 @@ function getAttribute ($element, attributeName) {
/**
* Gets the accessible name of the given element, if it exists in the accessibility tree
*
* @param {import('puppeteer').Page} page - Puppeteer page object
* @param {import('puppeteer').ElementHandle} $element - Puppeteer element handle
* @returns {string} The element's accessible name
* @throws {TypeError} If the element has no corresponding node in the accessibility tree
*/
async function getAccessibleName ($element) {
async function getAccessibleName (page, $element) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We've seen Jest Puppeteer replace the global page between concurrent tests so now a param

// Purposefully doesn't use `?.` to return undefined if there's no node in the
// accessibility tree. This lets us distinguish different kinds of failures:
// - assertion on the name failing: we need to figure out
Expand Down
43 changes: 29 additions & 14 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Expand Up @@ -99,7 +99,7 @@
"jest-serializer-html": "^7.1.0",
"lint-staged": "^13.1.0",
"outdent": "^0.8.0",
"puppeteer": "^19.3.0",
"puppeteer": "^19.4.0",
"sass-color-helpers": "^2.1.1",
"standard": "^17.0.0",
"stylelint": "^14.16.0",
Expand Down
10 changes: 6 additions & 4 deletions src/govuk/components/accordion/accordion.test.js
View file
Open in desktop
Expand Up @@ -251,13 +251,14 @@ describe('/components/accordion', () => {

const $element = await page.$('.govuk-accordion__section-button')

expect(getAccessibleName($element)).resolves.toBe(
await expect(getAccessibleName(page, $element)).resolves.toBe(
Copy link
Contributor

Choose a reason for hiding this comment

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

Puppeteer was receiving the .click() that follows concurrent to Jest resolving this promise

It was flagged by TypeScript in another PR so was my first guess at fixing the issue. We now wait for the assertion to complete before the Accordion button gets a .click()

Copy link
Member

Choose a reason for hiding this comment

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

Neat! Thanks for fixing that 🙌🏻

'Section A , Show this section'
)

await $element.click()
await page.waitForTimeout(100)
Copy link
Contributor

Choose a reason for hiding this comment

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

We see child is undefined in src/common/Accessibility.ts#L319 unless a short delay is added

For a single test run 50ms is sufficient but still saw occasional failures. As mentioned in the comments, this is possibly due to accessibility tree (and text announcement) changes when aria-expanded is updated

Copy link
Member

Choose a reason for hiding this comment

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

suggestion Rather than a timeout, we could use an aria selector to make Puppeteer wait. Once we're past that lookup, we know the accessibility tree has been updated and we can run our check that it's the element we hold a reference of that's actually been updated.

Suggested change
await page.waitForTimeout(100)
// Puppeteer doesn't update the accessibility tree in time for our next expectation
// So we use an aria selector to wait for the accessibility tree to be ready
// before running our actual check.
await page.$('aria/Section A , Hide this section[role="button"]')

Unfortunately Puppeteer still doesn't expose the necessary APIs to compare that this page.$ references the same node as $element (though looks like they're making progress).

Copy link
Member

Choose a reason for hiding this comment

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

Thinking we could encapsulate that inside the getAccessibleName() helper with a {wait: true} option that would save typing the accessible name twice and keep things neatly encapsulated 😄


expect(getAccessibleName($element)).resolves.toBe(
await expect(getAccessibleName(page, $element)).resolves.toBe(
'Section A , Hide this section'
)
})
Expand All @@ -269,13 +270,14 @@ describe('/components/accordion', () => {

const $element = await page.$('.govuk-accordion__section-button')

expect(getAccessibleName($element)).resolves.toBe(
await expect(getAccessibleName(page, $element)).resolves.toBe(
'Test , Additional description , Show this section'
)

await $element.click()
await page.waitForTimeout(100)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
await page.waitForTimeout(100)
await page.$('aria/Test , Additional description , Hide this section[role="button"]')

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah don't worry, like you said we're back to not knowing if $element is the same "element" initially clicked

We may well see puppeteer-core include the delay at their end should it be an intentional Chromium change

I'm happy with this workaround temporarily

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. I've opened an issue with Puppeteer to improve our chances of it being fixed 😄


expect(getAccessibleName($element)).resolves.toBe(
await expect(getAccessibleName(page, $element)).resolves.toBe(
'Test , Additional description , Hide this section'
)
})
Expand Down