Core: Fix "source" attribution for test definitions

Follows-up commits 07de4b1 and 835b7c1.
This introduced `test.each()`, but also changed `QUnit.test()` to
have 3 instead of 2 function calls until `new Test()` internally,
which broke the stacktrace extraction in most cases. This was not
covered well by tests.

The `test.each()` itself also had a different offset. It had 6 frames
until `new Test` for the case of array data, and 7 frames for the case
of object data. This difference is due to `data.forEach(fn)` verses
`keys.forEach(x => fn())`.

* Cover it all with tests.
* Increase default stackOffset from 2 to 3.
* Change `runEach` to always have 5 frames, by using a for-loop.
* Allow internal override.
* Use the override.

Fixes #1679.

Gruntfile.js

@@ -128,7 +128,7 @@ module.exports = function (grunt) {
         'test/main/onUncaughtException.js',
         'test/main/promise.js',
         'test/main/setTimeout.js',
-        'test/main/stack.js',
+        'test/main/stacktrace.js',
         'test/main/test.js',
         'test/main/utilities.js',
 

src/test.js

@@ -31,6 +31,15 @@ export default function Test (settings) {
   this.withData = false;
   this.pauses = new StringMap();
   this.nextPauseId = 1;
+
+  // For the most common case, we have:
+  // - 0: new Test
+  // - 1: addTest
+  // - 2: QUnit.test
+  // - 3: user file
+  //
+  // This needs is customised by test.each()
+  this.stackOffset = 3;
   extend(this, settings);
 
   // If a module is skipped, all its tests and the tests of the child suites
@@ -129,9 +138,11 @@ function getNotStartedModules (startModule) {
 
 Test.prototype = {
 
-  // generating a stack trace can be expensive, so using a getter defers this until we need it
+  // Use a getter to avoid computing a stack trace (which can be expensive),
+  // This is displayed by the HTML Reporter, but most other integrations do
+  // not access it.
   get stack () {
-    return extractStacktrace(this.errorForStack, 2);
+    return extractStacktrace(this.errorForStack, this.stackOffset);
   },
 
   before: function () {
@@ -183,8 +194,8 @@ Test.prototype = {
     try {
       runTest(this);
     } catch (e) {
-      this.pushFailure('Died on test #' + (this.assertions.length + 1) + ' ' +
-        this.stack + ': ' + (e.message || e), extractStacktrace(e, 0));
+      this.pushFailure('Died on test #' + (this.assertions.length + 1) + ': ' +
+        (e.message || e) + '\n' + this.stack, extractStacktrace(e, 0));
 
       // Else next test will carry the responsibility
       saveGlobal();
@@ -935,12 +946,13 @@ function makeEachTestName (testName, argument) {
 
 function runEach (data, eachFn) {
   if (Array.isArray(data)) {
-    data.forEach(eachFn);
+    for (let i = 0; i < data.length; i++) {
+      eachFn(data[i], i);
+    }
   } else if (typeof data === 'object' && data !== null) {
-    const keys = Object.keys(data);
-    keys.forEach((key) => {
+    for (let key in data) {
       eachFn(data[key], key);
-    });
+    }
   } else {
     throw new Error(
       `test.each() expects an array or object as input, but
@@ -965,6 +977,7 @@ extend(test, {
         testName: makeEachTestName(testName, testKey),
         callback,
         withData: true,
+        stackOffset: 5,
         data
       });
     });
@@ -978,6 +991,7 @@ test.todo.each = function (testName, dataset, callback) {
       callback,
       todo: true,
       withData: true,
+      stackOffset: 5,
       data
     });
   });
@@ -986,6 +1000,7 @@ test.skip.each = function (testName, dataset) {
   runEach(dataset, (_, testKey) => {
     addTest({
       testName: makeEachTestName(testName, testKey),
+      stackOffset: 5,
       skip: true
     });
   });
@@ -997,6 +1012,7 @@ test.only.each = function (testName, dataset, callback) {
       testName: makeEachTestName(testName, testKey),
       callback,
       withData: true,
+      stackOffset: 5,
       data
     });
   });

test/cli/cli-main.js

@@ -162,8 +162,11 @@ QUnit.module('CLI Main', () => {
     } catch (e) {
       assert.equal(e.code, 1);
       assert.equal(e.stderr, '');
-      assert.true(e.stdout.includes('Died on test #2     at '));
-      assert.true(e.stdout.includes('Error: expected error thrown in test'));
+      assert.pushResult({
+        result: e.stdout.includes('Died on test #2: expected error thrown in test') &&
+          e.stdout.includes('Error: expected error thrown in test'),
+        actual: e.stdout
+      });
     }
   });
 

test/cli/fixtures/expected/tap-outputs.js

@@ -524,7 +524,10 @@ not ok 1 times out before scheduled done is called
 `TAP version 13
 not ok 1 Test A
   ---
-  message: "Died on test #2     at Object.test (/qunit/qunit/qunit.js): this is an intentional error"
+  message: |+
+    Died on test #2: this is an intentional error
+        at /qunit/test/cli/fixtures/drooling-done.js:5:7
+        at internal
   severity: failed
   actual  : null
   expected: undefined
@@ -545,8 +548,10 @@ ok 1 Test A
 not ok 2 Test B
   ---
   message: |+
-    Died on test #2     at Object.test (/qunit/qunit/qunit.js): Unexpected release of async pause during a different test.
+    Died on test #2: Unexpected release of async pause during a different test.
     > Test: Test A [async #1]
+        at /qunit/test/cli/fixtures/drooling-extra-done.js:13:7
+        at internal
   severity: failed
   actual  : null
   expected: undefined
@@ -586,8 +591,10 @@ Bail out! Error: Unexpected release of async pause after tests finished.
 not ok 1 Test A
   ---
   message: |+
-    Died on test #2     at Object.test (/qunit/qunit/qunit.js): Tried to release async pause that was already released.
+    Died on test #2: Tried to release async pause that was already released.
     > Test: Test A [async #1]
+        at /qunit/test/cli/fixtures/too-many-done-calls.js:1:7
+        at internal
   severity: failed
   actual  : null
   expected: undefined

test/index.html

@@ -18,7 +18,7 @@
   <script src="main/onUncaughtException.js"></script>
   <script src="main/promise.js"></script>
   <script src="main/setTimeout.js"></script>
-  <script src="main/stack.js"></script>
+  <script src="main/stacktrace.js"></script>
   <script src="main/test.js"></script>
   <script src="main/utilities.js"></script>
 

test/main/stacktrace.js

@@ -0,0 +1,99 @@
+// Skip on browsers that doesn't support stack trace
+(QUnit.stack() ? QUnit.module : QUnit.module.skip)('stacktrace', function () {
+  function fooCurrent () {
+    return QUnit.stack();
+  }
+  function fooParent () {
+    return fooCurrent();
+  }
+
+  function fooInternal () {
+    return QUnit.stack(2);
+  }
+  function fooPublic () {
+    return fooInternal();
+  }
+  function barCaller () {
+    return fooPublic();
+  }
+
+  function norm (str) {
+    // CRLF
+    return str.replace(/\\/g, '/');
+  }
+
+  QUnit.test('QUnit.stack()', function (assert) {
+    var simple = norm(QUnit.stack());
+    assert.pushResult({
+      result: simple.indexOf('/main/stacktrace.js') !== -1,
+      actual: simple,
+      message: 'current file'
+    });
+
+    var nested = norm(fooParent());
+    assert.pushResult({
+      result: nested.indexOf('fooCurrent') !== -1,
+      actual: nested,
+      message: 'include current function'
+    });
+    assert.pushResult({
+      result: nested.indexOf('fooParent') !== -1,
+      actual: nested,
+      message: 'include parent function'
+    });
+  });
+
+  QUnit.test('QUnit.stack(offset)', function (assert) {
+    var stack = norm(barCaller());
+    var line = stack.split('\n')[0];
+
+    assert.pushResult({
+      result: line.indexOf('/main/stacktrace.js') !== -1,
+      actual: line,
+      message: 'current file'
+    });
+    assert.pushResult({
+      result: line.indexOf('barCaller') !== -1,
+      actual: line,
+      message: 'start at offset'
+    });
+    assert.pushResult({
+      result: stack.indexOf('fooInternal') === -1,
+      actual: stack,
+      message: 'skip internals'
+    });
+  });
+
+  QUnit.test('QUnit.test() source details', function (assert) {
+    var stack = norm(QUnit.config.current.stack);
+    var line = stack.split('\n')[0];
+    assert.pushResult({
+      result: line.indexOf('/main/stacktrace.js') !== -1,
+      expected: '/main/stacktrace.js',
+      actual: stack,
+      message: 'start at current file'
+    });
+  });
+
+  QUnit.test.each('QUnit.test.each(list) source details', [0], function (assert) {
+    var stack = norm(QUnit.config.current.stack);
+    var line = stack.split('\n')[0];
+    assert.pushResult({
+      result: line.indexOf('/main/stacktrace.js') !== -1,
+      expected: '/main/stacktrace.js',
+      actual: stack,
+      message: 'start at current file'
+    });
+  });
+
+  QUnit.test.each('QUnit.test.each(object) source details', { a: 0 }, function (assert) {
+    var stack = norm(QUnit.config.current.stack);
+    var line = stack.split('\n')[0];
+    assert.pushResult({
+      result: line.indexOf('/main/stacktrace.js') !== -1,
+      expected: '/main/stacktrace.js',
+      actual: stack,
+      message: 'start at current file'
+    });
+  });
+});

test/mozjs.js

@@ -47,7 +47,7 @@ loadRelativeToScript('../test/main/onError.js');
 loadRelativeToScript('../test/main/onUncaughtException.js');
 loadRelativeToScript('../test/main/promise.js');
 loadRelativeToScript('../test/main/setTimeout.js');
-loadRelativeToScript('../test/main/stack.js');
+loadRelativeToScript('../test/main/stacktrace.js');
 loadRelativeToScript('../test/main/test.js');
 loadRelativeToScript('../test/main/utilities.js');
 

test/webWorker-worker.js

@@ -18,7 +18,7 @@ importScripts(
   'main/onUncaughtException.js',
   'main/promise.js',
   'main/setTimeout.js',
-  'main/stack.js',
+  'main/stacktrace.js',
   'main/test.js',
   'main/utilities.js'
 );