Skip to content
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

chore: Re-enable Firefox unit tests on Linux with WebRender disabled #6861

Merged
merged 1 commit into from Feb 18, 2021

Conversation

jackfranklin
Copy link
Collaborator

@jackfranklin jackfranklin commented Feb 10, 2021

Trying to debug some of the Firefox test failures we're seeing in:

What's odd is that the failures are always in the beforeEach, so it's hard to know exactly what's causing the failures, but they are consistently within this block of tests.

@google-cla google-cla bot added the cla: yes label Feb 10, 2021
@mathiasbynens
Copy link
Member

@whimboo FYI we're starting to see Firefox flakes for the latest nightly.

@jackfranklin jackfranklin changed the title chore: increase CI unit test timeout chore: skip Page.addScriptTag and Frame.waitForSelector specs in Firefox Feb 10, 2021
@jackfranklin
Copy link
Collaborator Author

@whimboo as Mathias says, unfortunately FF Nightly seems to suddenly be flaking a lot.

I thought it was a specific set of tests (see the diff) but unfortunately everytime I skip one describe block another flakes, so it seems like the issue is in the setup or launching of Firefox - the failures are always in the beforeEach too which indicates the problem isn't with any particular single test.

Unfortunately I've not been able to replicate locally; if you wouldn't mind having a look into this that'd be great as it's blocking us releasing; our only option at this point seems to be to disable running the FF Tests on CI, which we obviously aren't keen to do :)

@whimboo
Copy link
Collaborator

whimboo commented Feb 10, 2021

Are there some more verbose logs from CI available? Would it be possible to run a CI job with the Firefox preference remote.log.level set to Trace?

On our side we are still testing with Puppeteer 5.5.0 and we aren't seeing any issue in our CI. Are those new commands? I tried to call these with the chrome-remote-interface client, but it claims they are unknown.

What I landed yesterday were some patches that refactor our Remote Protocol codebase, but again nothing is broken in our internal tests, nor the Puppeteer tests. So that is strange.

@whimboo
Copy link
Collaborator

whimboo commented Feb 10, 2021

Note that because of #6690 (comment) I cannot really help at the moment. There is no way for me to just run a fresh checkout of the puppeteer repository.

@whimboo
Copy link
Collaborator

whimboo commented Feb 11, 2021

Also good to know for me would be how to get mocha to output the stdout/stderr logs coming from Firefox while the tests are running?

jackfranklin added a commit that referenced this pull request Feb 11, 2021
The tests are regularly flaking (see #6861 for some investigation). In
the mean time it's blocking us landing and releasing, so we'll
temporarily skip FF tests for now.
jackfranklin added a commit that referenced this pull request Feb 11, 2021
The tests are regularly flaking (see #6861 for some investigation). In
the mean time it's blocking us landing and releasing, so we'll
temporarily skip FF tests for now.
jackfranklin added a commit that referenced this pull request Feb 11, 2021
* chore: temporarily disable FF tests on CI

The tests are regularly flaking (see #6861 for some investigation). In
the mean time it's blocking us landing and releasing, so we'll
temporarily skip FF tests for now.

Co-authored-by: Mathias Bynens <mathias@qiwi.be>
@whimboo
Copy link
Collaborator

whimboo commented Feb 11, 2021

Now that I can run all the tests locally, I did a complete funit run and that's what I get:

  373 passing (3m)
  298 pending
  4 failing

  1) Keyboard
       should report shiftKey:
     Error: expect(received).toBe(expected) // Object.is equality

- Expected  - 0
+ Received  + 1

  Keydown: ! Digit1 49 [Alt]
+ Keypress: ! Digit1 33 33 [Alt]
      at Context.<anonymous> (test/keyboard.spec.ts:164:67)
      at runMicrotasks (<anonymous>)
      at processTicksAndRejections (node:internal/process/task_queues:93:5)

  2) navigation
       "before each" hook for "should work when navigating to 404":
     Uncaught Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
      at new NodeError (node:internal/errors:258:15)
      at ServerResponse.setHeader (node:_http_outgoing:563:11)
      at /Users/henrik/code/puppeteer/utils/testserver/index.js:266:16
      at FSReqCallback.readFileAfterClose [as oncomplete] (node:internal/fs/read_file_context:63:3)

  3) navigation
       "after each" hook for "should work when navigating to 404":
     TypeError: Cannot read property 'close' of null
      at Context.<anonymous> (test/mocha-utils.ts:238:25)
      at processImmediate (node:internal/timers:462:21)

  4) navigation
       "before each" hook for "should work when navigating to 404":
     done() called multiple times in hook <navigation "before each" hook for "should work when navigating to 404"> of file /Users/henrik/code/puppeteer/test/navigation.spec.ts; in addition, done() received error: Error: Protocol error (Target.attachToTarget): Target closed.
    at /Users/henrik/code/puppeteer/src/common/Connection.ts:94:57
    at new Promise (<anonymous>)
    at Connection.send (/Users/henrik/code/puppeteer/src/common/Connection.ts:93:12)
    at Connection.createSession (/Users/henrik/code/puppeteer/src/common/Connection.ts:176:38)
    at Target._sessionFactory (/Users/henrik/code/puppeteer/src/common/Browser.ts:287:30)
    at Target.page (/Users/henrik/code/puppeteer/src/common/Target.ts:119:32)
    at Browser._createPageInContext (/Users/henrik/code/puppeteer/src/common/Browser.ts:374:31)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:93:5)
    at Context.<anonymous> (/Users/henrik/code/puppeteer/test/mocha-utils.ts:234:18)
  Error: done() called multiple times in hook <navigation "before each" hook for "should work when navigating to 404"> of file /Users/henrik/code/puppeteer/test/navigation.spec.ts; in addition, done() received error: Error: Protocol error (Target.attachToTarget): Target closed.
      at /Users/henrik/code/puppeteer/src/common/Connection.ts:94:57
      at new Promise (<anonymous>)
      at Connection.send (src/common/Connection.ts:93:12)
      at Connection.createSession (src/common/Connection.ts:176:38)
      at Target._sessionFactory (src/common/Browser.ts:287:30)
      at Target.page (src/common/Target.ts:119:32)
      at Browser._createPageInContext (src/common/Browser.ts:374:31)
      at runMicrotasks (<anonymous>)
      at processTicksAndRejections (node:internal/process/task_queues:93:5)
      at Context.<anonymous> (test/mocha-utils.ts:234:18)
      at runMicrotasks (<anonymous>)
      at processTicksAndRejections (node:internal/process/task_queues:93:5)

As such my question again, on which platforms does it fail, and how could we get more detailed information from a CI run, like with DEBUG="puppeteer:*" enabled, or even better with the preferences remote.log.level=Trace` set.

@jschfflr could you trigger such a job?

@jschfflr
Copy link
Contributor

I added a bunch of flags to the run - let's see if they help pin down the problem.
Not sure though how remote.log.level works - does it affect logging to stdout?

@google-cla
Copy link

google-cla bot commented Feb 11, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Feb 11, 2021
@whimboo
Copy link
Collaborator

whimboo commented Feb 11, 2021

I added a bunch of flags to the run - let's see if they help pin down the problem.

Thanks.

Not sure though how remote.log.level works - does it affect logging to stdout?

Yes, just add it next to this line in Launcher.ts. When doing that you can remove the DEBUG env variable because it would only duplicate the output to stdout.

It will output way more detailed information about internal states. I hope it will also help here. As it looks like something seems to be wrong with Firefox startup.

@google-cla
Copy link

google-cla bot commented Feb 11, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Feb 11, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Feb 11, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@jschfflr
Copy link
Contributor

@whimboo The current run has tracing enabled now

@jschfflr
Copy link
Contributor

So apparently the problem does not occur with tracing enabled...

@jschfflr
Copy link
Contributor

@whimboo Let me know if there's anything else I can do to help.
I also reached out to you via email in case you want to have a quick VC.

@whimboo
Copy link
Collaborator

whimboo commented Feb 11, 2021

Interesting. Mind removing the remote.log.level preference, and replacing it with DEBUG? Maybe that will allow us to see the problem again. Once it's reproducible I'm happy to chat. Feel free to also join us on https://chat.mozilla.org/#/room/#remote-protocol:mozilla.org.

@google-cla
Copy link

google-cla bot commented Feb 11, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Feb 17, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

2 similar comments
@google-cla
Copy link

google-cla bot commented Feb 17, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Feb 18, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Feb 18, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@jschfflr jschfflr added cla: yes and removed cla: no labels Feb 18, 2021
@google-cla
Copy link

google-cla bot commented Feb 18, 2021

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@jschfflr jschfflr merged commit 00d88ef into main Feb 18, 2021
@jschfflr jschfflr deleted the debug-ff-flakes branch February 18, 2021 10:51
@whimboo whimboo changed the title chore: skip Page.addScriptTag and Frame.waitForSelector specs in Firefox chore: Re-enable Firefox unit tests on Linux with WebRender disabled Feb 18, 2021
@whimboo
Copy link
Collaborator

whimboo commented Feb 18, 2021

Great to see that we haven't had to disable those tests. As just I updated the summary to reflect the re-enable change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants