Open Bug 1616452 Opened 3 years ago Updated 2 months ago

(Puppeteer) Replace Page.setContent with data URL navigation in most Puppeteer unit tests

Categories

(Remote Protocol :: Agent, task, P3)

task

Tracking

(Not tracked)

REOPENED

People

(Reporter: impossibus, Unassigned)

References

Details

Most tests that use Puppeteer's Page.setContent aren't meant to exercise setContent, which means they can be simplified. We should open an upstream PR for replacing most unit-test setContent usage with data URLs, in addition to fixing this bug to have Firefox emit init. See Bug 1612174 for context.

Chrome failures when using data URLs:

1) <fixed>

2) Chromium Browser Page Page.waitForNavigation should work with clicking on anchor links (navigation.spec.js:364:5)
  Message:
    Timeout Exceeded 10000ms

3) Chromium Browser Page Page.waitForNavigation should work with history.pushState() (navigation.spec.js:374:5)
  Message:
    Timeout Exceeded 10000ms

4) Chromium Browser Page Page.waitForNavigation should work with history.replaceState() (navigation.spec.js:389:5)
  Message:
    Timeout Exceeded 10000ms

5) Chromium Browser Page Page.waitForNavigation should work with DOM history.back()/history.forward() (navigation.spec.js:404:5)
  Message:
    expect.toBe failed: data:text/html;charset=utf-8,%3C!doctype%20html%3E%0A%0A%20%20%20%20%20%20%20%20%3Ca%20id%3Dback%20onclick%3D'javascript%3AgoBack()'%3Eback%3C%2Fa%3E%0A%20%20%20%20%20%20%20%20%3Ca%20id%3Dforward%20onclick%3D'javascript%3AgoForward()'%3Eforward%3C%2Fa%3E%0A%20%20%20%20%20%20%20%20%3Cscript%3E%0A%20%20%20%20%20%20%20%20%20%20function%20goBack()%20%7B%20history.back()%3B%20%7D%0A%20%20%20%20%20%20%20%20%20%20function%20goForward()%20%7B%20history.forward()%3B%20%7D%0A%20%20%20%20%20%20%20%20%20%20history.pushState(%7B%7D%2C%20''%2C%20'%2Ffirst.html')%3B%0A%20%20%20%20%20%20%20%20%20%20history.pushState(%7B%7D%2C%20''%2C%20'%2Fsecond.html')%3B%0A%20%20%20%20%20%20%20%20%3C%2Fscript%3E%0A%20%20%20%20%20%20 == http://localhost:8907/second.html

6) Chromium Browser Page Page.setRequestInterception should work when POST is redirected with 302 (requestinterception.spec.js:48:5)
  Message:
    Timeout Exceeded 10000ms

7) Chromium Browser Page Page.Events.Popup should work with clicking target=_blank (page.spec.js:137:5)
  Message:
    Timeout Exceeded 10000ms

8) Chromium Browser Page Page.setContent should work (page.spec.js:715:5)
  Message:
    expect.toBe failed: <!DOCTYPE html><html><head></head><body><div>hello</div></body></html> == <html><head></head><body><div>hello</div></body></html>


9) Chromium Browser Page Page.setContent should work with HTML 4 doctype (page.spec.js:726:5)
  Message:
    expect.toBe failed: <!DOCTYPE html><html><head></head><body><div>hello</div></body></html> == <!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd"><html><head></head><body><div>hello</div></body></html>


Ran 692 (ok - 677, failed - 9, skipped - 6) of 692 test(s)

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&tier=1%2C2%2C3&revision=961754fe12aef229a7265a949a632da4ec7a0232&selectedJob=289463473

Fixing this and/or Bug 1612174 will increase our pass rate to 280/638 (57 more passing tests). Fixing Bug 1612174 would bring that up to 285, I believe.

Oh, I didn't mean to actually change the Puppeteer unit tests to get rid of setContent. My intention was only to check what happens in our CI and with Firefox when we use data URLs, to circumvent bug 1612174 . Given that such a high number of additional tests are passing we should clearly go ahead and fix bug 1612174.

Thank you for the investigation Maja, but I'm going to close this bug as wontfix.

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX

I know that's not what you meant, but I filed this bug to document that it actually would make sense to change most of their tests to dataURLs to make them less flaky in general, not just as a fix for Firefox. It doesn't really matter for us, though.

Oh I see. Maybe then we should get filed an issue on the Puppeteer repo? And well, we would also need that for playwright then. Not sure if that all will happen? We could try.

Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.