Skip to content

Commit

Permalink
Core: Fix inaccurate test count in reporter output after re-run
Browse files Browse the repository at this point in the history
When using the HTML Reporter to re-run a specific test, the summary
would accurately report the number of assertions, but inaccurately
report the number of tests.

This was because the `Test.valid()` function, which decides whether
the test will run and is where the testId re-run filter is applied,
was called at different points with different state. Once it was called
in the constructor when we pass it to the TestReport, and then again
later at run-time to definitively decide whether to run it.

The `valid()` function isn't meant to be able to change what it
returns, but the first call could wrongly returns false because it
observes that a `config.testId` filter is active, but `this.testId`
is not yet assigned at this point in the constructor.

Fixes #1687.
  • Loading branch information
Krinkle committed Oct 16, 2022
1 parent e04ec8a commit bc1cc38
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 12 deletions.
19 changes: 9 additions & 10 deletions src/test.js
Expand Up @@ -66,7 +66,7 @@ export default function Test (settings) {
// (Meaning the CI passes irregardless of the added tests).
//
// TODO: Make this an error in QUnit 3.0
// throw new Error( "Unexpected new test after the run already ended" );
// throw new Error( "Unexpected test after runEnd" );
Logger.warn('Unexpected test after runEnd. This is unstable and will fail in QUnit 3.0.');
return;
}
Expand All @@ -75,6 +75,14 @@ export default function Test (settings) {
throw new TypeError(`You must provide a callback to ${method}("${this.testName}")`);
}

// Register unique strings
for (let i = 0, l = this.module.tests; i < l.length; i++) {
if (this.module.tests[i].name === this.testName) {
this.testName += ' ';
}
}
this.testId = generateHash(this.module.name, this.testName);

// No validation after this. Beyond this point, failures must be recorded as
// a completed test with errors, instead of early bail out.
// Otherwise, internals may be left in an inconsistent state.
Expand All @@ -94,15 +102,6 @@ export default function Test (settings) {
valid: this.valid()
});

// Register unique strings
for (let i = 0, l = this.module.tests; i < l.length; i++) {
if (this.module.tests[i].name === this.testName) {
this.testName += ' ';
}
}

this.testId = generateHash(this.module.name, this.testName);

this.module.tests.push({
name: this.testName,
testId: this.testId,
Expand Down
4 changes: 2 additions & 2 deletions test/cli/fixtures/expected/tap-outputs.js
Expand Up @@ -632,8 +632,8 @@ ok 1 test 2
ok 2 module A > module B > test 1
ok 3 module A > module C > test 2
ok 4 module D > test 1
1..0
# pass 0
1..4
# pass 4
# skip 0
# todo 0
# fail 0`,
Expand Down

0 comments on commit bc1cc38

Please sign in to comment.