From 41b36a367a8803861961fb2d8732798b21a68603 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Tue, 29 Mar 2022 19:34:02 +0100 Subject: [PATCH] Core: Fix "source" attribution for test definitions Follows-up commits 07de4b1d1bb and 835b7c1c91. 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 https://github.com/qunitjs/qunit/issues/1679. --- Gruntfile.js | 2 +- src/test.js | 32 ++++++-- test/cli/cli-main.js | 7 +- test/cli/fixtures/expected/tap-outputs.js | 13 ++- test/index.html | 2 +- test/main/stack.js | 14 ---- test/main/stacktrace.js | 99 +++++++++++++++++++++++ test/mozjs.js | 2 +- test/webWorker-worker.js | 2 +- 9 files changed, 142 insertions(+), 31 deletions(-) delete mode 100644 test/main/stack.js create mode 100644 test/main/stacktrace.js diff --git a/Gruntfile.js b/Gruntfile.js index 59c979f00..bc9076f04 100644 --- a/Gruntfile.js +++ b/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', diff --git a/src/test.js b/src/test.js index 8ce06b01f..b992d5704 100644 --- a/src/test.js +++ b/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 }); }); diff --git a/test/cli/cli-main.js b/test/cli/cli-main.js index e9c4190a3..f7c02319f 100644 --- a/test/cli/cli-main.js +++ b/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 + }); } }); diff --git a/test/cli/fixtures/expected/tap-outputs.js b/test/cli/fixtures/expected/tap-outputs.js index 14e26b510..254a616aa 100644 --- a/test/cli/fixtures/expected/tap-outputs.js +++ b/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 diff --git a/test/index.html b/test/index.html index dbe4177b1..0a6d7232e 100644 --- a/test/index.html +++ b/test/index.html @@ -18,7 +18,7 @@ - + diff --git a/test/main/stack.js b/test/main/stack.js deleted file mode 100644 index c5ea6b434..000000000 --- a/test/main/stack.js +++ /dev/null @@ -1,14 +0,0 @@ -(function () { - var stack = QUnit.stack(); - - QUnit.module('QUnit.stack'); - - // Flag this test as skipped on browsers that doesn't support stack trace - QUnit[stack ? 'test' : 'skip']('returns the proper stack line', function (assert) { - assert.true(/(\/|\\)test(\/|\\)main(\/|\\)stack\.js/.test(stack)); - - stack = QUnit.stack(2); - assert.true(/(\/|\\)qunit(\/|\\)qunit\.js/.test(stack), 'can use offset argument to return a different stacktrace line'); - assert.false(/\/test\/main\/stack\.js/.test(stack), 'stack with offset argument'); - }); -}()); diff --git a/test/main/stacktrace.js b/test/main/stacktrace.js new file mode 100644 index 000000000..39d528b68 --- /dev/null +++ b/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' + }); + }); +}); diff --git a/test/mozjs.js b/test/mozjs.js index 525962bf1..de679c34a 100644 --- a/test/mozjs.js +++ b/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'); diff --git a/test/webWorker-worker.js b/test/webWorker-worker.js index 1879b1a16..fbe667d7a 100644 --- a/test/webWorker-worker.js +++ b/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' );