Regression: Failures rendering twice

[ahamid]

I just happened to start up a simple hello world mocha test project (to test something else :) ), and I noticed that versions >= 2.4 now double-render the failure in my example. Reverting to 2.3.3 makes the behavior go away. I will try to get to the bottom of it, but I see several releases in the last few days so I thought maybe you'd want to know.

https://github.com/ahamid/mocha-phantomjs-blanket/tree/mocha-failure-double-render

[ahamid]

It might be this commit: ae4fbca

The HTML reporter is now emitting another 'test end' event on any test failure, which I believe causes the result to be printed twice (since it's rendered on 'test end'). That's my hunch anyway.

[ahamid]

Commenting this line has the same effect: https://github.com/mochajs/mocha/blob/v2.4.5/lib/runner.js#L551

Both avoid 'test end' being emitted twice.

[danielstjules]

Thanks for letting us know! Looks like not all failures are emitted twice - likely why it was missed.

Edit: Unfortunately, just commenting out that line won't work as it'll result in a failing build (retry logic) when running make test-all More specifically, commenting out that test end actually breaks the json reporter.

[danielstjules]

I'm more inclined to just revert the PR: #1981

[boneskull]

the fix may be as simple as checking if the test has already failed in the callback there.

[danielstjules]

@boneskull As in?

diff --git a/lib/reporters/html.js b/lib/reporters/html.js
index 7da2508..6366542 100644
--- a/lib/reporters/html.js
+++ b/lib/reporters/html.js
@@ -131,8 +131,8 @@ function HTML(runner) {
   runner.on('fail', function(test) {
     // For type = 'test' its possible that the test failed due to multiple
     // done() calls. So report the issue here.
-    if (test.type === 'hook'
-      || test.type === 'test') {
+    if (test.type === 'hook' ||
+        (test.type === 'test' && test.state !== 'failed')) {
       runner.emit('test end', test);
     }
   });

While it prevents double errors, it suppresses any error messages dealing with async errors after a test passed, or after multiple calls to done.

Edit: I'm pooched, gonna catch some 😴 and look into it tomorrow. Sorry about that.

[boneskull]

here

just an idea

[ohnnyj]

Hello all.

I am by no means an expert with the mocha code base but here is just a thought.

Can we mark the test as being ended something like:

    if (test.type === 'hook'
      || test.type === 'test') {
      test.didEnd = true;
      runner.emit('test end', test);
    }

Then in runTest callback:

!test.didEnd && self.emit('test end', test);

[mckoss]

I'm seeing this too - pretty annoying. Please revert #1981.

[Standard8]

I only got notified of this issue a couple of days ago. I would like to ask that we don't revert PR 1981 - that causes other types of failures to be completely hidden from view, and cost a lot of time in debugging an issue.

Having thought about the code a bit more, I think the solution here might be to change the HTML reporter, and rather than listening to test end, make it listen to pass. We'll probably also have to make it listen to pending.

I think that would most likely correctly handle the failure cases.

I'm not sure if I'll get time to try this today, but should do tomorrow unless anyone beats me to it.

[mckoss]

I think it's pretty clear, that no test should emit more than one 'test end' event. This change (#1981) is a regression in that it does just that in the most basic synchronous test case (failure). There needs to be some test state that ensures that this invariant is maintained.

[boneskull]

@Standard8 @mckoss @ahamid

thanks for bringing this to our attention. sorry about the slow response.

I believe a simple memoization of the test end listener (code) in the HTML reporter would address this, e.g.:

var memoize = require('lodash/memoize');
runner.on('test end', memoize(function(test) {
  // etc
});

Note that we don't have lodash as a dependency, and can't add it right now, so the possibilities include:

• find some other (small) memoization module which is compatible w/ Node.js 0.8.x out of the box
• do it manually by tossing Test objects into an Array and checking if the test is already in there; if so, just return (slow)
• toss Test objects into an object instead (faster; but you need a key)

To support an object-based lookup (which we should), you'll need a unique (string) identifier. The problem is Test objects don't currently have a unique identifier. Unless we give it one (perhaps an id prop), I can't think of anything we could potentially use--no temporal data is kept in the Test object other than duration, which is not necessarily unique when combined with title.

So I'd recommend just adding an id prop to each Test object in lib/test.js. An auto-incrementing integer is fine. Then we can use it in this manner (assuming we don't end up doing it manually):

var memoize = require('lodash/memoize');
runner.on('test end', memoize(function(test) {
  // etc
}, function resolver(test) {
  return test.id;
});

To ensure v0.8.x compatibility (if adding a 3p module), do something like this:

$ cd /tmp
$ nvm install 0.8
$ npm install /path/to/my/working/copy/of/mocha

If that fails, then whatever package you added won't work (you may need to try an older version of it?).

Would happily accept a PR!

[boneskull]

(also, if you can find another sane way to fix it, that's great too)

[danielstjules]

#2083 (comment) won't work because that line doesn't actually see the test twice. Memoization would def work, though I'm hoping we can come up with an alternative.

[danielstjules]

no test should emit more than one 'test end' event.

True, though we shouldn't rely on the test end event at all since a test can succeed, then later emit async errors resulting in subsequent failures. Updating the HTML reporter to listen to the pass|fail|pending events like the other reporters would probably help.

[mckoss]

I thought it already did listen to pass/fail/pending - but injects the HTML
at 'test end' time.

+Mike Koss https://plus.google.com/+MikeKoss/posts
(425) 246-7701 (m)

On Tue, Feb 16, 2016 at 12:19 AM, Daniel St. Jules <notifications@github.com

wrote:

no test should emit more than one 'test end' event.

True, though we shouldn't rely on the test end event at all since a test
can succeed, then later emit async errors resulting in subsequent failures.
Updating the HTML reporter to listen to the pass|fail|pending events like
the other reporters would probably help.

—
Reply to this email directly or view it on GitHub
#2083 (comment).

[danielstjules]

I thought it already did listen to pass/fail/pending - but injects the HTML
at 'test end' time.

If that were the case, https://github.com/mochajs/mocha/pull/1981/files wouldn't have been required. We need to handle specs that fail after having already ended. For example:

it('test', function(done) {
  setTimeout(done, 1000);
  setTimeout(done, 1100);
});

// Events:
// - pass
// - test end
// - fail (error: multiple calls to done)

I've opened a PR that fixes this.

[chemzqm]

Issue exists with latest mocha 2.4.5, pleeeeease fix this.

[Standard8]

It looks like a fix is in progress in PR #2112 but has currently stalled.

[maciejjankowski]

Somebody, please. What happened to PR #2112 ?

[Standard8]

@danielstjules thank you for fixing this! Sorry that I caused it in the first place.