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

Improve type of predicate function #6997

Merged
merged 5 commits into from
Apr 6, 2021
Merged

Improve type of predicate function #6997

merged 5 commits into from
Apr 6, 2021

Conversation

karlhorky
Copy link
Contributor

@karlhorky karlhorky commented Mar 16, 2021

Hi there, thanks for all of your hard work on Puppeteer! So amazing, the projects that can be built with it.

A quick PR to improve the types for page.waitForRequest and page.waitForResponse - types are inspired by the DefinitelyTyped @types/puppeteer package:

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/c43191a8f7a7d2a47bbff0bc3a7d95ecc64d2269/types/puppeteer/index.d.ts#L1883-L1885

Copy link
Contributor

@jackfranklin jackfranklin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement, thank you!

@jackfranklin jackfranklin enabled auto-merge (squash) March 24, 2021 11:41
@karlhorky
Copy link
Contributor Author

karlhorky commented Mar 25, 2021

No problem!

Do I need to fix something so that it auto-merges? The tests seem like they flaked out...

@karlhorky
Copy link
Contributor Author

@mathiasbynens @jackfranklin do you have any guidance for these reconnect timeouts? Still failing after I merged main in again...

  386 passing (3m)
  317 pending
  1 failing

  1) Launcher specs
       Puppeteer
         Browser.disconnect
           should reject navigation when browser closes:
     Error: expect(received).toBe(expected) // Object.is equality

Expected: "Navigation failed because browser has disconnected!"
Received: "Protocol error (Page.navigate): Target closed."
      at Context.<anonymous> (test/launcher.spec.ts:146:31)
      at processTicksAndRejections (internal/process/task_queues.js:97:5)

@jackfranklin
Copy link
Contributor

Might be a FF nightly test issue. @mathiasbynens wdyt? We could disable the test if it continues to flake. The changes in this PR are only TS types so shouldn't cause this issue.

@karlhorky
Copy link
Contributor Author

karlhorky commented Mar 31, 2021

Similar, but slightly different error this time:

  386 passing (4m)
  317 pending
  1 failing

  1) Launcher specs
       Puppeteer
         Puppeteer.connect
           should be able to reconnect:
     Error: Timeout of 25000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/puppeteer/puppeteer/test/launcher.spec.ts)

@jackfranklin
Copy link
Contributor

@whimboo would you mind taking a look at this PR? It seems to always hit some FF issue around timeouts. I'm not sure if this is a problem with a recent FF nightly build, or some other issue, but it seems to happen consistently at the moment on this and other PRs.

@karlhorky
Copy link
Contributor Author

@jackfranklin Since this PR is unrelated to the test failures, maybe this PR could be merged in the meantime?

@whimboo
Copy link
Collaborator

whimboo commented Apr 6, 2021

@jackfranklin yes this is a known issue with Firefox Nightly at the moment. See https://bugzilla.mozilla.org/show_bug.cgi?id=1701168. Also you disabled this test for now via #7062.

@karlhorky please rebase against main and push again. The tests should pass now.

@jackfranklin jackfranklin merged commit c273e47 into puppeteer:main Apr 6, 2021
@karlhorky
Copy link
Contributor Author

Nice, thanks @jackfranklin and @whimboo !

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.

3 participants