Core: Fix bug calling hooks with skipped tests #1156

bendemboski · 2017-04-12T19:26:18Z

When the first/last test in a module was skipped, it would cause the module's
before()/after() hook to not be called. This ensures those hooks will always be
called.

jsf-clabot · 2017-04-12T19:26:20Z

All committers have signed the CLA.

rwjblue · 2017-04-12T19:32:30Z

Nice, sleuth work!

trentmwillis

Thanks for digging into this! This approach works, but I'd prefer to have it such that before runs before the first test that actually executes. In other words, if all tests in a module were skipped, then before/after will never run. This is more inline with how beforeEach/afterEach work and I think is more intuitive.

leobalter · 2017-04-12T19:37:33Z

test/main/modules.js

+
+QUnit.test( "has run by the time second (unskipped) test runs", function( assert ) {
+	assert.expect( 1 );
+	assert.equal( this.beforeCount, 1, "beforeCount should be one" );


if we remove the this.assert = new Assert( this ); for skipped tests line will this fail?

leobalter · 2017-04-12T19:43:31Z

test/main/modules.js

+QUnit.test( "after hook ran after last (skipped) test", function( assert ) {
+	assert.expect( 1 );
+	assert.ok( ranAfterHook, "after hook ran" );
+} );


can we nest the after (skip) and after (skip) verifier modules in a single module? mostly a

QUnit.module( "after (skip)", function() { var ranAfterHook = 0; QUnit.module( "main", function(hooks) { hooks.after = function( assert ) { ranAfterHook += 1; }; QUnit.test( "does not run after initial tests", ... ); QUnit.skip( "skipped, but should still run after hook", ... ); } ); QUnit.module( "verifier", function() { QUnit.test( "after hook ran after last (skipped) test", ... ); } ); } );

leobalter · 2017-04-12T19:44:27Z

test/main/modules.js

+} );
+
+QUnit.skip( "skipped, but should still run after hook", function( assert ) {
+	assert.expect( 1 );


this should not be necessary. Some throwing expression here, like throw 'foo' here would help better

leobalter · 2017-04-12T19:45:05Z

test/main/modules.js

+
+QUnit.skip( "skipped, but should still run before hook", function( assert ) {
+	assert.expect( 1 );
+	assert.ok( true );


this should not be necessary. Some throwing expression here, like throw 'foo' here would help better, or even assert.ok( false );

leobalter · 2017-04-12T19:45:49Z

if all tests in a module were skipped, then before/after will never run

+1

When the first/last test in a module was skipped, it would cause the module's before()/after() hook to not be called. This ensures those hooks will always be called.

bendemboski · 2017-04-12T22:00:11Z

@trentmwillis @leobalter how does this look? These changes didn't really have much in common with the previous version, so I squashed the commits.

leobalter

The changes in the src code are a bit confusing, but I'm +1 for this PR after following the tests.

I found a couple nit-picks in the comments below.

leobalter · 2017-04-12T23:24:59Z

src/test.js

 	const modules = [ ...module.childModules ];

 	// Do a breadth-first traversal of the child modules
 	while ( modules.length ) {
 		const nextModule =  modules.shift();
-		count += nextModule.tests.length;
+		tests = tests.concat( nextModule[ key ] );


tests.push.apply( tests, nextModule[ keys ] );

it avoids creating a new array and discarding the previous one.

leobalter · 2017-04-12T23:28:56Z

test/main/modules.js

+
+	QUnit.module( "verifier", function() {
+		QUnit.test( "hooks did not run", function( assert ) {
+			assert.expect( 2 );


this is not necessary here.

leobalter · 2017-04-12T23:29:32Z

test/main/modules.js

+	assert.expect( 1 );
+} );
+
+QUnit.skip( "last test in module is skipped" );


this is nice!

bendemboski · 2017-04-12T23:57:14Z

@leobalter I addresses your feedback in new commit so you can easily see the diff, but I'd like to squash it with the previous before merging, so let me know if the latest commit is 👍 by you, and if so I'll squash them and wait for @trentmwillis to provide feedback or merge.

trentmwillis

Thanks for updating; some suggestions on the exact implementation, but the tests look good!

trentmwillis · 2017-04-13T01:40:24Z

src/core.js

 		moduleId: generateHash( moduleName ),
 		testsRun: 0,
+		unskippedTestsRun: 0,


testsRun isn't used for anything other than determining whether to invoke module hooks, so we can just use that instead of introducing a new variable. Basically, we should count a test that actually ran should be an unskipped test. Make sense?

I created a new property because testsRun is also referenced here and here. I'm not sure what the first one is doing, but if we change the meaning of testsRun then per the second one, if a module ends with skipped tests, wouldn't we log the end of the suite before logging those skipped tests?

Ah good point, carry on.

trentmwillis · 2017-04-13T01:45:37Z

src/test.js

@@ -64,6 +64,10 @@ export default function Test( settings ) {
 		this.async = false;
 		this.expected = 0;
 	} else {
+		this.module.unskippedTests.push( {


We should create a single object up on line 55 and push it into both the tests and unskippedTests array, since we want that info to be in sync.

Also, I'd appreciate a flag skipped to be added to the object, for more info.

I actually considered keeping just one array of tests (module.tests) and adding a skip flag to its entries. That way we wouldn't need anything extra because we can always find out if the current test is the first/last unskipped test by looking through module.tests (and child modules) and comparing testId to this.testId and looking at skip flags.

I didn't go in that direction because module.tests is passed into various callbacks as the context, and I didn't want to make any public-facing changes. If you prefer that route (then we don't need any new counts or arrays or anything, just a skip flag on the module.tests entries), I can certainly do that.

If not, can you elaborate on exactly where you want the skipped flag? From your comment I'm not entirely sure.

I think adding the flag is fine, it's an additive change so it should be considered backwards compatible.

Blerg. Turns out we need to keep an unskipped count. I was hoping to just look at the list of tests and when we run the first/last unskipped test, run the before/after hook, but when looking through the module/childModule/tests tree, we can't tell the difference between this:

QUnit.module("outer", function() { test("outer test", function() { ... }); QUnit.module("inner", function() { test("inner test", function() { ... }); }); });

and this:

QUnit.module("outer", function() { QUnit.module("inner", function() { test("inner test", function() { ... }); }); test("outer test", function() { ... }); });

So we don't know whether to run outer's before hook when we run outer or inner. Without some knowledge of how a module's tests are ordered with respect to its child modules, this approach will not work.

So I guess I'll fall back on still adding the skip flag to module.tests (so we can dynamically count the number of unskipped tests in a module and don't have to keep a separate list/count of them), but also keeping an unskippedTestsRun on the module.

trentmwillis · 2017-04-13T01:47:11Z

src/test.js

 	const modules = [ ...module.childModules ];

 	// Do a breadth-first traversal of the child modules
 	while ( modules.length ) {
 		const nextModule =  modules.shift();
-		count += nextModule.tests.length;
+		tests.push.apply( tests, nextModule[ key ] );


I'd prefer to keep this as a counter instead of an array as we don't need the flattened array for anything other than length.

Instead of storing an array of unskipped tests, we can just set a skip flag on each test, and compute the number of unskipped tests on the fly.

bendemboski · 2017-04-13T21:26:38Z

@trentmwillis I've addressed your feedback, but left the commits un-squashed so it's easier to see the changes. Let me know if you'd like me to squash this PR down to a single commit (or if you just want to do it as part of the merge).

trentmwillis

Okay, sorry for the runaround, one final set of changes and this should be good to merge. Thanks again for working on it!

trentmwillis · 2017-04-14T00:32:59Z

src/test.js

 	module.testsRun++;
 	while ( ( module = module.parentModule ) ) {
 		module.testsRun++;
 	}
+
+	if ( !skipped ) {


We should perform this check inside the while loop above so that we're only iterating once.

I cringe at checking a loop invariant inside the loop, although this time I guess I went the direction of incurring a performance cost in the common case rather that the uncommon one 🤦‍♂️

How about this:

let notify; if ( skipped ) { notify = function(module) { module.testsRun++; }; } else { notify = function(module) { module.testsRun++; module.unskippedTestsRun++; }; } notify(module); while ( ( module = module.parentModule ) ) { notify(module); }

Not a big deal if you don't like it and prefer just checking the invariant inside the loop -- let me know.

I'd prefer just checking the invariant. It's a fast check and less code to reason about.

trentmwillis · 2017-04-14T00:37:56Z

test/main/modules.js

+
+QUnit.skip( "last test in module is skipped" );
+
+QUnit.module( "before/after with all tests skipped (wrapper)", function() {


We can make this test much simpler:

QUnit.module( "before/after with all tests skipped", { before: function( assert ) { assert.ok( false, "should not occur" ); }, after: function( assert ) { assert.ok( false, "should not occur" ); } } ); QUnit.skip( "verifier" );

👍 -- let me know your decision on the other two comments, and I'll roll all three into one commit.

trentmwillis · 2017-04-14T00:38:35Z

test/main/modules.js

+
+QUnit.test( "runs before first unskipped test", function( assert ) {
+	assert.expect( 2 );
+	assert.equal( this.beforeCount, 1, "beforeCount should be one" );


We can ditch the beforeCount stuff. Since we assert.expect() we know whether or not the hook ran.

I was following the convention in a few other tests in this file like this one. I'm happy either way, but want to double-check that you prefer terseness over consistency here -- let me know which way you prefer.

I'd prefer the terse version. It's likely that the other ones should be changed as well, but that doesn't need to be done here.

bendemboski · 2017-04-14T05:16:45Z

Okay @trentmwillis comments addressed. Let me know if you see anything else or want me to squash commits, otherwise merge away.

trentmwillis

Thanks! @leobalter I'm dismissing your review since your comments have been addressed.

Comments have been addressed.

trentmwillis · 2017-04-14T17:54:58Z

Thanks again @bendemboski!

This was referenced Apr 12, 2017

Fix modules with no callbacks emberjs/ember-qunit#267

Merged

Tests failing in beforeEach() due to module being undefined emberjs/ember-qunit#268

Closed

trentmwillis requested changes Apr 12, 2017

View reviewed changes

leobalter requested changes Apr 12, 2017

View reviewed changes

Core: Fix bug calling hooks with skipped tests

4d8bf05

When the first/last test in a module was skipped, it would cause the module's before()/after() hook to not be called. This ensures those hooks will always be called.

bendemboski force-pushed the fix-skip-hooks branch from 42352e0 to 4d8bf05 Compare April 12, 2017 21:48

leobalter previously requested changes Apr 12, 2017

View reviewed changes

Core: address PR feedback (to be squashed)

4962775

trentmwillis requested changes Apr 13, 2017

View reviewed changes

Core: Don't store array of unskipped tests

12b789c

Instead of storing an array of unskipped tests, we can just set a skip flag on each test, and compute the number of unskipped tests on the fly.

trentmwillis requested changes Apr 14, 2017

View reviewed changes

Core: code review feedback

8dfd6ff

trentmwillis approved these changes Apr 14, 2017

View reviewed changes

trentmwillis merged commit 2ce7e91 into qunitjs:master Apr 14, 2017

rwjblue mentioned this pull request Apr 14, 2017

a skip before test fails when this.subject() is called in beforeEach emberjs/ember-qunit#269

Closed


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

		QUnit.module( "before/after with all tests skipped (wrapper)", function() {

Core: Fix bug calling hooks with skipped tests #1156

Core: Fix bug calling hooks with skipped tests #1156

Conversation

bendemboski commented Apr 12, 2017

jsf-clabot commented Apr 12, 2017 • edited Loading

rwjblue commented Apr 12, 2017

trentmwillis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leobalter commented Apr 12, 2017

bendemboski commented Apr 12, 2017

leobalter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendemboski commented Apr 12, 2017

trentmwillis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendemboski commented Apr 13, 2017

trentmwillis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendemboski commented Apr 14, 2017

trentmwillis left a comment

Choose a reason for hiding this comment

trentmwillis commented Apr 14, 2017

jsf-clabot commented Apr 12, 2017 •

edited

Loading