From be552110b5d86cab2653cb7ba8b4c7a88999e8a9 Mon Sep 17 00:00:00 2001 From: Nathan McKinley Date: Thu, 2 Aug 2018 13:01:35 -0700 Subject: [PATCH 1/8] Functional generator of unit tests. --- provider/puppet.rb | 16 ++ provider/puppet/common~compile~before.yaml | 2 + provider/puppet/common~copy.yaml | 1 + templates/puppet/Gemfile | 7 +- templates/puppet/Rakefile | 1 + templates/puppet/fixtures.yml.erb | 24 +++ templates/puppet/integration_spec.rb | 194 +++++++++++++++++++++ templates/puppet/mock_healthcheck_fn.pp | 21 +++ templates/puppet/spec_helper.rb.erb | 48 +++-- 9 files changed, 286 insertions(+), 28 deletions(-) create mode 100644 templates/puppet/Rakefile create mode 100644 templates/puppet/fixtures.yml.erb create mode 100644 templates/puppet/integration_spec.rb create mode 100644 templates/puppet/mock_healthcheck_fn.pp diff --git a/provider/puppet.rb b/provider/puppet.rb index 828f1ed7a4cd..471812a05e82 100644 --- a/provider/puppet.rb +++ b/provider/puppet.rb @@ -190,6 +190,22 @@ def compile_examples(output_folder) "products/#{@api.prefix[1..-1]}/examples/puppet/#{file}"] end ) + + # Compile the spec tests which depend on the examples. + # If a create and delete example both exist, generate + # the spec file. Select all the objects which have + # both create and delete examples, named the standard way. + @api.objects.each do |obj| + ex = @config.examples[obj.name] + next if ex.nil? + o = Google::StringUtils.underscore(obj.name) + # We can only generate the spec file if both examples exist. + next unless ex.include?("#{o}.pp") && ex.include?("delete_#{o}.pp") + compile_file_list(output_folder, + [["spec/integration/#{o}/integration_spec.rb", + 'templates/puppet/integration_spec.rb']], + obj: obj) + end end def compile_end2end_tests(output_folder) diff --git a/provider/puppet/common~compile~before.yaml b/provider/puppet/common~compile~before.yaml index af0eff7edb59..71efbbe9c639 100644 --- a/provider/puppet/common~compile~before.yaml +++ b/provider/puppet/common~compile~before.yaml @@ -17,6 +17,7 @@ '.gitignore': 'templates/dot~gitignore' '.rubocop.yml': 'templates/dot~rubocop~root.yml' 'Gemfile': 'templates/puppet/Gemfile' +'.fixtures.yml': 'templates/puppet/fixtures.yml.erb' 'README.md': 'templates/puppet/README.md.erb' 'metadata.json': 'templates/puppet/metadata.json.erb' 'spec/.rubocop.yml': 'templates/dot~rubocop~spec.yml' @@ -25,3 +26,4 @@ 'spec/puppetlint_spec.rb': 'templates/puppet/puppetlint_spec.rb.erb' 'spec/spec_helper.rb': 'templates/puppet/spec_helper.rb.erb' 'spec/test_constants.rb': 'templates/test_constants.rb.erb' +'spec/fixtures/mock_hc_fn.pp': 'templates/puppet/mock_healthcheck_fn.pp' diff --git a/provider/puppet/common~copy.yaml b/provider/puppet/common~copy.yaml index ad4facb9b511..7b2b534ae6ce 100644 --- a/provider/puppet/common~copy.yaml +++ b/provider/puppet/common~copy.yaml @@ -26,6 +26,7 @@ end <% end # unless config.nil? -%> 'LICENSE': 'templates/LICENSE' 'Gemfile.lock': 'templates/puppet/Gemfile.lock' +'Rakefile' : 'templates/puppet/Rakefile' 'lib/google/hash_utils.rb': 'google/hash_utils.rb' 'lib/google/string_utils.rb': 'google/string_utils.rb' 'spec/copyright.rb': 'spec/copyright.rb' diff --git a/templates/puppet/Gemfile b/templates/puppet/Gemfile index 1dfcafa9df48..db58282a05c9 100644 --- a/templates/puppet/Gemfile +++ b/templates/puppet/Gemfile @@ -18,7 +18,10 @@ source 'https://rubygems.org' group :test do + gem 'google-api-client' + gem 'googleauth' gem 'metadata-json-lint' + gem 'parallel_tests' gem 'puppet', ENV['PUPPET_GEM_VERSION'] || '>= 4.2.0' gem 'puppet-lint' gem 'puppet-lint-unquoted_string-check' @@ -28,7 +31,9 @@ group :test do gem 'rspec' gem 'rspec-mocks' gem 'rspec-puppet' - gem 'parallel_tests' gem 'rubocop' + gem 'semantic_puppet' gem 'simplecov' + gem 'vcr' + gem 'webmock' end diff --git a/templates/puppet/Rakefile b/templates/puppet/Rakefile new file mode 100644 index 000000000000..cd3d37995894 --- /dev/null +++ b/templates/puppet/Rakefile @@ -0,0 +1 @@ +require 'puppetlabs_spec_helper/rake_tasks' diff --git a/templates/puppet/fixtures.yml.erb b/templates/puppet/fixtures.yml.erb new file mode 100644 index 000000000000..afeb935d6df7 --- /dev/null +++ b/templates/puppet/fixtures.yml.erb @@ -0,0 +1,24 @@ +--- +<% if false # the license inside this if block pertains to this file -%> +# Copyright 2017 Google Inc. +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +<% end -%> +<%= compile 'templates/license.erb' -%> + +<%= lines(autogen_notice :ruby) -%> + +fixtures: + forge_modules: + gauth: google/gauth + symlinks: + <%= @api.prefix -%>: "#{source_dir}" diff --git a/templates/puppet/integration_spec.rb b/templates/puppet/integration_spec.rb new file mode 100644 index 000000000000..d524d3bb7eda --- /dev/null +++ b/templates/puppet/integration_spec.rb @@ -0,0 +1,194 @@ +<% if false # the license inside this if block pertains to this file -%> +# Copyright 2017 Google Inc. +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +<% end -%> +<%# +This file is complicated, and deserves some documentation here, so that +you, brave adventurer, can understand what's happened. This file's +goal is to run the examples required by puppet compilation. Essentially, +you can think of it as a shell script that runs 'puppet apply examples/foo.pp', +then 'puppet apply examples/delete_foo.pp', and checks for errors. + +It has to be more complicated than that. :) +"Fiddling with puppet internal APIs is a lonely and painful path to take." +- David Schmitt, Puppet SWE, September 2018 + +First, let's talk about the reason that it *isn't* a shell script. +We need unit tests that really, really validate our puppet modules - we +can't have them breaking on us, because the community isn't active enough +that we can trust that we'll find out through regular channels. We +absolutely must have tests that we can count on to tell us if the module +is working as expected. And those tests need to run on every PR, because +these modules are complicated enough that they can break in subtle ways. +Consequently, we need to be able to run tests that validate use against the +real GCP APIs, but without spending incredible amounts of time (or money) +or actually creating hordes of resources. That means a VCR test. + +A VCR test is a kind of test that runs once, making real HTTP requests along +the way. It records the requests and responses. On later runs, it mocks +those same HTTP requests with those same responses, making your tests +repeatable (desirable), fast (desirable), and in our case, also free! + +You can't write a test like that using the puppet command line tool, so +instead we plumb into the guts of puppet. Our main hook is a function called +apply_compiled_manifest (and its sibling, apply_with_error_check). + +These functions are not really very similar to `puppet apply`. Instead of +taking a filename, they take the entire manifest as a string. This causes +subtle problems deep within Puppet - Puppet depends on the filename of the +manifest in a handful of ways, not least of which is to find out what +module it is part of (to automagically load dependencies and functions). +Since that isn't possible, we tack on the Puppet override of our own +environment and loader. Since these functions don't call out to Facter +the way that `puppet apply` does, we need to substitute in our own project +name and credential path (which all our examples use). Hence the +get_example() function - it does more than just read the file. + +There's also the issue of the begin/rescue pattern you'll see here. It's +ugly! It's also necessary. compile_to_ral() calls out to our mocked up +loader, but fails to successfully compile the manifest due to some +initialization which is mandatory, but isn't possible to complete from +outside puppet. HOWEVER! We're in luck! In calling some cleanup code, +in reporting on the failure, compile_to_ral actually *performs* the +initialization we need. + +So! We *can* successfully apply a manifest from +under webmock, as long as we: +- try twice, eating the first failure. +- don't need to use any functions that are loaded from modules +- ensure all our dependent modules have their 'lib' dir in $LOAD_PATH +- do all our own variable substitution +- rewrite any necessary functions in puppet and stick them in + the manifest. +- import all the magic from puppetlabs_spec_helper/module_spec_helper, + which I have done without fully understanding it. The comments in + the code suggest that the authors also do not fully understand it. + +Hopefully that explains the complexity here in a way that will continue +to be useful as the tests develop further. + +Let me also explain the order in which these commands are run. First, +there is a pre-destroy - in the event that an older test failed to clean +up after itself, the next run should take care of it. Second, we create +the resources in the manifest. Third, we run the create again, mocking +out any requests during 'flush' and making them crash the test - this is +to check idempotency of the manifests. Fourth, we delete the resources +in the manifest, and fifth, we check the idempotency of that delete by +confirming that no resources make web requests during 'flush'. + +VCR recordings are called 'cassettes', and they are stored in +'spec/cassettes'. If present, they are used - if absent, created. +#-%> +<%= compile 'templates/license.erb' -%> + +<%= lines(autogen_notice :puppet) -%> +require 'spec_helper' +require 'vcr' + +VCR.configure do |c| + c.cassette_library_dir = 'spec/cassettes' + c.hook_into :webmock + c.configure_rspec_metadata! +end + +describe '<%= Google::StringUtils.underscore(obj.name) -%>.create', vcr: true do + mods = [File.expand_path('.'), File.expand_path('./spec/fixtures/modules/')] + it 'creates and destroys non-existent <%= Google::StringUtils.underscore(obj.name) -%>' do + puts 'pre-destroying <%= Google::StringUtils.underscore(obj.name) -%>' + VCR.use_cassette('pre_destroy_<%= Google::StringUtils.underscore(obj.name) -%>') do + example = get_example('delete_<%= Google::StringUtils.underscore(obj.name) -%>') + test = Puppet::Node::Environment.create(:test, mods) + loaders = Puppet::Pops::Loaders.new(test, true) + Puppet.override(current_environment: test, loaders: loaders) do + begin + compile_to_ral(example) + rescue + apply_with_error_check(example) + end + end + end + + puts 'creating <%= Google::StringUtils.underscore(obj.name) -%>' + VCR.use_cassette('create_<%= Google::StringUtils.underscore(obj.name) -%>') do + example = get_example('<%= Google::StringUtils.underscore(obj.name) -%>') + test = Puppet::Node::Environment.create(:test, mods) + loaders = Puppet::Pops::Loaders.new(test, true) + Puppet.override(current_environment: test, loaders: loaders) do + begin + compile_to_ral(example) + rescue + apply_with_error_check(example) + end + end + end + puts 'checking that <%= Google::StringUtils.underscore(obj.name) -%> is created' + VCR.use_cassette('check_<%= Google::StringUtils.underscore(obj.name) -%>') do + example = get_example('<%= Google::StringUtils.underscore(obj.name) -%>') + test = Puppet::Node::Environment.create(:test, mods) + loaders = Puppet::Pops::Loaders.new(test, true) + Puppet.override(current_environment: test, loaders: loaders) do + begin + compile_to_ral(example) + rescue + apply_compiled_manifest(example) do |res| + if res.provider&.respond_to? 'flush' + # Any request to Google APIs during a flush is not + # acceptable - that means that a diff was detected. + omnistub = stub_request(:any, /google/) + .to_raise("Shouldn't have made network call.") + res.provider.flush + remove_request_stub(omnistub) + end + end + end + end + end + puts 'destroying <%= Google::StringUtils.underscore(obj.name) -%>' + VCR.use_cassette('destroy_<%= Google::StringUtils.underscore(obj.name) -%>') do + example = get_example('delete_<%= Google::StringUtils.underscore(obj.name) -%>') + test = Puppet::Node::Environment.create(:test, mods) + loaders = Puppet::Pops::Loaders.new(test, true) + Puppet.override(current_environment: test, loaders: loaders) do + begin + compile_to_ral(example) + rescue + apply_with_error_check(example) + end + end + end + puts 'confirming <%= Google::StringUtils.underscore(obj.name) -%> destroyed' + VCR.use_cassette('check_destroy_<%= Google::StringUtils.underscore(obj.name) -%>') do + example = get_example('delete_<%= Google::StringUtils.underscore(obj.name) -%>') + test = Puppet::Node::Environment.create(:test, mods) + loaders = Puppet::Pops::Loaders.new(test, true) + Puppet.override(current_environment: test, loaders: loaders) do + begin + compile_to_ral(example) + rescue + apply_compiled_manifest(example) do |res| + if res.provider&.respond_to? 'flush' + # Any request to Google APIs during a flush is not + # acceptable - that means that the object still needs + # to be deleted. + omnistub = stub_request(:any, /google/) + .to_raise("Shouldn't have made network call.") + res.provider.flush + remove_request_stub(omnistub) + expect(res.provider.instance_variable_get(:@deleted)).to eq(nil) + end + end + end + end + end + end +end diff --git a/templates/puppet/mock_healthcheck_fn.pp b/templates/puppet/mock_healthcheck_fn.pp new file mode 100644 index 000000000000..11567b140539 --- /dev/null +++ b/templates/puppet/mock_healthcheck_fn.pp @@ -0,0 +1,21 @@ +<% if false # the license inside this if block assertains to this file -%> +# Copyright 2017 Google Inc. +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +<% end -%> +<%= compile 'templates/license.erb' -%> + +<%= lines(autogen_notice :ruby) -%> + +function gcompute_health_check_ref(String $name, String $project_name) >> String { + "https://www.googleapis.com/compute/v1/projects/$project_name/global/healthChecks/$name" +} diff --git a/templates/puppet/spec_helper.rb.erb b/templates/puppet/spec_helper.rb.erb index 555ea74096ac..f740ad507d55 100644 --- a/templates/puppet/spec_helper.rb.erb +++ b/templates/puppet/spec_helper.rb.erb @@ -24,42 +24,21 @@ ENV['TZ'] = 'UTC' -#---------------------------------------------------------- -# Setup code coverage - -require 'simplecov' -SimpleCov.start unless ENV['DISABLE_COVERAGE'] - #---------------------------------------------------------- # Add test path to the search libs $LOAD_PATH.unshift(File.expand_path('.')) <% if spec_stubs? -%> $LOAD_PATH.unshift(File.expand_path('./spec/stubs')) +# Add fixtures so that they can be loaded by integration tests. +$LOAD_PATH.unshift(File.expand_path('./spec/fixtures/modules/gauth/lib')) +$LOAD_PATH.unshift(File.expand_path('./spec/fixtures/modules/<%= @api.prefix -%>/lib')) <% end # spec_stubs? -%> #---------------------------------------------------------- # Block all network traffic - -require 'network_blocker' - -#---------------------------------------------------------- -# Auto require files - -files = [] -files << 'spec/bundle.rb' -files << 'spec/copyright.rb' -files << 'spec/fake_auth.rb' -files << 'spec/test_constants.rb' -files << File.join('lib', '**', '*.rb') - -# Require all files so we can track them via code coverage -Dir[*files].reject { |p| File.directory? p } - .each do |f| - puts "Auto requiring #{f}" \ - if ENV['RSPEC_DEBUG'] - require f - end +require 'puppetlabs_spec_helper/module_spec_helper' +require 'webmock/rspec' #---------------------------------------------------------- # Setup PuppetSpec to allow executing the Puppet manifests from within tests @@ -106,11 +85,26 @@ RSpec.configure do |c| c.after :each do Puppet::Test::TestHelper.after_each_test - Dir.stub(:entries) PuppetSpec::Files.cleanup end end +def get_example(example_name) + ex = read_example(example_name) + healthcheck_fn = File.open('spec/fixtures/mock_hc_fn.pp').read + healthcheck_fn + "\n\n" + ex +end + +def read_example(example_name) + # It's not ideal, but puppet unit tests don't support + # these facts-as-variables, so we have to do these + # substitutions ourselves. + File.open("examples/#{example_name}.pp", 'rb').read\ + .gsub('$project', '"personal-graphite-testing"')\ + .gsub('$cred_path', '"/home/nmckinley/.gcloud/Terraform.json"') +end + + require 'mocha/test_unit' #---------------------------------------------------------- From 8af5373985655de928bdcc387e72347d48023cb4 Mon Sep 17 00:00:00 2001 From: Nathan McKinley Date: Tue, 4 Sep 2018 17:25:13 -0700 Subject: [PATCH 2/8] Use environment substitutions instead of hard-coding. --- templates/puppet/spec_helper.rb.erb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/templates/puppet/spec_helper.rb.erb b/templates/puppet/spec_helper.rb.erb index f740ad507d55..f0a77d8668f3 100644 --- a/templates/puppet/spec_helper.rb.erb +++ b/templates/puppet/spec_helper.rb.erb @@ -100,8 +100,8 @@ def read_example(example_name) # these facts-as-variables, so we have to do these # substitutions ourselves. File.open("examples/#{example_name}.pp", 'rb').read\ - .gsub('$project', '"personal-graphite-testing"')\ - .gsub('$cred_path', '"/home/nmckinley/.gcloud/Terraform.json"') + .gsub('$project', "\"#{ENV['GOOGLE_PROJECT']}\"")\ + .gsub('$cred_path', "\"#{ENV['GOOGLE_CREDENTIALS_PATH']}\"") end From 480e9d3b2139d22f06a979277dc751099964d2fd Mon Sep 17 00:00:00 2001 From: Nathan McKinley Date: Wed, 5 Sep 2018 12:57:14 -0700 Subject: [PATCH 3/8] Clarify and extract to function the nasty parts of integration_spec.rb. --- templates/puppet/integration_spec.rb | 81 ++++------------------------ templates/puppet/spec_helper.rb.erb | 51 ++++++++++++++++++ 2 files changed, 60 insertions(+), 72 deletions(-) diff --git a/templates/puppet/integration_spec.rb b/templates/puppet/integration_spec.rb index d524d3bb7eda..ffb293bcf5e4 100644 --- a/templates/puppet/integration_spec.rb +++ b/templates/puppet/integration_spec.rb @@ -54,13 +54,16 @@ module it is part of (to automagically load dependencies and functions). name and credential path (which all our examples use). Hence the get_example() function - it does more than just read the file. +You can see all this stuff in spec_helper.rb.erb. + There's also the issue of the begin/rescue pattern you'll see here. It's ugly! It's also necessary. compile_to_ral() calls out to our mocked up loader, but fails to successfully compile the manifest due to some initialization which is mandatory, but isn't possible to complete from outside puppet. HOWEVER! We're in luck! In calling some cleanup code, in reporting on the failure, compile_to_ral actually *performs* the -initialization we need. +initialization we need. So, if we try it twice, it works the second +time. Lucky. So! We *can* successfully apply a manifest from under webmock, as long as we: @@ -106,89 +109,23 @@ module it is part of (to automagically load dependencies and functions). it 'creates and destroys non-existent <%= Google::StringUtils.underscore(obj.name) -%>' do puts 'pre-destroying <%= Google::StringUtils.underscore(obj.name) -%>' VCR.use_cassette('pre_destroy_<%= Google::StringUtils.underscore(obj.name) -%>') do - example = get_example('delete_<%= Google::StringUtils.underscore(obj.name) -%>') - test = Puppet::Node::Environment.create(:test, mods) - loaders = Puppet::Pops::Loaders.new(test, true) - Puppet.override(current_environment: test, loaders: loaders) do - begin - compile_to_ral(example) - rescue - apply_with_error_check(example) - end - end + run_example('delete_<%= Google::StringUtils.underscore(obj.name) -%>') end - puts 'creating <%= Google::StringUtils.underscore(obj.name) -%>' VCR.use_cassette('create_<%= Google::StringUtils.underscore(obj.name) -%>') do - example = get_example('<%= Google::StringUtils.underscore(obj.name) -%>') - test = Puppet::Node::Environment.create(:test, mods) - loaders = Puppet::Pops::Loaders.new(test, true) - Puppet.override(current_environment: test, loaders: loaders) do - begin - compile_to_ral(example) - rescue - apply_with_error_check(example) - end - end + run_example('<%= Google::StringUtils.underscore(obj.name) -%>') end puts 'checking that <%= Google::StringUtils.underscore(obj.name) -%> is created' VCR.use_cassette('check_<%= Google::StringUtils.underscore(obj.name) -%>') do - example = get_example('<%= Google::StringUtils.underscore(obj.name) -%>') - test = Puppet::Node::Environment.create(:test, mods) - loaders = Puppet::Pops::Loaders.new(test, true) - Puppet.override(current_environment: test, loaders: loaders) do - begin - compile_to_ral(example) - rescue - apply_compiled_manifest(example) do |res| - if res.provider&.respond_to? 'flush' - # Any request to Google APIs during a flush is not - # acceptable - that means that a diff was detected. - omnistub = stub_request(:any, /google/) - .to_raise("Shouldn't have made network call.") - res.provider.flush - remove_request_stub(omnistub) - end - end - end - end + validate_no_flush_calls('<%= Google::StringUtils.underscore(obj.name) -%>') end puts 'destroying <%= Google::StringUtils.underscore(obj.name) -%>' VCR.use_cassette('destroy_<%= Google::StringUtils.underscore(obj.name) -%>') do - example = get_example('delete_<%= Google::StringUtils.underscore(obj.name) -%>') - test = Puppet::Node::Environment.create(:test, mods) - loaders = Puppet::Pops::Loaders.new(test, true) - Puppet.override(current_environment: test, loaders: loaders) do - begin - compile_to_ral(example) - rescue - apply_with_error_check(example) - end - end + run_example('delete_<%= Google::StringUtils.underscore(obj.name) -%>') end puts 'confirming <%= Google::StringUtils.underscore(obj.name) -%> destroyed' VCR.use_cassette('check_destroy_<%= Google::StringUtils.underscore(obj.name) -%>') do - example = get_example('delete_<%= Google::StringUtils.underscore(obj.name) -%>') - test = Puppet::Node::Environment.create(:test, mods) - loaders = Puppet::Pops::Loaders.new(test, true) - Puppet.override(current_environment: test, loaders: loaders) do - begin - compile_to_ral(example) - rescue - apply_compiled_manifest(example) do |res| - if res.provider&.respond_to? 'flush' - # Any request to Google APIs during a flush is not - # acceptable - that means that the object still needs - # to be deleted. - omnistub = stub_request(:any, /google/) - .to_raise("Shouldn't have made network call.") - res.provider.flush - remove_request_stub(omnistub) - expect(res.provider.instance_variable_get(:@deleted)).to eq(nil) - end - end - end - end + validate_no_flush_calls('delete_<%= Google::StringUtils.underscore(obj.name) -%>') end end end diff --git a/templates/puppet/spec_helper.rb.erb b/templates/puppet/spec_helper.rb.erb index f0a77d8668f3..eaf3bd980da6 100644 --- a/templates/puppet/spec_helper.rb.erb +++ b/templates/puppet/spec_helper.rb.erb @@ -90,6 +90,13 @@ RSpec.configure do |c| end def get_example(example_name) + # This is a little nutty, but we actually put a mocked up + # version of the ruby function for healthchecks at the top + # of the manifest. Any other function which gets used in an + # example is going to need the same treatment. This *IS* + # limiting, not everything that can be expressed in a ruby + # function can be expressed in a puppet function - but see comment + # in integration_spec.rb. We haven't figured out a better way yet. ex = read_example(example_name) healthcheck_fn = File.open('spec/fixtures/mock_hc_fn.pp').read healthcheck_fn + "\n\n" + ex @@ -104,6 +111,50 @@ def read_example(example_name) .gsub('$cred_path', "\"#{ENV['GOOGLE_CREDENTIALS_PATH']}\"") end +def run_example(example_name) + # This function runs an example specified by name (not + # including '.pp'. It generates a puppet environment, + # tricks puppet into feeling semi-initialized by + # compiling the example, catching the failure, then + # compiling it again within the context of the failure, + # and then applies it. + example = get_example(example_name) + test = Puppet::Node::Environment.create(:test, mods) + loaders = Puppet::Pops::Loaders.new(test, true) + Puppet.override(current_environment: test, loaders: loaders) do + begin + compile_to_ral(example) + rescue + apply_with_error_check(example) + end + end +end + +def validate_no_flush_calls(example_name) + # This function is very much like run_example, above, except + # that it also validates that no POST calls are made to a + # google API during the 'flush' step (i.e. 'update if + # necessary' step). + example = get_example(example_name) + test = Puppet::Node::Environment.create(:test, mods) + loaders = Puppet::Pops::Loaders.new(test, true) + Puppet.override(current_environment: test, loaders: loaders) do + begin + compile_to_ral(example) + rescue + apply_compiled_manifest(example) do |res| + if res.provider&.respond_to? 'flush' + # Any request to Google APIs during a flush is not + # acceptable - that means that a diff was detected. + omnistub = stub_request(:any, /google/) + .to_raise("Shouldn't have made network call.") + res.provider.flush + remove_request_stub(omnistub) + end + end + end + end +end require 'mocha/test_unit' From 9ab06bcd71dda178b39082977cce156577933cf0 Mon Sep 17 00:00:00 2001 From: Nathan McKinley Date: Wed, 5 Sep 2018 13:49:07 -0700 Subject: [PATCH 4/8] Convince old tests to pass under new spec-helper. --- templates/puppet/provider_spec.erb | 5 +++++ templates/puppet/spec_helper.rb.erb | 4 ++++ 2 files changed, 9 insertions(+) diff --git a/templates/puppet/provider_spec.erb b/templates/puppet/provider_spec.erb index 9cdd14af73f2..0e636fd165cd 100644 --- a/templates/puppet/provider_spec.erb +++ b/templates/puppet/provider_spec.erb @@ -17,6 +17,11 @@ <%= lines(autogen_notice :puppet) -%> require 'spec_helper' +require 'network_blocker' +require 'spec/bundle.rb' +require 'spec/copyright.rb' +require 'spec/fake_auth.rb' +require 'spec/test_constants.rb' <% if object.readonly diff --git a/templates/puppet/spec_helper.rb.erb b/templates/puppet/spec_helper.rb.erb index eaf3bd980da6..ec5b24f1f958 100644 --- a/templates/puppet/spec_helper.rb.erb +++ b/templates/puppet/spec_helper.rb.erb @@ -69,6 +69,10 @@ RSpec.configure do |c| Puppet::Test::TestHelper.after_all_tests end + c.mock_with :rspec do |conf| + conf.syntax = [:should, :expect] + end + c.before :each do base = PuppetSpec::Files.tmpdir('tmp_settings') Puppet[:vardir] = File.join(base, 'var') From 863e7e7171d29e79f988b53a2c2ebe0084f76fba Mon Sep 17 00:00:00 2001 From: Nathan McKinley Date: Wed, 5 Sep 2018 13:52:42 -0700 Subject: [PATCH 5/8] Don't run integration tests at unit test time. --- .ci/unit-tests/puppet-chef.sh | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.ci/unit-tests/puppet-chef.sh b/.ci/unit-tests/puppet-chef.sh index 8e5a0d4002fa..7409cb5308c5 100755 --- a/.ci/unit-tests/puppet-chef.sh +++ b/.ci/unit-tests/puppet-chef.sh @@ -10,11 +10,13 @@ if [ $PROVIDER = "chef" ]; then # Re-enable chef tests by deleting this if block once the tests are fixed. echo "Skipping tests... See issue #236" elif [ -z "$EXCLUDE_PATTERN" ]; then - DISABLE_COVERAGE=true bundle exec parallel_rspec spec/ + if ls spec/g$PRODUCT* > /dev/null 2&>1; then + DISABLE_COVERAGE=true bundle exec parallel_rspec spec/g$PRODUCT* + fi else # parallel_rspec doesn't support --exclude_pattern IFS="," read -ra excluded <<< "$EXCLUDE_PATTERN" - filtered=$(find spec -name '*_spec.rb' $(printf "! -wholename %s " ${excluded[@]})) + filtered=$(find spec -name "g$PRODUCT*_spec.rb" $(printf "! -wholename %s " ${excluded[@]})) DISABLE_COVERAGE=true bundle exec parallel_rspec ${filtered[@]} fi From ed327477d54451781d0946b77def7d06c8e12d3c Mon Sep 17 00:00:00 2001 From: Nathan McKinley Date: Wed, 5 Sep 2018 16:13:01 -0700 Subject: [PATCH 6/8] Make more of the new tests pass. --- templates/puppet/integration_spec.rb | 1 - templates/puppet/spec_helper.rb.erb | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/templates/puppet/integration_spec.rb b/templates/puppet/integration_spec.rb index ffb293bcf5e4..c3b95b4a587d 100644 --- a/templates/puppet/integration_spec.rb +++ b/templates/puppet/integration_spec.rb @@ -105,7 +105,6 @@ module it is part of (to automagically load dependencies and functions). end describe '<%= Google::StringUtils.underscore(obj.name) -%>.create', vcr: true do - mods = [File.expand_path('.'), File.expand_path('./spec/fixtures/modules/')] it 'creates and destroys non-existent <%= Google::StringUtils.underscore(obj.name) -%>' do puts 'pre-destroying <%= Google::StringUtils.underscore(obj.name) -%>' VCR.use_cassette('pre_destroy_<%= Google::StringUtils.underscore(obj.name) -%>') do diff --git a/templates/puppet/spec_helper.rb.erb b/templates/puppet/spec_helper.rb.erb index ec5b24f1f958..8be3a68474a7 100644 --- a/templates/puppet/spec_helper.rb.erb +++ b/templates/puppet/spec_helper.rb.erb @@ -122,6 +122,7 @@ def run_example(example_name) # compiling the example, catching the failure, then # compiling it again within the context of the failure, # and then applies it. + mods = [File.expand_path('.'), File.expand_path('./spec/fixtures/modules/')] example = get_example(example_name) test = Puppet::Node::Environment.create(:test, mods) loaders = Puppet::Pops::Loaders.new(test, true) @@ -139,6 +140,7 @@ def validate_no_flush_calls(example_name) # that it also validates that no POST calls are made to a # google API during the 'flush' step (i.e. 'update if # necessary' step). + mods = [File.expand_path('.'), File.expand_path('./spec/fixtures/modules/')] example = get_example(example_name) test = Puppet::Node::Environment.create(:test, mods) loaders = Puppet::Pops::Loaders.new(test, true) From d3b895c5308838ffe343ccbfdee11193a4d6870e Mon Sep 17 00:00:00 2001 From: Nathan McKinley Date: Thu, 6 Sep 2018 18:18:40 -0700 Subject: [PATCH 7/8] Update Gemfile.lock for new Gemfile gems. --- templates/puppet/Gemfile.lock | 54 +++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/templates/puppet/Gemfile.lock b/templates/puppet/Gemfile.lock index eb424232b060..f64cad47fb36 100644 --- a/templates/puppet/Gemfile.lock +++ b/templates/puppet/Gemfile.lock @@ -4,9 +4,15 @@ GEM addressable (2.5.2) public_suffix (>= 2.0.2, < 4.0) ast (2.4.0) + crack (0.4.3) + safe_yaml (~> 1.0.0) + declarative (0.0.10) + declarative-option (0.1.0) diff-lcs (1.3) docile (1.1.5) facter (2.5.1) + faraday (0.15.2) + multipart-post (>= 1.2, < 3) fast_gettext (1.1.2) gettext (3.2.6) locale (>= 2.0.5) @@ -15,17 +21,42 @@ GEM fast_gettext (~> 1.1.0) gettext (>= 3.0.2) locale + google-api-client (0.23.8) + addressable (~> 2.5, >= 2.5.1) + googleauth (>= 0.5, < 0.7.0) + httpclient (>= 2.8.1, < 3.0) + mime-types (~> 3.0) + representable (~> 3.0) + retriable (>= 2.0, < 4.0) + signet (~> 0.9) + googleauth (0.6.6) + faraday (~> 0.12) + jwt (>= 1.4, < 3.0) + memoist (~> 0.12) + multi_json (~> 1.11) + os (>= 0.9, < 2.0) + signet (~> 0.7) + hashdiff (0.3.7) hiera (3.4.2) + httpclient (2.8.3) json (2.1.0) json-schema (2.8.0) addressable (>= 2.4) + jwt (2.1.0) locale (2.1.2) + memoist (0.16.0) metaclass (0.0.4) metadata-json-lint (2.0.2) json-schema (~> 2.8) spdx-licenses (~> 1.0) + mime-types (3.2.2) + mime-types-data (~> 3.2015) + mime-types-data (3.2018.0812) mocha (1.3.0) metaclass (~> 0.0.1) + multi_json (1.13.1) + multipart-post (2.0.0) + os (1.0.0) parallel (1.12.1) parallel_tests (2.21.3) parallel @@ -50,6 +81,11 @@ GEM rspec-puppet (~> 2.0) rainbow (3.0.0) rake (10.5.0) + representable (3.0.4) + declarative (< 0.1.0) + declarative-option (< 0.2.0) + uber (< 0.2.0) + retriable (3.1.2) rspec (3.7.0) rspec-core (~> 3.7.0) rspec-expectations (~> 3.7.0) @@ -73,6 +109,13 @@ GEM ruby-progressbar (~> 1.7) unicode-display_width (~> 1.0, >= 1.0.1) ruby-progressbar (1.9.0) + safe_yaml (1.0.4) + semantic_puppet (1.0.2) + signet (0.9.1) + addressable (~> 2.3) + faraday (~> 0.9) + jwt (>= 1.5, < 3.0) + multi_json (~> 1.10) simplecov (0.15.1) docile (~> 1.1.0) json (>= 1.8, < 3) @@ -80,12 +123,20 @@ GEM simplecov-html (0.10.2) spdx-licenses (1.1.0) text (1.3.1) + uber (0.1.0) unicode-display_width (1.3.0) + vcr (4.0.0) + webmock (3.4.2) + addressable (>= 2.3.6) + crack (>= 0.3.2) + hashdiff PLATFORMS ruby DEPENDENCIES + google-api-client + googleauth metadata-json-lint parallel_tests puppet (>= 4.2.0) @@ -98,7 +149,10 @@ DEPENDENCIES rspec-mocks rspec-puppet rubocop + semantic_puppet simplecov + vcr + webmock BUNDLED WITH 1.16.1 From f60ad22bc6d6f4ba54bf3294fb33a0218ffd6a98 Mon Sep 17 00:00:00 2001 From: Nathan McKinley Date: Thu, 6 Sep 2018 18:21:47 -0700 Subject: [PATCH 8/8] Comment in the test. --- .ci/unit-tests/puppet-chef.sh | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.ci/unit-tests/puppet-chef.sh b/.ci/unit-tests/puppet-chef.sh index 7409cb5308c5..0999932e847b 100755 --- a/.ci/unit-tests/puppet-chef.sh +++ b/.ci/unit-tests/puppet-chef.sh @@ -10,6 +10,13 @@ if [ $PROVIDER = "chef" ]; then # Re-enable chef tests by deleting this if block once the tests are fixed. echo "Skipping tests... See issue #236" elif [ -z "$EXCLUDE_PATTERN" ]; then + # The unit tests are all under e.g. spec/gcompute* - the integration tests + # are better run using `bundle exec rake spec` instead of + # `bundle exec parallel_rspec`, but if you just let it run the default + # set of specs, parallel_rspec will try to run the integration tests + # in addition. This is just running all the tests that aren't excluded - + # or, in the event of an empty exclude list, all the tests with the right + # names. if ls spec/g$PRODUCT* > /dev/null 2&>1; then DISABLE_COVERAGE=true bundle exec parallel_rspec spec/g$PRODUCT* fi