Spruce up Puppeteer usage to stop timeouts, page conflicts etc #2897

colinrotherham · 2022-10-05T09:41:08Z

This PR has now been split into two

Part 1 – #2897 Spruce up Puppeteer usage to stop timeouts, page conflicts etc (this one)
Part 2 – #2861 Use async file operations for helpers

Everything "Puppeteer" runs reliably locally now, with each change is split up by commit so it's clearer what's changed

What's included

Test run locally (concurrent workers) can talk to the wrong Puppeteer page
Browser event handling issues when running "headed" { headless: false }

Some examples:

Initial .focus() tests won't pass when tabs/windows are opened in the background
Element selectors page.waitForSelector() could have different pages than via page.goto()
Reused Puppeteer pages could (still) have JavaScript disabled via page.setJavaScriptEnabled(false)
Browsers can throttle timers in background tabs/windows affecting our tests (battery saver too)

More info on Google Chrome background tab timer changes
WHATWG discussion notes: Throttling and clamping of setTimeout and setInterval

What's not included

When running "headed" { headless: false } it's still impossible for multiple windows to have a document.activeElement in focus. Thankfully as headless mode is the default this can be a fix for another day

const activeElement = await page.evaluate(() => document.activeElement.dataset.module)

Perhaps grabbing the page earlier than { waitUntil: 'load' } and before .initAll(), then waiting for the focus event to fire would be more reliable than expecting focus to still be set "right now".

lib/puppeteer-helpers.js

36degrees · 2022-10-05T11:04:22Z

src/govuk/components/button/button.test.js

@@ -47,7 +49,7 @@ describe('/components/button', () => {
      ])

      const url = await page.url()
-      expect(url).toBe(baseUrl + href)
+      expect(url).toBe(`http://localhost:${PORT}${href}`)


To avoid needing to know the port in this file, wondering if we could match on just the paths?

Think we could use HTMLElement.pathname when evaluating href (rather than reading the href attribute).

Less sure on the best way to get the current page's path out of Puppeteer!

Perfect, I didn't feel happy losing the port but I've got your support now 😊

Suggested change

expect(url).toBe(`http://localhost:${PORT}${href}`)

const { pathname } = new URL(url)

expect(pathname).toBe(href)

lib/puppeteer-helpers.js

romaricpascal

That's neat, really appreciate the goTo and goTo... helpers that avoid dragging a baseUrl in each tests 🙌🏻 Just go a little nitpick on a destructuring assignment and a question on the keyboard throttling.

romaricpascal · 2022-10-05T15:59:35Z

src/govuk/components/character-count/character-count.test.js

 const { getExamples } = require('../../../../lib/jest-helpers.js')
 const { goToComponent, renderAndInitialise } = require('../../../../lib/puppeteer-helpers')

+// Detect when browser has been launched headless
+const { launch: { headless = true } } = jestPuppeteerConfig


Two levels of destructuring muddies that it's headlessthat ends up as the const. I think it's clearer as const headless = jestPuppeteerConfig.launch.headless.

Absolutely, I'll change it 👍

@romaricpascal Hope you don't mind a single (with default) destructured property?

Otherwise it defaults to undefined as it's not in the launch config

// Detect when browser has been launched headless const { headless = true } = jestPuppeteerConfig.launch

How do we feel about the Nullish coalescing operator?

// Detect when browser has been launched headless const headless = jestPuppeteerConfig.launch.headless ?? true;

Yeah, it was having the two levels of destructuring that I think is too much (not sure if we can lint for that, but that's a separate issue).

How do we feel about the Nullish coalescing operator?

// Detect when browser has been launched headless const headless = jestPuppeteerConfig.launch.headless ?? true;

Yeah I love it BUT only where falsy things 0, '', undefined are actually wanted 😮

const headless = jestPuppeteerConfig.launch.headless || true; // true const headless = jestPuppeteerConfig.launch.headless ?? true; // 0

That said, it's super useful in assignment form with user defaults you don't want to override by accident:

process.env.NODE_ENV ??= 'development'

For me, I think I'm so used to ESM import from + import() that simple destructuring feels neater?

import { example } from './examples.mjs

That's not my understanding of how ?? works…

node Welcome to Node.js v16.17.0. Type ".help" for more information. > const config = { truthy: true, falsey: false, nullish: null } undefined > config.truthy ?? true true > config.falsey ?? true false > config.nullish ?? true true > config.undefined ?? true true

Sorry I used the assignment form ??= above

Depends if we want to guard against config values?

launch: { headless: 0 } launch: { headless: '' } launch: { headless: NaN }

Personally, I'm not too concerned about launch.headless being something unexpected as it's in our control, and find the coalesced version easier to understand than the destructured version. But recognise this is probably down to a lack of familiarity with how de-structuring with defaults works and basically personal preference, so this isn't blocking.

Thanks @36degrees, appreciate the feedback

I suppose it's deciding when something's ready to be added to the code style arsenal?

const hello = example.hello // Yes const hello = example['hello'] // Yes const { hello } = example // Maybe?

With all the clever Sass we do, it's nice to have the same artistic licence in JavaScript too 🎩

// Start with three const more = { one: 1, two: 2, three: 3 } // Take one out const { one, ...less } = more // Then there were two console.log(less) // { two: 2, three: 3 }

(That would be a bit too clever)

romaricpascal · 2022-10-05T16:02:20Z

src/govuk/components/character-count/character-count.test.js

+// in background pages will be aligned, clamped or throttled
+const debouncedWaitTime = headless ? 1500 : 2000
+
+// To avoid typing lag, keyup events are also throttled automatically


The different wait time is pretty clear, but I'm not understanding this explanation about throttling the keyup events. Why do they need to be throttled when the browser is not headless?

Remember back in the day, using a slower device (old Android phone) and you're typing furiously into a <textarea> with key event listeners, yet no characters are appearing? There was a bit of a lag

With { headless: false } I'm either seeing Chromium automatically debounce keyup events under heavy load (lots of concurrent tests running), background event throttling, or an issue with the Character Count component.

Try it and see? 😭

Open jest-puppeteer.config.js and add launch: { headless: false }

Tests that type in letters too fast will see the screen reader status not change

Might need a better description if it's not clear?

Argh, damn. Yeah, I think a better description describing that Chrome messes with keyup events under load would help future us 😄

I'm also wondering if we shouldn't move the component out of using keyboard events for detecting inputs and use the input Event. I believe this'll have to wait the work on browser support though as that event is not supported in IE8 😢

@romaricpascal @36degrees How's this new explanation?

// When headless, keydown-to-keyup time appears to affect event throttling, // affecting `lastInputTimestamp` and screen reader status message updates const keyupWaitTime = headless ? 0 : 50

I'm not 100% on the explanation as perhaps it's related to the whole string of key presses taking less than 500ms, affecting the this.lastInputTimestamp Date.now() check etc?

@romaricpascal Yeah input is very interesting. It's probably the fact that the older Input Events (including keyup) are technically "cancellable" so for performance maybe they do debounce?

36degrees

Thanks for this, @colinrotherham 😊 👏🏻

I'm happy that the things I flagged have been addressed, but agree with Romaric's comments so would like to see them resolved before merging.

Suggestion during code review that `page.waitForSelector()` can be used instead

…t runs Although parallel browser test runs will still push windows from foreground to background, this change ensures that initial `.focus()` events will still fire

…eadless Timers in background pages will be aligned, clamped or throttled https://developer.chrome.com/blog/background_tabs whatwg/html#7941

Allows more time for slower machines to complete the entire test suite without individual tests timing out

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2897 October 5, 2022 09:41 Inactive

colinrotherham added tooling javascript squad labels Oct 5, 2022

colinrotherham changed the title ~~Spruce up Puppeteer usage to prevent disconnects, timeouts, page conflicts~~ Spruce up Puppeteer usage to stop timeouts, page conflicts etc Oct 5, 2022

colinrotherham force-pushed the puppeteer-page-object branch from 8d3d5e1 to 0bd8e22 Compare October 5, 2022 09:59

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2897 October 5, 2022 10:00 Inactive

36degrees reviewed Oct 5, 2022

View reviewed changes

colinrotherham mentioned this pull request Oct 5, 2022

Use async file operations for helpers #2861

Merged

colinrotherham force-pushed the puppeteer-page-object branch from 0bd8e22 to aece8dd Compare October 5, 2022 15:08

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2897 October 5, 2022 15:08 Inactive

romaricpascal reviewed Oct 5, 2022

View reviewed changes

36degrees approved these changes Oct 5, 2022

View reviewed changes

colinrotherham added 6 commits October 5, 2022 21:31

Ensure renderAndInitialise() uses scoped page object

a1fe2ca

Add new goTo() and goToComponent() helpers

7d825d4

Add new goToExample() helper

acf8e0f

Remove custom “wait for selector” helpers

31c6373

Suggestion during code review that `page.waitForSelector()` can be used instead

Ensure page JavaScript is enabled again after screenshots

0d865ec

Fix focus detection via document.activeElement for non-headless tes…

759eaa3

…t runs Although parallel browser test runs will still push windows from foreground to background, this change ensures that initial `.focus()` events will still fire

colinrotherham force-pushed the puppeteer-page-object branch from aece8dd to 24532e7 Compare October 5, 2022 21:23

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2897 October 5, 2022 21:24 Inactive

colinrotherham added 3 commits October 6, 2022 08:10

Adjust debouncedWaitTime (with key typing delay) when not running h…

7e353d3

…eadless Timers in background pages will be aligned, clamped or throttled https://developer.chrome.com/blog/background_tabs whatwg/html#7941

Increase browser test timeout from 5s to 15s

28e1c88

Allows more time for slower machines to complete the entire test suite without individual tests timing out

Prevent empty browser startup windows during test runs

3d885e7

colinrotherham force-pushed the puppeteer-page-object branch from 24532e7 to 3d885e7 Compare October 6, 2022 07:10

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2897 October 6, 2022 07:11 Inactive

colinrotherham merged commit 33d9d57 into main Oct 6, 2022

colinrotherham deleted the puppeteer-page-object branch October 6, 2022 10:39

colinrotherham mentioned this pull request Oct 7, 2022

Reduce GitHub Test workflow from 4-5 mins to ~2 mins #2902

Merged

colinrotherham mentioned this pull request Jan 13, 2023

Upgrade browser/accessibility tests for Puppeteer v18 alphagov/govuk-design-system#2532

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spruce up Puppeteer usage to stop timeouts, page conflicts etc #2897

Spruce up Puppeteer usage to stop timeouts, page conflicts etc #2897

colinrotherham commented Oct 5, 2022 •

edited

Loading

36degrees Oct 5, 2022

colinrotherham Oct 5, 2022

colinrotherham Oct 5, 2022

romaricpascal left a comment

romaricpascal Oct 5, 2022

colinrotherham Oct 5, 2022

colinrotherham Oct 6, 2022 •

edited

Loading

36degrees Oct 6, 2022

romaricpascal Oct 6, 2022

colinrotherham Oct 6, 2022 •

edited

Loading

36degrees Oct 6, 2022

colinrotherham Oct 6, 2022

36degrees Oct 6, 2022

colinrotherham Oct 6, 2022

romaricpascal Oct 5, 2022

colinrotherham Oct 5, 2022

romaricpascal Oct 5, 2022 •

edited

Loading

colinrotherham Oct 6, 2022

colinrotherham Oct 6, 2022

36degrees left a comment

	expect(url).toBe(`http://localhost:${PORT}${href}`)
	const { pathname } = new URL(url)
	expect(pathname).toBe(href)

Spruce up Puppeteer usage to stop timeouts, page conflicts etc #2897

Spruce up Puppeteer usage to stop timeouts, page conflicts etc #2897

Conversation

colinrotherham commented Oct 5, 2022 • edited Loading

What's included

What's not included

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romaricpascal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinrotherham Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinrotherham Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romaricpascal Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

36degrees left a comment

Choose a reason for hiding this comment

colinrotherham commented Oct 5, 2022 •

edited

Loading

colinrotherham Oct 6, 2022 •

edited

Loading

colinrotherham Oct 6, 2022 •

edited

Loading

romaricpascal Oct 5, 2022 •

edited

Loading