Introduce Puppeteer for handling browsers during tests #11807
Conversation
timvandermeij
force-pushed
the
puppeteer
branch
from
ea73559 to
121a040
Compare
|
/botio fonttest |
From: Bot.io (Linux m4)ReceivedCommand cmd_fonttest from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/4de8a98d85563dd/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_fonttest from @timvandermeij received. Current queue size: 0 Live output at: http://54.215.176.217:8877/7e3c47a09cb3310/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/4de8a98d85563dd/output.txt Total script time: 1.78 mins
|
|
Well, at least one of the bots is happy. The Windows bot was already broken, so perhaps that fixes itself after a gentle kick. |
|
/botio-linux unittest |
From: Bot.io (Linux m4)ReceivedCommand cmd_unittest from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/8d0c71333122669/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/8d0c71333122669/output.txt Total script time: 3.44 mins
|
|
/botio-linux test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/ac4661a6e0a102c/output.txt |
timvandermeij
force-pushed
the
puppeteer
branch
from
121a040 to
9b80d76
Compare
|
The browser versions are most likely different from those installed on the bots, so I'm expecting some changes in the reference images here. |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/ac4661a6e0a102c/output.txt Total script time: 18.31 mins
Image differences available at: http://54.67.70.0:8877/ac4661a6e0a102c/reftest-analyzer.html#web=eq.log |
|
I'm pleasantly surprised that this is working immediately on the Linux bot. It looks like I broke the Windows bot even more, but I'll contact Brendan to get that fixed so we can also trigger the tests there to see how that goes. It did help to identify one more point that should be fixed, so I added it to my to do list above, which should now be complete. |
timvandermeij
force-pushed
the
puppeteer
branch
from
9b80d76 to
3b1509a
Compare
|
This looks like a very good improvement, and the amount of code (or lack thereof) that was required to get this to work is a pleasant surprise! A couple of questions and things I noticed (based on a quick look):
[1] I'd expect something similar to e.g. |
|
Thank you for the feedback!
Yes, that sounds like a good idea. I have added it to the to do list.
That's a good point, I totally missed that. I have no idea where it went, but I'll look into it; it's also on the list now. |
|
One more thing, does benchmarking[1] still work as well? I'd expect the answer to be yes, since everything else seem to work nicely, but please test and/or confirm that that's the case :-) [1] Following the instructions at https://github.com/mozilla/pdf.js/wiki/Benchmarking-your-changes |
timvandermeij
force-pushed
the
puppeteer
branch
from
3b1509a to
3ac7a15
Compare
|
/botio-linux test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/73aeb91470d6572/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/73aeb91470d6572/output.txt Total script time: 18.57 mins
Image differences available at: http://54.67.70.0:8877/73aeb91470d6572/reftest-analyzer.html#web=eq.log |
|
/botio-linux test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/1e6b7f010cdba21/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/1e6b7f010cdba21/output.txt Total script time: 18.58 mins
Image differences available at: http://54.67.70.0:8877/1e6b7f010cdba21/reftest-analyzer.html#web=eq.log |
timvandermeij
force-pushed
the
puppeteer
branch
from
701f65a to
3ac7a15
Compare
|
/botio-linux test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/7bd981c6bb54691/output.txt |
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/438819961d6a563/output.txt Total script time: 2.87 mins
|
timvandermeij
force-pushed
the
puppeteer
branch
from
caac46c to
a6e331b
Compare
|
/botio fonttest |
From: Bot.io (Linux m4)ReceivedCommand cmd_fonttest from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/dbef503c05e7122/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_fonttest from @timvandermeij received. Current queue size: 0 Live output at: http://54.215.176.217:8877/8a1ffad353fa0d9/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/dbef503c05e7122/output.txt Total script time: 1.87 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/8a1ffad353fa0d9/output.txt Total script time: 2.94 mins
|
|
This should be ready for prime time now. I have updated the PR description with which upstream issues exist, but they are no show stoppers for us because we have workarounds in place. They are simply filed to improve the upstream code and allow us to remove workarounds once they get fixed upstream. @Snuffleupagus Would you be willing to review this? |
There was a problem hiding this comment.
The various test-commands still seem to work just fine locally on Windows, which I believe was the most important part for me to check here :-)
Thanks for putting in the, clearly not small, amount of work required to make this happen!
The only slight annoyance I found is that the elements containing logs and ref-test images are now a lot smaller than previously, and doesn't seem to auto-expand with the window size, to the point of being very difficult to use.
- The following DOME element is smaller than previously (during test runs): https://github.com/mozilla/pdf.js/blob/master/test/test_slave.html#L31
- The DOM element containing the ref-test image comparison is a lot smaller (requiring too much scrolling when comparing failures): https://github.com/mozilla/pdf.js/blob/master/test/resources/reftest-analyzer.html#L53
I suppose that this could be Windows specific, but if there's a simple fix for this that'd be great!
(This is the only thing that I'd really appreciate being fixed before this PR lands.)
I've not reviewed this in too much detail, since I see the value of actually getting this landed without further delays. (If there's further improvements necessary, they can simply be done in follow-ups.)
This commit replaces our own infrastructure for handling browsers during tests with Puppeteer. Using our own infrastructure for this had a few downsides: - It has proven to not always be reliable, especially when closing the browser, causing failures on the bots because browsers were still running even though they should have been stopped. Puppeteer should do a better job with this because it uses the browser's test built-in instrumentation tools for this (the devtools protocol) which our code didn't. This also means that we don't have to pass parameters/preferences to tweak browser behavior anymore. - It requires the browsers under test to be installed on the system, whereas Puppeteer downloads the browsers before the test. This means that setup is much easier (no more manual installations and browser manifest files) as well as testing with different browser versions (since they can be provisioned on demand). Moreover, this ensures that contributors always run the tests in both Firefox and Chrome, regardless of which browsers they have installed locally. - It's all code we have to maintain, so Puppeteer abstracts away how the browsers start/stop for us so we don't have to keep that code. By default, Puppeteer only installs one browser during installation, hence the need for a post-install script to install the second browser. This requires `cross-env` to make passing the environment variable work on both Linux and Windows.
To save time or resources during development it can be useful to run tests only in Firefox. Previously this could be done by editing the browser manifest file, but since that file is no longer used for Puppeteer, this command line flag replaces it. For example, executing `gulp unittest --noChrome` will only run the unit tests in Firefox.
timvandermeij
force-pushed
the
puppeteer
branch
from
a6e331b to
9ebb18f
Compare
|
I have fixed the too small elements. It turns out by default Puppeteer uses an 800 x 600 viewport. This is configurable and I have now disabled that to make all elements render normally. /botio makeref |
From: Bot.io (Windows)ReceivedCommand cmd_makeref from @timvandermeij received. Current queue size: 0 Live output at: http://54.215.176.217:8877/178d43100165155/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_makeref from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/5cef3c760c72de6/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/5cef3c760c72de6/output.txt Total script time: 23.86 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/178d43100165155/output.txt Total script time: 28.72 mins
|
The manifest files in this folder have been obsolete ever since April 2020 when we switched our custom browser manager to Puppeteer in mozilla/pdf.js#11807.
In PR mozilla#11300 Gitpod support got introduced, but we re-evaluated that decision in mozilla#11732. In PR mozilla#11800 the support was partially reverted, but the actual Gitpod files were kept to not outright break potential workflows because at the time we were not sure if, and if so how often, Gitpod was actually used for contributing to PDF.js. However, in addition to the concerns mentioned in mozilla#11732 after five years we haven't seen any contributions that clearly originated from Gitpod, and the Dockerfile has not been updated after e.g. PR mozilla#11807 and PR mozilla#17913 because it's not a workflow that we maintain or are able to test (nor have we seen Gitpod community contributions for this). This commit therefore removes the remaining Gitpod files too to reduce maintainance burden for PDF.js. Note that users of Gitpod can still contribute to PDF.js via the platform; we just don't provide/manage workspace files from this repository anymore.
In PR mozilla#11300 Gitpod support got introduced, but we re-evaluated that decision in mozilla#11732. In PR mozilla#11800 the support was partially reverted, but the actual Gitpod files were kept to not outright break potential workflows because at the time we were not sure if, and if so how often, Gitpod was actually used for contributing to PDF.js. However, in addition to the concerns mentioned in mozilla#11732 after five years we haven't seen any contributions that clearly originated from Gitpod, and the files have not been updated after e.g. PR mozilla#11807 and PR mozilla#17913 because it's not a workflow that we maintain or are able to test (nor have we seen Gitpod community contributions for this). This commit therefore removes the remaining Gitpod files to reduce maintainance burden for PDF.js. Note that users of Gitpod can still contribute to PDF.js via the platform; we just don't provide/manage workspace files from this repository anymore.
The commit messages contain more information about the individual changes.
Fixes #11628.