Please review pull request #406: Bug/2.7.x/6771 always close activerecord connections in tests opened by (daniel-pittman)

Description:

This makes sure we don't leak state around open ActiveRecord connections between tests.

It also makes some minor cleanups, including one ordering dependent test.

  • Opened: Wed Jan 25 21:25:13 UTC 2012
  • Based on: puppetlabs:2.7.x (b9353392eb9b94db80cf329daddfb5fb49f4a9ea)
  • Requested merge: daniel-pittman:bug/2.7.x/6771-always-close-activerecord-connections-in-tests (3892875a5e97180a0d71278faed51d85912456b5)

Diff follows:

diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index e0227be..9b61d35 100755
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -34,6 +34,9 @@ module PuppetSpec
   config.mock_with :mocha
 
   config.before :each do
+    # Disabling garbage collection inside each test, and only running it at
+    # the end of each block, gives us an ~ 15 percent speedup, and more on
+    # some platforms *cough* windows *cough* that are a little slower.
     GC.disable
 
     # We need to preserve the current state of all our indirection cache and
@@ -102,6 +105,14 @@ module PuppetSpec
     end
     $saved_indirection_state = nil
 
+    # Some tests can cause us to connect, in which case the lingering
+    # connection is a resource that can cause unexpected failure in later
+    # tests, as well as sharing state accidentally.
+    Puppet.features.rails? and ActiveRecord::Base.remove_connection
+
+    # This will perform a GC between tests, but only if actually required.  We
+    # experimented with forcing a GC run, and that was less efficient than
+    # just letting it run all the time.
     GC.enable
   end
 end
diff --git a/spec/unit/indirector/facts/inventory_active_record_spec.rb b/spec/unit/indirector/facts/inventory_active_record_spec.rb
index 43dcc7c..b14262b 100755
--- a/spec/unit/indirector/facts/inventory_active_record_spec.rb
+++ b/spec/unit/indirector/facts/inventory_active_record_spec.rb
@@ -34,7 +34,6 @@
 
   after :each do
     Puppet::Rails.teardown
-    ActiveRecord::Base.remove_connection
   end
 
   describe "#save" do
diff --git a/spec/unit/indirector/resource/active_record_spec.rb b/spec/unit/indirector/resource/active_record_spec.rb
index c923f79..4c00976 100755
--- a/spec/unit/indirector/resource/active_record_spec.rb
+++ b/spec/unit/indirector/resource/active_record_spec.rb
@@ -20,10 +20,6 @@
     Puppet[:storeconfigs] = true
   end
 
-  after :each do
-    ActiveRecord::Base.remove_connection
-  end
-
   subject {
     require 'puppet/indirector/resource/active_record'
     Puppet::Resource.indirection.terminus(:active_record)
diff --git a/spec/unit/parser/collector_spec.rb b/spec/unit/parser/collector_spec.rb
index c651ab8..633254b 100755
--- a/spec/unit/parser/collector_spec.rb
+++ b/spec/unit/parser/collector_spec.rb
@@ -287,22 +287,16 @@
 
   context "with storeconfigs enabled" do
     before :each do
-      ActiveRecord::Base.remove_connection
-
       dir = Pathname(tmpdir('puppet-var'))
       Puppet[:vardir]       = dir.to_s
       Puppet[:dbadapter]    = 'sqlite3'
       Puppet[:dblocation]   = (dir + 'storeconfigs.sqlite').to_s
       Puppet[:storeconfigs] = true
       Puppet[:environment]  = "production"
-      Puppet[:storeconfigs_backend] = :active_record
+      Puppet[:storeconfigs_backend] = "active_record"
       Puppet::Rails.init
     end
 
-    after :each do
-      ActiveRecord::Base.remove_connection
-    end
-
     it "should return all matching resources from the current compile and mark them non-virtual and non-exported" do
       _one_ = Puppet::Parser::Resource.new('notify', 'one',
                                          :virtual  => true,
diff --git a/spec/unit/provider/confine/feature_spec.rb b/spec/unit/provider/confine/feature_spec.rb
index 959c7a3..04040dd 100755
--- a/spec/unit/provider/confine/feature_spec.rb
+++ b/spec/unit/provider/confine/feature_spec.rb
@@ -18,24 +18,22 @@
 
   describe "when testing values" do
     before do
-      @features = mock 'features'
-      Puppet.stubs(:features).returns @features
       @confine = Puppet::Provider::Confine::Feature.new("myfeature")
       @confine.label = "eh"
     end
 
     it "should use the Puppet features instance to test validity" do
-      @features.expects(:myfeature?)
+      Puppet.features.expects(:myfeature?)
       @confine.valid?
     end
 
     it "should return true if the feature is present" do
-      @features.expects(:myfeature?).returns true
+      Puppet.features.add(:myfeature) do true end
       @confine.pass?("myfeature").should be_true
     end
 
     it "should return false if the value is false" do
-      @features.expects(:myfeature?).returns false
+      Puppet.features.add(:myfeature) do false end
       @confine.pass?("myfeature").should be_false
     end
 

    

--
You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to [email protected].
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.

Reply via email to