Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core: fix module.only logic for "flat" structures #1527

Merged
merged 9 commits into from
Jan 10, 2021
8 changes: 4 additions & 4 deletions Gruntfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -135,14 +135,12 @@ module.exports = function( grunt ) {
"test/moduleId.html",
"test/onerror/inside-test.html",
"test/onerror/outside-test.html",
"test/only.html",
"test/seed.html",
"test/overload.html",
"test/preconfigured.html",
"test/regex-filter.html",
"test/regex-exclude-filter.html",
"test/string-filter.html",
"test/module-only.html",
"test/module-skip.html",
"test/module-todo.html",

Expand Down Expand Up @@ -170,12 +168,15 @@ module.exports = function( grunt ) {
"test/events-in-test.js",
"test/onerror/inside-test.js",
"test/onerror/outside-test.js",
"test/only.js",
"test/setTimeout.js",
"test/main/dump.js",
"test/node/storage-1.js",
"test/node/storage-2.js",

"test/cli/fixtures/only/test.js",
"test/cli/fixtures/only/module.js",
"test/cli/fixtures/only/module-flat.js",

// FIXME: These tests use an ugly hack that re-opens
// an already finished test run. This only works reliably
// via the HTML Reporter thanks to some delays in the bridge.
Expand All @@ -187,7 +188,6 @@ module.exports = function( grunt ) {
//
// Ref https://github.com/qunitjs/qunit/issues/1511
//
// "test/module-only.js",
// "test/module-skip.js",
// "test/module-todo.js",
"test/es2018/async-functions.js",
Expand Down
11 changes: 7 additions & 4 deletions src/module.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ import SuiteReport from "./reports/suite";
import { extend, objectType, generateHash } from "./core/utilities";
import { globalSuite } from "./core";

let focused = false;

const moduleStack = [];

function isParentModuleInQueue() {
Expand Down Expand Up @@ -100,27 +98,31 @@ function processModule( name, options, executeNow, modifiers = {} ) {
}
}

let focused = false;

export default function module( name, options, executeNow ) {
if ( focused && !isParentModuleInQueue() ) {
config.currentModule.closed = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me a while to realize that this is actually mutating the "previous" module. The one we are processing now, never makes it into the registry. This feels a bit fragile as it means test() calls are thus interacting with a closed module and I can see that leading to some confusing failures in the future.

On the other hand, given the way that only is meant to act like a filter, the module isn't meant to be visible in any way. Perhaps, though, it would be best to still let it enter the registry, but as an empty module that (if all goes well) will never start or stop, because it is empty. Afaik that is how empty modules are treated.

If empty modules are exposed to reporters or callbacks in some way, then we'd need some additional logic to make sure e.g. empty modules with currentModule.ignored get bypassed in our events/callback layer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if you like that direction. I haven't thought it through fully, so it's not a strong suggestion, just an idea.

For now my only suggestion would be to add an inline comment indicating what we have done, which is to keep the previous module on the stack and that "closed" means it was implicitly closed by a subsequent module() call.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm certainly intrigued by that suggestion, and the notion of currentModule.ignored has a much more natural feel and intuition.

I think it's inside module.only where we set config.modules.length = 0; to effectively wipe out the others that were registered before. I'm trying to make that work with something like

config.modules.forEach( module => {
   module.ignored = true;
} );

but I'm definitely wrestling with the unittests - I don't have everything quite right.

I'll keep this open and continue noodling. I agree we should document the exposed property, and ignored is just so much easier to digest than closed (or alternative).

return;
}

processModule( name, options, executeNow );
}

module.only = function() {
module.only = function( ...args ) {
if ( !focused ) {
smcclure15 marked this conversation as resolved.
Show resolved Hide resolved
config.modules.length = 0;
config.queue.length = 0;
}

processModule( ...arguments );
processModule( ...args );

focused = true;
};

module.skip = function( name, options, executeNow ) {
if ( focused ) {
config.currentModule.closed = true;
return;
}

Expand All @@ -129,6 +131,7 @@ module.skip = function( name, options, executeNow ) {

module.todo = function( name, options, executeNow ) {
if ( focused ) {
config.currentModule.closed = true;
return;
}

Expand Down
10 changes: 5 additions & 5 deletions src/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ import ProcessingQueue from "./core/processing-queue";

import TestReport from "./reports/test";

let focused = false;

export default function Test( settings ) {
this.expected = null;
this.assertions = [];
Expand Down Expand Up @@ -685,9 +683,11 @@ function checkPollution() {
}
}

let focused = false;

// Will be exposed as QUnit.test
export function test( testName, callback ) {
if ( focused ) {
if ( focused || config.currentModule.closed ) {
return;
}

Expand All @@ -701,7 +701,7 @@ export function test( testName, callback ) {

extend( test, {
todo: function todo( testName, callback ) {
if ( focused ) {
if ( focused || config.currentModule.closed ) {
return;
}

Expand All @@ -714,7 +714,7 @@ extend( test, {
newTest.queue();
},
skip: function skip( testName ) {
if ( focused ) {
if ( focused || config.currentModule.closed ) {
return;
}

Expand Down
49 changes: 49 additions & 0 deletions test/cli/fixtures/expected/tap-outputs.js
Original file line number Diff line number Diff line change
Expand Up @@ -282,5 +282,54 @@ ok 2 later thing > has released all foos
# pass 2
# skip 0
# todo 0
# fail 0`,

"qunit only/test.js":
`TAP version 13
ok 1 run this test
ok 2 all tests with only run
1..3
# pass 3
# skip 0
# todo 0
# fail 0`,

"qunit only/module.js":
`TAP version 13
not ok 1 # TODO module B > Only this module should run > a todo test
---
message: "not implemented yet"
severity: todo
actual : false
expected: true
stack: .*
...
ok 2 # SKIP module B > Only this module should run > implicitly skipped test
ok 3 module B > Only this module should run > normal test
ok 4 module D > test D
ok 5 module E > module F > test F
ok 6 module E > test E
1..8
# pass 6
# skip 1
# todo 1
# fail 0`,

"qunit only/module-flat.js":
`TAP version 13
not ok 1 # TODO module B > test B
---
message: "not implemented yet"
severity: todo
actual : false
expected: true
stack: .*
...
ok 2 # SKIP module B > test C
ok 3 module B > test D
1..4
# pass 2
# skip 1
# todo 1
# fail 0`
};
27 changes: 27 additions & 0 deletions test/cli/fixtures/only/module-flat.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@

QUnit.module( "module A" );
QUnit.test( "test A", function( assert ) {
assert.true( false, "this test should not run" );
} );

QUnit.module.only( "module B" );
QUnit.todo( "test B", function( assert ) {
assert.true( false, "not implemented yet" );
} );
QUnit.skip( "test C", function( assert ) {
assert.true( false, "test should be skipped" );
} );
QUnit.test( "test D", function( assert ) {
assert.true( true, "this test should run" );
} );

QUnit.module( "module C" );
QUnit.todo( "test E", function( assert ) {
assert.true( false, "not implemented yet" );
} );
QUnit.skip( "test F", function( assert ) {
assert.true( false, "test should be skipped" );
} );
QUnit.test( "test G", function( assert ) {
assert.true( false, "this test should not run" );
} );
53 changes: 0 additions & 53 deletions test/module-only.js → test/cli/fixtures/only/module.js
Original file line number Diff line number Diff line change
@@ -1,56 +1,3 @@
var tests = {};

var done = false;

QUnit.testDone( function( details ) {
if ( done ) {
return;
}

tests[ details.testId ] = {
skipped: details.skipped,
todo: details.todo
};
} );

QUnit.done( function() {
if ( done ) {
return;
}

done = true;

QUnit.test( "Compare stats", function( assert ) {
assert.expect( 1 );

assert.deepEqual( tests, {
"1fb73641": {
skipped: false,
todo: true
},
"5fe457c4": {
skipped: true,
todo: false
},
"75e1bf3f": {
skipped: false,
todo: false
},
"c7ae85c2": {
skipped: false,
todo: false
},
"74b800d1": {
skipped: false,
todo: false
},
"2f8fb2a2": {
skipped: false,
todo: false
}
} );
} );
} );

QUnit.module( "module A", function() {
QUnit.test( "test A", function( assert ) {
Expand Down
15 changes: 15 additions & 0 deletions test/cli/fixtures/only/test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
QUnit.test( "implicitly skipped test", function( assert ) {
assert.true( false, "test should be skipped" );
} );

QUnit.only( "run this test", function( assert ) {
assert.true( true, "only this test should run" );
} );

QUnit.test( "another implicitly skipped test", function( assert ) {
assert.true( false, "test should be skipped" );
} );

QUnit.only( "all tests with only run", function( assert ) {
assert.true( true, "this test should run as well" );
} );
43 changes: 43 additions & 0 deletions test/cli/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,15 @@ const expectedOutput = require( "./fixtures/expected/tap-outputs" );
const execute = require( "./helpers/execute" );
const semver = require( "semver" );

QUnit.assert.matches = function( actual, expected, message ) {
this.pushResult( {
result: expected.test( actual ),
actual,
expected: expected.toString(),
message
} );
};

QUnit.module( "CLI Main", function() {
QUnit.test( "defaults to running tests in 'test' directory", async function( assert ) {
const command = "qunit";
Expand Down Expand Up @@ -295,4 +304,38 @@ QUnit.module( "CLI Main", function() {
}
} );
} );

QUnit.module( "only", function() {
QUnit.test( "test", async function( assert ) {

const command = "qunit only/test.js";
const execution = await execute( command );

assert.equal( execution.code, 0 );
assert.equal( execution.stderr, "" );
assert.equal( execution.stdout, expectedOutput[ command ] );
} );

QUnit.test( "nested modules", async function( assert ) {

const command = "qunit only/module.js";
const execution = await execute( command );

assert.equal( execution.code, 0 );
assert.equal( execution.stderr, "" );
const re = new RegExp( expectedOutput[ command ] );
assert.matches( execution.stdout, re );
} );

QUnit.test( "flat modules", async function( assert ) {

const command = "qunit only/module-flat.js";
const execution = await execute( command );

assert.equal( execution.code, 0 );
assert.equal( execution.stderr, "" );
const re = new RegExp( expectedOutput[ command ] );
assert.matches( execution.stdout, re );
} );
} );
} );
13 changes: 0 additions & 13 deletions test/module-only.html

This file was deleted.

13 changes: 0 additions & 13 deletions test/only.html

This file was deleted.

2 changes: 1 addition & 1 deletion test/sandboxed-iframe-contents.html
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
<title>QUnit in Sandboxed Iframe Test Suite - Inside Iframe</title>
<link rel="stylesheet" href="../dist/qunit.css">
<script src="../dist/qunit.js"></script>
<script src="only.js"></script>
<script src="sandboxed-iframe.js"></script>
<script src="../node_modules/grunt-contrib-qunit/chrome/bridge.js"></script>
</head>
<body>
Expand Down
File renamed without changes.