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

Callback failures should result in cancellation of remaining tests #1633

Closed
Krinkle opened this issue Jul 5, 2021 · 0 comments
Closed

Callback failures should result in cancellation of remaining tests #1633

Krinkle opened this issue Jul 5, 2021 · 0 comments
Assignees
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
Copy link
Member

Krinkle commented Jul 5, 2021

From #1446

I could still reproduce the issue of swalled stacktraces for uncaught errors and rejections from QUnit.begin(), however all this needs is a .catch() handler for the queue advance step of the early callbacks.

... and, that's exactly what @step2yeung already proposed with #1391.

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(), and QUnit.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 or moduleStart callback as well. Because we use a test() to report global failures between runStart and runEnd, this would end up reporting something like this:

not ok: global failure            # from: QUnit.begin -> onUncaughtException -> new test
  message: global failure: Boo    # from: QUnit.testStart -> onUncaughtException -> push assertion
  result: false
  ...
  message: Boo                    # from: QUnit.begin -> onUncaughtException -> new test -> assertion
  result: false

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 another moduleDone handler. This is a problem since moduleDone handling has already closed the module hooks, but then a new test is inserted, increasing its count, and then reaching moduleDone for the second time, but this time triggering the good ol TypeError: 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 then process.on('exit') would report Error: 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.

@Krinkle 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 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
@Krinkle Krinkle self-assigned this Jul 7, 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.
Development

No branches or pull requests

1 participant