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: Fix async dialog specs when they fail #5859
Conversation
When this test was failing, it would cause no future tests to run. This was because the `expect` call within the `page.on` callback would throw an error, and that would trigger a unhandled promise rejection that caused the test framework to stop. The fundamental issue here is making `expect` calls within callbacks. They are brittle due to the fact that they throw, and the test framework won't catch it, but also because you have no guarantee that they will run. If the callback is never executed you dont' know about it. Although it's slightly more code, using a stub is the way to do this. Not only can we assert that the stub was called, we can make synchronous `expect` calls that Mocha will pick up properly if they fail. Before this change, running the tests (and making it fail on purpose) would cause all test execution to stop: ``` > puppeteer@3.0.4-post unit /Users/jacktfranklin/src/puppeteer > mocha --config mocha-config/puppeteer-unit-tests.js .(node:69580) UnhandledPromiseRejectionWarning: Error: expect(received).toBe(expected) // Object.is equality Expected: "yes." Received: "" at Page.<anonymous> (/Users/jacktfranklin/src/puppeteer/test/dialog.spec.js:42:37) [snip] (node:69580) UnhandledPromiseRejectionWarning: Unhandled promise rejection ... [snip] ``` But with this change, the rest of the tests run: ``` > puppeteer@3.0.4-post unit /Users/jacktfranklin/src/puppeteer > mocha --config mocha-config/puppeteer-unit-tests.js Page.Events.Dialog ✓ should fire 1) should allow accepting prompts ✓ should dismiss the prompt 2 passing (2s) 1 failing 1) Page.Events.Dialog should allow accepting prompts: Error: expect(received).toBe(expected) // Object.is equality Expected: "yes." Received: "" at Context.<anonymous> (test/dialog.spec.js:53:35) at processTicksAndRejections (internal/process/task_queues.js:94:5) ``` This is much better because one failing test now doesn't stop the rest of the test suite. This probably isn't the only instance of this in the codebase so I propose as we encounter them we fix them usng this commit as the template.
513f267
to
c8670e9
Compare
See also the second half of my patch on PR #5473, which fixes another test in |
@whimboo are you happy with this being merged? |
I think that's more a decision between you and @mathiasbynens. If we go that way I assume all other instances will also have to be updated in the same way? |
Yes, we can do that incrementally as we find them. @mathiasbynens let me know if you're happy with this. Pretty confident it solves the issues. |
Alright, let's do it. |
When this test was failing, it would cause no future tests to run. This
was because the
expect
call within thepage.on
callback would throwan error, and that would trigger a unhandled promise rejection that
caused the test framework to stop.
The fundamental issue here is making
expect
calls within callbacks.They are brittle due to the fact that they throw, and the test framework
won't catch it, but also because you have no guarantee that they will
run. If the callback is never executed you dont' know about it.
Although it's slightly more code, using a stub is the way to do this.
Not only can we assert that the stub was called, we can make synchronous
expect
calls that Mocha will pick up properly if they fail.Before this change, running the tests (and making it fail on purpose)
would cause all test execution to stop:
But with this change, the rest of the tests run:
This is much better because one failing test now doesn't stop the rest
of the test suite.
This probably isn't the only instance of this in the codebase so I
propose as we encounter them we fix them usng this commit as the
template.