Please review pull request #497: Bug/2.7.x/active record test failures with 3.2.1 opened by (daniel-pittman)
Description:
This makes Puppet compatibly with ActiveRecord 3.2 releases, at least in the tests. I think it fixes most of the problems people have seen in the world, but I can't be sure, as I can't reproduce all of them. So, this at least moves us closer to being able to figure that out.
- Opened: Tue Feb 14 23:39:58 UTC 2012
- Based on: puppetlabs:2.7.x (b9655fea47c5eefaa6f4768b750e634aeb063aca)
- Requested merge: daniel-pittman:bug/2.7.x/active-record-test-failures-with-3.2.1 (600a6e7f1e8d5c2e7df6ac8df86adb43f386f7f8)
Diff follows:
diff --git a/lib/puppet/rails/inventory_node.rb b/lib/puppet/rails/inventory_node.rb
index da7e610..8d6266e 100644
--- a/lib/puppet/rails/inventory_node.rb
+++ b/lib/puppet/rails/inventory_node.rb
@@ -3,19 +3,19 @@
class Puppet::Rails::InventoryNode < ::ActiveRecord::Base
has_many :facts, :class_name => "Puppet::Rails::InventoryFact", :foreign_key => :node_id, :dependent => :delete_all
- if Puppet::Util.activerecord_version >= 3.0
- # Prevents "DEPRECATION WARNING: Base.named_scope has been deprecated, please use Base.scope instead"
- ActiveRecord::NamedScope::ClassMethods.module_eval { alias :named_scope :scope }
+ if Puppet::Util.activerecord_version < 3.0
+ # For backward compatibility, add the newer name to older implementations.
+ ActiveRecord::NamedScope::ClassMethods.module_eval { alias :scope :named_scope }
end
- named_scope :has_fact_with_value, lambda { |name,value|
+ scope :has_fact_with_value, lambda { |name,value|
{
:conditions => ["inventory_facts.name = ? AND inventory_facts.value = ?", name, value],
:joins => :facts
}
}
- named_scope :has_fact_without_value, lambda { |name,value|
+ scope :has_fact_without_value, lambda { |name,value|
{
:conditions => ["inventory_facts.name = ? AND inventory_facts.value != ?", name, value],
:joins => :facts
diff --git a/spec/lib/puppet_spec/database.rb b/spec/lib/puppet_spec/database.rb
new file mode 100644
index 0000000..2f7c209
--- /dev/null
+++ b/spec/lib/puppet_spec/database.rb
@@ -0,0 +1,30 @@
+# This just makes some nice things available at global scope, and for setup of
+# tests to use a real fake database, rather than a fake stubs-that-don't-work
+# version of the same. Fun times.
+def sqlite?
+ if $sqlite.nil?
+ begin
+ require 'sqlite3'
+ $sqlite = true
+ rescue LoadError
+ $sqlite = false
+ end
+ end
+ $sqlite
+end
+
+def can_use_scratch_database?
+ sqlite? and Puppet.features.rails?
+end
+
+
+# This is expected to be called in your `before :each` block, and will get you
+# ready to roll with a serious database and all. Cleanup is handled
+# automatically for you. Nothing to do there.
+def setup_scratch_database
+ dir = PuppetSpec::Files.tmpdir('puppet-sqlite')
+ Puppet[:dbadapter] = 'sqlite3'
+ Puppet[:dblocation] = (dir + 'storeconfigs.sqlite').to_s
+ Puppet[:railslog] = '/dev/null'
+ Puppet::Rails.init
+end
diff --git a/spec/lib/puppet_spec/files.rb b/spec/lib/puppet_spec/files.rb
index 49e50a0..5608454 100755
--- a/spec/lib/puppet_spec/files.rb
+++ b/spec/lib/puppet_spec/files.rb
@@ -35,7 +35,8 @@ def make_absolute(path)
path
end
- def tmpfile(name)
+ def tmpfile(name) PuppetSpec::Files.tmpfile(name) end
+ def self.tmpfile(name)
# Generate a temporary file, just for the name...
source = Tempfile.new(name)
path = source.path
@@ -49,7 +50,8 @@ def tmpfile(name)
path
end
- def tmpdir(name)
+ def tmpdir(name) PuppetSpec::Files.tmpdir(name) end
+ def self.tmpdir(name)
path = tmpfile(name)
FileUtils.mkdir_p(path)
path
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index d5beddb..d00590a 100755
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -21,6 +21,7 @@ module PuppetSpec
require 'puppet_spec/files'
require 'puppet_spec/fixtures'
require 'puppet_spec/matchers'
+require 'puppet_spec/database'
require 'monkey_patches/alias_should_to_must'
require 'monkey_patches/publicize_methods'
diff --git a/spec/unit/indirector/catalog/active_record_spec.rb b/spec/unit/indirector/catalog/active_record_spec.rb
index 35d0117..242f30d 100755
--- a/spec/unit/indirector/catalog/active_record_spec.rb
+++ b/spec/unit/indirector/catalog/active_record_spec.rb
@@ -1,35 +1,21 @@
#!/usr/bin/env rspec
require 'spec_helper'
+describe "Puppet::Resource::Catalog::ActiveRecord", :if => can_use_scratch_database? do
+ include PuppetSpec::Files
-describe "Puppet::Resource::Catalog::ActiveRecord", :if => Puppet.features.rails? do
require 'puppet/rails'
- before :all do
- class Tableless < ActiveRecord::Base
- def self.columns
- @columns ||= []
- end
- def self.column(name, sql_type=nil, default=nil, null=true)
- columns << ActiveRecord::ConnectionAdapters::Column.new(name.to_s, default, sql_type.to_s, null)
- end
- end
-
- class Host < Tableless
- column :name, :string, :null => false
- column :ip, :string
- column :environment, :string
- column :last_compile, :datetime
- end
+ before :each do
+ require 'puppet/indirector/catalog/active_record'
+ setup_scratch_database
end
- before do
- require 'puppet/indirector/catalog/active_record'
- Puppet.features.stubs(:rails?).returns true
- Puppet::Rails.stubs(:init)
- @terminus = Puppet::Resource::Catalog::ActiveRecord.new
+ let :terminus do
+ Puppet::Resource::Catalog::ActiveRecord.new
end
+
it "should be a subclass of the ActiveRecord terminus class" do
Puppet::Resource::Catalog::ActiveRecord.ancestors.should be_include(Puppet::Indirector::ActiveRecord)
end
@@ -39,35 +25,35 @@ class Host < Tableless
end
describe "when finding an instance" do
- before do
- @request = stub 'request', :key => "foo", :options => {:cache_integration_hack => true}
+ let :request do
+ stub 'request', :key => "foo", :options => {:cache_integration_hack => true}
end
# This hack is here because we don't want to look in the db unless we actually want
# to look in the db, but our indirection architecture in 0.24.x isn't flexible
# enough to tune that via configuration.
it "should return nil unless ':cache_integration_hack' is set to true" do
- @request.options[:cache_integration_hack] = false
+ request.options[:cache_integration_hack] = false
Puppet::Rails::Host.expects(:find_by_name).never
- @terminus.find(@request).should be_nil
+ terminus.find(request).should be_nil
end
it "should use the Hosts ActiveRecord class to find the host" do
Puppet::Rails::Host.expects(:find_by_name).with { |key, args| key == "foo" }
- @terminus.find(@request)
+ terminus.find(request)
end
it "should return nil if no host instance can be found" do
Puppet::Rails::Host.expects(:find_by_name).returns nil
- @terminus.find(@request).should be_nil
+ terminus.find(request).should be_nil
end
it "should return a catalog with the same name as the host if the host can be found" do
host = stub 'host', :name => "foo", :resources => []
Puppet::Rails::Host.expects(:find_by_name).returns host
- result = @terminus.find(@request)
+ result = terminus.find(request)
result.should be_instance_of(Puppet::Resource::Catalog)
result.name.should == "foo"
end
@@ -87,70 +73,66 @@ class Host < Tableless
catalog.expects(:add_resource).with "trans_res1"
catalog.expects(:add_resource).with "trans_res2"
- @terminus.find(@request)
+ terminus.find(request)
end
end
describe "when saving an instance" do
- before do
- @host = Host.new(:name => "foo")
- @host.stubs(:merge_resources)
- @host.stubs(:save)
- @host.stubs(:railsmark).yields
-
- @node = Puppet::Node.new("foo", :environment => "environment")
- Puppet::Node.indirection.stubs(:find).with("foo").returns(@node)
-
- Puppet::Rails::Host.stubs(:find_by_name).returns @host
- @catalog = Puppet::Resource::Catalog.new("foo")
- @request = Puppet::Indirector::Request.new(:active_record, :save, @catalog)
+ let :catalog do Puppet::Resource::Catalog.new("foo") end
+ let :request do Puppet::Indirector::Request.new(:active_record, :save, catalog) end
+ let :node do Puppet::Node.new("foo", :environment => "environment") end
+
+ before :each do
+ Puppet::Node.indirection.stubs(:find).with("foo").returns(node)
end
it "should find the Rails host with the same name" do
- Puppet::Rails::Host.expects(:find_by_name).with("foo").returns @host
-
- @terminus.save(@request)
+ Puppet::Rails::Host.expects(:find_by_name).with("foo")
+ terminus.save(request)
end
it "should create a new Rails host if none can be found" do
- Puppet::Rails::Host.expects(:find_by_name).with("foo").returns nil
-
- Puppet::Rails::Host.expects(:create).with(:name => "foo").returns @host
-
- @terminus.save(@request)
+ Puppet::Rails::Host.find_by_name('foo').should be_nil
+ terminus.save(request)
+ Puppet::Rails::Host.find_by_name('foo').should be_valid
end
it "should set the catalog vertices as resources on the Rails host instance" do
- @catalog.expects(:vertices).returns "foo"
- @host.expects(:merge_resources).with("foo")
+ # We need to stub this so we get the same object, not just the same
+ # content, otherwise the expect can't fire. :(
+ host = Puppet::Rails::Host.create!(:name => "foo")
+ Puppet::Rails::Host.expects(:find_by_name).with("foo").returns(host)
+ catalog.expects(:vertices).returns("foo")
+ host.expects(:merge_resources).with("foo")
- @terminus.save(@request)
+ terminus.save(request)
end
it "should set host ip if we could find a matching node" do
- @node.stubs(:parameters).returns({"ipaddress" => "192.168.0.1"})
-
- @terminus.save(@request)
- @host.ip.should == '192.168.0.1'
+ node.merge("ipaddress" => "192.168.0.1")
+ terminus.save(request)
+ Puppet::Rails::Host.find_by_name("foo").ip.should == '192.168.0.1'
end
it "should set host environment if we could find a matching node" do
- @terminus.save(@request)
- @host.environment.should == "environment"
+ terminus.save(request)
+ Puppet::Rails::Host.find_by_name("foo").environment.should == "environment"
end
it "should set the last compile time on the host" do
now = Time.now
- Time.expects(:now).returns now
+ Time.stubs(:now).returns now
- @terminus.save(@request)
- @host.last_compile.should == now
+ terminus.save(request)
+ Puppet::Rails::Host.find_by_name("foo").last_compile.should == now
end
it "should save the Rails host instance" do
- @host.expects(:save)
+ host = Puppet::Rails::Host.create!(:name => "foo")
+ Puppet::Rails::Host.expects(:find_by_name).with("foo").returns(host)
+ host.expects(:save)
- @terminus.save(@request)
+ terminus.save(request)
end
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 b14262b..a79f537 100755
--- a/spec/unit/indirector/facts/inventory_active_record_spec.rb
+++ b/spec/unit/indirector/facts/inventory_active_record_spec.rb
@@ -1,24 +1,18 @@
#!/usr/bin/env rspec
require 'spec_helper'
-begin
- require 'sqlite3'
-rescue LoadError
-end
-require 'tempfile'
require 'puppet/rails'
-describe "Puppet::Node::Facts::InventoryActiveRecord", :if => (Puppet.features.rails? and defined? SQLite3) do
+describe "Puppet::Node::Facts::InventoryActiveRecord", :if => can_use_scratch_database? do
+ include PuppetSpec::Files
+
let(:terminus) { Puppet::Node::Facts::InventoryActiveRecord.new }
before :all do
require 'puppet/indirector/facts/inventory_active_record'
- @dbfile = Tempfile.new("testdb")
- @dbfile.close
end
- after :all do
+ after :each do
Puppet::Node::Facts.indirection.reset_terminus_class
- @dbfile.unlink
end
before :each do
@@ -26,14 +20,7 @@
Puppet::Node.indirection.cache_class = nil
Puppet::Node::Facts.indirection.terminus_class = :inventory_active_record
- Puppet[:dbadapter] = 'sqlite3'
- Puppet[:dblocation] = @dbfile.path
- Puppet[:railslog] = "/dev/null"
- Puppet::Rails.init
- end
-
- after :each do
- Puppet::Rails.teardown
+ setup_scratch_database
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 4c00976..e9ccea3 100755
--- a/spec/unit/indirector/resource/active_record_spec.rb
+++ b/spec/unit/indirector/resource/active_record_spec.rb
@@ -1,22 +1,13 @@
#!/usr/bin/env rspec
require 'spec_helper'
-
-begin
- require 'sqlite3'
-rescue LoadError
-end
-
require 'puppet/rails'
require 'puppet/node/facts'
-describe "Puppet::Resource::ActiveRecord", :if => (Puppet.features.rails? and defined? SQLite3) do
+describe "Puppet::Resource::ActiveRecord", :if => can_use_scratch_database? do
include PuppetSpec::Files
before :each do
- dir = Pathname(tmpdir('puppet-var'))
- Puppet[:vardir] = dir.to_s
- Puppet[:dbadapter] = 'sqlite3'
- Puppet[:dblocation] = (dir + 'storeconfigs.sqlite').to_s
+ setup_scratch_database
Puppet[:storeconfigs] = true
end
diff --git a/spec/unit/parser/collector_spec.rb b/spec/unit/parser/collector_spec.rb
index 79a6a17..251fec7 100755
--- a/spec/unit/parser/collector_spec.rb
+++ b/spec/unit/parser/collector_spec.rb
@@ -1,11 +1,5 @@
#!/usr/bin/env rspec
require 'spec_helper'
-
-begin
- require 'sqlite3'
-rescue LoadError
-end
-
require 'puppet/rails'
require 'puppet/parser/collector'
@@ -267,7 +261,7 @@
end
end
-describe Puppet::Parser::Collector, "when collecting exported resources", :if => (Puppet.features.rails? and defined? SQLite3) do
+describe Puppet::Parser::Collector, "when collecting exported resources", :if => can_use_scratch_database? do
include PuppetSpec::Files
before do
@@ -287,14 +281,10 @@
context "with storeconfigs enabled" do
before :each do
- dir = Pathname(tmpdir('puppet-var'))
- Puppet[:vardir] = dir.to_s
- Puppet[:dbadapter] = 'sqlite3'
- Puppet[:dblocation] = (dir + 'storeconfigs.sqlite').to_s
+ setup_scratch_database
Puppet[:storeconfigs] = true
Puppet[:environment] = "production"
Puppet[:storeconfigs_backend] = "active_record"
- Puppet::Rails.init
end
it "should return all matching resources from the current compile and mark them non-virtual and non-exported" do
-- 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.
