-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Upgrade to Puppeteer 12 #14321
Conversation
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.
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! |
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.241.84.105:8877/7bac3bd5b1ed909/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.193.163.58:8877/dc62617ad018490/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/7bac3bd5b1ed909/output.txt Total script time: 21.41 mins
Image differences available at: http://54.241.84.105:8877/7bac3bd5b1ed909/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/dc62617ad018490/output.txt Total script time: 41.88 mins
Image differences available at: http://54.193.163.58:8877/dc62617ad018490/reftest-analyzer.html#web=eq.log |
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 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!
/botio makeref |
From: Bot.io (Linux m4)ReceivedCommand cmd_makeref from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.241.84.105:8877/8964cb038f093a8/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_makeref from @Snuffleupagus received. Current queue size: 1 Live output at: http://54.193.163.58:8877/152dba0bc399726/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/8964cb038f093a8/output.txt Total script time: 20.56 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.193.163.58:8877/152dba0bc399726/output.txt Total script time: 38.46 mins
|
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.