Skip to content

Commit

Permalink
CLI: Remove confusing expected: undefined from TAP for non-assert fail
Browse files Browse the repository at this point in the history
== pushFailure ==

This was setting `actual: null` and, left expected unset/undefined.
In HtmlReporter this idom was recognised to mean "don't show values",
based on hasOwn for `expected`. This worked because QUnit.log() passes
the internal assert result object as-is.

In TAP output, this was not skipped because Test.js#logAssertion
copies the object for the testEnd.errors array, and in doing so forges
an `expected` property to exist no matter what, thus with an implied
value of undefined. The hasOwn checks in TapReporter thus always
evaluate to true.

This meant TAP output for pushFailure() calls not only showed
redundant actual/expected entries, but actively created confusion
by setting them to different values, drawing attention to a supposed
difference that has no meaning

> actual: null
> expected: undefined

Fix by changing pushFailure to omit both actual and expected,
and change the condition in both reporters to skip rendering of values
based on both being strictly equal to `undefined`, instead of based
on `hasOwn('expected')`.

== onUncaughtException / `QUnit.on('error')` ==

For uncaught errors, we already omitted both actual and undefined,
the HtmlReporter thus already skipped them (for the reason above),
but in TAP output they showed, for the same reason as above as:

> actual: undefined
> expected: undefined

Which, while not as actively confusing, is at least redundant.
This is naturally fixed by the same change, which now omits this.
  • Loading branch information
Krinkle authored Aug 7, 2024
1 parent 62cc6d9 commit 9a1fc05
Show file tree
Hide file tree
Showing 28 changed files with 18 additions and 86 deletions.
4 changes: 2 additions & 2 deletions docs/api/assert/pushResult.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ Report the result of a custom assertion.
| name | description |
|------|-------------|
| `data.result` (boolean) | Result of the assertion |
| `data.actual` | Expression being tested |
| `data.expected` | Known comparison value |
| `data.actual` | Expression being tested (optional) |
| `data.expected` | Known comparison value (optional) |
| `data.message` (string or undefined) | Short description of the assertion |

## Examples
Expand Down
2 changes: 1 addition & 1 deletion docs/resources/example-fail.html
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
});
QUnit.test('banana', function (assert) {
assert.true(true, 'foo');
var sentence = 'This is actual.';
const sentence = 'This is actual.';
assert.equal(sentence, 'This is expected.', 'example sentence');
assert.true(true, 'bar');
assert.true(true, 'baz');
Expand Down
1 change: 1 addition & 0 deletions src/core/on-uncaught-exception.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { emit } from './events.js';
*/
export default function onUncaughtException (error) {
if (config.current) {
// This omits 'actual' and 'expected' (undefined)
config.current.assert.pushResult({
result: false,
message: `global failure: ${errorString(error)}`,
Expand Down
5 changes: 3 additions & 2 deletions src/core/reporters/HtmlReporter.js
Original file line number Diff line number Diff line change
Expand Up @@ -842,11 +842,12 @@ export default class HtmlReporter {
let actual;

// When pushFailure() is called, it is implied that no expected value
// or diff should be shown. It does not set details.expected.
// or diff should be shown, because both expected and actual as undefined.
//
// This must check details.expected existence. If it exists as undefined,
// that's a regular assertion for which to render actual/expected and a diff.
if (!details.result && hasOwn.call(details, 'expected')) {
const showAnyValues = !details.result && (details.expected !== undefined || details.actual !== undefined);
if (showAnyValues) {
if (details.negative) {
expected = 'NOT ' + dump.parse(details.expected);
} else {
Expand Down
11 changes: 5 additions & 6 deletions src/core/reporters/TapReporter.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ import { errorString } from '../utilities.js';
import { console } from '../globals.js';
import { annotateStacktrace } from '../stacktrace.js';

const hasOwn = Object.prototype.hasOwnProperty;

/**
* Format a given value into YAML.
*
Expand Down Expand Up @@ -259,11 +257,12 @@ export default class TapReporter {
out += `\n message: ${prettyYamlValue(error.message || 'failed')}`;
out += `\n severity: ${prettyYamlValue(severity || 'failed')}`;

if (hasOwn.call(error, 'actual')) {
// When pushFailure() is used, actual/expected are initially unset but
// eventually in Test#logAssertion, for testReport#pushAssertion, these are
// forged into existence as undefined.
const hasAny = (error.expected !== undefined || error.actual !== undefined);
if (hasAny) {
out += `\n actual : ${prettyYamlValue(error.actual)}`;
}

if (hasOwn.call(error, 'expected')) {
out += `\n expected: ${prettyYamlValue(error.expected)}`;
}

Expand Down
1 change: 0 additions & 1 deletion src/core/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -572,7 +572,6 @@ Test.prototype = {
this.pushResult({
result: false,
message: message || 'error',
actual: null,
source
});
},
Expand Down
2 changes: 0 additions & 2 deletions test/cli/cli-main.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,6 @@ not ok 2 slow
---
message: Test took longer than 7ms; test timed out.
severity: failed
actual : null
expected: undefined
stack: |
at internal
...
Expand Down
8 changes: 0 additions & 8 deletions test/cli/fixtures/assert-expect-failure-step.tap.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ not ok 3 wrong [a little off]
---
message: Expected 2 assertions, but 1 were run
severity: failed
actual : null
expected: undefined
stack: |
at /qunit/test/cli/fixtures/assert-expect-failure-step.js:22:7
at internal
Expand All @@ -17,8 +15,6 @@ not ok 4 wrong [way off]
---
message: Expected 5 assertions, but 1 were run
severity: failed
actual : null
expected: undefined
stack: |
at /qunit/test/cli/fixtures/assert-expect-failure-step.js:30:7
at internal
Expand All @@ -29,8 +25,6 @@ not ok 5 previously passing [once]
Expected 4 assertions, but 2 were run.
It looks like you are upgrading from QUnit 2. Steps no longer count as separate assertions. https://qunitjs.com/api/assert/expect/
severity: failed
actual : null
expected: undefined
stack: |
at /qunit/test/cli/fixtures/assert-expect-failure-step.js:40:7
at internal
Expand All @@ -41,8 +35,6 @@ not ok 6 previously passing [twice]
Expected 9 assertions, but 4 were run.
It looks like you are upgrading from QUnit 2. Steps no longer count as separate assertions. https://qunitjs.com/api/assert/expect/
severity: failed
actual : null
expected: undefined
stack: |
at /qunit/test/cli/fixtures/assert-expect-failure-step.js:49:7
at internal
Expand Down
2 changes: 0 additions & 2 deletions test/cli/fixtures/assert-expect-failure.tap.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ not ok 1 failing test
---
message: Expected 2 assertions, but 1 were run
severity: failed
actual : null
expected: undefined
stack: |
at /qunit/test/cli/fixtures/assert-expect-failure.js:1:7
at internal
Expand Down
2 changes: 0 additions & 2 deletions test/cli/fixtures/assert-expect-no-assertions.tap.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ not ok 1 no assertions
---
message: Expected at least one assertion, but none were run - call expect(0) to accept zero assertions.
severity: failed
actual : null
expected: undefined
stack: |
at /qunit/test/cli/fixtures/assert-expect-no-assertions.js:1:7
at internal
Expand Down
6 changes: 0 additions & 6 deletions test/cli/fixtures/async-test-throw.tap.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ not ok 1 throw early
---
message: "Promise rejected during \"throw early\": boo"
severity: failed
actual : null
expected: undefined
stack: |
Error: boo
at /qunit/test/cli/fixtures/async-test-throw.js:2:9
Expand All @@ -15,8 +13,6 @@ not ok 2 throw late
---
message: "Promise rejected during \"throw late\": boo"
severity: failed
actual : null
expected: undefined
stack: |
Error: boo
at /qunit/test/cli/fixtures/async-test-throw.js:8:9
Expand All @@ -25,8 +21,6 @@ not ok 3 test with bad thenable
---
message: "Promise rejected during \"test with bad thenable\": boo"
severity: failed
actual : null
expected: undefined
stack: |
Error: boo
at /qunit/test/cli/fixtures/async-test-throw.js:16:13
Expand Down
2 changes: 0 additions & 2 deletions test/cli/fixtures/config-noglobals-add.tap.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ not ok 1 adds global var
---
message: Introduced global variable(s): dummyGlobal
severity: failed
actual : null
expected: undefined
stack: |
at qunit.js
...
Expand Down
4 changes: 1 addition & 3 deletions test/cli/fixtures/config-noglobals-remove.tap.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ not ok 1 deletes global var
---
message: Deleted global variable(s): dummyGlobal
severity: failed
actual : null
expected: undefined
stack: |
at qunit.js
...
Expand All @@ -17,4 +15,4 @@ not ok 1 deletes global var
# todo 0
# fail 1

# exit code: 1
# exit code: 1
4 changes: 0 additions & 4 deletions test/cli/fixtures/config-notrycatch-hook-rejection.tap.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,12 @@ not ok 1 example > passing test
---
message: global failure: bad things happen
severity: failed
actual : undefined
expected: undefined
stack: |
at internal
...
---
message: Test took longer than 1000ms; test timed out.
severity: failed
actual : null
expected: undefined
stack: |
at internal
...
Expand Down
4 changes: 0 additions & 4 deletions test/cli/fixtures/config-notrycatch-test-rejection.tap.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,12 @@ not ok 1 example > returns a rejected promise
---
message: global failure: bad things happen
severity: failed
actual : undefined
expected: undefined
stack: |
at internal
...
---
message: Test took longer than 1000ms; test timed out.
severity: failed
actual : null
expected: undefined
stack: |
at internal
...
Expand Down
4 changes: 1 addition & 3 deletions test/cli/fixtures/config-requireExpects.tap.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ not ok 1 passing test
---
message: Expected number of assertions to be defined, but expect() was not called.
severity: failed
actual : null
expected: undefined
stack: |
at /qunit/test/cli/fixtures/config-requireExpects.js:3:7
at internal
Expand All @@ -18,4 +16,4 @@ not ok 1 passing test
# todo 0
# fail 1

# exit code: 1
# exit code: 1
2 changes: 0 additions & 2 deletions test/cli/fixtures/config-testTimeout.tap.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ not ok 1 slow
---
message: Test took longer than 10ms; test timed out.
severity: failed
actual : null
expected: undefined
stack: |
at internal
...
Expand Down
4 changes: 1 addition & 3 deletions test/cli/fixtures/done-after-timeout.tap.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ not ok 1 times out before scheduled done is called
---
message: Test took longer than 10ms; test timed out.
severity: failed
actual : null
expected: undefined
stack: |
at internal
...
Expand All @@ -17,4 +15,4 @@ not ok 1 times out before scheduled done is called
# todo 0
# fail 1

# exit code: 1
# exit code: 1
2 changes: 0 additions & 2 deletions test/cli/fixtures/drooling-done.tap.txt
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ not ok 1 Test A
at /qunit/test/cli/fixtures/drooling-done.js:5:7
at internal
severity: failed
actual : null
expected: undefined
stack: |
Error: this is an intentional error
at /qunit/test/cli/fixtures/drooling-done.js:8:9
Expand Down
2 changes: 0 additions & 2 deletions test/cli/fixtures/drooling-extra-done.tap.txt
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ not ok 2 Test B
at /qunit/test/cli/fixtures/drooling-extra-done.js:13:7
at internal
severity: failed
actual : null
expected: undefined
stack: |
Error: Unexpected release of async pause during a different test.
> Test: Test A [async #1]
Expand Down
4 changes: 1 addition & 3 deletions test/cli/fixtures/hanging-test.tap.txt
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
# name: test that hangs
# command: ["qunit","hanging-test.js"]
# command: ["qunit", "hanging-test.js"]

TAP version 13
not ok 1 hanging
---
message: Test took longer than 3000ms; test timed out.
severity: failed
actual : null
expected: undefined
stack: |
at internal
...
Expand Down
12 changes: 0 additions & 12 deletions test/cli/fixtures/hooks-global-throw.tap.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,13 @@ not ok 1 global hook throws
---
message: Global beforeEach failed on global hook throws: Error: banana
severity: failed
actual : null
expected: undefined
stack: |
Error: banana
at /qunit/test/cli/fixtures/hooks-global-throw.js:3:11
...
---
message: Global afterEach failed on global hook throws: Error: apple
severity: failed
actual : null
expected: undefined
stack: |
Error: apple
at /qunit/test/cli/fixtures/hooks-global-throw.js:17:11
Expand All @@ -24,17 +20,13 @@ not ok 2 global hook rejects
---
message: "Promise rejected before \"global hook rejects\": banana"
severity: failed
actual : null
expected: undefined
stack: |
Error: banana
at /qunit/test/cli/fixtures/hooks-global-throw.js:5:27
...
---
message: "Promise rejected after \"global hook rejects\": apple"
severity: failed
actual : null
expected: undefined
stack: |
Error: apple
at /qunit/test/cli/fixtures/hooks-global-throw.js:19:27
Expand All @@ -43,17 +35,13 @@ not ok 3 global hook with bad thenable
---
message: "Promise rejected before \"global hook with bad thenable\": global brocoli"
severity: failed
actual : null
expected: undefined
stack: |
Error: global brocoli
at /qunit/test/cli/fixtures/hooks-global-throw.js:9:15
...
---
message: "Promise rejected after \"global hook with bad thenable\": global artichoke"
severity: failed
actual : null
expected: undefined
stack: |
Error: global artichoke
at /qunit/test/cli/fixtures/hooks-global-throw.js:23:15
Expand Down
6 changes: 2 additions & 4 deletions test/cli/fixtures/pending-async-after-timeout.tap.txt
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
# name: test with pending async after timeout
# command: ["qunit","pending-async-after-timeout.js"]
# command: ["qunit", "pending-async-after-timeout.js"]

TAP version 13
not ok 1 example
---
message: Test took longer than 10ms; test timed out.
severity: failed
actual : null
expected: undefined
stack: |
at internal
...
Expand All @@ -17,4 +15,4 @@ not ok 1 example
# todo 0
# fail 1

# exit code: 1
# exit code: 1
2 changes: 0 additions & 2 deletions test/cli/fixtures/timeout.tap.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ not ok 1 timeout > first
---
message: Test took longer than 10ms; test timed out.
severity: failed
actual : null
expected: undefined
stack: |
at internal
...
Expand Down
2 changes: 0 additions & 2 deletions test/cli/fixtures/too-many-done-calls.tap.txt
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@ not ok 1 Test A
at /qunit/test/cli/fixtures/too-many-done-calls.js:1:7
at internal
severity: failed
actual : null
expected: undefined
stack: |
Error: Tried to release async pause that was already released.
> Test: Test A [async #1]
Expand Down
Loading

0 comments on commit 9a1fc05

Please sign in to comment.