qunit does not run all tests in a module #1322
Labels
Comments
Krinkle
added
Category: API
Type: Bug
|
This looks similar to what we recently investigated at #1377 (comment). Those relate to async tests, where QUnit tries to report a global error but in doing so re-enters a closed test module and hits the problem of @shlomif Could you extract a minimal test case with the problem you encountered? That would help with confirming it as the same problem, or to learn that we have a another issue on the loose. You can fork CodePen: QUnit 2.15, for example. |
|
Hi Timo!
On Sat, 08 May 2021 16:02:58 -0700 Timo Tijhof ***@***.***> wrote:
This looks similar to what we recently investigated at
#1377 (comment), and
#1432 (comment). Those
relate to async tests, where QUnit tries to report a global error but in
doing so re-enters a closed test module and hits the problem of
`module.hooks[thing]` being undefined.
@shlomif Could you extract a minimal test case with the problem you
encountered? That would help with confirming it as the same problem, or to
learn that we have a another issue on the loose.
It's not really a priority for me now (I have too much on my mind now), so
please feel free to do it yourself. I'll gladly do a code review of that once
it is ready. Thanks and good luck!
… You can fork [CodePen: QUnit
2.15](https://codepen.io/Krinkle/pen/KKWpzzY?editors=1000), for example.
--
Shlomi Fish https://www.shlomifish.org/
Summer Glau Facts - https://shlom.in/sglau-facts
My Commodore 64 is suffering from slowness and insufficiency of memory, and its
display device is grievously short of pixels. Can anybody help?
— Omer Zak
Please reply to list if it's a mailing list post - https://shlom.in/reply .
|
Krinkle
added a commit
to Krinkle/qunit
that referenced
this issue
Jun 28, 2021
== 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.
Krinkle
added a commit
to Krinkle/qunit
that referenced
this issue
Jul 3, 2021
== 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.
Krinkle
added a commit
that referenced
this issue
Jul 3, 2021
== 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.
|
@shlomif I think the various releases have since fixed this. It wasn't obvious to me how to get your code running, but if you could check it against 2.17.2 or later that'd be great. (Later versions don't affect this logic.) I'll close it in a week or two, but feel free to leave a comment anytime and I'll re-open this. |
Everything seems fine now with qunit 2.17.2. thanks. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Tell us about your runtime:
What are you trying to do?
Code that reproduces the problem:
See https://www.shlomifish.org/fc-solve--qunit-1/js-fc-solve/automated-tests/i.html - source is here - https://github.com/shlomif/fc-solve/tree/qunit-bug--module-hooks/fc-solve/site/wml (note the branch).
If you have any relevant configuration information, please include that here:
What did you expect to happen?
Run all tests.
What actually happened?
not all tests were run and I get something in the console.
If I comment out this line:
then the problem disappears.
The text was updated successfully, but these errors were encountered: