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.

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

Print failures for pending/only forbidden #2874

Merged
merged 9 commits into from
Sep 29, 2017
Merged

Print failures for pending/only forbidden #2874

merged 9 commits into from
Sep 29, 2017

Conversation

ScottFreeCode
Copy link
Contributor

Followup to #2696; this is going to look crazy when somebody puts .only or .skip on 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).

@@ -529,7 +533,13 @@ Runner.prototype.runTests = function (suite, fn) {
}

if (test.isPending()) {
self.emit('pending', test);
if (self.forbidPending) {
test.isPending = alwaysFalse;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(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?)

Copy link
Contributor

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

Choose a reason for hiding this comment

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

And Runner.prototype.fail is what reporters depend on for events...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

@ScottFreeCode ScottFreeCode Jun 15, 2017

Choose a reason for hiding this comment

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

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();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two if (test.isPending()) { ... sections are identical or nearly so; we should consider breaking it out into a function or something.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 88.313% when pulling e0b785f on forbid into 7647e18 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 88.313% when pulling e0b785f on forbid into 7647e18 on master.

@coveralls
Copy link

coveralls commented Jun 13, 2017

Coverage Status

Coverage increased (+0.05%) to 88.446% when pulling 2307511 on forbid into 7647e18 on master.

@boneskull
Copy link
Contributor

@ScottFreeCode Is this needed to release alongside #2696? In other words, can we release #2696 without it?

@ScottFreeCode
Copy link
Contributor Author

ScottFreeCode commented Jun 15, 2017

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:

  • Consider this a bugfix as there should have been output to begin with, or...
  • Make this semver-major as it significantly changes output.

(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:

  • The options in question are likely to be used in CI, leading to CI failures with no message about why.
  • The output added here not only indicates that these particular options were violated, but indicates which test(s) are at fault (albeit clumsily in the case of suites, as it will list every one of their tests rather than listing the suite or suites with the only or skip set).

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 88.49% when pulling 245c9cc on forbid into 7647e18 on master.

@ScottFreeCode
Copy link
Contributor Author

I'm toying with adding a test for describe.only as well (similar to my test for describe.skip). It's redundant with this implementation, where any it run when only is in effect is what will error, but may be nice to have explicitly tested so the test is already there if we ever change the implementation -- or if anything accidentally affects the implementation, for that matter.

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.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.7%) to 89.11% when pulling 6cdfa05 on forbid into 7647e18 on master.

@ScottFreeCode
Copy link
Contributor Author

Added that last test previously mentioned, and it looks like we're passing CI here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants