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

Fix HTML reporting display to show errors if done is called multiple times, or if an exception is thrown after done is called. #1981

Merged
merged 1 commit into from
Dec 25, 2015

Conversation

Standard8
Copy link
Contributor

In either of the test cases below, when using the HTML reporter, the failure details are not currently shown. The failure count is increased, but there's no specific details.

This patch adjusts the reporter so that failures after the initial done() are displayed. This doesn't seem to cause any regressions in the manual browser tests.

The output does show the test as passing, and then shows it as failing, but I think that's reasonable given what is happening and is better than not showing any errors.

Test cases:

describe('Multiple Done calls', function(){
  it('should report an error if done was called more than once', function(done){
    done();
    done();
  })

  it('should report an error if an exception happened after done was called', function(done){
    done();
    throw new Error("thrown error");
  })
})

Review on Reviewable

@dasilvacontin
Copy link
Contributor

It's definitely an improvement. failures are correctly updated, passes don't decrease, we could improve that.

It would be nice to add the following test – it's rendered outside the suite, since the suite has "ended" already, though. At least the error is not reported as belonging to a different test, but that depends on mocha's internals.

diff --git a/test/browser/multiple-done.js b/test/browser/multiple-done.js
index 7ac3ff3..37b2703 100644
--- a/test/browser/multiple-done.js
+++ b/test/browser/multiple-done.js
@@ -1,10 +1,15 @@
describe('Multiple Done calls', function(){
   it('should report an error if done was called more than once', function(done){
     done();
     done();
   })

-  it('should report an error if an exception happened after done was called', function(done){
+  it('should report an error if an exception happened async after done was called', function (done) {
+    done()
+    setTimeout(done, 50)
+  })
+
+  it('should report an error if an exception happened sync after done was called', function(done){
     done();
     throw new Error("thrown error");
   })

…times, or if an exception is thrown after done is called.
@Standard8
Copy link
Contributor Author

@dasilvacontin Sorry for the delay, I've now added the extra test.

@danielstjules
Copy link
Contributor

Nice PR! :) Looks like this does a good job of aligning behavior with the node reporters:

$ mocha test.js

  ․․․․

  2 passing (9ms)
  2 failing

  1) Multiple Done calls should report an error if done was called more than once:
     Error: done() called multiple times
      at Suite.<anonymous> (test.js:2:3)
      at Object.<anonymous> (test.js:1:63)
      at Array.forEach (native)
      at node.js:961:3

  2) Multiple Done calls should report an error if an exception happened after done was called:
     Error: thrown error
      at Context.<anonymous> (test.js:9:11)

@danielstjules
Copy link
Contributor

@dasilvacontin I don't think it's necessary to have passes decrease, since we don't do this for any of the other reporters. Furthermore, any async exceptions will be reported as Uncaught Error, and we can't correctly attribute them to a specific test anyway. So if multiple uncaught errors are thrown, we don't know if they're from a test that previously passed or failed, and can't correctly update the "passes" number

danielstjules added a commit that referenced this pull request Dec 25, 2015
Fix HTML reporting display to show errors if done is called multiple times, or if an exception is thrown after done is called.
@danielstjules danielstjules merged commit 829df1d into mochajs:master Dec 25, 2015
@danielstjules
Copy link
Contributor

Thanks again! :)

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