Please review pull request #647: FOR REVIEW ONLY: Refactor/2.7.x/13690 spec helper compatibility opened by (cprice-puppet)
Description:
... with external projects.
This basically entails:
- Introducing a new class Puppet::Test::TestHelper that will contain basic setup / teardown methods, which external projects can use to make sure that puppet is initialized properly during testing.
- Remove or abstract the Puppet::Util::Settings#initialize_for_tests / clear_for_tests methods in P::U::S
- Modify puppet core spec_helper.rb to use the new API.
- Opened: Mon Apr 09 19:57:15 UTC 2012
- Based on: puppetlabs:2.7.x (c8db033b5c881b0869a8f89bbbc0973035f3271a)
- Requested merge: cprice-puppet:refactor/2.7.x/13690-spec_helper_compatibility (62da550588eda618b39714e9169ada3867a864db)
Diff follows:
diff --git a/lib/puppet/test/test_helper.rb b/lib/puppet/test/test_helper.rb
new file mode 100644
index 0000000..5b01728
--- /dev/null
+++ b/lib/puppet/test/test_helper.rb
@@ -0,0 +1,140 @@
+module Puppet::Test
+ # This class is intended to provide an API to be used by external projects
+ # when they are running tests that depend on puppet core. This should
+ # allow us to vary the implementation details of managing puppet's state
+ # for testing, from one version of puppet to the next--without forcing
+ # the external projects to do any of that state management or be aware of
+ # the implementation details.
+ #
+ # For now, this consists of a few very simple signatures. The plan is
+ # that it should be the responsibility of the puppetlabs_spec_helper
+ # to broker between external projects and this API; thus, if any
+ # hacks are required (e.g. to determine whether or not a particular)
+ # version of puppet supports this API, those hacks will be consolidated in
+ # one place and won't need to be duplicated in every external project.
+ #
+ # This should also alleviate the anti-pattern that we've been following,
+ # wherein each external project starts off with a copy of puppet core's
+ # test_helper.rb and is exposed to risk of that code getting out of
+ # sync with core.
+ #
+ # Since this class will be "library code" that ships with puppet, it does
+ # not use API from any existing test framework such as rspec. This should
+ # theoretically allow it to be used with other unit test frameworks in the
+ # future, if desired.
+ #
+ # Note that in the future this API could potentially be expanded to handle
+ # other features such as "around_test", but we didn't see a compelling
+ # reason to deal with that right now.
+ class TestHelper
+ # Call this method once, when beginning a test run--prior to running
+ # any individual tests.
+ # @return nil
+ def self.before_all_tests()
+
+ end
+
+ # Call this method once, at the end of a test run, when no more tests
+ # will be run.
+ # @return nil
+ def self.after_all_tests()
+
+ end
+
+ # Call this method once per test, prior to execution of each invididual test.
+ # @return nil
+ def self.before_each_test()
+ # We need to preserve the current state of all our indirection cache and
+ # terminus classes. This is pretty important, because changes to these
+ # are global and lead to order dependencies in our testing.
+ #
+ # We go direct to the implementation because there is no safe, sane public
+ # API to manage restoration of these to their default values. This
+ # should, once the value is proved, be moved to a standard API on the
+ # indirector.
+ #
+ # To make things worse, a number of the tests stub parts of the
+ # indirector. These stubs have very specific expectations that what
+ # little of the public API we could use is, well, likely to explode
+ # randomly in some tests. So, direct access. --daniel 2011-08-30
+ $saved_indirection_state = {}
+ indirections = Puppet::Indirector::Indirection.send(:class_variable_get, :@@indirections)
+ indirections.each do |indirector|
+ $saved_indirection_state[indirector.name] = {
+ :@terminus_class => indirector.instance_variable_get(:@terminus_class),
+ :@cache_class => indirector.instance_variable_get(:@cache_class)
+ }
+ end
+
+ initialize_settings_before_each()
+
+ # Longer keys are secure, but they sure make for some slow testing - both
+ # in terms of generating keys, and in terms of anything the next step down
+ # the line doing validation or whatever. Most tests don't care how long
+ # or secure it is, just that it exists, so these are better and faster
+ # defaults, in testing only.
+ #
+ # I would make these even shorter, but OpenSSL doesn't support anything
+ # below 512 bits. Sad, really, because a 0 bit key would be just fine.
+ Puppet[:req_bits] = 512
+ Puppet[:keylength] = 512
+
+ end
+
+ # Call this method once per test, after execution of each individual test.
+ # @return nil
+ def self.after_each_test()
+ clear_settings_after_each()
+
+ Puppet::Node::Environment.clear
+ Puppet::Util::Storage.clear
+ Puppet::Util::ExecutionStub.reset
+
+ PuppetSpec::Files.cleanup
+
+ # Restore the indirector configuration. See before hook.
+ indirections = Puppet::Indirector::Indirection.send(:class_variable_get, :@@indirections)
+ indirections.each do |indirector|
+ $saved_indirection_state.fetch(indirector.name, {}).each do |variable, value|
+ indirector.instance_variable_set(variable, value)
+ end
+ 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.
+ # We're testing if ActiveRecord::Base is defined because some test cases
+ # may stub Puppet.features.rails? which is how we should normally
+ # introspect for this functionality.
+ ActiveRecord::Base.remove_connection if defined?(ActiveRecord::Base)
+
+ end
+
+
+ #########################################################################################
+ # PRIVATE METHODS (not part of the public TestHelper API--do not call these from outside
+ # of this class!)
+ #########################################################################################
+
+ def self.initialize_settings_before_each()
+ # these globals are set by Application
+ $puppet_application_mode = nil
+ $puppet_application_name = nil
+ # Set the confdir and vardir to gibberish so that tests
+ # have to be correctly mocked.
+ Puppet[:confdir] = "/dev/null"
+ Puppet[:vardir] = "/dev/null"
+
+ # Avoid opening ports to the outside world
+ Puppet[:bindaddress] = "127.0.0.1"
+ end
+ private_class_method :initialize_settings_before_each
+
+ def self.clear_settings_after_each()
+ Puppet.settings.clear
+ end
+ private_class_method :clear_settings_after_each
+ end
+end
\ No newline at end of file
diff --git a/lib/puppet/util/settings.rb b/lib/puppet/util/settings.rb
index e57cc8c..b0614e5 100644
--- a/lib/puppet/util/settings.rb
+++ b/lib/puppet/util/settings.rb
@@ -922,23 +922,5 @@ def set_metadata(meta)
end
end
- def initialize_everything_for_tests()
- # these globals are set by Application
- $puppet_application_mode = nil
- $puppet_application_name = nil
- # Set the confdir and vardir to gibberish so that tests
- # have to be correctly mocked.
- self[:confdir] = "/dev/null"
- self[:vardir] = "/dev/null"
-
- # Avoid opening ports to the outside world
- self[:bindaddress] = "127.0.0.1"
- end
- private :initialize_everything_for_tests
-
- def clear_everything_for_tests()
- self.clear
- end
- private :clear_everything_for_tests
end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index a21bb3a..f641cb4 100755
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -24,6 +24,7 @@ module PuppetSpec
require 'puppet_spec/database'
require 'monkey_patches/alias_should_to_must'
require 'monkey_patches/publicize_methods'
+require 'puppet/test/test_helper'
Pathname.glob("#{dir}/shared_behaviours/**/*.rb") do |behaviour|
require behaviour.relative_path_from(Pathname.new(dir))
@@ -34,89 +35,55 @@ module PuppetSpec
config.mock_with :mocha
+ config.before :all do
+ Puppet::Test::TestHelper.before_all_tests()
+ end
+
+ config.after :all do
+ Puppet::Test::TestHelper.after_all_tests()
+ end
+
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
- # terminus classes. This is pretty important, because changes to these
- # are global and lead to order dependencies in our testing.
- #
- # We go direct to the implementation because there is no safe, sane public
- # API to manage restoration of these to their default values. This
- # should, once the value is proved, be moved to a standard API on the
- # indirector.
- #
- # To make things worse, a number of the tests stub parts of the
- # indirector. These stubs have very specific expectations that what
- # little of the public API we could use is, well, likely to explode
- # randomly in some tests. So, direct access. --daniel 2011-08-30
- $saved_indirection_state = {}
- indirections = Puppet::Indirector::Indirection.send(:class_variable_get, :@@indirections)
- indirections.each do |indirector|
- $saved_indirection_state[indirector.name] = {
- :@terminus_class => indirector.instance_variable_get(:@terminus_class),
- :@cache_class => indirector.instance_variable_get(:@cache_class)
- }
- end
-
-
# REVISIT: I think this conceals other bad tests, but I don't have time to
# fully diagnose those right now. When you read this, please come tell me
# I suck for letting this float. --daniel 2011-04-21
Signal.stubs(:trap)
- Puppet.settings.send(:initialize_everything_for_tests)
- # Longer keys are secure, but they sure make for some slow testing - both
- # in terms of generating keys, and in terms of anything the next step down
- # the line doing validation or whatever. Most tests don't care how long
- # or secure it is, just that it exists, so these are better and faster
- # defaults, in testing only.
+ # TODO: in a saner world, we'd move this logging redirection into our TestHelper class.
+ # Without doing so, external projects will all have to roll their own solution for
+ # redirecting logging, and for validating expected log messages. However, because the
+ # current implementation of this involves creating an instance variable "@logs" on
+ # EVERY SINGLE TEST CLASS, and because there are over 1300 tests that are written to expect
+ # this instance variable to be available--we can't easily solve this problem right now.
#
- # I would make these even shorter, but OpenSSL doesn't support anything
- # below 512 bits. Sad, really, because a 0 bit key would be just fine.
- Puppet[:req_bits] = 512
- Puppet[:keylength] = 512
-
-
+ # redirecting logging away from console, because otherwise the test output will be
+ # obscured by all of the log output
@logs = []
Puppet::Util::Log.newdestination(Puppet::Test::LogCollector.new(@logs))
@log_level = Puppet::Util::Log.level
+
+ Puppet::Test::TestHelper.before_each_test()
+
end
config.after :each do
- Puppet.settings.send(:clear_everything_for_tests)
- Puppet::Node::Environment.clear
- Puppet::Util::Storage.clear
- Puppet::Util::ExecutionStub.reset
-
- PuppetSpec::Files.cleanup
+ Puppet::Test::TestHelper.after_each_test()
+ # TODO: this should be abstracted in the future--see comments above the '@logs' block in the
+ # "before" code above.
+ #
+ # clean up after the logging changes that we made before each test.
@logs.clear
Puppet::Util::Log.close_all
Puppet::Util::Log.level = @log_level
- # Restore the indirector configuration. See before hook.
- indirections = Puppet::Indirector::Indirection.send(:class_variable_get, :@@indirections)
- indirections.each do |indirector|
- $saved_indirection_state.fetch(indirector.name, {}).each do |variable, value|
- indirector.instance_variable_set(variable, value)
- end
- 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.
- # We're testing if ActiveRecord::Base is defined because some test cases
- # may stub Puppet.features.rails? which is how we should normally
- # introspect for this functionality.
- ActiveRecord::Base.remove_connection if defined?(ActiveRecord::Base)
-
# 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.
-- 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.
