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

Add failing test: after hook is not run if test skipped in beforeEach #2287

Merged
merged 5 commits into from
Jun 11, 2016
Merged

Add failing test: after hook is not run if test skipped in beforeEach #2287

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2c9966c
Add failing test: after hook not run if beforeEach skipped test
dasilvacontin May 29, 2016
22dbad7
wip Fix suite being marked as pending when beforeEach skips
dasilvacontin May 29, 2016
4f2e454
update pending logic to closely mimic previous behavior
boneskull Jun 9, 2016
d2ba7a9
Clarify nature of meta test
ScottFreeCode Jun 10, 2016
d165c82
Nested suites for meta test
ScottFreeCode Jun 10, 2016
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
14 changes: 10 additions & 4 deletions lib/runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,13 @@ Runner.prototype.hook = function(name, fn) {
}
if (err) {
if (err instanceof Pending) {
suite.pending = true;
if (name === 'beforeEach' || name === 'afterEach') {
self.test.pending = true;
} else {
suite.tests.forEach(function(test) {
test.pending = true;
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't it use suite.pending if it's not beforeEach or afterEach? Or in the older versions does after run even if before calls this.skip()?

Copy link
Contributor

Choose a reason for hiding this comment

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

setting the suite to be pending, at this point, will cause the after hook to not get executed.

Copy link
Contributor

Choose a reason for hiding this comment

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

(so, the answer is "no")

}
} else {
self.failHook(hook, err);

Expand Down Expand Up @@ -516,7 +522,7 @@ Runner.prototype.runTests = function(suite, fn) {
// execute test and hook(s)
self.emit('test', self.test = test);
self.hookDown('beforeEach', function(err, errSuite) {
if (suite.isPending()) {
if (test.isPending()) {
self.emit('pending', test);
self.emit('test end', test);
return next();
Expand Down Expand Up @@ -843,8 +849,8 @@ function filterLeaks(ok, globals) {
}

// in firefox
// if runner runs in an iframe, this iframe's window.getInterface method not init at first
// it is assigned in some seconds
// if runner runs in an iframe, this iframe's window.getInterface method
// not init at first it is assigned in some seconds
if (global.navigator && (/^getInterface/).test(key)) {
return false;
}
Expand Down
5 changes: 5 additions & 0 deletions test/integration/fixtures/regression/issue-2286.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
describe('suite', function () {
beforeEach(function () { this.skip() })
after(function () { console.log('after in suite') })
it('test', function () {})
})
Copy link
Contributor

Choose a reason for hiding this comment

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

How about some names that make this file's function obvious (both in viewing this file and if it were to end up run among the main tests in addition to the meta test calling it by run)? e.g.:
describe: 'testception for issue 2286 meta test'
beforeEach: 'skips tests'
after: 'should run'
it: 'skipped by beforeEach'

Copy link
Contributor

Choose a reason for hiding this comment

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

@ScottFreeCode I don't see this stuff as absolutely necessary given the filename, but if you wish to add to the branch, go for it

Copy link
Contributor

Choose a reason for hiding this comment

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

actually on second thought, I like your test above. please use it instead

Copy link
Contributor

Choose a reason for hiding this comment

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

(to the first reply:) Yeah, it's pretty clear from the filename what issue this is testing, but it's not so obvious to me from looking just at this file that this is meant to be run by another test in another file that tests the output of this one (unless that's what all the stuff in this fixtures folder is -- I haven't looked at the files that were already there), so it looked at first as though this weren't actually testing it programmatically but only printing something to look at manually.

Copy link
Contributor

Choose a reason for hiding this comment

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

(to the second:) You mean the one that uses nested suites with code like this in the inner one and a test in the after of the outer one?

Copy link
Contributor

Choose a reason for hiding this comment

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

(Went ahead and committed both; feel free to rebase away either you don't want.)

14 changes: 14 additions & 0 deletions test/integration/regression.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,18 @@ describe('regressions', function() {
done();
});
})

it('issue-2286: after doesn\'t execute if test was skipped in beforeEach', function(done) {
var args = [];
run('regression/issue-2286.js', args, function(err, res) {
var occurences = function(str) {
var pattern = new RegExp(str, 'g');
return (res.output.match(pattern) || []).length;
};

assert(!err);
assert.equal(occurences('after in suite'), 1);
done();
});
})
});