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

Jump to bottom

Core: Introduce before/after hooks for modules #919

Closed
wants to merge 1 commit into from
Closed

Core: Introduce before/after hooks for modules #919

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,10 @@ extend( QUnit, {
module = createModule();

moduleFns = {
before: setHook( module, "before" ),
beforeEach: setHook( module, "beforeEach" ),
afterEach: setHook( module, "afterEach" )
afterEach: setHook( module, "afterEach" ),
after: setHook( module, "after" )
};

if ( executeNow instanceof Function ) {
Expand All @@ -55,11 +57,13 @@ extend( QUnit, {
name: moduleName,
parentModule: parentModule,
tests: [],
moduleId: generateHash( moduleName )
moduleId: generateHash( moduleName ),
testsRun: 0
Copy link
Member

Choose a reason for hiding this comment

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

this might be my only concern on this PR. Exposing a new detail on the API conflicts with the plans for the EventEmitter changes (#882).

Although, the tests array will still prevail and I wonder if we can set an equivalent property on the reference object for the test on that array. Doing that, we would have the tradeoff to filter the array, but it would save us a new API exposure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at the "events details types" issue, I think we would be able to replace module.testsRun with module.status.total when it is implemented since it looks like it will be similar.

I would also be fine with setting an equivalent property on the tests array, though it feels a little less correct.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at the "events details types" issue, I think we would be able to replace module.testsRun with module.status.total when it is implemented since it looks like it will be similar.

👍 that would be great!

Copy link
Member Author

Choose a reason for hiding this comment

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

In the interim, should we proceed with this current implementation? (After I clean up the style issues, of course.)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, sure.

We will need to have the API docs on http://github.com/jquery/api.qunitjs.com to be able to land it, would you mind to help on that part too?

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'll open a PR documenting the new before/after hooks for module. Is there anything else I should document?

Copy link
Member

Choose a reason for hiding this comment

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

I believe they'll be enough for now, QUnit docs will probably face a whole logging details review on a near future.

};

var env = {};
if ( parentModule ) {
parentModule.childModule = module;
extend( env, parentModule.testEnvironment );
delete env.beforeEach;
delete env.afterEach;
Expand Down
50 changes: 48 additions & 2 deletions src/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,10 @@ Test.prototype = {
config.current = this;

if ( this.module.testEnvironment ) {
delete this.module.testEnvironment.before;
delete this.module.testEnvironment.beforeEach;
delete this.module.testEnvironment.afterEach;
delete this.module.testEnvironment.after;
Copy link
Member

Choose a reason for hiding this comment

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

This may require treating the change as breaking. @leobalter?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so. We have this delete statement only because we're introducing new properties to the modules' hook: before and after. Anyone previously extending that module with these keywords would get their tests affected.

That same could happen when we moved setup and teardown to beforeEach and afterEach. Similarly we could mention the introduction of QUnit.skip to the API that could interfere with any other use of custom made methods.

Copy link
Member

Choose a reason for hiding this comment

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

Anyone previously extending that module with these keywords would get their tests affected.

Exactly. Code like QUnit.module( name, { after: new Date(2016, 0, 1) }, … ), which is currently supported, will be broken. This is why I abhor the conflation of module options and test context in a single parameter, but there it is. And it's different from adding QUnit.skip because QUnit is our object, but that parameter is explicitly wide open for users, minus our exceptions.

Anyway, while maybe not a huge deal, this is technically breaking.

Copy link
Member

Choose a reason for hiding this comment

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

This is why I abhor the conflation of module options and test context in a single parameter

We should discuss a way to solve this before 2.0. Maybe we need to isolate the hooks from the environment or event remove the possibility to define env values on this object (let this do its job).

}
this.testEnvironment = extend( {}, this.module.testEnvironment );

Expand Down Expand Up @@ -133,10 +135,22 @@ Test.prototype = {
checkPollution();
},

queueHook: function( hook, hookName ) {
queueHook: function( hook, hookName, hookOwner ) {
var promise,
test = this;
return function runHook() {
if ( hookName === "before" ) {
if ( hookOwner.testsRun !== 0 ) {
return;
}

test.preserveEnvironment = true;
}

if ( hookName === "after" && hookOwner.testsRun !== numberOfTests( hookOwner ) - 1 ) {
return;
}

config.current = test;
if ( config.notrycatch ) {
callHook();
Expand Down Expand Up @@ -166,7 +180,7 @@ Test.prototype = {
}
if ( module.testEnvironment &&
QUnit.objectType( module.testEnvironment[ handler ] ) === "function" ) {
hooks.push( test.queueHook( module.testEnvironment[ handler ], handler ) );
hooks.push( test.queueHook( module.testEnvironment[ handler ], handler, module ) );
}
}

Expand Down Expand Up @@ -205,6 +219,7 @@ Test.prototype = {
}
}

notifyTestsRan( this.module );
runLoggingCallbacks( "testDone", {
name: this.testName,
module: this.module.name,
Expand Down Expand Up @@ -233,6 +248,13 @@ Test.prototype = {
config.current = undefined;
},

preserveTestEnvironment: function() {
if ( this.preserveEnvironment ) {
this.module.testEnvironment = this.testEnvironment;
this.testEnvironment = extend( {}, this.module.testEnvironment );
}
},

queue: function() {
var priority,
test = this;
Expand All @@ -249,16 +271,25 @@ Test.prototype = {
test.before();
},

test.hooks( "before" ),

function() {
test.preserveTestEnvironment();
},

test.hooks( "beforeEach" ),

function() {
test.run();
},

test.hooks( "afterEach" ).reverse(),
test.hooks( "after" ).reverse(),

function() {
test.after();
},

function() {
test.finish();
}
Expand Down Expand Up @@ -645,3 +676,18 @@ function only( testName, expected, callback, async ) {

newTest.queue();
}

function numberOfTests( module ) {
var count = module.tests.length;
while ( module = module.childModule ) {
count += module.tests.length;
}
return count;
}

function notifyTestsRan( module ) {
module.testsRun++;
while ( module = module.parentModule ) {
module.testsRun++;
}
}
14 changes: 12 additions & 2 deletions test/amd.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,22 @@
define( [ "qunit" ], function( QUnit ) {

return function() {
QUnit.module( "AMD autostart" );
QUnit.module( "AMD autostart", {
after: function( assert ) {
assert.ok( true, "after hook ran" );
}
} );

QUnit.test( "Prove the test run started as expected", function( assert ) {
assert.expect( 1 );
assert.expect( 2 );
assert.strictEqual( beginData.totalTests, 1, "Should have expected 1 test" );
} );

setTimeout( function() {
QUnit.test( "Async-loaded tests should not run after hook again", function( assert ) {
assert.expect( 0 );
} );
}, 5000 );
};

} );
161 changes: 159 additions & 2 deletions test/main/async.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,33 @@ QUnit.test( "fails if callback is called more than callback call count", functio

} );

QUnit.module( "assert.async fails if callback is called more than once in", {
before: function( assert ) {

// Having an outer async flow in this test avoids the need to manually modify QUnit
// internals in order to avoid post-`done` assertions causing additional failures
var done = assert.async();

assert.expect( 1 );

// Duck-punch to force an Error to be thrown instead of a `pushFailure` call
assert.test.pushFailure = function( msg ) {
throw new Error( msg );
};

var overDone = assert.async();
overDone();

assert.throws( function() {
overDone();
}, new RegExp( "Too many calls to the `assert.async` callback" ) );

done();
}
} );

QUnit.test( "before", function() {} );

QUnit.module( "assert.async fails if callback is called more than once in", {
beforeEach: function( assert ) {

Expand Down Expand Up @@ -286,6 +313,50 @@ QUnit.module( "assert.async fails if callback is called more than once in", {

QUnit.test( "afterEach", function( /* assert */ ) { } );

QUnit.module( "assert.async fails if callback is called more than once in", {
after: function( assert ) {

// Having an outer async flow in this test avoids the need to manually modify QUnit
// internals in order to avoid post-`done` assertions causing additional failures
var done = assert.async();

assert.expect( 1 );

// Duck-punch to force an Error to be thrown instead of a `pushFailure` call
assert.test.pushFailure = function( msg ) {
throw new Error( msg );
};

var overDone = assert.async();
overDone();

assert.throws( function() {
overDone();
}, new RegExp( "Too many calls to the `assert.async` callback" ) );

done();
}
} );

QUnit.test( "after", function() {} );

QUnit.module( "assert.async in before", {
before: function( assert ) {
var done = assert.async(),
testContext = this;
setTimeout( function() {
testContext.state = "before";
done();
} );
}
} );

QUnit.test( "before synchronized", function( assert ) {
assert.expect( 1 );
assert.equal( this.state, "before", "before synchronized before test callback was " +
"executed" );
} );

QUnit.module( "assert.async in beforeEach", {
beforeEach: function( assert ) {
var done = assert.async(),
Expand Down Expand Up @@ -334,6 +405,37 @@ QUnit.test( "afterEach will synchronize", function( assert ) {
assert.expect( 1 );
} );

QUnit.module( "assert.async before after", {
after: function( assert ) {
assert.equal( this.state, "done", "test callback synchronized before after was " +
"executed" );
}
} );

QUnit.test( "after will synchronize", function( assert ) {
assert.expect( 1 );
var done = assert.async(),
testContext = this;
setTimeout( function() {
testContext.state = "done";
done();
} );
} );

QUnit.module( "assert.async in after", {
after: function( assert ) {
var done = assert.async();
setTimeout( function() {
assert.ok( true, "after synchronized before test was finished" );
done();
} );
}
} );

QUnit.test( "after will synchronize", function( assert ) {
assert.expect( 1 );
} );

QUnit.module( "assert.async callback event loop timing" );

QUnit.test( "`done` can be called synchronously", function( assert ) {
Expand Down Expand Up @@ -457,6 +559,30 @@ QUnit.test( "cannot allow assertions between first `done` call and second `asser
} );
} );

QUnit.module( "assert after last done in before fail, but allow other phases to run", {
before: function( assert ) {
_setupForFailingAssertionsAfterAsyncDone.call( this, assert );

// THIS IS THE ACTUAL TEST!
assert.expect( 3 );
this._assertCatch( function() {
assert.async()();

// FAIL!!! (with duck-punch to force an Error to be thrown instead of `pushFailure`)
assert.ok( true, "should fail with a special `done`-related error message if called " +
"after final `done` even if result is passing" );
} );
},

after: function( assert ) {
assert.ok( true, "This assertion should still run in after" );
}
} );

QUnit.test( "before will fail but test and after will still run", function( assert ) {
assert.ok( true, "This assertion should still run in the test callback" );
} );

QUnit.module( "assert after last done in beforeEach fail, but allow other phases to run", {
beforeEach: function( assert ) {
_setupForFailingAssertionsAfterAsyncDone.call( this, assert );
Expand All @@ -482,19 +608,27 @@ QUnit.test( "beforeEach will fail but test and afterEach will still run", functi
} );

QUnit.module( "assert after last done in test fail, but allow other phases to run", {
before: function( assert ) {
assert.ok( true, "This assertion should still run in before" );
},

beforeEach: function( assert ) {
_setupForFailingAssertionsAfterAsyncDone.call( this, assert );

assert.expect( 3 );
assert.expect( 5 );
assert.ok( true, "This assertion should still run in beforeEach" );
},

afterEach: function( assert ) {
assert.ok( true, "This assertion should still run in afterEach" );
},

after: function( assert ) {
assert.ok( true, "This assertion should still run in after" );
}
} );

QUnit.test( "test will fail, but beforeEach and afterEach will still run", function( assert ) {
QUnit.test( "test will fail, but other hooks will still run", function( assert ) {
this._assertCatch( function() {
assert.async()();

Expand Down Expand Up @@ -526,3 +660,26 @@ QUnit.module( "assert after last done in afterEach fail, but allow other phases
QUnit.test( "afterEach will fail but beforeEach and test will still run", function( assert ) {
assert.ok( true, "This assertion should still run in the test callback" );
} );

QUnit.module( "assert after last done in after fail, but allow other phases to run", {
before: function( assert ) {
_setupForFailingAssertionsAfterAsyncDone.call( this, assert );

assert.expect( 3 );
assert.ok( true, "This assertion should still run in before" );
},

after: function( assert ) {
this._assertCatch( function() {
assert.async()();

// FAIL!!! (with duck-punch to force an Error to be thrown instead of `pushFailure`)
assert.ok( true, "should fail with a special `done`-related error message if called " +
"after final `done` even if result is passing" );
} );
}
} );

QUnit.test( "after will fail but before and test will still run", function( assert ) {
assert.ok( true, "This assertion should still run in the test callback" );
} );