-
Notifications
You must be signed in to change notification settings - Fork 783
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
Core: ensure process-queue advances with rejected promises #1391
base: main
Are you sure you want to change the base?
Conversation
2b36e3f
to
2f83202
Compare
} ); | ||
}; | ||
|
||
return promiseChain.then( moduleStartCallback, moduleStartCallback ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this will swallow any errors at this point - is that the desired behavior here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikrostew I assuming you are talking about const callbackPromises = notStartedModules.reduce(...);
?
if any of the promise from callbackPromises reject.. the whole promise should reject.. which should be handled on line 154:
return callbackPromises.then( testStartHandler, ( err ) => {
return Promise.reject( err ).catch( ( err ) => {
setTimeout( testStartHandler );
throw err;
} );
} );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was specifically looking at the line
return promiseChain.then( moduleStartCallback, moduleStartCallback );
If something in promiseChain
resolves, then it calls the first function argument moduleStartCallback
, no problem. But it looks like if it rejects (because of some error) it will call the second function argument, which is also moduleStartCallback
, which doesn't do anything with the error that it receives. So that rejection will stop there, so that callbackPromises
won't reject.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read this again, this shouldn't be a problem, but I can understand why it looks like the error is swallowed.
For a bit more context.. const callbackPromises = notStartedModules.reduce(...);
ensures the callbacks from notStartedModules are promisfied sequentually, so each callback that was made into a promise, an awaited for until the next callback is executed and made into a promise.
so You will get an chain of promises. In that chain, if there is one rejected promise, then the whole promise chain is rejected.
return promiseChain.then( moduleStartCallback, moduleStartCallback );
in this case, only ensures the next callback is promisfied even if the previous promise rejects. But as stated above, lien 154 will ensure the error is thrown.
I hope that makes more sense.. I think I should likely find a better way to code the explanation above up
@step2yeung - Would it be possible to split this into two changes (one for the actual bug being fixed along with its test, and another for the refactorings)? |
@step2yeung could you elaborate on the "problem" in the description above, can you relate your description to a user facing example. Currently, I'm having some trouble connecting the two. |
@stefanpenner Updated the description, hope it helps clarify the user facing issue. @rwjblue Separated out the fix and the refactor (will submit that afterwards) |
throw err; | ||
}; | ||
|
||
// Without throwAndAdvance, qunit does not continue advanceing the processing queue if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
advanceing
-> advancing
|
||
// Without throwAndAdvance, qunit does not continue advanceing the processing queue if | ||
// task() throws an error | ||
Promise.resolve( task() ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If task()
throws a synchronous error, the catch
bellow which would invoke throwAndAdvance
won't be invoked. I'm guessing this isn't intentional, and regardless if task throws synchronously or asynchronously, we would expect the catchAndAdance
to be invoked.
The following would rectify that issue.
new Promise(resolve => resolve(task()).then(processNextTaskOrAdance).catch(throwAndAdvance)
// task() throws an error | ||
Promise.resolve( task() ) | ||
.then( processNextTaskOrAdvance, throwAndAdvance ) | ||
.catch( throwAndAdvance ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to have throwAndAdvance
both in the then's rejection handler, and in the subsequent catch
?
As implemented if the throwAndAdvance
on line 79 is invoked, it will also cause the throwAndAdvance
on line 80 to be invoked.
I believe the intention here may be:
then(processNextTaskOrAdvance).catch(throwAndAdvance)
For added robustness, allow the DOM node to be missing at this point just in case. Extracted from #1391.
For added robustness, allow the DOM node to be missing at this point just in case. Extracted from #1391.
For added robustness, allow the DOM node to be missing at this point just in case. Extracted from #1391. Co-authored-by: step2yeung <syeung@linkedin.com>
return promiseChain.then( | ||
executeCallback, | ||
( err ) => { | ||
setTimeout( executeCallback ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use of setTimeout
must be conditional for non-browser environments such as SpiderMonkey (see other references). An alternate approach here can be to use Promise.resolve()
as fallback for setTimeout
, or to avoid this alltogether by using Promise.finally()
for the executeCallback, which naturally forwards the original rejection/error.
executeCallback, | ||
( err ) => { | ||
setTimeout( executeCallback ); | ||
throw err; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect that this particular thenable chain might be better without a local error handler as otherwise it means that the last error will be reported instead of the first one. I general QUnit's test logic is modelled to continue after assertion failures, but for uncaught exceptions I think we generally want to stop after the first for any given sequence.
You may ignore these inline comments. I've uncovered a couple of other things that I'll summarise at #1446. |
|
||
if ( !tests ) { | ||
if ( !tests || !testItem ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've extracted and applied this, with credit, via #1562 (released since then).
@@ -176,7 +176,10 @@ export function begin() { | |||
runLoggingCallbacks( "begin", { | |||
totalTests: Test.count, | |||
modules: modulesLog | |||
} ).then( unblockAndAdvanceQueue ); | |||
} ).then( unblockAndAdvanceQueue, function( err ) { | |||
setTimeout( unblockAndAdvanceQueue ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added tests for this scenario in #1634, and addressed the bug this fixes (#1446) via a different approach in #1638. In a nutshell, we handle this via the unhandledrejection
event in the browser and Node.js, same as before, but previously that didn't work right due to it creating a fake test which we no longer do.
The one part I believe we still haven't fixed is advancing the process-queue. It makes sense to advance the queue, but, I'd also like to make sure (to avoid future regressions) that the negative impact of this is quantified in an integration test. For the QUnit CLI (TAP) this is probably not particularly significant as Node.js processes naturally end at this point, and the error is reported with TAP-formatted bail out. For browsers, I imagine the process would remain stuck pending timeout, exactly as before, as we'd not proceed and thus not reach the |
I'm adding this as a CLI fixture instead of an HTML fixture because we don't currently have a good way of expecting a failure in an HTML test run. I've confirmed that the "hanging" appearance described in #1391 is equivalent to the "Process exited before tests finished running" message reported. Once we switch to capturing TAP by default, this will make the HTML tests much easier to verify no matter whether the pass/fail outcome. Ref #1391.
Problem:
processTaskQueue processes each task from the task queue as a promise. When the task throws an error, causing the promise to reject, the processing-queue does not continue to process the rest of the tasks queue. The user effect of this problem is qunit hanging, without continuing to process the rest of the task queue, meaning the test will not finish.
In our emberjs app that is uses qunit, the test execution will hang. Testem (the test runner) will recognize the browser has be hanging (using a timeout value) and exit the browser with some number of tests not executed.
Fix:
advance()
the processing-queue when a promise is rejected.The fix will throw the error, and continue to process the tasks, so the test run will actually run to completion.
Fixes #1446.