This is a proposal to add example duration to the the html formatter's output #451

Merged
merged 1 commit into from Nov 15, 2011

Projects

None yet

2 participants

@richievos
Contributor

This is just a conversation starter. Happy to modify the actual implementation as you see fit. In particular, I haven't updated the html formatter spec yet, since I want to get feedback on this implementation before jumping through all the specs.

@dchelimsky
Member

Couple of thoughts: this should be implemented in as a method in the base formatter that subclasses can use. Also, probably better to measure in milliseconds. WDYT?

@dchelimsky
Member

Actually, the measurement already exists as metadata on each example: example.execution_result[:run_time], so we don't need to measure it. That can get wrapped in a method in the formatter, but best if that method uses what's already there.

See https://github.com/rspec/rspec-core/blob/master/lib/rspec/core/formatters/base_text_formatter.rb#L57 for an example of how we're already using it.

@richievos
Contributor

Updated the diff:

Thanks for the pointer on run time. Glad to not have to reinvent the wheel.
I made the output go down to ms: sprintf("%.3f", example.execution_result[:run_time])

I think it makes sense to not give the full precision on it, since it's probably not meaningful to multiple decimal places, especially when compared to the space it'd take up. To ms doesn't seem to bad though. Your thoughts?

Updated image

Also could see truncating Finished in to 3 decimal places.

@dchelimsky dchelimsky merged commit 001522b into rspec:master Nov 15, 2011
@dchelimsky dchelimsky added a commit that referenced this pull request Nov 15, 2011
@dchelimsky dchelimsky Use 5 decimal places for time reports in html formatter.
- also update formatter specs and gold masters
- #451
79587d5
@dchelimsky
Member

@richievos - 3 decimal places resulted in a bunch of examples with 0.000s. On the one hand it made me happy that they took virtually no time, but it was also not very helpful :)

I played around w/ different numbers and landed on 5, and updated the total run time and the time for failed examples to use the same format.

@dchelimsky dchelimsky added a commit that referenced this pull request Nov 15, 2011
@dchelimsky dchelimsky changelog for #451 8de2a97
@richievos
Contributor

I was debating the number of decimal places to go with. I chose 3 because it seemed like anything below milliseconds was probably going to be noise, or something I personally wouldn't be that interested in. Meaning, that the 4th and 5th decimal place were probably below the level where they'd be realistically something I'd bother looking into, and may be noisy enough that they're not reliably useful.

However, your specs hopefully run faster than mine, so your call :)

Thanks for the merge!

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