Permalink
Browse files

scrub instance variables from test cases on teardown

this prevents test state from accumulating, resulting in leaked
objects and slow tests due to overactive GC.
  • Loading branch information...
jamis committed Jan 19, 2011
1 parent 7938039 commit 79a06225ef0e148eb5da541711f74163b5efb18d
@@ -6,6 +6,7 @@
require 'active_support/testing/pending'
require 'active_support/testing/isolation'
require 'active_support/core_ext/kernel/reporting'
+require 'active_support/testing/garbage_collection'
begin
silence_warnings { require 'mocha' }
@@ -37,5 +38,6 @@ def self.for_tag(tag)
include ActiveSupport::Testing::Deprecation
include ActiveSupport::Testing::Pending
extend ActiveSupport::Testing::Declarative
+ include ActiveSupport::Testing::GarbageCollection
end
end
@@ -0,0 +1,19 @@
+module ActiveSupport
+ module Testing
+ module GarbageCollection
+ def self.included(base)
+ base.teardown :scrub_leftover_instance_variables
+ end
+
+ private
+
+ RESERVED_INSTANCE_VARIABLES = %w(@test_passed @passed @method_name @__name__ @_result).map(&:to_sym)

This comment has been minimized.

Show comment Hide comment
@tenderlove

tenderlove Jan 19, 2011

Owner

This list must be changed every time minitest changes. In fact, this list is inaccurate for 1.9.3 trunk and anyone on 1.9.2 who upgrades minitest.

Can we write this in such a way that if minitest is updated, we don't need to update this list? Or remove this from Rails and make it a plugin so that it can be release out of band from Rails?

This list is missing the @__io__ variable found here.

@tenderlove

tenderlove Jan 19, 2011

Owner

This list must be changed every time minitest changes. In fact, this list is inaccurate for 1.9.3 trunk and anyone on 1.9.2 who upgrades minitest.

Can we write this in such a way that if minitest is updated, we don't need to update this list? Or remove this from Rails and make it a plugin so that it can be release out of band from Rails?

This list is missing the @__io__ variable found here.

This comment has been minimized.

Show comment Hide comment
@tenderlove

tenderlove Jan 19, 2011

Owner

I made this to illustrate:

doh

Also, it looks like minitest does not suffer the same deficiency as test/unit. If you look at the minitest run loop, you'll see that it doesn't hang on to test instances. It only holds on to the pass / fail statistics. It leads me to believe this will not do anything on minitest.

@tenderlove

tenderlove Jan 19, 2011

Owner

I made this to illustrate:

doh

Also, it looks like minitest does not suffer the same deficiency as test/unit. If you look at the minitest run loop, you'll see that it doesn't hang on to test instances. It only holds on to the pass / fail statistics. It leads me to believe this will not do anything on minitest.

This comment has been minimized.

Show comment Hide comment
@parndt

parndt Jan 19, 2011

Contributor

Best illustration of the reason for changing something that I've seen in a long, long time.

@parndt

parndt Jan 19, 2011

Contributor

Best illustration of the reason for changing something that I've seen in a long, long time.

This comment has been minimized.

Show comment Hide comment
@jamis

jamis Jan 19, 2011

Contributor

The sad bit is that we're working around a memory leak in both minitest and test::unit with this. How often is minitest updated? And what is @io used for? It appears to only be used immediately after the test is run, and before teardown is invoked, so there may not be any reason for it to be in our list here. (This list only needs to exclude variables that might conceivably be queried after the teardown has been invoked.)

Yeah, having an explicit list is fragile. If a way exists to avoid explicitly listing ivars, I'm in favor. Maybe we take a snapshot immediately before the setup method is invoked, and then remove the difference?

@jamis

jamis Jan 19, 2011

Contributor

The sad bit is that we're working around a memory leak in both minitest and test::unit with this. How often is minitest updated? And what is @io used for? It appears to only be used immediately after the test is run, and before teardown is invoked, so there may not be any reason for it to be in our list here. (This list only needs to exclude variables that might conceivably be queried after the teardown has been invoked.)

Yeah, having an explicit list is fragile. If a way exists to avoid explicitly listing ivars, I'm in favor. Maybe we take a snapshot immediately before the setup method is invoked, and then remove the difference?

This comment has been minimized.

Show comment Hide comment
@tenderlove

tenderlove Jan 19, 2011

Owner

@jamis, I don't believe this "memory leak" exists in minitest. Please look at the minitest run loop and you'll see that it doesn't hold on to test instances, but rather to the resulting statistics.

@tenderlove

tenderlove Jan 19, 2011

Owner

@jamis, I don't believe this "memory leak" exists in minitest. Please look at the minitest run loop and you'll see that it doesn't hold on to test instances, but rather to the resulting statistics.

This comment has been minimized.

Show comment Hide comment
@jonleighton

jonleighton Jan 19, 2011

Member

Hi folks - I agree with Aaron here. I don't get any speed up from this patch on 1.9.2 (minitest). However I do get a small speed-up from the GC deferment change (from 45 secs to 40/41 secs with rake test_sqlite3_mem on active record).

It would be good to package this as a gem I think (to benefit non-rails people) and only require it if using test/unit.

@jonleighton

jonleighton Jan 19, 2011

Member

Hi folks - I agree with Aaron here. I don't get any speed up from this patch on 1.9.2 (minitest). However I do get a small speed-up from the GC deferment change (from 45 secs to 40/41 secs with rake test_sqlite3_mem on active record).

It would be good to package this as a gem I think (to benefit non-rails people) and only require it if using test/unit.

This comment has been minimized.

Show comment Hide comment
@tenderlove

tenderlove Jan 19, 2011

Owner

I think we should:

  1. Revert this
  2. Send patches up stream to test/unit that fix it for 1.8.x
  3. Write a plugin so that people who are not on 1.9 can opt-in to this functionality before 1.8.x is fixed

I am very -1 on adding these hacks. It's brittle, it adds extra overhead on teardown when it isn't necessary (minitest), and it doesn't fix the problem for anyone outside of rails.

@tenderlove

tenderlove Jan 19, 2011

Owner

I think we should:

  1. Revert this
  2. Send patches up stream to test/unit that fix it for 1.8.x
  3. Write a plugin so that people who are not on 1.9 can opt-in to this functionality before 1.8.x is fixed

I am very -1 on adding these hacks. It's brittle, it adds extra overhead on teardown when it isn't necessary (minitest), and it doesn't fix the problem for anyone outside of rails.

This comment has been minimized.

Show comment Hide comment
@jfirebaugh

jfirebaugh Jan 19, 2011

Contributor

This comment has been minimized.

Show comment Hide comment
@jamis

jamis Jan 19, 2011

Contributor

I've reverted this and the other change. Good arguments all around.

@jamis

jamis Jan 19, 2011

Contributor

I've reverted this and the other change. Good arguments all around.

This comment has been minimized.

Show comment Hide comment
@volkanunsal

volkanunsal Jan 20, 2011

Came from Reddit to upvote this. Can't seem to find the uparrow.

Okay.

@volkanunsal

volkanunsal Jan 20, 2011

Came from Reddit to upvote this. Can't seem to find the uparrow.

Okay.

+
+ def scrub_leftover_instance_variables
+ (instance_variables.map(&:to_sym) - RESERVED_INSTANCE_VARIABLES).each do |var|
+ remove_instance_variable(var)
+ end
+ end
+ end
+ end
+end
@@ -74,5 +74,23 @@ def test_true; assert true end
assert_match %r{oh noes}, exception.message
end
+
+ def test_teardown_should_scrub_instance_variables
+ tc = Class.new(TestCase) do
+ def test_true; @alpha = "a"; assert_equal "a", @alpha; end
+ end
+
+ test_name = 'test_true'
+ fr = FakeRunner.new
+
+ test = tc.new test_name
+ test.run(fr) {}
+
+ passed_var = IS_MINITEST ? :@passed : :@test_passed
+ ivars = test.instance_variables.map(&:to_sym)
+
+ assert ivars.include?(passed_var), "#{passed_var} should not have been scrubbed"
+ assert !ivars.include?(:@alpha), "@alpha should have been scrubbed"
+ end
end
end
@@ -137,7 +137,7 @@ class SetupAndTeardownTest < ActiveSupport::TestCase
def test_inherited_setup_callbacks
assert_equal [:reset_callback_record, :foo], self.class._setup_callbacks.map(&:raw_filter)
assert_equal [:foo], @called_back
- assert_equal [:foo, :sentinel, :foo], self.class._teardown_callbacks.map(&:raw_filter)
+ assert_equal [:scrub_leftover_instance_variables, :foo, :sentinel, :foo], self.class._teardown_callbacks.map(&:raw_filter)
end
def setup
@@ -169,7 +169,7 @@ class SubclassSetupAndTeardownTest < SetupAndTeardownTest
def test_inherited_setup_callbacks
assert_equal [:reset_callback_record, :foo, :bar], self.class._setup_callbacks.map(&:raw_filter)
assert_equal [:foo, :bar], @called_back
- assert_equal [:foo, :sentinel, :foo, :bar], self.class._teardown_callbacks.map(&:raw_filter)
+ assert_equal [:scrub_leftover_instance_variables, :foo, :sentinel, :foo, :bar], self.class._teardown_callbacks.map(&:raw_filter)
end
protected

0 comments on commit 79a0622

Please sign in to comment.