page.spec.ts "removing and adding event handlers" fails in non-headless or in Firefox

[juliandescottes]

For https://bugzilla.mozilla.org/show_bug.cgi?id=1673104, I was investigating why "removing and adding event handlers"was intermittent on Firefox. It is currently fully skipped on Firefox at https://github.com/puppeteer/puppeteer/blob/main/test/page.spec.ts#L125

The issue on Firefox is the call to favicon.ico which sometimes happens while we are listening for response events. When it does the expected count is greater than expected and the test fails. I was wondering why Chrome didn't have the issue, because AFAICT Chrome also issues this favicon.ico request.

Testing this out, it actually only works in headless mode for Chrome. Running HEADLESS=false npm run unit consistently fails on those tests. I imagine favicon requests are skipped in headless?

I started listing possible workarounds for Firefox in https://bugzilla.mozilla.org/show_bug.cgi?id=1673104 (eg we have a preference in Firefox we can disable such calls), but since this also impacts Chrome, maybe we should rather fix it in the test itself?

I can see two options:

• modify empty.html to avoid the favicon call (eg add <link rel="icon" href="data:,">)
• update the test to ignore responses for favicon.ico (note: we cannot change the assertion to expect the favicon response because that would not work on headless chrome)

[whimboo]

@OrKoN would you mind having a look at this issue? IMHO it would be good to not have these network requests for the favicon for tests that rely on a specific amount and order of requests. If that wouldn't be doable maybe the related tests could simply ignore any of the favicon requests?

[whimboo]

Also I wonder if the CI jobs could be expanded to also run non-headless for both Chrome and Firefox? Maybe there are also other issues that would be visible that way?

[OrKoN]

@whimboo happy to review a CL, it seems it makes sense to ignore/disable requests for favicon.ico in the test. Also, we have been also thinking about running the same test set in headful and headless but have not got to it yet. I suspect there could be a lot of flakiness / errors. Historically, it seems there have been only one headful spec

puppeteer/test/headful.spec.ts

Line 36 in 08e9978

describeChromeOnly('headful tests', function () {

[juliandescottes]

Thanks for the feedback!
I'll send a PR to skip those responses in the test. Also I just noticed that we had #6020 which probably covers the same thing. Shall we close one of the issues?

[whimboo]

Might be good to continue on issue #6020 given that it has history. Maybe you could give a quick summary over there of what we discussed here so far? Thanks!