Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Decoupling ActiveSupport from ActionView #6526

Merged
merged 1 commit into from

4 participants

@amutz

Hi,

I'm interested in using the ActiveSupport::Testing::Performance module and was surprised to see that it requires and calls code from ActionView. I'd like to use this module in a context without ActionView, and now that we've moved NumberHelpers to ActiveSupport (#6315), we can do this.

In this pull request I'm changing ActiveSupport::Testing::Performance to use ActiveSupport::NumberHelper instead of ActionView::Helpers::NumberHelper. Also, to my knowledge this logic was untested, so I have added tests for the formatting logic (which pass before and after the code change).

Any and all feedback is welcome.

Thanks!
-Andrew.

activesupport/test/testing/performance_test.rb
@@ -0,0 +1,41 @@
+require 'abstract_unit'
+require 'active_support/testing/performance'
+
+module ActiveSupport
+ module Testing
+ module Performance
+ class PerformanceTest < ActiveSupport::TestCase

I think there's no need for the Performance module, wdyt?

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

Other than that, looks :+1: to me, thanks!

@amutz

I needed some module because I didn't want the class name PerformanceTest polluting things globally. I chose to put it in the module ActiveSupport::Testing::Performance, just because that's also the name of the module whose logic it is testing.

It works just as well without it however, i'll update the PR to remove the Performance module and squash it.

Thanks for the feedback!

@carlosantoniodasilva

Sure, your idea is definitely a good logic to follow, I agree about having some module namespacing in such scenarios. I just thought that having both Performance and PerformanceTest under the same level would be enough and unlikely to collide with anything else. Thanks!

@amutz

Ok, I have removed the extra module and squashed it. Let me know if you'd like any other changes. :)

activesupport/test/testing/performance_test.rb
((9 lines not shown))
+ assert_equal "0", amount_metric.format(0)
+ assert_equal "1", amount_metric.format(1.23)
+ assert_equal "40,000,000", amount_metric.format(40000000)
+ end
+
+ def test_amount_time
+ time_metric = ActiveSupport::Testing::Performance::Metrics[:time].new
+ assert_equal "0 ms", time_metric.format(0)
+ assert_equal "40 ms", time_metric.format(0.04)
+ assert_equal "40 ms", time_metric.format(0.0415)
+ assert_equal "1.23 sec", time_metric.format(1.23)
+ assert_equal "40000.00 sec", time_metric.format(40000)
+ assert_equal "-5000 ms", time_metric.format(-5)
+ end
+
+ def test_amount_time

I just noticed both tests are named the same test_amount_time :)

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

Also, it seems there's a missing require (totally unrelated, but lets take the opportunity): active_support/core_ext/module/delegation. Can you add it, please?

I get this when running that single file:

/active_support/testing/performance.rb:96:in `<class:Performer>': undefined method `delegate' for ActiveSupport::Testing::Performance::Performer:Class (NoMethodError)`

Thanks.

@amutz

Great catch on the methods names! I've fixed them.

I've also added the missing require and the file loads fine now individually.

Let me know if you'd like any other changes. Thanks!

@carlosantoniodasilva

Looking great, thank you.

@carlosantoniodasilva carlosantoniodasilva merged commit d4a7dee into from
@josevalim
Owner

This is awesome! :+1: :+1: :+1: :+1:

@tenderlove
Owner

ruby-prof isn't available on every version of Ruby, so sometimes we don't include it in the Gemfile. The commit makes the AS tests complete halt on Ruby 2.0. :cry:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 29, 2012
  1. @amutz
This page is out of date. Refresh to see the latest.
View
8 activesupport/lib/active_support/testing/performance.rb
@@ -3,7 +3,8 @@
require 'active_support/concern'
require 'active_support/core_ext/class/delegating_attributes'
require 'active_support/core_ext/string/inflections'
-require 'action_view/helpers/number_helper'
+require 'active_support/core_ext/module/delegation'
+require 'active_support/number_helper'
module ActiveSupport
module Testing
@@ -195,8 +196,7 @@ def self.[](name)
end
class Base
- include ActionView::Helpers::NumberHelper
- include ActionView::Helpers::OutputSafetyHelper
+ include ActiveSupport::NumberHelper
attr_reader :total
@@ -240,7 +240,7 @@ def format(measurement)
class Amount < Base
def format(measurement)
- number_with_delimiter(measurement.floor)
+ number_to_delimited(measurement.floor)
end
end
View
40 activesupport/test/testing/performance_test.rb
@@ -0,0 +1,40 @@
+require 'abstract_unit'
+require 'active_support/testing/performance'
+
+
+module ActiveSupport
+ module Testing
+ class PerformanceTest < ActiveSupport::TestCase
+ def test_amount_format
+ amount_metric = ActiveSupport::Testing::Performance::Metrics[:amount].new
+ assert_equal "0", amount_metric.format(0)
+ assert_equal "1", amount_metric.format(1.23)
+ assert_equal "40,000,000", amount_metric.format(40000000)
+ end
+
+ def test_time_format
+ time_metric = ActiveSupport::Testing::Performance::Metrics[:time].new
+ assert_equal "0 ms", time_metric.format(0)
+ assert_equal "40 ms", time_metric.format(0.04)
+ assert_equal "41 ms", time_metric.format(0.0415)
+ assert_equal "1.23 sec", time_metric.format(1.23)
+ assert_equal "40000.00 sec", time_metric.format(40000)
+ assert_equal "-5000 ms", time_metric.format(-5)
+ end
+
+ def test_space_format
+ space_metric = ActiveSupport::Testing::Performance::Metrics[:digital_information_unit].new
+ assert_equal "0 Bytes", space_metric.format(0)
+ assert_equal "0 Bytes", space_metric.format(0.4)
+ assert_equal "1 Byte", space_metric.format(1.23)
+ assert_equal "123 Bytes", space_metric.format(123)
+ assert_equal "123 Bytes", space_metric.format(123.45)
+ assert_equal "12 KB", space_metric.format(12345)
+ assert_equal "1.2 MB", space_metric.format(1234567)
+ assert_equal "9.3 GB", space_metric.format(10**10)
+ assert_equal "91 TB", space_metric.format(10**14)
+ assert_equal "910000 TB", space_metric.format(10**18)
+ end
+ end
+ end
+end
Something went wrong with that request. Please try again.