Add failing test: after hook is not run if test skipped in beforeEach #2287
Conversation
|
@dasilvacontin I think this fixes it, but I need a sanity check |
|
cc @mochajs/mocha or anyone else who wants to test this out |
boneskull
force-pushed
the
skip-after-regression
branch
from
284e154
to
4f2e454
Compare
|
as per discussion, I've updated my code to be more weird, but also more backwards-compatible. cc @mochajs/mocha |
|
I'm wary of depending on the describe('issue-2286: after doesn\'t execute if test was skipped in beforeEach', function () {
var afterWasRun = false
describe('suite with skipped test for meta test', function () {
beforeEach(function () { this.skip() })
after(function () { afterWasRun = true })
it('should be pending', function () {})
})
after('meta test', function () {
afterWasRun.should.be.ok();
})
}) |
| beforeEach(function () { this.skip() }) | ||
| after(function () { console.log('after in suite') }) | ||
| it('test', function () {}) | ||
| }) |
There was a problem hiding this comment.
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'
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
actually on second thought, I like your test above. please use it instead
There was a problem hiding this comment.
(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.
There was a problem hiding this comment.
(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?
There was a problem hiding this comment.
(Went ahead and committed both; feel free to rebase away either you don't want.)
This should make it possible to understand when just looking at the fixture.
| @@ -1,4 +1,4 @@ | |||
| var assert = require('assert'); | |||
| var assert = require('assert'); | |||
| var fs = require('fs'); | |||
There was a problem hiding this comment.
Did this... pick up a UTF-8 BOM or something? I will see if I can fix that when I get another free moment.
There was a problem hiding this comment.
Or not, if it's already merged into the history and isn't hurting anything...
Related to #2286.