Core: Fix test counter bug when nesting invalid test functions (#1515)

This doesn't affect end-users, since nesting QUnit.test() calls
isn't supported. In our own unit tests, we assert early bail out
behaviour for passing invalid arguments to QUnit.test(), and naturally
we do this from inside another QUnit.test() call.

The previous code left bailed out *after* we had already registered
the test and handed out references to reporting code. This meant
that it would count this as two tests of which one had an unknown
state, which js-reporters casts to a failed test.

This wasn't caught in CI because:

- When we run the test via the HTML Reporter and via the
  test-on-node task, we report it via the legacy QUnit.testDone()
  and QUnit.done() which didn't pay attention to this half-known
  test.

- And we don't currently run this test via the QUnit CLI
  (if we did, it would be failing)

Fixes #1514

src/test.js

@@ -25,17 +25,13 @@ import TestReport from "./reports/test";
 let focused = false;
 
 export default function Test( settings ) {
-	var i, l;
-
-	++Test.count;
-
 	this.expected = null;
 	this.assertions = [];
 	this.semaphore = 0;
 	this.module = config.currentModule;
 	this.steps = [];
 	this.timeout = undefined;
-	this.errorForStack = new Error();
+	extend( this, settings );
 
 	// If a module is skipped, all its tests and the tests of the child suites
 	// should be treated as skipped even if they are defined as `only` or `todo`.
@@ -45,24 +41,34 @@ export default function Test( settings ) {
 	// So, if a test is defined as `todo` and is inside a skipped module, we should
 	// then treat that test as if was defined as `skip`.
 	if ( this.module.skip ) {
-		settings.skip = true;
-		settings.todo = false;
+		this.skip = true;
+		this.todo = false;
 
 	// Skipped tests should be left intact
-	} else	if ( this.module.todo && !settings.skip ) {
-		settings.todo = true;
+	} else if ( this.module.todo && !this.skip ) {
+		this.todo = true;
 	}
 
-	extend( this, settings );
+	if ( !this.skip && typeof this.callback !== "function" ) {
+		const method = this.todo ? "QUnit.todo" : "QUnit.test";
+		throw new TypeError( `You must provide a callback to ${method}("${this.testName}")` );
+	}
 
-	this.testReport = new TestReport( settings.testName, this.module.suiteReport, {
-		todo: settings.todo,
-		skip: settings.skip,
+	// 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.
+	// Ref https://github.com/qunitjs/qunit/issues/1514
+
+	++Test.count;
+	this.errorForStack = new Error();
+	this.testReport = new TestReport( this.testName, this.module.suiteReport, {
+		todo: this.todo,
+		skip: this.skip,
 		valid: this.valid()
 	} );
 
 	// Register unique strings
-	for ( i = 0, l = this.module.tests; i < l.length; i++ ) {
+	for ( let i = 0, l = this.module.tests; i < l.length; i++ ) {
 		if ( this.module.tests[ i ].name === this.testName ) {
 			this.testName += " ";
 		}
@@ -73,23 +79,16 @@ export default function Test( settings ) {
 	this.module.tests.push( {
 		name: this.testName,
 		testId: this.testId,
-		skip: !!settings.skip
+		skip: !!this.skip
 	} );
 
-	if ( settings.skip ) {
+	if ( this.skip ) {
 
 		// Skipped tests will fully ignore any sent callback
 		this.callback = function() {};
 		this.async = false;
 		this.expected = 0;
 	} else {
-		if ( typeof this.callback !== "function" ) {
-			const method = this.todo ? "todo" : "test";
-
-			// eslint-disable-next-line max-len
-			throw new TypeError( `You must provide a function as a test callback to QUnit.${method}("${settings.testName}")` );
-		}
-
 		this.assert = new Assert( this );
 	}
 }

test/main/test.js

@@ -229,11 +229,11 @@ QUnit.module( "Missing Callbacks" );
 QUnit.test( "QUnit.test without a callback logs a descriptive error", function( assert ) {
 	assert.throws( function() {
 		QUnit.test( "should throw an error" );
-	}, /You must provide a function as a test callback to QUnit.test\("should throw an error"\)/ );
+	}, /You must provide a callback to QUnit.test\("should throw an error"\)/ );
 } );
 
 QUnit.test( "QUnit.todo without a callback logs a descriptive error", function( assert ) {
 	assert.throws( function() {
 		QUnit.todo( "should throw an error" );
-	}, /You must provide a function as a test callback to QUnit.todo\("should throw an error"\)/ );
+	}, /You must provide a callback to QUnit.todo\("should throw an error"\)/ );
 } );