Print failures for pending/only forbidden #2874
Conversation
(...somehow only noticed this when reviewing to create a PR...)
| @@ -529,7 +533,13 @@ Runner.prototype.runTests = function (suite, fn) { | |||
| } | |||
|
|
|||
| if (test.isPending()) { | |||
| self.emit('pending', test); | |||
| if (self.forbidPending) { | |||
| test.isPending = alwaysFalse; | |||
There was a problem hiding this comment.
I'm not entirely happy with this hack to circumvent Runner.prototype.fail's check that ignores pending tests based on test.isPending(), but I'm not sure if there's a better option. Any opinions would be appreciated, provided we can avoid stalling over the matter.
There was a problem hiding this comment.
(Actually, I honestly wonder why Runner.prototype.fail needs to ignore pending tests. Wouldn't it be more correct not to call it on pending tests? Do we know where it's being called on pending tests so we could evaluate whether that could be adjusted instead?)
There was a problem hiding this comment.
A pending test is a failure, because a pending test (a synchronous one, anyway) actually throws an exception. We catch that in the Runnable, then set the pending flag, then continue on to the "fail" handler in Runner. At least, if memory serves...
There was a problem hiding this comment.
And Runner.prototype.fail is what reporters depend on for events...
There was a problem hiding this comment.
Hmm. The Pending exception is being handled in the Runner around line 533, in which a pending event is emitted instead of going to the fail function at all. (The fail function ignores pending tests altogether, rather than emitting anything pending for them; hence the need to circumvent it in this line.)
The Runnable is only throwing (and in one case passing to done) Pending exceptions, not catching any (it also only emits an "error" event, in the case that done is called multiple times, so it's not handling the pending emission before sending an ignored error of some sort to the Runner either).
There was a problem hiding this comment.
Additionally, Coveralls indicates that the fail function's pending test ignore branch is never hit: https://coveralls.io/builds/11947829/source?filename=lib%2Frunner.js#L224 However, to confirm that we aren't just missing a test for this case, I'd have to go look at whether we have tests for all... five? six?... cases where tests can be pending: describe.skip, it.skip, it("title and no callback"), this.skip in it, this.skip in beforeEach, this.skip in before...
| delete test.isPending; | ||
| } else { | ||
| self.emit('pending', test); | ||
| } | ||
| self.emit('test end', test); | ||
| return next(); | ||
| } |
There was a problem hiding this comment.
These two if (test.isPending()) { ... sections are identical or nearly so; we should consider breaking it out into a function or something.
|
@ScottFreeCode Is this needed to release alongside #2696? In other words, can we release #2696 without it? |
|
In my own opinion: it is needed to release alongside #2696. In more detail and less opinion: #2696 adds the options and a return code, but no output to actually indicate the reason for the failing return code. This adds the output. #2696, or #2696 plus this if the two are released together, is a new feature. We can release #2696 without this if we are willing to either:
(I don't have any strong preference which, so long as it happens; my stronger preference is that the decision not be necessary by releasing this and #2696 together as one feature.) If it makes any difference:
|
|
I'm toying with adding a test for It'd be nice if we could get the CI issues fixed first so pushing a commit with a test we know passes won't make everything go from green like it is now to red for no reason. |
|
Added that last test previously mentioned, and it looks like we're passing CI here. |
Followup to #2696; this is going to look crazy when somebody puts
.onlyor.skipon a suite, but I can live with that (whereas I can't take not having any printout for these failures at all) and if we figure out how to emit one failure per relevant suite we can always tweak that later (whether it has to be semver-major or not).