Fix #2083: HTML reporter regression causing duplicate error output

[danielstjules]

Fixed #2083

This is a minor refactor of the HTML reporter to listen to pass/fail/pending events rather than test end, just like the spec/dot reporters.

---

As seen below, the sync error is displayed only a single time:

---

---

And the async errors are each correctly displayed. This is the behavior #1981 was intended to fix. Prior to that PR, none of the errors below were displayed in the HTML reporter.

---

[danielstjules]

cc @boneskull

[danielstjules]

This is compatible with #2081 since the pending event now fires after checking isPending()

[boneskull]

@danielstjules cool. do you have any recommendations for how to test this?

[boneskull]

#2079 is still a thing.

[boneskull]

I really don't want to release anything without #2079 being addressed in some fashion

[danielstjules]

Aside from manual testing? If you open test/browser/index.html in master, you'll see that normal errors are duplicated, but async errors after a test completed are correctly shown:

Comparing that against the build of this PR/branch, async errors post-test completion are still shown, and normal errors are only rendered once.

[danielstjules]

In regards to the async errors, this is the same expected behavior seen from the node reporters:

$ cat example.js
it('test', function(done) {
  setTimeout(done, 10);
  setTimeout(done, 20);
});

it('test2', function(done) {
  setTimeout(done, 30);
});

$ ./bin/mocha example.js


  ․․․

  2 passing (51ms)
  1 failing

  1)  test:
     Error: done() called multiple times
      at Test.Runnable (lib/runnable.js:54:17)
      at new Test (lib/test.js:22:12)
      at context.it.context.specify (lib/interfaces/bdd.js:85:18)
      at Object.<anonymous> (example.js:1:63)
      at Array.forEach (native)
      at Object.<anonymous> (bin/_mocha:403:18)
      at node.js:868:3

[danielstjules]

I really don't want to release anything without #2079 being addressed in some fashion

I can work on that tonight. But I don't think I'll have an exhaustive enough automated browser test suite ready to reveal issues beyond manually running those HTML files in IE8/Safari/Chrome.

[boneskull]

at minimum then I'd want someone to test IE8, phantomjs 1.x and phantomjs 2.x manually. specifically, see if mochify breaks.

[jnbt]

Any updates on this? I think the HTML reports are still broken...

[justinfagnani]

Also curious about this PR, the bug is messing with reporting downstream of mocha. Are all PRs on hold until #2079 is resolved?

[dasilvacontin]

@justindujardin pretty much, sorry for the hassle.

[justinfagnani]

Honestly, this makes me question our dependence on Mocha. This fix is 2 months old.

[ScottFreeCode]

I've been experimenting with a set of tests to check for problems like this. It isn't pretty, I don't know if it will work in any kind of automatic testing such as discussed in #2079, and the CLI won't work unless I can get the same HTML report as the browser uses; but at least the set of test cases should be a good starting point (although you may have to go through and tweak which situations are intended to be reported as how many passes and failures, I'm not 100% sure I got it right). It's also not really unit-testing the matter -- if there's the wrong number of results there's no indication as to whether it's a flaw in the test runner or the reporter, if there's the right number of results that doesn't mean the responsibilities involved in getting them are correctly divided between the test runner and the reporter -- but it's better than nothing, I figure (I guess if we wanted to unit test Mocha's internals we'd have to use the non-bundled scripts... and to make it run in the browser would probably end up basically creating a second bundle containing the internal unit tests?...).

I've put what I've got so far up in a gist here: https://gist.github.com/ScottFreeCode/f8b0864781414f96594f67b77ddee469

[maciejjankowski]

I wonder why is this stalled...
shouldn't you add PR Please label?

[pluma]

I can see why you would want to fix #2079 first but this change is trivial compared to the rabbit hole that is automated browser testing. I believe greenlighting this fix despite the freeze would help a lot of people. As it is, #2083 is making browser reports significantly less useful, so the question of browser support doesn't really come into it.

[boneskull]

I'm going to rebase this onto master and see what happens.

[boneskull]

@danielstjules does this TODO mean this PR isn't ready?

[danielstjules]

The comment already existed above https://github.com/mochajs/mocha/pull/2112/files#diff-998e3fb668a2094eedccab3e56199e4fL141

[dasilvacontin]

@danielstjules true again

[boneskull]

In general, I think this looks good. Just want some feedback from @danielstjules to ensure it's ready to merge.

[dasilvacontin]

In the comment you mention [Object Error], but in the code it uses a different casing. (O->o)

[pluma]

@dasilvacontin the code is correct, the comment is wrong.

[dasilvacontin]

Thanks for clearing it up @pluma!

[danielstjules]

If you look further down, I didn't write the comment, just moved around the code :)

[dasilvacontin]

@danielstjules true :)

[dasilvacontin]

LGTM. Sync error is only reported once now. Waiting for the reply about the TODO, will release in a day or two if no response is given.

[dasilvacontin]

btw: sorry for the wait internet people.

[danielstjules]

The diff is unfortunately long for what was a minor refactor. A lot of the code was moved around rather than modified.

[dasilvacontin]

Thanks for the fix @danielstjules :). I released it already.