Skip to content

Upgrade to Puppeteer 12 #14321

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

Merged
merged 5 commits into from
Nov 29, 2021
Merged

Upgrade to Puppeteer 12 #14321

merged 5 commits into from
Nov 29, 2021

Conversation

timvandermeij
Copy link
Contributor

@timvandermeij timvandermeij commented Nov 28, 2021

The commit messages contain more information about the individual changes.

@Snuffleupagus Note that the only change that requires an action here is that contributors are now required to run Node.js 15 or higher. I think this is reasonable, but that would mean that you'd have to upgrade your Node.js 12 version (which is 2.5 years old as of now); unfortunately I don't see a way around that. This is because of the fourth commit, about package-lock.json, which I had to do since Node.js 15+ doesn't offer a way to produce the old lock file version 1 files anymore, and otherwise I couldn't upgrade Puppeteer. I hope this is OK.

If a browser cannot be started, we currently get the following log:
`Error while starting firefox: [object Object]`. This is simply an
oversight from the initial Puppeteer integration work since we never got
into this code path before. With this fix the error log becomes more
useful: `Error while starting firefox: connect ECONNREFUSED ::1:45387`
Node.js 17, which as of writing is the most recent version, contains a
breaking change in its DNS resolver, causing Firefox not to start
anymore in our test framework. The inline comment together with the
following resources provide more background:

- nodejs/node#40702
- nodejs/node#39987
- cyrus-and/chrome-remote-interface#467
- https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V17.md#other-notable-changes
- DeviceFarmer/adbkit#209
- https://nodejs.org/api/dns.html#dnssetdefaultresultorderorder

This commit ensures that versions both older and newer than Node.js 17
work as expected. This is mainly necessary since the bots as of writing
run Node.js 14.17.0 which is from before this API got introduced and for
example Node.js 12 LTS is only end-of-life in April 2022, so we have to
keep support for those older versions unfortunately.
In Puppeteer 11 we noticed that Firefox doesn't shut down once the tests
are done anymore. I tracked this down to the `page.goto` call, in
`startBrowser`, never resolving anymore. I can only assume that
something changed in Puppeteer, possibly in combination with recent
Firefox Nightly versions, that caused this, but haven't been able to
fully track it down.

However, I did find that the problem is that the `load` event no longer
triggers, so fortunately we can fix the problem by explicitly waiting
for the `domcontentloaded` event instead. In general this change might
even be better since we now wait until the test framework is fully
loaded before we continue. Note that this also still works for the
current Puppeteer version.

I did find two upstream references that appear to track this issue, both
on the Puppeteer side and on the Firefox side, making me further suspect
that the issue is partly on both sides:

- puppeteer/puppeteer#5806
- https://bugzilla.mozilla.org/show_bug.cgi?id=1706353
Since NPM 7, which is over a year old now since it released in October
2020, NPM automatically transforms lock files from version 1 to version
2. In the NPM 7 release notes they reported:

"One change to take note of is the new lockfile format, which is
backwards compatible with npm 6 users. The lockfile v2 unlocks the
ability to do deterministic and reproducible builds to produce a
package tree."

Not only is this change backwards compatible (so older versions of NPM
will still be able to install everything as expected), reproducability
is also a nice property to have and modern NPM versions will otherwise
constantly do the conversion anyway, causing contributors to explicitly
have to revert the change. Therefore, I believe we should do this now
since it doesn't break backwards compatibility for consumers of this
file. It only means that producers of this file (i.e., us contributors)
need to use at least NPM 7 or higher (as of writing NPM 8 is even
available). According to https://nodejs.org/en/download/releases/ this
means contributors should at least run Node.js 15.0.0, while 17.1.0 is
the most recent as of writing, so to me that sounds reasonable to ask.
@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Nov 28, 2021

I think this is reasonable, but that would mean that you'd have to upgrade your Node.js 12 version (which is 2.5 years old as of now); unfortunately I don't see a way around that.

Generally that'd be reasonable, but unfortunately I'm personally still stuck on Windows 7 for the time being (where Node 12 is the highest officially supported version).
Is there really no other way to update Puppeteer, since according to https://nodejs.org/en/about/releases/ Node 12 should still be "supported" for a little while longer?

I think that I've managed to manually force-install Node 16 (and npm 8) and things seem to work now (fingers crossed); I'll try using that for a day (or two) and check that everything still works!

@Snuffleupagus
Copy link
Collaborator

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/7bac3bd5b1ed909/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/dc62617ad018490/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/7bac3bd5b1ed909/output.txt

Total script time: 21.41 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 524

Image differences available at: http://54.241.84.105:8877/7bac3bd5b1ed909/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/dc62617ad018490/output.txt

Total script time: 41.88 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 404
  different first/second rendering: 1

Image differences available at: http://54.193.163.58:8877/dc62617ad018490/reftest-analyzer.html#web=eq.log

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

It looks like some serious detective work and debugging went into this patch series!
Really nice work, and excellent comments and detailed/helpful commit messages all around; thanks a lot for fixing this so quickly!

@Snuffleupagus Snuffleupagus merged commit 700eaec into mozilla:master Nov 29, 2021
@Snuffleupagus
Copy link
Collaborator

/botio makeref

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_makeref from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/8964cb038f093a8/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_makeref from @Snuffleupagus received. Current queue size: 1

Live output at: http://54.193.163.58:8877/152dba0bc399726/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/8964cb038f093a8/output.txt

Total script time: 20.56 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://54.193.163.58:8877/152dba0bc399726/output.txt

Total script time: 38.46 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@timvandermeij timvandermeij deleted the puppeteer branch December 2, 2021 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants