-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Mocha silently skips tests when an async error is thrown #15658
Comments
@jpettitt From looking at the mocha documentation at https://mochajs.org/#asynchronous-code, I wonder if these tests should be calling |
Another theory: Here are the last few lines of amphtml/extensions/amp-user-notification/0.1/amp-user-notification.js Lines 165 to 172 in acc1927
|
/cc @choumx |
Found the same issue with amp-iframe test as well. |
For amphtml/extensions/amp-iframe/0.1/amp-iframe.js Lines 165 to 177 in 1c555f2
Looks like user().assert calls |
Seems like several AMP tests do asynchronous stuff (like throwing errors), but don't correctly use the done() callback to indicate that the asynchronous tasks have completed. I'm looking for a centralized way to detect such anti-patterns, and isolate / fail tests that contain them. |
After some investigation, it turns out that functions like One potential fix for this issue is to stub out |
/cc @dvoytenko for comment on this, since we might have to grapple with complications due to how |
I can probably help here since I'm familiar with describes. What's the question? |
@zhouyx brought up the possibility of describes using an inner context, with separate console and error instances. |
IIUC, I don't think shadowing globals is generally a good idea. We should just use standard testing mechanisms e.g. stubbing. |
@choumx Here's what I found yesterday. We stub Using |
This is still happening, as reported today by @gmajoulet Repro steps:
At this point, Karma reports
|
This is still an issue. See mochajs/mocha#94 for reference. |
I think our best bet to resolve this issue in the intermediate-term is by setting up our test runner to intercept the |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. |
@jpettitt reported this while running the following test:
amphtml/extensions/amp-user-notification/0.1/test/test-amp-user-notification.js
Lines 80 to 88 in 97847f2
The error at the end of the test gets thrown asynchronously, by which time the test has completed. So Mocha makes it look like the test has passed, but the thrown error prevents it from running subsequent tests.
See mochajs/mocha#3223 and mochajs/mocha#3257 for more info on why this is a mocha problem.
The text was updated successfully, but these errors were encountered: