1693763 - Re-enable CI runs for Windows on Puppeteer repository

Status: RESOLVED FIXED

(Remote Protocol :: CDP, enhancement, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Currently Puppeteer doesn't run the unit tests on Windows due to problems with shutting down the browser. See bug 1656962. For getting out of the experimental status we should get the tests running again on Windows.

The following PR will make it happen:
https://github.com/puppeteer/puppeteer/pull/6895

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #0)

The following PR will make it happen:
https://github.com/puppeteer/puppeteer/pull/6895

The most recent patch works as expected and the PR is now under review.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

We won't need bug 1656962 to make it work.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

https://github.com/puppeteer/puppeteer/pull/6895 has been merged.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I will reopen this bug to get the upstream change synced to mozilla-central given that it was not part of the v8.0.0 release.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1693763 - [remote] Downstream sync graceful browser shutdown.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d29ba47da66
[remote] Downstream sync graceful browser shutdown. r=remote-protocol-reviewers,jdescottes

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/4d29ba47da66

Comment 8

[Dorel Luca [:dluca]]

Backed out changeset 4d29ba47da66 (bug 1693763) for causing Bug 1701960. CLOSED TREE

https://hg.mozilla.org/integration/autoland/rev/7e1c207a752cccb9917cafacb40434cdf698e073

Comment 9

[Julian Descottes [:jdescottes]]

I investigated the failures for "Launcher specs Puppeteer Puppeteer.launch should be able to launch Firefox (launcher.spec.ts)"

From what I can tell, the test will pass as long as you first properly installed puppeteer, including the firefox dependency.
Run:

• cd remote/test/puppeteer
• PUPPETEER_PRODUCT=firefox npm i

After that the test passes successfully.
It's also something that puppeteer CI does: https://github.com/puppeteer/puppeteer/blob/main/.github/workflows/main.yml#L35

On try, we set the current path to the Firefox binary in executablePath to avoid downloading any binary.
But this test asserts await puppeteer.launch({ product: 'firefox' });, which is by definition not using executablePath (I think it needs to be explicitly passed via something like await puppeteer.launch({ executablePath });).

It seems to me that for the time being we should expect this launcher test to FAIL on Try.
Should we reland this patch, but change the expectation to FAIL?

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout merged: https://hg.mozilla.org/mozilla-central/rev/7e1c207a752c

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:whimboo, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Julian Descottes [:jdescottes] from comment #9)

It seems to me that for the time being we should expect this launcher test to FAIL on Try.
Should we reland this patch, but change the expectation to FAIL?

Good find! And it sounds good. I will update the patch shortly. Would you mind to file a new bug for your findings?

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Actually I made a mistake in the upstream patch:
https://github.com/puppeteer/puppeteer/blob/cf8c08d9919c5ab5247e23d55bbbc2ac4e48215f/test/launcher.spec.ts#L450

It should not be it but itOnlyRegularInstall similar to what is used for Chrome. In such a case this test will just be skipped.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I created a follow-up PR upstream: https://github.com/puppeteer/puppeteer/pull/7106

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d63f2fb2698
[remote] Downstream sync graceful browser shutdown. r=remote-protocol-reviewers,jdescottes

Comment 16

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/7d63f2fb2698