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

Core: Restore fake test for "No tests were run" message #1655

Merged
merged 1 commit into from
Sep 13, 2021

Conversation

Krinkle
Copy link
Member

@Krinkle Krinkle commented 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 therunEnd 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.

@Krinkle Krinkle requested a review from smcclure15 September 13, 2021 04:40
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 Krinkle force-pushed the restore-noerror-test branch from 24d4e6c to f08cde1 Compare September 13, 2021 04:45
@Krinkle
Copy link
Member Author

Krinkle commented Sep 13, 2021

@smcclure15 I regret not realizing this earlier. Could use a second pair of eyes for this, but would like to release relatively soon as a patch release.

Krinkle added a commit to Krinkle/qunit that referenced this pull request Sep 13, 2021
Including an end-to-end failure test for qunitjs#1655.
Krinkle added a commit to Krinkle/qunit that referenced this pull request Sep 13, 2021
Including an end-to-end failure test for qunitjs#1655.
@Krinkle Krinkle self-assigned this Sep 13, 2021
@Krinkle Krinkle requested a review from gibson042 September 13, 2021 16:28
@Krinkle Krinkle merged commit f30d5de into qunitjs:main Sep 13, 2021
@Krinkle Krinkle deleted the restore-noerror-test branch September 13, 2021 21:52
Krinkle added a commit to Krinkle/qunit that referenced this pull request Sep 18, 2021
A basic passing test, and an end-to-end "expect failure" test
to prevent regression qunitjs#1655.
Krinkle added a commit that referenced this pull request Sep 19, 2021
A basic passing test, and an end-to-end "expect failure" test
to prevent regression #1655.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants