Skip to content

Conversation

@vadimdemedes
Copy link
Contributor

Hello Sindre,

This pull request closes issue #14. It measures each test's total execution time (duration) using process.hrtime() and then outputs nicely formatted time near test's title:

screen shot 2015-09-01 at 12 27 17 pm

pretty-hrtime module is used for converting process.hrtime() result to a human-readable form.

Thanks for a great project, will be switching off mocha to ava soon.

@Qix-
Copy link
Contributor

Qix- commented Sep 1, 2015

👍 Neat. Code looks good to me.

Could there be a way to optionally specify that I want to always show at least millisecond output?

index.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

log.success(title + ' ' + chalk.dim('(' + hrtime(duration) + ')'));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed an update.

@sindresorhus
Copy link
Member

Was thinking about doing something like this, but instead only show the time when it passes some kind of threshold, as the number is really only useful for identifying slow tests. Thoughts?

@Qix-
Copy link
Contributor

Qix- commented Sep 1, 2015

That's how mocha does it, and it makes sense. I don't care if a test took 200us. I do care if it took 200ms.

@sindresorhus
Copy link
Member

If so, what kind of threshold makes sense? Not interested in adding an option to change it, so it needs to cover most use-cases.

vdemedes added 2 commits September 1, 2015 13:24
Duration measured in nanoseconds does not give any valuable
information to the developer. It makes sense to display
time only for long-running tests, above some threshold.
Also, process.hrtime() is not available in browsers, so it
would need replacement anyway.
@vadimdemedes
Copy link
Contributor Author

Added code to display test duration only when a threshold (100ms) is reached. If you want, you can easily change it here: 2791234#diff-168726dbe96b3ce427e7fedce31bb0bcR31

@sindresorhus
Copy link
Member

Looks very good @vdemedes! Can you add a test?

@sindresorhus
Copy link
Member

Here's how it looks:

screen shot 2015-09-01 at 17 49 55

index.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Just remembered that dim doesn't work everywhere. Can you make it chalk.gray.dim?

Copy link
Member

Choose a reason for hiding this comment

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

And the below ' ' should be here instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, going to add that now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed an update.

@vadimdemedes
Copy link
Contributor Author

@sindresorhus of course, will add a test for sure

@vadimdemedes
Copy link
Contributor Author

@sindresorhus Added a test for measuring duration, let me know what you think!

sindresorhus pushed a commit that referenced this pull request Sep 1, 2015
@sindresorhus
Copy link
Member

Excellent work @vdemedes. Keep'em coming! ;)

More than happy to receive addition suggestion on how AVA can be better in form of issues or PRs.

@vadimdemedes
Copy link
Contributor Author

Thank you @sindresorhus, will definitely contribute more!

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.

3 participants