From 00435981fb1c94ae1314007b7b0ae1c841180753 Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Thu, 30 Jun 2016 11:17:08 -0700 Subject: [PATCH 1/4] (PUP-6398) Isolate module_working_dir in PMT tests Executing parallel specs on Windows is not reliable in part, because the module tool shares file system state across specs. Previously, executing `rake parallel:spec` on Windows would fail, even when passing on other platforms. This commit is one of several that will allow specs to be executed in parallel by: * Fixing colliding directory read/writes in installer_spec and upgrader_spec by stubbing Puppet.settings[:module_working_dir] to a new directory before each test in installer_spec and in upgrader_spec. This issue only appears when running these tests concurrently unpacker_spec was changed to follow a similar pattern due to discussion about Puppet settings maybe becoming immutable after first load in the future. --- spec/unit/module_tool/applications/installer_spec.rb | 11 ++++++++++- spec/unit/module_tool/applications/unpacker_spec.rb | 3 ++- spec/unit/module_tool/applications/upgrader_spec.rb | 8 ++++++++ 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/spec/unit/module_tool/applications/installer_spec.rb b/spec/unit/module_tool/applications/installer_spec.rb index 80f87162c0d..0da75553a5b 100644 --- a/spec/unit/module_tool/applications/installer_spec.rb +++ b/spec/unit/module_tool/applications/installer_spec.rb @@ -4,6 +4,8 @@ require 'puppet_spec/module_tool/stub_source' require 'semver' +require 'tmpdir' + describe Puppet::ModuleTool::Applications::Installer do include PuppetSpec::ModuleTool::SharedFunctions include PuppetSpec::Files @@ -32,6 +34,13 @@ installer.stubs(:module_repository).returns(remote_source) end + if Puppet.features.microsoft_windows? + before :each do + Puppet.settings.stubs(:[]) + Puppet.settings.stubs(:[]).with(:module_working_dir).returns(Dir.mktmpdir('installertmp')) + end + end + def installer(modname, target_dir, options) Puppet::ModuleTool.set_option_defaults(options) Puppet::ModuleTool::Applications::Installer.new(modname, target_dir, options) @@ -358,6 +367,6 @@ def options end end end - end + end diff --git a/spec/unit/module_tool/applications/unpacker_spec.rb b/spec/unit/module_tool/applications/unpacker_spec.rb index 32d1cc3f7ab..3d614d54138 100644 --- a/spec/unit/module_tool/applications/unpacker_spec.rb +++ b/spec/unit/module_tool/applications/unpacker_spec.rb @@ -14,7 +14,8 @@ let(:working_dir) { tmpdir("working_dir") } before :each do - Puppet.settings[:module_working_dir] = working_dir + Puppet.settings.stubs(:[]) + Puppet.settings.stubs(:[]).with(:module_working_dir).returns(working_dir) end it "should attempt to untar file to temporary location" do diff --git a/spec/unit/module_tool/applications/upgrader_spec.rb b/spec/unit/module_tool/applications/upgrader_spec.rb index 1399924419a..dd613f001e0 100644 --- a/spec/unit/module_tool/applications/upgrader_spec.rb +++ b/spec/unit/module_tool/applications/upgrader_spec.rb @@ -3,6 +3,7 @@ require 'puppet_spec/module_tool/shared_functions' require 'puppet_spec/module_tool/stub_source' require 'semver' +require 'tmpdir' describe Puppet::ModuleTool::Applications::Upgrader do include PuppetSpec::ModuleTool::SharedFunctions @@ -31,6 +32,13 @@ installer.stubs(:module_repository).returns(remote_source) end + if Puppet.features.microsoft_windows? + before :each do + Puppet.settings.stubs(:[]) + Puppet.settings.stubs(:[]).with(:module_working_dir).returns(Dir.mktmpdir('upgradertmp')) + end + end + def upgrader(name, options = {}) Puppet::ModuleTool.set_option_defaults(options) Puppet::ModuleTool::Applications::Upgrader.new(name, options) From b7336888e15a58b5d029dbb5372b3f367f99aa13 Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Thu, 30 Jun 2016 11:27:54 -0700 Subject: [PATCH 2/4] (PUP-6398) Load Puppet::Util::Plist all platforms Executing parallel specs on Windows is not reliable in part, because of how specs define Puppet::Util::Plist on platforms that don't have cfpropertylist support. Previously, executing `rake parallel:spec` on Windows would fail, even when passing on other platforms. This commit is one of several that will allow specs to be executed in parallel by: * Moving the cfpropertylist code guards from tests to plist.rb, so that any code may require 'util/plist' even if the current platform does not have cfpropertylist support (detectable via Puppet.features.cfpropertylist?). This has the benefit of removing definitions of Puppet::Util::Plist from specs designed to run on all platforms. Previously pkgdmg, launchd tests required Puppet::Util::Plist to be explicitly declared inside the specs to be able to execute on platforms without cfpropertylist support - which could cause a race when run in parallel. Note that prior to this, spec\unit\provider\service\launchd_spec.rb and spec\unit\provider\package\appdmg_spec.rb could not be run in isolation on Windows --- lib/puppet/provider/package/pkgdmg.rb | 2 +- lib/puppet/provider/service/launchd.rb | 2 +- lib/puppet/util/plist.rb | 2 +- spec/unit/provider/package/pkgdmg_spec.rb | 3 --- spec/unit/util/plist_spec.rb | 4 +--- 5 files changed, 4 insertions(+), 9 deletions(-) diff --git a/lib/puppet/provider/package/pkgdmg.rb b/lib/puppet/provider/package/pkgdmg.rb index 4b192489388..bf1e430ea2e 100644 --- a/lib/puppet/provider/package/pkgdmg.rb +++ b/lib/puppet/provider/package/pkgdmg.rb @@ -9,7 +9,7 @@ # in /var/db/.puppet_pkgdmg_installed_ require 'puppet/provider/package' -require 'puppet/util/plist' if Puppet.features.cfpropertylist? +require 'puppet/util/plist' require 'puppet/util/http_proxy' Puppet::Type.type(:package).provide :pkgdmg, :parent => Puppet::Provider::Package do diff --git a/lib/puppet/provider/service/launchd.rb b/lib/puppet/provider/service/launchd.rb index 36f0c6ec43a..1d85cc98f22 100644 --- a/lib/puppet/provider/service/launchd.rb +++ b/lib/puppet/provider/service/launchd.rb @@ -1,4 +1,4 @@ -require 'puppet/util/plist' if Puppet.features.cfpropertylist? +require 'puppet/util/plist' Puppet::Type.type(:service).provide :launchd, :parent => :base do desc <<-'EOT' This provider manages jobs with `launchd`, which is the default service diff --git a/lib/puppet/util/plist.rb b/lib/puppet/util/plist.rb index 3403475e030..115b1372f69 100644 --- a/lib/puppet/util/plist.rb +++ b/lib/puppet/util/plist.rb @@ -1,4 +1,4 @@ -require 'cfpropertylist' +require 'cfpropertylist' if Puppet.features.cfpropertylist? require 'puppet/util/execution' module Puppet::Util::Plist diff --git a/spec/unit/provider/package/pkgdmg_spec.rb b/spec/unit/provider/package/pkgdmg_spec.rb index e420b959df6..28be4fb5eb3 100644 --- a/spec/unit/provider/package/pkgdmg_spec.rb +++ b/spec/unit/provider/package/pkgdmg_spec.rb @@ -1,9 +1,6 @@ #! /usr/bin/env ruby require 'spec_helper' -module Puppet::Util::Plist -end - describe Puppet::Type.type(:package).provider(:pkgdmg) do let(:resource) { Puppet::Type.type(:package).new(:name => 'foo', :provider => :pkgdmg) } let(:provider) { described_class.new(resource) } diff --git a/spec/unit/util/plist_spec.rb b/spec/unit/util/plist_spec.rb index 3a4c412ccd7..b4977a15130 100644 --- a/spec/unit/util/plist_spec.rb +++ b/spec/unit/util/plist_spec.rb @@ -1,9 +1,7 @@ require 'spec_helper' -require 'puppet/util/plist' if Puppet.features.cfpropertylist? +require 'puppet/util/plist' require 'puppet_spec/files' -module Puppet::Util::Plist; end - describe Puppet::Util::Plist, :if => Puppet.features.cfpropertylist? do include PuppetSpec::Files From 4fb00a3a48d0c29934ebdb9e00d2a119774e5d0d Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Thu, 30 Jun 2016 11:30:29 -0700 Subject: [PATCH 3/4] (PUP-6398) Explicitly set Windows $CHILD_STATUS On Windows, $CHILD_STATUS is nil while the tests in spec/unit/provider/service/*.rb. and pkg_spec are running. This results in a "Can't modify frozen object" runtime error. However, the tests are able to pass when run serially with the tests in capability_spec, for example. The reason being, some tests in capability_spec make a compile_to_catalog() call which, as a side effect, generates a pid. $CHILD_STATUS is able to latch on to this pid, resolving the issue. The bug shows up when the tests are run in isolation or in parallel (on occasion, when the tests are grouped in such a way and executed in such an order that $CHILD_STATUS is not properly set by another test before these tests run). This commit makes the following changes: * Replaces $CHILD_STATUS.exitstatus calls with stubbed / mocked Puppet::Util::Execution.exitstatus calls in execution_spec and pacman_spec * In the other specs, it generates a pid for $CHILD_STATUS by spinning up a process. The previous strategy does not work for these files because they call into $CHILD_STATUS in lib/provider files, not just in the specs. And, Puppet::Util::Execution.exitstatus is a private class method, so that it is not possible to outright replace these $CHILD_STATUS.exitstatus calls. This may be resolved with some refactoring in the future. Also, require 'English' does not set $CHILD_STATUS on Windows as it does on other platforms. After requiring, $CHILD_STATUS (and $?) is still nil. --- spec/unit/provider/package/pacman_spec.rb | 8 ++++---- spec/unit/provider/package/pkg_spec.rb | 6 ++++++ spec/unit/provider/service/base_spec.rb | 6 ++++++ spec/unit/provider/service/debian_spec.rb | 6 ++++++ spec/unit/provider/service/gentoo_spec.rb | 6 ++++++ spec/unit/provider/service/init_spec.rb | 7 +++++++ spec/unit/provider/service/launchd_spec.rb | 6 ++++++ spec/unit/provider/service/openrc_spec.rb | 6 ++++++ spec/unit/provider/service/src_spec.rb | 6 ++++++ spec/unit/provider/service/systemd_spec.rb | 7 +++++++ spec/unit/provider/service/upstart_spec.rb | 6 ++++++ spec/unit/util/execution_spec.rb | 2 +- 12 files changed, 67 insertions(+), 5 deletions(-) diff --git a/spec/unit/provider/package/pacman_spec.rb b/spec/unit/provider/package/pacman_spec.rb index 6f750327e3c..4d1728922d7 100644 --- a/spec/unit/provider/package/pacman_spec.rb +++ b/spec/unit/provider/package/pacman_spec.rb @@ -438,13 +438,13 @@ it 'should raise an error on non-zero pacman exit without a filter' do executor.expects(:open).with('| /usr/bin/pacman -Sgg 2>&1').returns 'error!' - $CHILD_STATUS.stubs(:exitstatus).returns 1 + Puppet::Util::Execution.expects(:exitstatus).returns 1 expect { described_class.get_installed_groups(installed_packages) }.to raise_error(Puppet::ExecutionFailure, 'error!') end it 'should return empty groups on non-zero pacman exit with a filter' do executor.expects(:open).with('| /usr/bin/pacman -Sgg git 2>&1').returns '' - $CHILD_STATUS.stubs(:exitstatus).returns 1 + Puppet::Util::Execution.expects(:exitstatus).returns 1 expect(described_class.get_installed_groups(installed_packages, 'git')).to eq({}) end @@ -452,7 +452,7 @@ pipe = stub() pipe.expects(:each_line) executor.expects(:open).with('| /usr/bin/pacman -Sgg 2>&1').yields(pipe).returns '' - $CHILD_STATUS.stubs(:exitstatus).returns 0 + Puppet::Util::Execution.expects(:exitstatus).returns 0 expect(described_class.get_installed_groups(installed_packages)).to eq({}) end @@ -460,7 +460,7 @@ pipe = stub() pipe.expects(:each_line).multiple_yields(*groups) executor.expects(:open).with('| /usr/bin/pacman -Sgg 2>&1').yields(pipe).returns '' - $CHILD_STATUS.stubs(:exitstatus).returns 0 + Puppet::Util::Execution.expects(:exitstatus).returns 0 expect(described_class.get_installed_groups(installed_packages)).to eq({'foo' => 'package1 1.0, package2 2.0'}) end end diff --git a/spec/unit/provider/package/pkg_spec.rb b/spec/unit/provider/package/pkg_spec.rb index 9c8bcbb1585..4afdbc4b7b5 100644 --- a/spec/unit/provider/package/pkg_spec.rb +++ b/spec/unit/provider/package/pkg_spec.rb @@ -5,6 +5,12 @@ let (:resource) { Puppet::Resource.new(:package, 'dummy', :parameters => {:name => 'dummy', :ensure => :latest}) } let (:provider) { described_class.new(resource) } + if Puppet.features.microsoft_windows? + # Get a pid for $CHILD_STATUS to latch on to + command = "cmd.exe /c \"exit 0\"" + Puppet::Util::Execution.execute(command, {:failonfail => false}) + end + before :each do described_class.stubs(:command).with(:pkg).returns('/bin/pkg') end diff --git a/spec/unit/provider/service/base_spec.rb b/spec/unit/provider/service/base_spec.rb index 36890210180..d873abd00ce 100644 --- a/spec/unit/provider/service/base_spec.rb +++ b/spec/unit/provider/service/base_spec.rb @@ -17,6 +17,12 @@ subject { provider } + if Puppet.features.microsoft_windows? + # Get a pid for $CHILD_STATUS to latch on to + command = "cmd.exe /c \"exit 0\"" + Puppet::Util::Execution.execute(command, {:failonfail => false}) + end + context "basic operations" do subject do type.new( diff --git a/spec/unit/provider/service/debian_spec.rb b/spec/unit/provider/service/debian_spec.rb index b3f5df4c54a..582a31a4167 100644 --- a/spec/unit/provider/service/debian_spec.rb +++ b/spec/unit/provider/service/debian_spec.rb @@ -9,6 +9,12 @@ describe provider_class do + if Puppet.features.microsoft_windows? + # Get a pid for $CHILD_STATUS to latch on to + command = "cmd.exe /c \"exit 0\"" + Puppet::Util::Execution.execute(command, {:failonfail => false}) + end + before(:each) do # Create a mock resource @resource = stub 'resource' diff --git a/spec/unit/provider/service/gentoo_spec.rb b/spec/unit/provider/service/gentoo_spec.rb index ef2d0ab0dac..a370f7fba80 100644 --- a/spec/unit/provider/service/gentoo_spec.rb +++ b/spec/unit/provider/service/gentoo_spec.rb @@ -4,6 +4,12 @@ describe Puppet::Type.type(:service).provider(:gentoo) do + if Puppet.features.microsoft_windows? + # Get a pid for $CHILD_STATUS to latch on to + command = "cmd.exe /c \"exit 0\"" + Puppet::Util::Execution.execute(command, {:failonfail => false}) + end + before :each do Puppet::Type.type(:service).stubs(:defaultprovider).returns described_class FileTest.stubs(:file?).with('/sbin/rc-update').returns true diff --git a/spec/unit/provider/service/init_spec.rb b/spec/unit/provider/service/init_spec.rb index 2834b84d581..0dd1d280e67 100644 --- a/spec/unit/provider/service/init_spec.rb +++ b/spec/unit/provider/service/init_spec.rb @@ -6,6 +6,13 @@ require 'spec_helper' describe Puppet::Type.type(:service).provider(:init) do + + if Puppet.features.microsoft_windows? + # Get a pid for $CHILD_STATUS to latch on to + command = "cmd.exe /c \"exit 0\"" + Puppet::Util::Execution.execute(command, {:failonfail => false}) + end + before do Puppet::Type.type(:service).defaultprovider = described_class end diff --git a/spec/unit/provider/service/launchd_spec.rb b/spec/unit/provider/service/launchd_spec.rb index e544414290c..8a2c3ce0535 100644 --- a/spec/unit/provider/service/launchd_spec.rb +++ b/spec/unit/provider/service/launchd_spec.rb @@ -12,6 +12,12 @@ let (:launchd_overrides_10_) { '/var/db/com.apple.xpc.launchd/disabled.plist' } subject { resource.provider } + if Puppet.features.microsoft_windows? + # Get a pid for $CHILD_STATUS to latch on to + command = "cmd.exe /c \"exit 0\"" + Puppet::Util::Execution.execute(command, {:failonfail => false}) + end + describe "the type interface" do %w{ start stop enabled? enable disable status}.each do |method| it { is_expected.to respond_to method.to_sym } diff --git a/spec/unit/provider/service/openrc_spec.rb b/spec/unit/provider/service/openrc_spec.rb index f6516cfd156..559e3673323 100644 --- a/spec/unit/provider/service/openrc_spec.rb +++ b/spec/unit/provider/service/openrc_spec.rb @@ -4,6 +4,12 @@ describe Puppet::Type.type(:service).provider(:openrc) do + if Puppet.features.microsoft_windows? + # Get a pid for $CHILD_STATUS to latch on to + command = "cmd.exe /c \"exit 0\"" + Puppet::Util::Execution.execute(command, {:failonfail => false}) + end + before :each do Puppet::Type.type(:service).stubs(:defaultprovider).returns described_class ['/sbin/rc-service', '/bin/rc-status', '/sbin/rc-update'].each do |command| diff --git a/spec/unit/provider/service/src_spec.rb b/spec/unit/provider/service/src_spec.rb index 9c27a37aab7..160591dd61f 100644 --- a/spec/unit/provider/service/src_spec.rb +++ b/spec/unit/provider/service/src_spec.rb @@ -9,6 +9,12 @@ describe provider_class do + if Puppet.features.microsoft_windows? + # Get a pid for $CHILD_STATUS to latch on to + command = "cmd.exe /c \"exit 0\"" + Puppet::Util::Execution.execute(command, {:failonfail => false}) + end + before :each do @resource = stub 'resource' @resource.stubs(:[]).returns(nil) diff --git a/spec/unit/provider/service/systemd_spec.rb b/spec/unit/provider/service/systemd_spec.rb index 64fd50501b8..b3538dcc292 100644 --- a/spec/unit/provider/service/systemd_spec.rb +++ b/spec/unit/provider/service/systemd_spec.rb @@ -5,6 +5,13 @@ require 'spec_helper' describe Puppet::Type.type(:service).provider(:systemd) do + + if Puppet.features.microsoft_windows? + # Get a pid for $CHILD_STATUS to latch on to + command = "cmd.exe /c \"exit 0\"" + Puppet::Util::Execution.execute(command, {:failonfail => false}) + end + before :each do Puppet::Type.type(:service).stubs(:defaultprovider).returns described_class described_class.stubs(:which).with('systemctl').returns '/bin/systemctl' diff --git a/spec/unit/provider/service/upstart_spec.rb b/spec/unit/provider/service/upstart_spec.rb index a7bb32137ea..eae5281142c 100644 --- a/spec/unit/provider/service/upstart_spec.rb +++ b/spec/unit/provider/service/upstart_spec.rb @@ -7,6 +7,12 @@ let(:start_on_default_runlevels) { "\nstart on runlevel [2,3,4,5]" } let(:provider_class) { Puppet::Type.type(:service).provider(:upstart) } + if Puppet.features.microsoft_windows? + # Get a pid for $CHILD_STATUS to latch on to + command = "cmd.exe /c \"exit 0\"" + Puppet::Util::Execution.execute(command, {:failonfail => false}) + end + def given_contents_of(file, content) File.open(file, 'w') do |file| file.write(content) diff --git a/spec/unit/util/execution_spec.rb b/spec/unit/util/execution_spec.rb index de5c4a285ef..b0a0523075a 100644 --- a/spec/unit/util/execution_spec.rb +++ b/spec/unit/util/execution_spec.rb @@ -311,7 +311,7 @@ def stub_process_wait(exitstatus) Puppet::Util::Execution.stubs(:execute_windows).returns(proc_info_stub) stub_process_wait(real_exit_status) - $CHILD_STATUS.stubs(:exitstatus).returns(real_exit_status % 256) # The exitstatus is changed to be mod 256 so that ruby can fit it into 8 bits. + Puppet::Util::Execution.stubs(:exitstatus).returns(real_exit_status % 256) # The exitstatus is changed to be mod 256 so that ruby can fit it into 8 bits. expect(Puppet::Util::Execution.execute('test command', :failonfail => false).exitstatus).to eq(real_exit_status) end From d1afcc1ba92d76edca80dbab578e1a3d7a31ea04 Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Wed, 13 Jul 2016 07:50:12 -0700 Subject: [PATCH 4/4] (PUP-6398) Allow settings_spec to run parallel Executing parallel specs on Windows is not reliable in part, because the Puppet type system must have been initialized prior to these settings specs running, which may not be the case when run parallel. Executing `rake parallel:spec` on Windows would fail, even when passing on other platforms. This commit is one of several that will allow specs to be executed in parallel. * With Puppet.features.microsoft_windows? stubbed to return false the first time a settings catalog is built, it will autorequire the user, file and group type class definitions. When doing this, it triggers a URI building call through Puppet::Util.path_to_uri to specify a full path to the actual type implementation code from puppet/type/{file,user,group}.rb So when run in isolation, prior to this commit, 3 tests would fail as a result of this process because it would try to use invalid non-Windows path to load files. However, when this test is run after other tests that have loaded the user / group / file types, then there is no problem as the stub doesn't impact load behavior. This fix also addresses being able to run these specs parallel --- spec/unit/settings_spec.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/spec/unit/settings_spec.rb b/spec/unit/settings_spec.rb index 37560030eed..7fa236a5905 100644 --- a/spec/unit/settings_spec.rb +++ b/spec/unit/settings_spec.rb @@ -1375,8 +1375,21 @@ def assert_accessing_setting_is_deprecated(settings, setting) end describe "when adding users and groups to the catalog" do + before :all do + # when this spec is run in isolation to build a settings catalog + # it will not be able to autorequire and load types for the first time + # on Windows with microsoft_windows? stubbed to false, because + # Puppet::Util.path_to_uri is called to generate a URI to load code + # and it manipulates the path based on OS + # so instead we forcefully "prime" the cached types + Puppet::Type.type(:user).new(:name => 'foo') + Puppet::Type.type(:group).new(:name => 'bar') + Puppet::Type.type(:file).new(:name => Dir.pwd) # appropriate for OS + end + before do Puppet.features.stubs(:root?).returns true + # stubbed to false, as Windows catalogs don't add users / groups Puppet.features.stubs(:microsoft_windows?).returns false @settings.define_settings :foo,