From 99aee51a8a4dfce3fa87559e171398fdf72c6886 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Sun, 7 Jul 2024 04:19:04 +0100 Subject: [PATCH] Core: Fix infinite loop crash when mixing test.only() with module.only() I noticed that the first branch of `addOnlyTest()` was uncovered in test coverage reports. In expanding the `only-module-then-test.js` fixture to cover this (by adding a late test.only call), which was originally added in https://github.com/qunitjs/qunit/pull/1658. I decided to also add an early test.only call to see what would happen. The outcome is irrelevant/ambigious, but it seemed worth observing. To my surprise, it created an infinite loop. This further convinces me that we should make "early errors" show up in the HTML Reporter (TODO added in 37f6e4b6613), and subsequently get rid of this re-entry hack. The TapReporter and QUnit CLI are already able to render early errors. The HtmlReporter not yet. It's not generally a priority imho, but, because "No tests" is a common scenario, if we utilize an early error to report it, that makes it a priority to support in HtmlReporter as otherwise there'd be no visible feedback for it. It's okay to have to pop open devtools when causing an early syntax error or something like that in your own code, but when using the UI to match no tests, there should at least be some kind of visible feedback and not a blank page. --- src/module.js | 7 +++ src/reports/suite.js | 4 ++ src/test.js | 11 +++- test/cli/fixtures/only-module-then-test.js | 6 +- .../cli/fixtures/only-test-only-module-mix.js | 58 +++++++++++++++++++ .../only-test-only-module-mix.tap.txt | 21 +++++++ 6 files changed, 105 insertions(+), 2 deletions(-) create mode 100644 test/cli/fixtures/only-test-only-module-mix.js create mode 100644 test/cli/fixtures/only-test-only-module-mix.tap.txt diff --git a/src/module.js b/src/module.js index f1cdef561..d197e5f01 100644 --- a/src/module.js +++ b/src/module.js @@ -144,6 +144,13 @@ module.only = function (...args) { if (!focused) { // Upon the first module.only() call, // delete any and all previously registered modules and tests. + // + // TODO: This does not clear SuiteReport, which means the empty modules are + // left behind and wrongly reported as "skipped" (skipped == total), and + // deleted tests wrongly count toward runEnd.testCounts as passing test + // (this.getFailedAssertions().length === 0). + // This is why /test/cli/fixtures/only-test-only-module-mix.tap.txt reports + // 1..3 instead of 1..1. config.modules.length = 0; config.queue.length = 0; diff --git a/src/reports/suite.js b/src/reports/suite.js index 621b4ad9b..393ebc364 100644 --- a/src/reports/suite.js +++ b/src/reports/suite.js @@ -29,6 +29,10 @@ export default class SuiteReport { tests: this.tests.map(test => test.start()), childSuites: this.childSuites.map(suite => suite.start()), testCounts: { + // TODO: Due to code reuse, this ends up computing getStatus() + // for tests that obviously haven't run yet. It's harmless but + // quite inefficient recursion, that we repeat many times over + // also via test.start() and suite.start() above. total: this.getTestCounts().total } }; diff --git a/src/test.js b/src/test.js index 7e56576f5..ce2811aba 100644 --- a/src/test.js +++ b/src/test.js @@ -911,7 +911,16 @@ function checkPollution () { let focused = false; // indicates that the "only" filter was used function addTest (settings) { - if (focused || config.currentModule.ignored) { + if ( + // Never ignore the internal "No tests" error, as this would infinite loop. + // See /test/cli/fixtures/only-test-only-module-mix.tap.txt + // + // TODO: If we improve HtmlReporter to buffer and replay early errors, + // we can change "No tests" to be a global error, and thus get rid of + // the "validTest" concept and the ProcessQueue re-entry hack. + !(settings.callback && settings.callback.validTest) && + (focused || config.currentModule.ignored) + ) { return; } diff --git a/test/cli/fixtures/only-module-then-test.js b/test/cli/fixtures/only-module-then-test.js index 90a31184e..d3ca15c14 100644 --- a/test/cli/fixtures/only-module-then-test.js +++ b/test/cli/fixtures/only-module-then-test.js @@ -10,7 +10,11 @@ QUnit.module('module A', function () { }); }); - QUnit.test('test A2', function (assert) { + QUnit.test.only('test A2', function (assert) { + assert.true(false, 'not run'); + }); + + QUnit.test('test A3', function (assert) { assert.true(false, 'not run'); }); }); diff --git a/test/cli/fixtures/only-test-only-module-mix.js b/test/cli/fixtures/only-test-only-module-mix.js new file mode 100644 index 000000000..e7050c2bb --- /dev/null +++ b/test/cli/fixtures/only-test-only-module-mix.js @@ -0,0 +1,58 @@ +// This currently runs 0 tests because stuff cancels out. +// The intent is ambigious and outcome doesn't really matter. +// It's covered as regression test against a past crash: +// +// * module A: +// - start module scope A +// * test A1: +// - queue A1 +// * test.only A2: +// - start "test" focus mode. +// - clear queue (delete A1). +// - queue A2 +// * module.only B: +// - start "module" focus mode. +// - clear queue (delete A2). +// - modify parent A to become "ignored". +// - start child-module scope B +// * test B1: +// - ignored, because it's a non-only test and +// we are in "focus" mode due to the earlier test.only. +// thus the only test we would run is a "test.only" test, +// inside a "module.only" block, which this file doesn't have any of. +// * test A4: +// - ignored +// * ProcessingQueue.end: +// - create new Error('No tests were run.') +// - emit it via a dynamically created test +// - despite forcing validTest=true, the added test (as of QUnit 2.21.0) is ignored +// because it is a non-only test and we're in "focus" mode. +// This causes an infinite loop between ProcessingQueue.end and ProcessingQueue.advance +// because it thinks it ads a test, but doesn't. +// This scenerio is surprising because the usual way to active "focused" test mode, +// is by defining a test.only() case, in which case the queue by definition isn't +// empty. Even if the test.only() case was skipped by a filter (or config.moduleId/testId) +// this is taken care of by forcing validTest=true. + +QUnit.module('module A', function () { + // ... start module scope A + + // ... queue A1 + QUnit.test('test A1', function (assert) { + assert.true(false, 'not run'); + }); + + QUnit.test.only('test A2', function (assert) { + assert.true(false, 'not run'); + }); + + QUnit.module.only('module B', function () { + QUnit.test('test B1', function (assert) { + assert.true(true, 'run'); + }); + }); + + QUnit.test('test A4', function (assert) { + assert.true(false, 'not run'); + }); +}); diff --git a/test/cli/fixtures/only-test-only-module-mix.tap.txt b/test/cli/fixtures/only-test-only-module-mix.tap.txt new file mode 100644 index 000000000..0342b745d --- /dev/null +++ b/test/cli/fixtures/only-test-only-module-mix.tap.txt @@ -0,0 +1,21 @@ +# command: ["qunit", "only-test-only-module-mix.js"] + +TAP version 13 +not ok 1 global failure + --- + message: No tests were run. + severity: failed + actual : undefined + expected: undefined + stack: | + Error: No tests were run. + at qunit.js + at internal + ... +1..3 +# pass 2 +# skip 0 +# todo 0 +# fail 1 + +# exit code: 1