Performance testing: add total value of metric #7290

Closed
wants to merge 2 commits into
from

Projects

None yet

4 participants

@y8
y8 commented Aug 8, 2012

In most cases, when you benchmarking something you are interested not only in average metric value, but in total metric value.

This change adds total value, along with average. 

By the way: atm, nothing in rails docs saying that you will get average metric value instead of total.

Also, it will be nice to have this change in stable branch too.

@y8 y8 Performance testing: add total value of metric
In most cases, when you benchmarking something you are interested not only in average metric value, but in total metric value.

This change adds total value, along with average. 

By the way: atm, nothing in rails docs saying that you will get average metric value instead of total.
bbd1d1f
@carlosantoniodasilva

Don't this break any test? And aldo, can you add tests for this? Thanks.

@carlosantoniodasilva

@y8 hey, do you have any news on this? Otherwise I'll consider it closed, thanks!

@y8
y8 commented Aug 22, 2012

Ooops, sorry, missed your comment.

What kind of tests do you want to see? Actually, I don't see any examples for things like this in active support. Any advices/best practices/etc?

@y8
y8 commented Nov 2, 2012

Anyone? :)

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Nov 2, 2012
activesupport/lib/active_support/testing/performance.rb
@@ -102,7 +102,7 @@ def initialize(harness, metric)
def report
if @supported
rate = @total / full_profile_options[:runs]
- '%20s: %s' % [@metric.name, @metric.format(rate)]
+ '%20s: %s (avg: %s)' % [@metric.name, @metric.format(@total), @metric.format(rate)]
@carlosantoniodasilva
carlosantoniodasilva Nov 2, 2012 Member

It's probably ok to add total, but changing the order both are being shown might seem weird. People relying on the first value will get confused, so perhaps adding total inside () would be better imo.

@y8
y8 commented Nov 3, 2012

Yep, showing average by default as it was before this patch is better. Changed the order. Any other suggestions? :)

@carlosantoniodasilva

Thanks :). It'd be awesome if you could test it somehow, although I'm not sure there's any related test somewhere in the code. But more important than that, a changelog entry would be nice.

@y8
y8 commented Nov 3, 2012

I'm not sure what the best way to test this stuff: unit test or integration test which parse rake test:performance output (with changing values of time results).

Btw, before adding changelog entry, should I merge current master into patch-1 branch or it's preferred to rebase against fresh master?

@carlosantoniodasilva

I believe a unit test should be good enough in this case. About rebase vs merge, we've been using rebase on most pull requests, should be fine with it. Thanks!

@steveklabnik
Member

@y8 any chance to add this test?

@senny
Member
senny commented Jan 14, 2013

As of Rails 4 the performance tests live in a separate gem rails-perftest. Since this is a new feature it does not target 3.2.x and therefore should be moved to the new repository.

I'm closing this for now but as mentioned feel free to submit the work as a new PR in rails/rails-perftest.

@senny senny closed this Jan 14, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment