Details of QUnit.begin error are lost #1446

asakusuma · 2020-06-10T16:06:38Z

QUnit version: 2.10.0
What environment are you running QUnit in? (e.g., browser, Node): node 12.16.0
How are you running QUnit? (e.g., script, testem, Grunt): yarn script
Env: Mac OS 10.15.5

If QUnit.begin returns a rejected promise, I would expect the rejection error to be shown to the user, but it is not. You get a Error: Process exited before tests finished running error with no error info.

Reproduction repo: https://github.com/asakusuma/qunit-begin-issue

The text was updated successfully, but these errors were encountered:

trentmwillis · 2020-06-10T22:36:22Z

Definitely sounds like a bug. Based on the report, my guess is it has something to do with the handlers in this code:

qunit/src/cli/run.js

Lines 70 to 91 in 474a708

    
           // Listen for unhandled rejections, and call QUnit.onUnhandledRejection 
        
           process.on( "unhandledRejection", function( reason ) { 
        
           	QUnit.onUnhandledRejection( reason ); 
        
           } ); 
        
           process.on( "uncaughtException", function( error ) { 
        
           	QUnit.onError( error ); 
        
           } ); 
        
           process.on( "exit", function() { 
        
           	if ( running ) { 
        
           		console.error( "Error: Process exited before tests finished running" ); 
        
           		const currentTest = QUnit.config.current; 
        
           		if ( currentTest && currentTest.semaphore ) { 
        
           			const name = currentTest.testName; 
        
           			console.error( "Last test to run (" + name + ") has an async hold. " + 
        
           				"Ensure all assert.async() callbacks are invoked and Promises resolve. " + 
        
           				"You should also set a standard timeout via QUnit.config.testTimeout." ); 
        
           		} 
        
           	} 
        
           } );

smcclure15 · 2021-04-19T19:00:41Z

#1595 has given me some better insight into this kind of exception management. Building on that PR makes this look very solvable - I'll wait for that to land and then iterate on this, given that there's a little overlap with how QUnit.onError might change.

Krinkle · 2021-05-08T23:23:04Z

I'm able to reproduce this still, and it seems like this is quite similar to #1377 indeed. And the same applies to QUnit.done() as well. We need to re-think how we handle these very early and very late errors.

Pen repro: https://codepen.io/Krinkle/pen/PomYvwN

== Background == Previously, QUnit.onError and QUnit.onUnhandledRejection could report global errors by synthesizing a new test, even after a run has ended. This is problematic when an errors ocurrs after all modules (and their hooks) have finished, and the overall test run has ended. The most immediate problem is that hooks having finished already, means it is illegal for a new test to start since "after" has already run. To protect against such illegal calls, the hooks object is emptied internally, and this new test causes an internal error: ``` TypeError: Cannot read property 'length' of undefined ``` This is not underlying problem though, but rather our internal safeguard working as intended. The higher-level problem is that there is no appropiate way to report a late error as a test since the run has already ended. The `QUnit.done()` callbacks have run, and the `runEnd` event has been emitted. == Approach == Instead of trying to report (late) errors as a test, only print them to `console.warn()`, which goes to stderr in Node.js. For the CLI, also remember that uncaught errors were found and use that to make sure we don't change exitCode back to zero (e.g. in case we have an uncaught error after the last test but before our `runEnd` callback is called). == Changes == * Generalise `QUnit.onUnhandledRejection` and re-use it for `window.onerror` (browser), and uncaught exceptions (CLI). * Fix broken use of `QUnit.onError` in `process.on( "uncaughtException" )`. This was passing the wrong parameters. Use the new onUncaughtException method instead. * Clarify that `QUnit.onError` is only for `window.onerror`. For now, keep its strange non-standard signature as-is (with the custom object parameter), but document this and its return value. * Remove the unused "..args" from `QUnit.onError`. This was only ever passed from one of our unit tests to give one extra argument (a string of "actual"), which then ended up passed as "actual" parameter to `pushFailure()`. We never used this in the actual onError binding, so remove this odd variadic construct for now. * Change `ProcessingQueue#done`, which is in charge of reporting the "No tests were run" error, to no longer rely on the way that `QUnit.onError` previously queued a late test. The first part of this function may run twice (same as before, once after an empty test run, and one more time after the synthetic test has finished and the queue is empty again). Change this so that we no longer assign `finished = true` in that first part. This means we will still support queueing of this one late test. But, since the quueue is empty, we do need to call `advance()` manually as otherwise it'd never get processed. Previously, `finished = true` was assigned first, which meant that `QUnit.onError` was adding a test under that condition. But this worked anyway because `Test#queue` internally had manual advancing exactly for this use case, which is also where we now emit a deprecation warning (to become an error in QUnit 3). Note that using this for anything other than the "No tests run" error was already unreliable since generally runEnd would have been emitted already. The "No tests run" test was exactly done from the one sweet spot where it was (and remains) safe because that threw an error and thus prevented runEnd from being emitted. Fixes qunitjs#1377. Ref qunitjs#1322. Ref qunitjs#1446.

== Background == Previously, QUnit.onError and QUnit.onUnhandledRejection could report global errors by synthesizing a new test, even after a run has ended. This is problematic when an errors ocurrs after all modules (and their hooks) have finished, and the overall test run has ended. The most immediate problem is that hooks having finished already, means it is illegal for a new test to start since "after" has already run. To protect against such illegal calls, the hooks object is emptied internally, and this new test causes an internal error: ``` TypeError: Cannot read property 'length' of undefined ``` This is not underlying problem though, but rather our internal safeguard working as intended. The higher-level problem is that there is no appropiate way to report a late error as a test since the run has already ended. The `QUnit.done()` callbacks have run, and the `runEnd` event has been emitted. == Approach == Instead of trying to report (late) errors as a test, only print them to `console.warn()`, which goes to stderr in Node.js. For the CLI, also remember that uncaught errors were found and use that to make sure we don't change exitCode back to zero (e.g. in case we have an uncaught error after the last test but before our `runEnd` callback is called). == Changes == * Generalise `QUnit.onUnhandledRejection` and re-use it for `window.onerror` (browser), and uncaught exceptions (CLI). * Fix broken use of `QUnit.onError` in `process.on( "uncaughtException" )`. This was passing the wrong parameters. Use the new onUncaughtException method instead. * Clarify that `QUnit.onError` is only for `window.onerror`. For now, keep its strange non-standard signature as-is (with the custom object parameter), but document this and its return value. * Remove the unused "..args" from `QUnit.onError`. This was only ever passed from one of our unit tests to give one extra argument (a string of "actual"), which then ended up passed as "actual" parameter to `pushFailure()`. We never used this in the actual onError binding, so remove this odd variadic construct for now. * Change `ProcessingQueue#done`, which is in charge of reporting the "No tests were run" error, to no longer rely on the way that `QUnit.onError` previously queued a late test. The first part of this function may run twice (same as before, once after an empty test run, and one more time after the synthetic test has finished and the queue is empty again). Change this so that we no longer assign `finished = true` in that first part. This means we will still support queueing of this one late test. But, since the quueue is empty, we do need to call `advance()` manually as otherwise it'd never get processed. Previously, `finished = true` was assigned first, which meant that `QUnit.onError` was adding a test under that condition. But this worked anyway because `Test#queue` internally had manual advancing exactly for this use case, which is also where we now emit a deprecation warning (to become an error in QUnit 3). Note that using this for anything other than the "No tests run" error was already unreliable since generally runEnd would have been emitted already. The "No tests run" test was exactly done from the one sweet spot where it was (and remains) safe because that threw an error and thus prevented runEnd from being emitted. Fixes #1377. Ref #1322. Ref #1446.

Krinkle · 2021-07-04T04:07:50Z

Circling back to this after the refactoring in #1629.

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 hooks.

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

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.

trentmwillis added Component: CLI Type: Bug Something isn't working right. labels Jun 10, 2020

Krinkle added the help welcome label Jul 30, 2020

Krinkle changed the title ~~QUnit.begin doesn't show rejection error or stack~~ Details of QUnit.begin error are lost May 8, 2021

Krinkle mentioned this issue Jun 28, 2021

Core: Fix late onerror handling #1629

Merged

Krinkle mentioned this issue Jul 4, 2021

Core: ensure process-queue advances with rejected promises #1391

Open

Krinkle self-assigned this Jul 4, 2021

Krinkle mentioned this issue Jul 5, 2021

Callback failures should result in cancellation of remaining tests #1633

Closed

Krinkle mentioned this issue Jul 5, 2021

Tests: Add tests covering callback failures #1634

Merged

Krinkle mentioned this issue Jul 26, 2021

Core: New error event for bailing on uncaught errors #1638

Merged

Krinkle closed this as completed in 8f5e7ed Jul 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Details of QUnit.begin error are lost #1446

Details of QUnit.begin error are lost #1446

asakusuma commented Jun 10, 2020 •

edited

Loading

trentmwillis commented Jun 10, 2020 •

edited

Loading

smcclure15 commented Apr 19, 2021

Krinkle commented May 8, 2021 •

edited

Loading

Krinkle commented Jul 4, 2021 •

edited

Loading

Details of QUnit.begin error are lost #1446

Details of QUnit.begin error are lost #1446

Comments

asakusuma commented Jun 10, 2020 • edited Loading

trentmwillis commented Jun 10, 2020 • edited Loading

smcclure15 commented Apr 19, 2021

Krinkle commented May 8, 2021 • edited Loading

Krinkle commented Jul 4, 2021 • edited Loading

asakusuma commented Jun 10, 2020 •

edited

Loading

trentmwillis commented Jun 10, 2020 •

edited

Loading

Krinkle commented May 8, 2021 •

edited

Loading

Krinkle commented Jul 4, 2021 •

edited

Loading