Merge pull request #2508 from opscode/lcg/script-resource-fixes

Lcg/script resource fixes

19 files changed, 525 insertions, 267 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index e0e439b56b..80e01bec49 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -23,6 +23,8 @@
 * ruby 1.9.3 support is dropped
 * Update Chef to use RSpec 3.
 * Create constant for LWRP before calling `provides`
+* Cleaned up script and execute provider + specs
+* Added deprecation warnings around the use of command attribute in script resources
 
 ## 12.0.0
 
diff --git a/lib/chef/exceptions.rb b/lib/chef/exceptions.rb
index c8d26dbed2..bf33cb9b52 100644
--- a/lib/chef/exceptions.rb
+++ b/lib/chef/exceptions.rb
@@ -45,6 +45,7 @@ class Chef
     class FileNotFound < RuntimeError; end
     class Package < RuntimeError; end
     class Service < RuntimeError; end
+    class Script < RuntimeError; end
     class Route < RuntimeError; end
     class SearchIndex < RuntimeError; end
     class Override < RuntimeError; end
diff --git a/lib/chef/provider/execute.rb b/lib/chef/provider/execute.rb
index 48b2a344d1..b44112c19e 100644
--- a/lib/chef/provider/execute.rb
+++ b/lib/chef/provider/execute.rb
@@ -18,67 +18,86 @@
 
 require 'chef/log'
 require 'chef/provider'
+require 'forwardable'
 
 class Chef
   class Provider
     class Execute < Chef::Provider
+      extend Forwardable
 
       provides :execute
 
+      def_delegators :@new_resource, :command, :returns, :environment, :user, :group, :cwd, :umask, :creates
+
       def load_current_resource
-        true
+        current_resource = Chef::Resource::Execute.new(new_resource.name)
+        current_resource
       end
 
       def whyrun_supported?
         true
       end
 
-      def action_run
-        opts = {}
-
-        if sentinel_file = sentinel_file_if_exists
-          Chef::Log.debug("#{@new_resource} sentinel file #{sentinel_file} exists - nothing to do")
-          return false
-        end
+      def define_resource_requirements
+         # @todo: this should change to raise in some appropriate major version bump.
+         if creates && creates_relative? && !cwd
+           Chef::Log.warn "Providing a relative path for the creates attribute without the cwd is deprecated and will be changed to fail (CHEF-3819)"
+         end
+      end
 
+      def timeout
         # original implementation did not specify a timeout, but ShellOut
         # *always* times out. So, set a very long default timeout
-        opts[:timeout] = @new_resource.timeout || 3600
-        opts[:returns] = @new_resource.returns if @new_resource.returns
-        opts[:environment] = @new_resource.environment if @new_resource.environment
-        opts[:user] = @new_resource.user if @new_resource.user
-        opts[:group] = @new_resource.group if @new_resource.group
-        opts[:cwd] = @new_resource.cwd if @new_resource.cwd
-        opts[:umask] = @new_resource.umask if @new_resource.umask
-        opts[:log_level] = :info
-        opts[:log_tag] = @new_resource.to_s
-        if STDOUT.tty? && !Chef::Config[:daemon] && Chef::Log.info? && !@new_resource.sensitive
-          opts[:live_stream] = STDOUT
+        new_resource.timeout || 3600
+      end
+
+      def action_run
+        if creates && sentinel_file.exist?
+          Chef::Log.debug("#{new_resource} sentinel file #{sentinel_file} exists - nothing to do")
+          return false
         end
-        description = @new_resource.sensitive ? "sensitive resource" : @new_resource.command
+
         converge_by("execute #{description}") do
-          result = shell_out!(@new_resource.command, opts)
-          Chef::Log.info("#{@new_resource} ran successfully")
+          result = shell_out!(command, opts)
+          Chef::Log.info("#{new_resource} ran successfully")
         end
       end
 
       private
 
-      def sentinel_file_if_exists
-        if sentinel_file = @new_resource.creates
-          relative = Pathname(sentinel_file).relative?
-          cwd = @new_resource.cwd
-          if relative && !cwd
-            Chef::Log.warn "You have provided relative path for execute#creates (#{sentinel_file}) without execute#cwd (see CHEF-3819)"
-          end
-
-          if ::File.exists?(sentinel_file)
-            sentinel_file
-          elsif cwd && relative
-            sentinel_file = ::File.join(cwd, sentinel_file)
-            sentinel_file if ::File.exists?(sentinel_file)
-          end
+      def sensitive?
+        !!new_resource.sensitive
+      end
+
+      def opts
+        opts = {}
+        opts[:timeout]     = timeout
+        opts[:returns]     = returns if returns
+        opts[:environment] = environment if environment
+        opts[:user]        = user if user
+        opts[:group]       = group if group
+        opts[:cwd]         = cwd if cwd
+        opts[:umask]       = umask if umask
+        opts[:log_level]   = :info
+        opts[:log_tag]     = new_resource.to_s
+        if STDOUT.tty? && !Chef::Config[:daemon] && Chef::Log.info? && !sensitive?
+          opts[:live_stream] = STDOUT
         end
+        opts
+      end
+
+      def description
+        sensitive? ? "sensitive resource" : command
+      end
+
+      def creates_relative?
+        Pathname(creates).relative?
+      end
+
+      def sentinel_file
+        Pathname.new(Chef::Util::PathHelper.cleanpath(
+           ( cwd && creates_relative? ) ? ::File.join(cwd, creates) : creates
+        ))
       end
     end
   end
diff --git a/lib/chef/provider/mount/solaris.rb b/lib/chef/provider/mount/solaris.rb
index cf04150322..d8cec24138 100644
--- a/lib/chef/provider/mount/solaris.rb
+++ b/lib/chef/provider/mount/solaris.rb
@@ -88,7 +88,7 @@ class Chef
           # FIXME: Should remount always do the remount or only if the options change?
           actual_options = options || []
           actual_options.delete('noauto')
-          mount_options = actual_options.empty? ? '' : ",#{actual_options.join(',')}" 
+          mount_options = actual_options.empty? ? '' : ",#{actual_options.join(',')}"
           shell_out!("mount -o remount#{mount_options} #{mount_point}")
         end
 
diff --git a/lib/chef/provider/script.rb b/lib/chef/provider/script.rb
index 1615517553..ea286cb0e4 100644
--- a/lib/chef/provider/script.rb
+++ b/lib/chef/provider/script.rb
@@ -18,10 +18,13 @@
 
 require 'tempfile'
 require 'chef/provider/execute'
+require 'forwardable'
 
 class Chef
   class Provider
     class Script < Chef::Provider::Execute
+      extend Forwardable
+
       provides :bash
       provides :csh
       provides :perl
@@ -29,30 +32,40 @@ class Chef
       provides :ruby
       provides :script
 
+      def_delegators :@new_resource, :code, :interpreter, :flags
+
       def initialize(new_resource, run_context)
         super
-        @code = @new_resource.code
+      end
+
+      def command
+        "\"#{interpreter}\" #{flags} \"#{script_file.path}\""
+      end
+
+      def load_current_resource
+        super
+        # @todo Chef-13: change this to an exception
+        if code.nil?
+          Chef::Log.warn "#{@new_resource}: No code attribute was given, resource does nothing, this behavior is deprecated and will be removed in Chef-13"
+        end
       end
 
       def action_run
-        script_file.puts(@code)
+        script_file.puts(code)
         script_file.close
 
         set_owner_and_group
 
-        @new_resource.command("\"#{interpreter}\" #{flags} \"#{script_file.path}\"")
         super
-        converge_by(nil) do
-          # ensure script is unlinked at end of converge!
-          unlink_script_file
-        end
+
+        unlink_script_file
       end
 
       def set_owner_and_group
         # FileUtils itself implements a no-op if +user+ or +group+ are nil
         # You can prove this by running FileUtils.chown(nil,nil,'/tmp/file')
         # as an unprivileged user.
-        FileUtils.chown(@new_resource.user, @new_resource.group, script_file.path)
+        FileUtils.chown(new_resource.user, new_resource.group, script_file.path)
       end
 
       def script_file
@@ -60,16 +73,9 @@ class Chef
       end
 
       def unlink_script_file
-        @script_file && @script_file.close!
+        script_file && script_file.close!
       end
 
-      def interpreter
-        @new_resource.interpreter
-      end
-
-      def flags
-        @new_resource.flags
-      end
     end
   end
 end
diff --git a/lib/chef/resource/execute.rb b/lib/chef/resource/execute.rb
index 980035b079..dfae88b24e 100644
--- a/lib/chef/resource/execute.rb
+++ b/lib/chef/resource/execute.rb
@@ -106,7 +106,7 @@ class Chef
         set_or_return(
           :timeout,
           arg,
-          :kind_of => [ Integer ]
+          :kind_of => [ Integer, Float ]
         )
       end
 
@@ -135,12 +135,12 @@ class Chef
       end
 
       set_guard_inherited_attributes(
-       :cwd,
-       :environment,
-       :group,
-       :user,
-       :umask
-       )
+        :cwd,
+        :environment,
+        :group,
+        :user,
+        :umask
+      )
 
     end
   end
diff --git a/lib/chef/resource/script.rb b/lib/chef/resource/script.rb
index 479295922c..fd0fd5a7fd 100644
--- a/lib/chef/resource/script.rb
+++ b/lib/chef/resource/script.rb
@@ -24,11 +24,13 @@ class Chef
   class Resource
     class Script < Chef::Resource::Execute
 
+      # Chef-13: go back to using :name as the identity attr
       identity_attr :command
 
       def initialize(name, run_context=nil)
         super
         @resource_name = :script
+        # Chef-13: the command variable should be initialized to nil
         @command = name
         @code = nil
         @interpreter = nil
@@ -36,6 +38,15 @@ class Chef
         @default_guard_interpreter = :default
       end
 
+      def command(arg=nil)
+        unless arg.nil?
+          # Chef-13: change this to raise if the user is trying to set a value here
+          Chef::Log.warn "Specifying command attribute on a script resource is a coding error, use the 'code' attribute, or the execute resource"
+          Chef::Log.warn "This attribute is deprecated and must be fixed or this code will fail on Chef-13"
+        end
+        super
+      end
+
       def code(arg=nil)
         set_or_return(
           :code,
diff --git a/spec/functional/resource/bash_spec.rb b/spec/functional/resource/bash_spec.rb
new file mode 100644
index 0000000000..209ec4a12f
--- /dev/null
+++ b/spec/functional/resource/bash_spec.rb
@@ -0,0 +1,88 @@
+#
+# Author:: Serdar Sutay (<serdar@opscode.com>)
+# Copyright:: Copyright (c) 2014 Opscode, Inc.
+# License:: Apache License, Version 2.0
+#
+# 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.
+#
+
+require 'spec_helper'
+require 'functional/resource/base'
+
+describe Chef::Resource::Bash, :unix_only do
+  let(:code) { "echo hello" }
+  let(:resource) {
+    resource = Chef::Resource::Bash.new("foo_resource", run_context)
+    resource.code(code)
+    resource
+  }
+
+  describe "when setting the command attribute" do
+    let (:command) { 'wizard racket' }
+
+    # in Chef-12 the `command` attribute is largely useless, but does set the identity attribute
+    # so that notifications need to target the value of the command.  it will not run the `command`
+    # and if it is given without a code block then it does nothing and always succeeds.
+    describe "in Chef-12", :chef_lt_13_only do
+      it "gets the commmand attribute from the name" do
+        expect(resource.command).to eql("foo_resource")
+      end
+
+      it "sets the resource identity to the command name" do
+        resource.command command
+        expect(resource.identity).to eql(command)
+      end
+
+      it "warns when the code is not present and a useless `command` is present" do
+        expect(Chef::Log).to receive(:warn).with(/coding error/)
+        expect(Chef::Log).to receive(:warn).with(/deprecated/)
+        resource.code nil
+        resource.command command
+        expect { resource.run_action(:run) }.not_to raise_error
+      end
+
+      describe "when the code is not present" do
+        let(:code) { nil }
+        it "warns" do
+          expect(Chef::Log).to receive(:warn)
+          expect { resource.run_action(:run) }.not_to raise_error
+        end
+      end
+    end
+
+    # in Chef-13 the `command` attribute needs to be for internal use only
+    describe "in Chef-13", :chef_gte_13_only do
+      it "should raise an exception when trying to set the command" do
+        expect { resource.command command }.to raise_error # FIXME: add a real error in Chef-13
+      end
+
+      it "should initialize the command to nil" do
+        expect(resource.command).to be_nil
+      end
+
+      describe "when the code is not present" do
+        let(:code) { nil }
+        it "raises an exception" do
+          expect { resource.run_action(:run) }.to raise_error # FIXME: add a real error in Chef-13
+          expect { resource.run_action(:run) }.not_to raise_error
+        end
+      end
+    end
+  end
+
+  it "times out when a timeout is set on the resource" do
+    resource.code 'sleep 600'
+    resource.timeout 0.1
+    expect { resource.run_action(:run) }.to raise_error(Mixlib::ShellOut::CommandTimeout)
+  end
+end
diff --git a/spec/functional/resource/execute_spec.rb b/spec/functional/resource/execute_spec.rb
index 39fef76ab0..cebcc52fcf 100644
--- a/spec/functional/resource/execute_spec.rb
+++ b/spec/functional/resource/execute_spec.rb
@@ -20,94 +20,99 @@ require 'spec_helper'
 require 'functional/resource/base'
 
 describe Chef::Resource::Execute do
-  let(:execute_resource) {
-    exec_resource = Chef::Resource::Execute.new("foo_resource", run_context)
-
-    exec_resource.environment(resource_environment) if resource_environment
-    exec_resource.cwd(resource_cwd) if resource_cwd
-    exec_resource.command("echo hello")
-    if guard
-      if guard_options
-        exec_resource.only_if(guard, guard_options)
-      else
-        exec_resource.only_if(guard)
-      end
-    end
-    exec_resource
+  let(:resource) {
+    resource = Chef::Resource::Execute.new("foo_resource", run_context)
+    resource.command("echo hello")
+    resource
   }
 
-  let(:resource_environment) { nil }
-  let(:resource_cwd) { nil }
-  let(:guard) { nil }
-  let(:guard_options) { nil }
-
   describe "when guard is ruby block" do
     it "guard can still run" do
-      execute_resource.only_if do
-        true
-      end
-      execute_resource.run_action(:run)
-      expect(execute_resource).to be_updated_by_last_action
+      resource.only_if { true }
+      resource.run_action(:run)
+      expect(resource).to be_updated_by_last_action
     end
   end
 
   describe "when parent resource sets :cwd" do
-    let(:resource_cwd) { CHEF_SPEC_DATA }
-
     let(:guard) { %{ruby -e 'exit 1 unless File.exists?("./big_json_plus_one.json")'} }
 
-    it "guard inherits :cwd from resource" do
-      execute_resource.run_action(:run)
-      expect(execute_resource).to be_updated_by_last_action
+    it "guard inherits :cwd from resource and runs" do
+      resource.cwd CHEF_SPEC_DATA
+      resource.only_if guard
+      resource.run_action(:run)
+      expect(resource).to be_updated_by_last_action
+    end
+
+    it "guard inherits :cwd from resource and does not run" do
+      resource.cwd CHEF_SPEC_DATA
+      resource.not_if guard
+      resource.run_action(:run)
+      expect(resource).not_to be_updated_by_last_action
     end
   end
 
+  # We use ruby command so that we don't need to deal with platform specific
+  # commands while testing execute resource. We set it so that the resource
+  # will be updated if the ENV variable is set to what we are intending
+  #
+  # FIXME: yeah, but invoking ruby is slow...
   describe "when parent resource sets :environment" do
-    let(:resource_environment) do
-      {
+    before do
+      resource.environment({
         "SAWS_SECRET"  => "supersecret",
-        "SAWS_KEY"     => "qwerty"
-      }
+        "SAWS_KEY"     => "qwerty",
+      })
     end
 
-    # We use ruby command so that we don't need to deal with platform specific
-    # commands while testing execute resource. We set it so that the resource
-    # will be updated if the ENV variable is set to what we are intending
-    let(:guard) { %{ruby -e 'exit 1 if ENV["SAWS_SECRET"] != "supersecret"'} }
-
-    it "guard inherits :environment value from resource" do
-      execute_resource.run_action(:run)
-      expect(execute_resource).to be_updated_by_last_action
+    it "guard inherits :environment value from resource and runs" do
+      resource.only_if %{ruby -e 'exit 1 if ENV["SAWS_SECRET"] != "supersecret"'}
+      resource.run_action(:run)
+      expect(resource).to be_updated_by_last_action
     end
 
-    describe "when guard sets additional values in the :environment" do
-      let(:guard) { %{ruby -e 'exit 1 if ENV["SGCE_SECRET"] != "regularsecret"'} }
-
-      let(:guard_options) do
-        {
-          :environment => { 'SGCE_SECRET' => "regularsecret" }
-        }
-      end
+    it "guard inherits :environment value from resource and does not run" do
+      resource.only_if %{ruby -e 'exit 1 if ENV["SAWS_SECRET"] == "supersecret"'}
+      resource.run_action(:run)
+      expect(resource).not_to be_updated_by_last_action
+    end
 
-      it "guard sees merged value for in its ENV" do
-        execute_resource.run_action(:run)
-        expect(execute_resource).to be_updated_by_last_action
-      end
+    it "guard adds additional values in its :environment and runs" do
+      resource.only_if %{ruby -e 'exit 1 if ENV["SGCE_SECRET"] != "regularsecret"'}, {
+        :environment => { 'SGCE_SECRET' => "regularsecret" }
+      }
+      resource.run_action(:run)
+      expect(resource).to be_updated_by_last_action
     end
 
-    describe "when guard sets same value in the :environment" do
-      let(:guard) { %{ruby -e 'exit 1 if ENV["SAWS_SECRET"] != "regularsecret"'} }
+    it "guard adds additional values in its :environment and does not run" do
+      resource.only_if %{ruby -e 'exit 1 if ENV["SGCE_SECRET"] == "regularsecret"'}, {
+        :environment => { 'SGCE_SECRET' => "regularsecret" }
+      }
+      resource.run_action(:run)
+      expect(resource).not_to be_updated_by_last_action
+    end
 
-      let(:guard_options) do
-        {
-          :environment => { 'SAWS_SECRET' => "regularsecret" }
-        }
-      end
+    it "guard overwrites value with its :environment and runs" do
+      resource.only_if %{ruby -e 'exit 1 if ENV["SAWS_SECRET"] != "regularsecret"'}, {
+        :environment => { 'SAWS_SECRET' => "regularsecret" }
+      }
+      resource.run_action(:run)
+      expect(resource).to be_updated_by_last_action
+    end
 
-      it "guard sees value from guard options in its ENV" do
-        execute_resource.run_action(:run)
-        expect(execute_resource).to be_updated_by_last_action
-      end
+    it "guard overwrites value with its :environment and does not runs" do
+      resource.only_if %{ruby -e 'exit 1 if ENV["SAWS_SECRET"] == "regularsecret"'}, {
+        :environment => { 'SAWS_SECRET' => "regularsecret" }
+      }
+      resource.run_action(:run)
+      expect(resource).not_to be_updated_by_last_action
     end
   end
+
+  it "times out when a timeout is set on the resource" do
+    resource.command %{ruby -e 'sleep 600'}
+    resource.timeout 0.1
+    expect { resource.run_action(:run) }.to raise_error(Mixlib::ShellOut::CommandTimeout)
+  end
 end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index e3de80f3f1..b155e6da24 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -129,6 +129,10 @@ RSpec.configure do |config|
   config.filter_run_excluding :ruby_gte_19_only => true unless ruby_gte_19?
   config.filter_run_excluding :ruby_20_only => true unless ruby_20?
   config.filter_run_excluding :ruby_gte_20_only => true unless ruby_gte_20?
+  # chef_gte_XX_only and chef_lt_XX_only pair up correctly with the same XX
+  # number.  please conserve this pattern & resist filling out all the operators
+  config.filter_run_excluding :chef_gte_13_only => true unless chef_gte_13?
+  config.filter_run_excluding :chef_lt_13_only => true unless chef_lt_13?
   config.filter_run_excluding :requires_root => true unless root?
   config.filter_run_excluding :requires_root_or_running_windows => true unless (root? || windows?)
   config.filter_run_excluding :requires_unprivileged_user => true if root?
diff --git a/spec/support/platform_helpers.rb b/spec/support/platform_helpers.rb
index 8d17af3993..db0f9ccee4 100644
--- a/spec/support/platform_helpers.rb
+++ b/spec/support/platform_helpers.rb
@@ -11,6 +11,14 @@ def ruby_lt_20?
   !ruby_gte_20?
 end
 
+def chef_gte_13?
+  Chef::VERSION.split('.').first.to_i >= 13
+end
+
+def chef_lt_13?
+  Chef::VERSION.split('.').first.to_i < 13
+end
+
 def ruby_gte_19?
   RUBY_VERSION.to_f >= 1.9
 end
diff --git a/spec/support/shared/unit/script_resource.rb b/spec/support/shared/unit/script_resource.rb
index 58f7f98f19..18ee94606e 100644
--- a/spec/support/shared/unit/script_resource.rb
+++ b/spec/support/shared/unit/script_resource.rb
@@ -21,47 +21,56 @@ require 'spec_helper'
 
 shared_examples_for "a script resource" do
 
-  before(:each) do
-    @resource = script_resource
-  end
-
   it "should create a new Chef::Resource::Script" do
-    expect(@resource).to be_a_kind_of(Chef::Resource)
-    expect(@resource).to be_a_kind_of(Chef::Resource::Script)
+    expect(script_resource).to be_a_kind_of(Chef::Resource)
+    expect(script_resource).to be_a_kind_of(Chef::Resource::Script)
   end
 
   it "should have a resource name of :script" do
-    expect(@resource.resource_name).to eql(resource_name)
+    expect(script_resource.resource_name).to eql(resource_name)
   end
 
-  it "should set command to the argument provided to new" do
-    expect(@resource.command).to eql(resource_instance_name)
+  it "should set command to nil on the resource", :chef_gte_13_only do
+    expect(script_resource.command).to be nil
+  end
+
+  it "should set command to the name on the resource", :chef_lt_13_only do
+    expect(script_resource.command).to eql script_resource.name
   end
 
   it "should accept a string for the code" do
-    @resource.code "hey jude"
-    expect(@resource.code).to eql("hey jude")
+    script_resource.code "hey jude"
+    expect(script_resource.code).to eql("hey jude")
   end
 
   it "should accept a string for the flags" do
-    @resource.flags "-f"
-    expect(@resource.flags).to eql("-f")
+    script_resource.flags "-f"
+    expect(script_resource.flags).to eql("-f")
   end
 
-  describe "when executing guards" do
-    let(:resource) { @resource }
-
-    before(:each) do
-      node = Chef::Node.new
+  it "should raise an exception if users set command on the resource", :chef_gte_13_only do
+    expect { script_resource.command('foo') }.to raise_error(Chef::Exceptions::Script)
+  end
 
-      node.automatic[:platform] = "debian"
-      node.automatic[:platform_version] = "6.0"
+  it "should not raise an exception if users set command on the resource", :chef_lt_13_only do
+    expect { script_resource.command('foo') }.not_to raise_error
+  end
 
-      events = Chef::EventDispatch::Dispatcher.new
-      run_context = Chef::RunContext.new(node, {}, events)
+  describe "when executing guards" do
+    let(:resource) {
+      resource = script_resource
       resource.run_context = run_context
       resource.code 'echo hi'
-    end
+      resource
+    }
+    let(:node) {
+      node = Chef::Node.new
+      node.automatic[:platform] = "debian"
+      node.automatic[:platform_version] = "6.0"
+      node
+    }
+    let(:events) { Chef::EventDispatch::Dispatcher.new }
+    let(:run_context) { Chef::RunContext.new(node, {}, events) }
 
     it "inherits exactly the :cwd, :environment, :group, :path, :user, and :umask attributes from a parent resource class" do
       inherited_difference = Chef::Resource::Script.guard_inherited_attributes -
@@ -87,4 +96,3 @@ shared_examples_for "a script resource" do
     end
   end
 end
-
diff --git a/spec/unit/cookbook/syntax_check_spec.rb b/spec/unit/cookbook/syntax_check_spec.rb
index 67d31cdca5..471fc01831 100644
--- a/spec/unit/cookbook/syntax_check_spec.rb
+++ b/spec/unit/cookbook/syntax_check_spec.rb
@@ -43,6 +43,7 @@ describe Chef::Cookbook::SyntaxCheck do
 
   before do
     Chef::Log.logger = Logger.new(StringIO.new)
+    @original_log_level = Chef::Log.level
     Chef::Log.level = :warn # suppress "Syntax OK" messages
 
     @attr_files = %w{default.rb smokey.rb}.map { |f| File.join(cookbook_path, 'attributes', f) }
@@ -61,6 +62,10 @@ describe Chef::Cookbook::SyntaxCheck do
     @template_files = basenames.map { |f| File.join(cookbook_path, 'templates', 'default', f)}
   end
 
+  after do
+    Chef::Log.level = @original_log_level
+  end
+
   it "creates a syntax checker given the cookbook name when Chef::Config.cookbook_path is set" do
     Chef::Config[:cookbook_path] = File.dirname(cookbook_path)
     syntax_check = Chef::Cookbook::SyntaxCheck.for_cookbook(:openldap)
diff --git a/spec/unit/http/validate_content_length_spec.rb b/spec/unit/http/validate_content_length_spec.rb
index 34b6a61a3a..79bd539af3 100644
--- a/spec/unit/http/validate_content_length_spec.rb
+++ b/spec/unit/http/validate_content_length_spec.rb
@@ -83,12 +83,17 @@ describe Chef::HTTP::ValidateContentLength do
   let(:debug_stream) { StringIO.new }
   let(:debug_output) { debug_stream.string }
 
-  before(:each) {
+  before(:each) do
+    @original_log_level = Chef::Log.level
     Chef::Log.level = :debug
     allow(Chef::Log).to receive(:debug) do |message|
       debug_stream.puts message
     end
-  }
+  end
+
+  after(:each) do
+    Chef::Log.level = @original_log_level
+  end
 
   describe "without response body" do
     let(:request_type) { :direct }
diff --git a/spec/unit/provider/execute_spec.rb b/spec/unit/provider/execute_spec.rb
index ea16d8cf54..794994533e 100644
--- a/spec/unit/provider/execute_spec.rb
+++ b/spec/unit/provider/execute_spec.rb
@@ -15,91 +15,161 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 #
-require File.expand_path(File.join(File.dirname(__FILE__), "..", "..", "spec_helper"))
-#require 'spec_helper'
+
+require 'spec_helper'
 
 describe Chef::Provider::Execute do
-  before do
-    @node = Chef::Node.new
-    @cookbook_collection = Chef::CookbookCollection.new([])
-    @events = Chef::EventDispatch::Dispatcher.new
-    @run_context = Chef::RunContext.new(@node, @cookbook_collection, @events)
-    @new_resource = Chef::Resource::Execute.new("foo_resource", @run_context)
-    @new_resource.timeout 3600
-    @new_resource.returns 0
-    @new_resource.creates "/foo_resource"
-    @provider = Chef::Provider::Execute.new(@new_resource, @run_context)
-    @current_resource = Chef::Resource::Ifconfig.new("foo_resource", @run_context)
-    @provider.current_resource = @current_resource
-    Chef::Log.level = :info
-    # FIXME: There should be a test for how STDOUT.tty? changes the live_stream option being passed
-    allow(STDOUT).to receive(:tty?).and_return(true)
-  end
+
+  let(:node) { Chef::Node.new }
+  let(:events) { Chef::EventDispatch::Dispatcher.new }
+  let(:run_context) { Chef::RunContext.new(node, {}, events) }
+  let(:provider) { Chef::Provider::Execute.new(new_resource, run_context) }
+  let(:current_resource) { Chef::Resource::Ifconfig.new("foo_resource", run_context) }
 
   let(:opts) do
     {
-      timeout:	@new_resource.timeout,
-      returns:	@new_resource.returns,
-      log_level: :info,
-      log_tag: @new_resource.to_s,
-      live_stream: STDOUT
+      timeout:      3600,
+      returns:      0,
+      log_level:    :info,
+      log_tag:      new_resource.to_s,
+      live_stream:  STDOUT,
     }
   end
 
-  it "should execute foo_resource" do
-    allow(@provider).to receive(:load_current_resource)
-    expect(@provider).to receive(:shell_out!).with(@new_resource.command, opts)
-    expect(@provider).to receive(:converge_by).with("execute foo_resource").and_call_original
-    expect(Chef::Log).not_to receive(:warn)
+  let(:new_resource) { Chef::Resource::Execute.new("foo_resource", run_context) }
 
-    @provider.run_action(:run)
-    expect(@new_resource).to be_updated
+  before do
+    @original_log_level = Chef::Log.level
+    Chef::Log.level = :info
+    allow(STDOUT).to receive(:tty?).and_return(true)
   end
 
-  it "should honor sensitive attribute" do
-    @new_resource.sensitive true
-    @provider = Chef::Provider::Execute.new(@new_resource, @run_context)
-    allow(@provider).to receive(:load_current_resource)
-    # Since the resource is sensitive, it should not have :live_stream set
-    expect(@provider).to receive(:shell_out!).with(@new_resource.command, opts.reject { |k| k == :live_stream })
-    expect(Chef::Log).not_to receive(:warn)
-    expect(@provider).to receive(:converge_by).with("execute sensitive resource").and_call_original
-    @provider.run_action(:run)
-    expect(@new_resource).to be_updated
+  after do
+    Chef::Log.level = @original_log_level
   end
 
-  it "should do nothing if the sentinel file exists" do
-    allow(@provider).to receive(:load_current_resource)
-    expect(File).to receive(:exists?).with(@new_resource.creates).and_return(true)
-    expect(@provider).not_to receive(:shell_out!)
-    expect(Chef::Log).not_to receive(:warn)
-
-    @provider.run_action(:run)
-    expect(@new_resource).not_to be_updated
+  describe "#initialize" do
+    it "should return a Chef::Provider::Execute provider" do
+      expect(provider.class).to eql(Chef::Provider::Execute)
+    end
   end
 
-  it "should respect cwd options for 'creates'" do
-    @new_resource.cwd "/tmp"
-    @new_resource.creates "foo_resource"
-    allow(@provider).to receive(:load_current_resource)
-    expect(File).to receive(:exists?).with(@new_resource.creates).and_return(false)
-    expect(File).to receive(:exists?).with(File.join("/tmp", @new_resource.creates)).and_return(true)
-    expect(Chef::Log).not_to receive(:warn)
-    expect(@provider).not_to receive(:shell_out!)
-
-    @provider.run_action(:run)
-    expect(@new_resource).not_to be_updated
+  describe "#load_current_resource" do
+    before do
+      expect(Chef::Resource::Execute).to receive(:new).with(new_resource.name).and_return(current_resource)
+    end
+
+    it "should return the current resource" do
+      expect(provider.load_current_resource).to eql(current_resource)
+    end
+
+    it "our timeout should default to 3600" do
+      provider.load_current_resource
+      expect(provider.timeout).to eql(3600)
+    end
   end
 
-  it "should warn if user specified relative path without cwd" do
-    @new_resource.creates "foo_resource"
-    allow(@provider).to receive(:load_current_resource)
-    expect(Chef::Log).to receive(:warn).with(/relative path/)
-    expect(File).to receive(:exists?).with(@new_resource.creates).and_return(true)
-    expect(@provider).not_to receive(:shell_out!)
+  describe "#action_run" do
+    it "runs shell_out with the default options" do
+      expect(provider).to receive(:shell_out!).with(new_resource.name, opts)
+      expect(provider).to receive(:converge_by).with("execute foo_resource").and_call_original
+      expect(Chef::Log).not_to receive(:warn)
+      provider.run_action(:run)
+      expect(new_resource).to be_updated
+    end
 
-    @provider.run_action(:run)
-    expect(@new_resource).not_to be_updated
+    it "if you pass a command attribute, it runs the command" do
+      new_resource.command "/usr/argelbargle/bin/oogachacka 12345"
+      expect(provider).to receive(:shell_out!).with(new_resource.command, opts)
+      expect(provider).to receive(:converge_by).with("execute #{new_resource.command}").and_call_original
+      expect(Chef::Log).not_to receive(:warn)
+      provider.run_action(:run)
+      expect(new_resource).to be_updated
+    end
+
+    it "should honor sensitive attribute" do
+      new_resource.sensitive true
+      # Since the resource is sensitive, it should not have :live_stream set
+      opts.delete(:live_stream)
+      expect(provider).to receive(:shell_out!).with(new_resource.name, opts)
+      expect(provider).to receive(:converge_by).with("execute sensitive resource").and_call_original
+      expect(Chef::Log).not_to receive(:warn)
+      provider.run_action(:run)
+      expect(new_resource).to be_updated
+    end
+
+    it "should do nothing if the sentinel file exists" do
+      new_resource.creates "/foo_resource"
+      expect(FileTest).to receive(:exist?).with(new_resource.creates).and_return(true)
+      expect(provider).not_to receive(:shell_out!)
+      expect(Chef::Log).not_to receive(:warn)
+      provider.run_action(:run)
+      expect(new_resource).not_to be_updated
+    end
+
+    describe "when the user specifies a relative path without cwd" do
+      before do
+        expect(new_resource.cwd).to be_falsey
+        new_resource.creates "foo_resource"
+      end
+
+      it "should warn in Chef-12", :chef_lt_13_only do
+        expect(Chef::Log).to receive(:warn).with(/relative path/)
+        expect(FileTest).to receive(:exist?).with(new_resource.creates).and_return(true)
+        expect(provider).not_to receive(:shell_out!)
+        provider.run_action(:run)
+        expect(new_resource).not_to be_updated
+      end
+
+      it "should raise if user specified relative path without cwd for Chef-13", :chef_gte_13_only do
+        expect(Chef::Log).to receive(:warn).with(/relative path/)
+        expect(FileTest).to receive(:exist?).with(new_resource.creates).and_return(true)
+        expect(provider).not_to receive(:shell_out!)
+        expect { provider.run_action(:run) }.to raise_error  # @todo: add a real error for Chef-13
+      end
+    end
+
+    it "should respect cwd options for 'creates'" do
+      new_resource.cwd "/tmp"
+      new_resource.creates "foo_resource"
+      expect(FileTest).not_to receive(:exist?).with(new_resource.creates)
+      expect(FileTest).to receive(:exist?).with(File.join("/tmp", new_resource.creates)).and_return(true)
+      expect(Chef::Log).not_to receive(:warn)
+      expect(provider).not_to receive(:shell_out!)
+
+      provider.run_action(:run)
+      expect(new_resource).not_to be_updated
+    end
+
+    it "should unset the live_stream if STDOUT is not a tty" do
+      expect(STDOUT).to receive(:tty?).and_return(false)
+      opts.delete(:live_stream)
+      expect(provider).to receive(:shell_out!).with(new_resource.name, opts)
+      expect(provider).to receive(:converge_by).with("execute foo_resource").and_call_original
+      expect(Chef::Log).not_to receive(:warn)
+      provider.run_action(:run)
+      expect(new_resource).to be_updated
+    end
+
+    it "should unset the live_stream if chef is running as a daemon" do
+      allow(Chef::Config).to receive(:[]).and_call_original
+      expect(Chef::Config).to receive(:[]).with(:daemon).and_return(true)
+      opts.delete(:live_stream)
+      expect(provider).to receive(:shell_out!).with(new_resource.name, opts)
+      expect(provider).to receive(:converge_by).with("execute foo_resource").and_call_original
+      expect(Chef::Log).not_to receive(:warn)
+      provider.run_action(:run)
+      expect(new_resource).to be_updated
+    end
+
+    it "should unset the live_stream if we are not running with a log level of at least :info" do
+      expect(Chef::Log).to receive(:info?).and_return(false)
+      opts.delete(:live_stream)
+      expect(provider).to receive(:shell_out!).with(new_resource.name, opts)
+      expect(provider).to receive(:converge_by).with("execute foo_resource").and_call_original
+      expect(Chef::Log).not_to receive(:warn)
+      provider.run_action(:run)
+      expect(new_resource).to be_updated
+    end
   end
 end
-
diff --git a/spec/unit/provider/git_spec.rb b/spec/unit/provider/git_spec.rb
index 1e282c098d..0106244665 100644
--- a/spec/unit/provider/git_spec.rb
+++ b/spec/unit/provider/git_spec.rb
@@ -22,6 +22,7 @@ describe Chef::Provider::Git do
 
   before(:each) do
     allow(STDOUT).to receive(:tty?).and_return(true)
+    @original_log_level = Chef::Log.level
     Chef::Log.level = :info
 
     @current_resource = Chef::Resource::Git.new("web2.0 app")
@@ -38,6 +39,10 @@ describe Chef::Provider::Git do
     @provider.current_resource = @current_resource
   end
 
+  after(:each) do
+    Chef::Log.level = @original_log_level
+  end
+
   context "determining the revision of the currently deployed checkout" do
 
     before do
diff --git a/spec/unit/provider/script_spec.rb b/spec/unit/provider/script_spec.rb
index 4979475b79..d1759981aa 100644
--- a/spec/unit/provider/script_spec.rb
+++ b/spec/unit/provider/script_spec.rb
@@ -19,77 +19,95 @@
 require 'spec_helper'
 
 describe Chef::Provider::Script, "action_run" do
-  before(:each) do
-    @node = Chef::Node.new
-    @events = Chef::EventDispatch::Dispatcher.new
-    @run_context = Chef::RunContext.new(@node, {}, @events)
-    @new_resource = Chef::Resource::Script.new('run some perl code')
-    @new_resource.code "$| = 1; print 'i like beans'"
-    @new_resource.interpreter 'perl'
+  let(:node) { Chef::Node.new }
+
+  let(:events) { Chef::EventDispatch::Dispatcher.new }
+
+  let(:run_context) { Chef::RunContext.new(node, {}, events) }
+
+  let(:new_resource) {
+    new_resource = Chef::Resource::Script.new('run some perl code')
+    new_resource.code "$| = 1; print 'i like beans'"
+    new_resource.interpreter 'perl'
+    new_resource
+  }
 
-    @provider = Chef::Provider::Script.new(@new_resource, @run_context)
+  let(:provider) { Chef::Provider::Script.new(new_resource, run_context) }
 
-    @script_file = StringIO.new
-    allow(@script_file).to receive(:path).and_return('/tmp/the_script_file')
+  let(:tempfile) { Tempfile.open("rspec-provider-script") }
 
-    allow(@provider).to receive(:shell_out!).and_return(true)
+  before(:each) do
+    allow(provider).to receive(:shell_out!).and_return(true)
+    allow(provider).to receive(:script_file).and_return(tempfile)
   end
 
-  it "creates a temporary file to store the script" do
-    expect(@provider.script_file).to be_an_instance_of(Tempfile)
+  context "#script_file" do
+    it "creates a temporary file to store the script" do
+      allow(provider).to receive(:script_file).and_call_original
+      expect(provider.script_file).to be_an_instance_of(Tempfile)
+    end
   end
 
-  it "unlinks the tempfile when finished" do
-    tempfile_path = @provider.script_file.path
-    @provider.unlink_script_file
-    expect(File.exist?(tempfile_path)).to be_falsey
+  context "#unlink_script_file" do
+    it "unlinks the tempfile" do
+      tempfile_path = tempfile.path
+      provider.unlink_script_file
+      expect(File.exist?(tempfile_path)).to be false
+    end
   end
 
-  it "sets the owner and group for the script file" do
-    @new_resource.user 'toor'
-    @new_resource.group 'wheel'
-    allow(@provider).to receive(:script_file).and_return(@script_file)
-    expect(FileUtils).to receive(:chown).with('toor', 'wheel', "/tmp/the_script_file")
-    @provider.set_owner_and_group
+  context "#set_owner_and_group" do
+    it "sets the owner and group for the script file" do
+      new_resource.user 'toor'
+      new_resource.group 'wheel'
+      expect(FileUtils).to receive(:chown).with('toor', 'wheel', tempfile.path)
+      provider.set_owner_and_group
+    end
   end
 
   context "with the script file set to the correct owner and group" do
     before do
-      allow(@provider).to receive(:set_owner_and_group)
-      allow(@provider).to receive(:script_file).and_return(@script_file)
+      allow(provider).to receive(:set_owner_and_group)
     end
+
     describe "when writing the script to the file" do
       it "should put the contents of the script in the temp file" do
-        @provider.action_run
-        @script_file.rewind
-        expect(@script_file.string).to eq("$| = 1; print 'i like beans'\n")
+        allow(provider).to receive(:unlink_script_file)  # stub to avoid remove
+        provider.action_run
+        expect(IO.read(tempfile.path)).to eq("$| = 1; print 'i like beans'\n")
+        provider.unlink_script_file
       end
 
       it "closes before executing the script and unlinks it when finished" do
-        @provider.action_run
-        expect(@script_file).to be_closed
+        tempfile_path = tempfile.path
+        provider.action_run
+        expect(tempfile).to be_closed
+        expect(File.exist?(tempfile_path)).to be false
       end
-
     end
 
     describe "when running the script" do
-      it 'should set the command to "interpreter"  "tempfile"' do
-        @provider.action_run
-        expect(@new_resource.command).to eq('"perl"  "/tmp/the_script_file"')
-      end
+      let (:default_opts) {
+        {timeout: 3600, returns: 0, log_level: :info, log_tag: "script[run some perl code]", live_stream: STDOUT}
+      }
 
-      describe "with flags set on the resource" do
-        before do
-          @new_resource.flags '-f'
-        end
+      before do
+        allow(STDOUT).to receive(:tty?).and_return(true)
+      end
 
-        it "should set the command to 'interpreter flags tempfile'" do
-          @provider.action_run
-          expect(@new_resource.command).to eq('"perl" -f "/tmp/the_script_file"')
-        end
+      it 'should set the command to "interpreter"  "tempfile"' do
+        expect(provider.command).to eq(%Q{"perl"  "#{tempfile.path}"})
+      end
 
+      it 'should call shell_out! with the command' do
+        expect(provider).to receive(:shell_out!).with(provider.command, default_opts).and_return(true)
+        provider.action_run
       end
 
+      it "should set the command to 'interpreter flags tempfile'" do
+        new_resource.flags '-f'
+        expect(provider.command).to eq(%Q{"perl" -f "#{tempfile.path}"})
+      end
     end
   end
 
diff --git a/spec/unit/resource/script_spec.rb b/spec/unit/resource/script_spec.rb
index 9d744baaa5..4affee8e8c 100644
--- a/spec/unit/resource/script_spec.rb
+++ b/spec/unit/resource/script_spec.rb
@@ -29,18 +29,16 @@ describe Chef::Resource::Script do
     expect(script_resource.interpreter).to eql("naaaaNaNaNaaNaaNaaNaa")
   end
 
-  describe "when it has interpreter and flags" do
+  context "when it has interpreter and flags" do
     before do
-      script_resource.command("grep")
       script_resource.interpreter("gcc")
       script_resource.flags("-al")
     end
 
-   it "returns the command as its identity" do
-      expect(script_resource.identity).to eq("grep")
+    it "returns the name as its identity" do
+      expect(script_resource.identity).to eq(resource_instance_name)
     end
   end
 
   it_behaves_like "a script resource"
 end
-
diff --git a/spec/unit/run_context_spec.rb b/spec/unit/run_context_spec.rb
index bb251c9cf4..cc112f332a 100644
--- a/spec/unit/run_context_spec.rb
+++ b/spec/unit/run_context_spec.rb
@@ -21,10 +21,11 @@
 require 'spec_helper'
 require 'support/lib/library_load_order'
 
-Chef::Log.level = :debug
 
 describe Chef::RunContext do
   before(:each) do
+    @original_log_level = Chef::Log.level
+    Chef::Log.level = :debug
     @chef_repo_path = File.expand_path(File.join(CHEF_SPEC_DATA, "run_context", "cookbooks"))
     cl = Chef::CookbookLoader.new(@chef_repo_path)
     cl.load_cookbooks
@@ -35,6 +36,10 @@ describe Chef::RunContext do
     @run_context = Chef::RunContext.new(@node, @cookbook_collection, @events)
   end
 
+  after(:each) do
+    Chef::Log.level = @original_log_level
+  end
+
   it "has a cookbook collection" do
     expect(@run_context.cookbook_collection).to eq(@cookbook_collection)
   end