-
Notifications
You must be signed in to change notification settings - Fork 780
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
Callback failures should result in cancellation of remaining tests #1633
Labels
Component: Core
For module, test, hooks, and reporters.
Type: Bug
Something isn't working right.
Type: Meta
Seek input from maintainers and contributors.
Comments
Krinkle
added
Type: Bug
Something isn't working right.
Type: Meta
Seek input from maintainers and contributors.
Component: Core
For module, test, hooks, and reporters.
labels
Jul 5, 2021
Krinkle
changed the title
Hook failures should result in cancellation of remaining tests
Callback failures should result in cancellation of remaining tests
Jul 5, 2021
Krinkle
added a commit
that referenced
this issue
Jul 5, 2021
Capture the status quo before changing it. Minor changes: * Switch remaining notEquals/indexOf uses to the preferred `assert.true( str.includes() )` idiom. * Fix duplicate printing of error message due to V8's `Error#stack`, as used by onUncaughtException. Ref #1629. * Start normalizing stderror in tests like we do with stdout. * Account for qunit.js stack frames from native Promise in V8, which doesn't include a function name or paranthesis. Ref #1446. Ref #1633.
Krinkle
added a commit
that referenced
this issue
Jul 5, 2021
Capture the status quo before changing it. Minor changes: * Switch remaining notEquals/indexOf uses to the preferred `assert.true( str.includes() )` idiom. * Fix duplicate printing of error message due to V8's `Error#stack`, as used by onUncaughtException. Ref #1629. * Start normalizing stderror in tests like we do with stdout. * Account for qunit.js stack frames from native Promise in V8, which doesn't include a function name or paranthesis. Ref #1446. Ref #1633.
Krinkle
added a commit
that referenced
this issue
Jul 5, 2021
Capture the status quo before changing it. Minor changes: * Switch remaining notEquals/indexOf uses to the preferred `assert.true( str.includes() )` idiom. * Fix duplicate printing of error message due to V8's `Error#stack`, as used by onUncaughtException. Ref #1629. * Start normalizing stderror in tests like we do with stdout. * Account for qunit.js stack frames from native Promise in V8, which doesn't include a function name or paranthesis. Ref #1446. Ref #1633.
Krinkle
added a commit
to qunitjs/js-reporters
that referenced
this issue
Jul 5, 2021
Krinkle
added a commit
to qunitjs/js-reporters
that referenced
this issue
Jul 5, 2021
Krinkle
added a commit
to qunitjs/js-reporters
that referenced
this issue
Jul 6, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Component: Core
For module, test, hooks, and reporters.
Type: Bug
Something isn't working right.
Type: Meta
Seek input from maintainers and contributors.
From #1446
So... this is technically true and would suffice, and it might make sense to patch and release that by itself in the name of progress. However, as part of working on this patch I figured it would make sense to add tests for failures from other callbacks as well and see how those actually get handled internally, and I'm not particularly happy with the state of that.
QUnit.begin()
,QUnit.moduleStart()
, andQUnit.testStart()
This is the most straight-forward in that it runs at a point where all internal state is naturally the most clean. It has its own call stack disconnected from any test, and so offers a natural point where we can tack on
.catch( onUncaughtException )
and change.then( advance )
to.finally( advance )
to make sure the reporting of the "global failure" test happens correctly.The downside of this approach is that this means we will try to run all tests to completion still, which depending on how important the begin handler was could result in a lot of noisy cascading failures. Perhaps not unreasonable in QUnit's spirit of trying to report all subsequent errors upfront, but something to keep in mind.
The real problem, however, comes when you have a failure in a
testStart
ormoduleStart
callback as well. Because we use atest()
to report global failures between runStart and runEnd, this would end up reporting something like this:QUnit.testDone()
,QUnit.moduleDone()
The real mess starts with the end handlers. These similarly try to create new tests. These tests either get queued to the end and reported rather late (possibly never), or if we use the
prioritize
flag they can be brought forward and with them the implicit "global" module that the new test would be contained by, and thus afterward anothermoduleDone
handler. This is a problem sincemoduleDone
handling has already closed the module hooks, but then a new test is inserted, increasing its count, and then reachingmoduleDone
for the second time, but this time triggering the good olTypeError: module.hooks[handler] is undefined
safeguard. In the browser this would go natively to the console, and in the CLI it would be swallowed due to recursive failures from uncaught errors, and thenprocess.on('exit')
would reportError: Process exited before tests finished running
.All of this doesn't even get into what happens when the "current" module is ignored, skipped, focussed, todo, etc. It's all rather arbitrary.
Path forward
I think we should consider following the "bail out" paradigm for uncaught exceptions. This means they would be passed directly to reporters instead of via an ad-hoc "test", and then the processing queue cancelled.
This is more or less what happens today, although mostly by accident and in a mostly silent/confusing manner. The TAP standard has already specified this feature and is in use by other systems.
The text was updated successfully, but these errors were encountered: