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.

Sign up for GitHub

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

Add testsNotRan to keep count of testsNotRan to ensure moduleDone is called #1417

Closed
wants to merge 1 commit into from
Closed

Add testsNotRan to keep count of testsNotRan to ensure moduleDone is called #1417

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Gruntfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ module.exports = function( grunt ) {
"test/reorderError2.html",
"test/callbacks.html",
"test/callbacks-promises.html",
"test/callbacks-filters.html",
"test/events.html",
"test/events-in-test.html",
"test/logs.html",
Expand Down
2 changes: 1 addition & 1 deletion src/core/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ const config = {
tests: [],
childModules: [],
testsRun: 0,
unskippedTestsRun: 0,
testsIgnored: 0,
hooks: {
before: [],
beforeEach: [],
Expand Down
2 changes: 1 addition & 1 deletion src/module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ function createModule( name, testEnvironment, modifiers ) {
tests: [],
moduleId: generateHash( moduleName ),
testsRun: 0,
unskippedTestsRun: 0,
testsIgnored: 0,
childModules: [],
suiteReport: new SuiteReport( name, parentSuite ),

Expand Down
52 changes: 31 additions & 21 deletions src/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ Test.prototype = {

const runHook = () => {
if ( hookName === "before" ) {
if ( hookOwner.unskippedTestsRun !== 0 ) {
if ( hookOwner.testsRun !== 0 ) {
return;
}

Expand All @@ -217,8 +217,7 @@ Test.prototype = {

// The 'after' hook should only execute when there are not tests left and
// when the 'after' and 'finish' tasks are the only tasks left to process
if ( hookName === "after" &&
hookOwner.unskippedTestsRun !== numberOfUnskippedTests( hookOwner ) - 1 &&
if ( hookName === "after" && !lastTestWithinModuleExecuted( hookOwner ) &&
( config.queue.length > 0 || ProcessingQueue.taskCount() > 2 ) ) {
return;
}
Expand Down Expand Up @@ -309,7 +308,11 @@ Test.prototype = {
}
}

notifyTestsRan( module, skipped );
if ( skipped ) {
incrementTestsIgnored( module );
} else {
incrementTestsRun( module );
}

// Store result when possible
if ( storage ) {
Expand Down Expand Up @@ -344,13 +347,13 @@ Test.prototype = {
// generating stack trace is expensive, so using a getter will help defer this until we need it
get source() { return test.stack; }
} ).then( function() {
if ( module.testsRun === numberOfTests( module ) ) {
if ( allTestsExecuted( module ) ) {
const completedModules = [ module ];

// Check if the parent modules, iteratively, are done. If that the case,
// we emit the `suiteEnd` event and trigger `moduleDone` callback.
let parent = module.parentModule;
while ( parent && parent.testsRun === numberOfTests( parent ) ) {
while ( parent && allTestsExecuted( parent ) ) {
completedModules.push( parent );
parent = parent.parentModule;
}
Expand Down Expand Up @@ -394,6 +397,7 @@ Test.prototype = {
const test = this;

if ( !this.valid() ) {
incrementTestsIgnored( this.module );
return;
}

Expand Down Expand Up @@ -551,10 +555,7 @@ Test.prototype = {
},

valid: function() {
var filter = config.filter,
regexFilter = /^(!?)\/([\w\W]*)\/(i?$)/.exec( filter ),
module = config.module && config.module.toLowerCase(),
fullName = ( this.module.name + ": " + this.testName );
var module = config.module && config.module.toLowerCase();

function moduleChainNameMatch( testModule ) {
var testModuleName = testModule.name ? testModule.name.toLowerCase() : null;
Expand Down Expand Up @@ -593,6 +594,14 @@ Test.prototype = {
return false;
}

return this.filterMatch();
},

filterMatch: function() {
var filter = config.filter,
regexFilter = /^(!?)\/([\w\W]*)\/(i?$)/.exec( filter ),
fullName = ( this.module.name + ": " + this.testName );

if ( !filter ) {
return true;
}
Expand Down Expand Up @@ -857,23 +866,24 @@ function collectTests( module ) {
return tests;
}

function numberOfTests( module ) {
return collectTests( module ).length;
function allTestsExecuted( module ) {
return module.testsRun + module.testsIgnored === collectTests( module ).length;
}

function numberOfUnskippedTests( module ) {
return collectTests( module ).filter( test => !test.skip ).length;
function lastTestWithinModuleExecuted( module ) {
return module.testsRun + module.testsIgnored === collectTests( module ).length - 1;
}

function notifyTestsRan( module, skipped ) {
function incrementTestsRun( module ) {
module.testsRun++;
if ( !skipped ) {
module.unskippedTestsRun++;
}
while ( ( module = module.parentModule ) ) {
module.testsRun++;
if ( !skipped ) {
module.unskippedTestsRun++;
}
}
}

function incrementTestsIgnored( module ) {
module.testsIgnored++;
while ( ( module = module.parentModule ) ) {
module.testsIgnored++;
}
}
14 changes: 14 additions & 0 deletions test/callbacks-filters.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="UTF-8">
<title>QUnit Callbacks Filters Test Suite</title>
<link rel="stylesheet" href="../dist/qunit.css">
<script src="../dist/qunit.js"></script>
<script src="callbacks-filters.js"></script>
</head>
<body>
<div id="qunit"></div>
<div id="qunit-fixture"></div>
</body>
</html>
171 changes: 171 additions & 0 deletions test/callbacks-filters.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
var invokedHooks = [];
var done = false;

function callback( name ) {
return function() {
if ( done ) {
return;
}

invokedHooks.push( name );
};
}

QUnit.begin( callback( "begin" ) );
QUnit.moduleStart( callback( "moduleStart" ) );
QUnit.testStart( callback( "testStart" ) );
QUnit.testDone( callback( "testDone" ) );
QUnit.moduleDone( callback( "moduleDone" ) );
QUnit.done( callback( "done" ) );

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

done = true;

QUnit.test( "verify callback order", function( assert ) {
assert.deepEqual( invokedHooks, [
"begin",
"moduleStart",
"testStart",
"testDone",
"testStart",
"testDone",
"moduleDone",
"moduleStart",
"testStart",
"testDone",
"moduleDone",
"moduleStart",
"testStart",
"testDone",
"moduleDone",
"moduleStart",
"testStart",
"testDone",
"moduleStart",
"testStart",
"testDone",
"moduleDone",
"moduleDone",
"moduleStart",
"testStart",
"testDone",
"moduleStart",
"testStart",
"testDone",
"testStart",
"after",
"testDone",
"moduleDone",
"testStart",
"after",
"testDone",
"moduleDone",
"done"
] );
} );
} );

QUnit.config.moduleId = [ "1cf055b9" ];

// match for module Id
QUnit.module( "module1", function() {
QUnit.test( "test should run 1.1", function( assert ) {
assert.ok( true );
} );

QUnit.test( "test should run 1.2", function( assert ) {
assert.ok( true );
} );
} );

// no match module Id
QUnit.module( "module2", function() {
QUnit.test( "test should NOT run 2.1", function( assert ) {
assert.ok( false );
} );

QUnit.test( "test should NOT run 2.2", function( assert ) {
assert.ok( false );
} );
} );

QUnit.config.moduleId = [];
QUnit.config.testId = [ "3b3c4e75" ];

QUnit.module( "module3", function() {

// match test Id
QUnit.test( "ABCtest should run 3.1", function( assert ) {
assert.ok( true );
} );

QUnit.test( "test should NOT run 3.2", function( assert ) {
assert.ok( false );
} );
} );

QUnit.config.testId = [];
QUnit.config.filter = "!.2";

QUnit.module( "module4", function() {

// match string filter
QUnit.test( "test should run 4.1", function( assert ) {
assert.ok( true );
} );

QUnit.test( "test should NOT run 4.2", function( assert ) {
assert.ok( false );
} );
} );

// Make sure nested module scenarios are correct
QUnit.module( "module5", function() {
QUnit.test( "test should run 5", function( assert ) {
assert.ok( true );
} );

QUnit.module( "nestedModule5A", function() {
QUnit.test( "test should run 5.A.1", function( assert ) {
assert.ok( true );
} );
} );
} );

QUnit.module( "module6", {
after: function( ) {
invokedHooks.push( "after" );
}
}, function() {
QUnit.test( "test should run 6.1", function( assert ) {
assert.ok( true );
} );

QUnit.test( "test should NOT run 6.2", function( assert ) {
assert.ok( true );
} );

QUnit.module( "nestedModule6A", {
after: function( ) {
invokedHooks.push( "after" );
}
}, function() {
QUnit.test( "test should run 6.A.1", function( assert ) {
assert.ok( true );
} );
QUnit.test( "test should run 6.A.2", function( assert ) {
assert.ok( true );
} );
QUnit.test( "test should run 6.A.3", function( assert ) {
assert.ok( true );
} );
} );

QUnit.test( "test should run 6.3", function( assert ) {
assert.ok( true );
} );
} );
6 changes: 1 addition & 5 deletions test/main/modules.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,10 @@ QUnit.module( "after (skip)", {
}
} );

QUnit.test( "does not run after initial tests", function( assert ) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that this is correct. Looking at it locally, this means the after hook never runs, which would be a regression. During the first/last actually running test, it previously checked if we're currently executing the last non-ignored test. Which meant that if the last registered test is skipped, the after hook will run correctly after the last actually run test.

However, that logic was removed as part of this patch, and thus we now get a similar bug but affecting the after hook rather than the moduleDone callback.

I think for this to work we need to continue to "look ahead" like it did before, so that it knows during this test that it will be the last.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, good catch @Krinkle! Let me see what I can do

Copy link
Member

Choose a reason for hiding this comment

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

@step2yeung Friendly ping to check if you're able to find time for this. If not, I might give it a go next week for the next patch release. Thanks again!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the reminder Krinkle! Will get back to it this week!

QUnit.test( "does not run after final non-skipped tests", function( assert ) {
assert.expect( 0 );
} );

QUnit.test( "runs after final unskipped test", function( assert ) {
assert.expect( 1 );
} );

QUnit.skip( "last test in module is skipped" );

QUnit.module( "before/after with all tests skipped", {
Expand Down