Skip to content

Conversation

@AviVahl
Copy link

@AviVahl AviVahl commented Jun 8, 2016

Hi mocha team.
This is a small fix for the progress bar staying consistently at 0 when using the HTML reporter.
Seems like a regression that happened due to the "double-error" fix/refactor.

Reasoning for the changes:
updateStats() accesses this.total, but this === the global object (window) if not specified explicitly here.
Also added a missing update when all tests end.

updateStats() accesses this.total, but this === the global object (window) if not specified explicitly.
Also added a missing update when all tests end.

runner.on('suite end', function(suite) {
if (suite.root) {
updateStats.call(this);
Copy link
Member

Choose a reason for hiding this comment

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

can we just use Function.prototype.bind() on updateStats instead?

@AviVahl
Copy link
Author

AviVahl commented Jun 10, 2016

@boneskull Thanks for reviewing.

You could bind updateStats to runner and set it as this.updateStats.
Both approaches would work. You would still need to add a call on 'suite end'.
I wasn't sure if you want updateStats set on the reporter instance here, so I used the a explicit call. Please clarify.

@boneskull
Copy link
Member

@AviVahl It looks like updateStats() is always called with the context of the Runner object, so actually call and bind shouldn't be necessary, yes?

@AviVahl
Copy link
Author

AviVahl commented Jun 11, 2016

Calling updateStats() in a block inside a context does not set that context to the call invocation.
I might have an alternative patch which you would like more.
I'll commit.

@AviVahl
Copy link
Author

AviVahl commented Jun 11, 2016

@boneskull new patch allows calling updateStats() directly (without specifying this), as it binds runner to extract the value of total.
Please let me know if this is a nicer solution in your opinion.

@AviVahl
Copy link
Author

AviVahl commented Jun 24, 2016

@boneskull Can we get this small bug-fix reviewed again?
Two lines change for such an annoying UI issue.
Thanks in advance.

@boneskull
Copy link
Member

@AviVahl looks good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: browser browser-specific

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants