-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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 2067: HTML/DOC reporter regression with async failures #2068
Conversation
Reviewed 4 of 4 files at r1. Comments from the review on Reviewable.io |
Fix 2067: HTML/DOC reporter regression with async failures
@@ -22,6 +22,7 @@ function Test(title, fn) { | |||
Runnable.call(this, title, fn); | |||
this.pending = !fn; | |||
this.type = 'test'; | |||
this.body = (fn || '').toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should Test#clone
copy this one as well?
cc @longlho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No - when the new test is created via Test#clone
, it'll get the exact same string body https://github.com/danielstjules/mocha/blob/d59cc6c166d0d112a62016909b33e04ed08d2274/lib/test.js#L34
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reason why Test#clone
doesn't apply this.type = 'test';
. It's already handled in the constructor :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the initialization of this.body
belong to Runnable
's constructor rather than Test
?
Right now, if you get an error in a hook, Mocha will bomb, because the Hook
object that it tries to process here does not have a body
field. Moving the creation of body
to Runnable
would make both Hook
and Test
get a body
field.
The issue I'm talking about here is fully reproducible with the code provided in this comment. I also am able to reproduce it independently in a test suite of mine (which is why I found the comment and am commenting here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be resolved by #2115 Thanks! :)
Fixes #2067