2.17.0 no longer throws an error in the browser runner when a filter does not match any tests #1651
Assignees
Comments
|
@kategengler Thanks, this is a regression indeed. There should definitely be something in the UI for that. |
Krinkle
added a commit
to Krinkle/qunit
that referenced
this issue
Sep 8, 2021
Follows-up 8f5e7ed, which fixed the bug where in the "No tests" error (or indeed any global error after tests are done running) could cause the test runner to get stuck. That change maintained backward compatibility by continuing to count these global errors as "failed tests", so that CI reporters listening for `QUnit.done()` on `QUnit.on("runEnd")` continue to get the same negative signal. And, it continued to visually render them as a failed test if they happen during test execution or as the last/only event. But... it inserted the element into the DOM without making visible, which fooled our internal unit tests. Fix qunitjs#1651.
|
The error message is actually still rendered in the DOM. But, it has
The reporting to the console as @kategengler @stefanpenner I reviewed various reporters and CI integrations and thought it would not be a breaking change to report these as test failures (or TAP error) instead - only falling back to a global error if there is no other way. If this requires changes in ember-cli to accomodate or otherwise feels like it might make anything difficult or different from before, do let me know as that that is in no way intended. I'd be happy to revise this or pull it back out for now. |
|
@Krinkle A failing test should work with ember-cli / testem. In the test that started failing, the results were reporting back via with '0 tests, 0 pass, 0 fail' reported. I can try to make a more involved reproduction. |
Krinkle
added a commit
that referenced
this issue
Sep 9, 2021
Follows-up 8f5e7ed, which fixed the bug where in the "No tests" error (or indeed any global error after tests are done running) could cause the test runner to get stuck. That change maintained backward compatibility by continuing to count these global errors as "failed tests", so that CI reporters listening for `QUnit.done()` on `QUnit.on("runEnd")` continue to get the same negative signal. And, it continued to visually render them as a failed test if they happen during test execution or as the last/only event. But... it inserted the element into the DOM without making visible, which fooled our internal unit tests. Fix #1651.
Krinkle
added a commit
to Krinkle/qunit
that referenced
this issue
Sep 13, 2021
Historically, the "No tests were run" message was thrown as a real uncaught error by us, recieved by the native error handler in the browser or Node.js, and we then listened to that to generate a failing test case to show in the UI for extra visibility. The use of fake tests in this way wasn't great and caused problems when they happened after a "skip" module, or if there error came from QUnit.begin, runStart, testStart, or beforeEach hooks, since that would prevent the test from coming through and left internals in a broken state. We fixed that in QUnit 2.17 by rendering uncaught errors directly to the UI instead of queueing them up as a fake test that may or may not make it to the UI. To preserve API compatibility, we still count this as a failed test and still make it flip "status" to "failed" in the `QUnit.done()` and `runEnd` event. Thus, just as before, all uncaught errors are reported to the browser/Node handler, result in a failing test run, and shown in the UI. == Problem 1. I wrongly changed the "No tests" message after 07de3c6 and 8f5e7ed to be passed directly to our `onUncaughtError` function, thus not it is not seen by the browser/Node as a real uncaught error. A good reporter will still fail their build based on our failing test count or "status" field from done/runEnd, but they could no longer find out why (e.g. the "No tests" message). 2. There are reporters, including grunt-contirb-qunit and testem, which use neither the`runEnd` event nor the `QUnit.done()` callback to find out the test counts, or the overall "status". Instead, they rely solely on "testDone" to discover all failures. In the case of testem, it does listen to QUnit.done but only to get the test duration and stop the browser, it uses its own test counts to determine the run status. [a] In the case of grunt-contrib-qunit, it did utilize the QUnit.done data to find the number of failed assertions, but had its own logic for counting the number of tests, and it used the test count to decide the outcome of the test run. [b] Also, the Puppeteer pageerror event in grunt-contrib-qunit is not printed and not used to fail the task. == Change Per problem 1, let's re-instate the old behaviour. Before QUnit 2.17 the "No tests" message was both an uncaught error *and* a fake test. With the new safeguards in place we have to choose which one we want. I went for a fake test because that seems most rebust for reporters, since evidently some reporters do not report not uncaught errors. I've also removed the `Logger.warn()` fallback in the HTML Reporter. I added this in 8f5e7ed, not realizing that uncaught errors are already rendered in the browser console by default, so this caused it to show up twice. There was however once case of an error being passed to `onUncaughtException` directly without actually being recieved by window.onerror, and that was the "No tests" message which would otherwise not be visible anywhere if the UI was disabled. However this is no longer an issue since this message is now reported as a fake test. [a] https://github.com/gruntjs/grunt-contrib-qunit/blob/v5.1.0/tasks/qunit.js#L197-L236 [b] https://github.com/testem/testem/blob/v3.5.0/public/testem/qunit_adapter.js Ref qunitjs#1651.
Krinkle
added a commit
to Krinkle/qunit
that referenced
this issue
Sep 13, 2021
Historically, the "No tests were run" message was thrown as a real uncaught error by us, recieved by the native error handler in the browser or Node.js, and we then listened to that to generate a failing test case to show in the UI for extra visibility. The use of fake tests in this way wasn't great and caused problems when they happened after a "skip" module, or if there error came from QUnit.begin, runStart, testStart, or beforeEach hooks, since that would prevent the test from coming through and left internals in a broken state. We fixed that in QUnit 2.17 by rendering uncaught errors directly to the UI instead of queueing them up as a fake test that may or may not make it to the UI. To preserve API compatibility, we still count this as a failed test and still make it flip "status" to "failed" in the `QUnit.done()` and `runEnd` event. Thus, just as before, all uncaught errors are reported to the browser/Node handler, result in a failing test run, and shown in the UI. == Problem 1. I wrongly changed the "No tests" message after 07de3c6 and 8f5e7ed to be passed directly to our `onUncaughtError` function, thus not it is not seen by the browser/Node as a real uncaught error. A good reporter will still fail their build based on our failing test count or "status" field from done/runEnd, but they could no longer find out why (e.g. the "No tests" message). 2. There are reporters, including grunt-contirb-qunit and testem, which use neither the`runEnd` event nor the `QUnit.done()` callback to find out the test counts, or the overall "status". Instead, they rely solely on "testDone" to discover all failures. In the case of testem, it does listen to QUnit.done but only to get the test duration and stop the browser, it uses its own test counts to determine the run status. [a] In the case of grunt-contrib-qunit, it did utilize the QUnit.done data to find the number of failed assertions, but had its own logic for counting the number of tests, and it used the test count to decide the outcome of the test run. [b] Also, the Puppeteer pageerror event in grunt-contrib-qunit is not printed and not used to fail the task. == Change Per problem 1, let's re-instate the old behaviour. Before QUnit 2.17 the "No tests" message was both an uncaught error *and* a fake test. With the new safeguards in place we have to choose which one we want. I went for a fake test because that seems most rebust for reporters, since evidently some reporters do not report not uncaught errors. I've also removed the `Logger.warn()` fallback in the HTML Reporter. I added this in 8f5e7ed, not realizing that uncaught errors are already rendered in the browser console by default, so this caused it to show up twice. There was however once case of an error being passed to `onUncaughtException` directly without actually being recieved by window.onerror, and that was the "No tests" message which would otherwise not be visible anywhere if the UI was disabled. However this is no longer an issue since this message is now reported as a fake test. [a] https://github.com/gruntjs/grunt-contrib-qunit/blob/v5.1.0/tasks/qunit.js#L197-L236 [b] https://github.com/testem/testem/blob/v3.5.0/public/testem/qunit_adapter.js Ref qunitjs#1651.
Krinkle
added a commit
that referenced
this issue
Sep 13, 2021
Historically, the "No tests were run" message was thrown as a real uncaught error by us, recieved by the native error handler in the browser or Node.js, and we then listened to that to generate a failing test case to show in the UI for extra visibility. The use of fake tests in this way wasn't great and caused problems when they happened after a "skip" module, or if there error came from QUnit.begin, runStart, testStart, or beforeEach hooks, since that would prevent the test from coming through and left internals in a broken state. We fixed that in QUnit 2.17 by rendering uncaught errors directly to the UI instead of queueing them up as a fake test that may or may not make it to the UI. To preserve API compatibility, we still count this as a failed test and still make it flip "status" to "failed" in the `QUnit.done()` and `runEnd` event. Thus, just as before, all uncaught errors are reported to the browser/Node handler, result in a failing test run, and shown in the UI. == Problem 1. I wrongly changed the "No tests" message after 07de3c6 and 8f5e7ed to be passed directly to our `onUncaughtError` function, thus not it is not seen by the browser/Node as a real uncaught error. A good reporter will still fail their build based on our failing test count or "status" field from done/runEnd, but they could no longer find out why (e.g. the "No tests" message). 2. There are reporters, including grunt-contirb-qunit and testem, which use neither the`runEnd` event nor the `QUnit.done()` callback to find out the test counts, or the overall "status". Instead, they rely solely on "testDone" to discover all failures. In the case of testem, it does listen to QUnit.done but only to get the test duration and stop the browser, it uses its own test counts to determine the run status. [a] In the case of grunt-contrib-qunit, it did utilize the QUnit.done data to find the number of failed assertions, but had its own logic for counting the number of tests, and it used the test count to decide the outcome of the test run. [b] Also, the Puppeteer pageerror event in grunt-contrib-qunit is not printed and not used to fail the task. == Change Per problem 1, let's re-instate the old behaviour. Before QUnit 2.17 the "No tests" message was both an uncaught error *and* a fake test. With the new safeguards in place we have to choose which one we want. I went for a fake test because that seems most rebust for reporters, since evidently some reporters do not report not uncaught errors. I've also removed the `Logger.warn()` fallback in the HTML Reporter. I added this in 8f5e7ed, not realizing that uncaught errors are already rendered in the browser console by default, so this caused it to show up twice. There was however once case of an error being passed to `onUncaughtException` directly without actually being recieved by window.onerror, and that was the "No tests" message which would otherwise not be visible anywhere if the UI was disabled. However this is no longer an issue since this message is now reported as a fake test. [a] https://github.com/gruntjs/grunt-contrib-qunit/blob/v5.1.0/tasks/qunit.js#L197-L236 [b] https://github.com/testem/testem/blob/v3.5.0/public/testem/qunit_adapter.js Ref #1651.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In the browser runner, v2.17.0 no longer throws an error when no tests match the filter.
On v2.16.0 in the browser
The following is a result of tests run with a filter that matches 0 tests:
On v2.17.0 using the browser:
No exception is thrown and the results are curious (
1 tests completed in [2] milliseconds, with 1 failed, 0 skipped, and 0 todo. 0 assertions of 1 passed, 1 failed.)On v2.17.0 using
qunitThe exception is thrown:
Reproduction
See https://github.com/kategengler/qunit-repro and follow directions in README.md
Tell us about your runtime:
The text was updated successfully, but these errors were encountered: