Fix windows_task idle_time validation

Two improvements:
- Compare frequency to :idle_time instead of comparing to an incomplete list of other frequencies
- Keep all the validation logic in the function instead of splitting it between the function and where the function is called

Added some specs tests too

Signed-off-by: Alan Gauthier <algaut35@gmail.com>

2 files changed, 13 insertions, 5 deletions

diff --git a/lib/chef/resource/windows_task.rb b/lib/chef/resource/windows_task.rb
index 10e8400c34..1b2c331e36 100644
--- a/lib/chef/resource/windows_task.rb
+++ b/lib/chef/resource/windows_task.rb
@@ -82,7 +82,7 @@ class Chef
         validate_create_frequency_modifier(frequency, frequency_modifier)
         validate_create_day(day, frequency) if day
         validate_create_months(months, frequency) if months
-        validate_idle_time(idle_time, frequency) if ( !idle_time.nil? && ([:minute, :hourly, :daily, :weekly, :monthly].include? frequency)) || (idle_time.nil? || !(idle_time > 0 && idle_time <= 999)) && !([:minute, :hourly, :daily, :weekly, :monthly].include? frequency)
+        validate_idle_time(idle_time, frequency)
       end
 
       private
@@ -196,13 +196,13 @@ class Chef
       end
 
       def validate_idle_time(idle_time, frequency)
-        unless [:on_idle].include?(frequency)
+        if !idle_time.nil? && frequency != :on_idle
           raise ArgumentError, "idle_time property is only valid for tasks that run on_idle"
         end
-        if idle_time.nil?
+        if idle_time.nil? && frequency == :on_idle
           raise ArgumentError, "idle_time value should be set for :on_idle frequency."
         end
-        unless idle_time > 0 && idle_time <= 999
+        unless idle_time.nil? || idle_time > 0 && idle_time <= 999
           raise ArgumentError, "idle_time value #{idle_time} is invalid. Valid values for :on_idle frequency are 1 - 999."
         end
       end
diff --git a/spec/unit/resource/windows_task_spec.rb b/spec/unit/resource/windows_task_spec.rb
index aed1e277db..4b74a4f78a 100644
--- a/spec/unit/resource/windows_task_spec.rb
+++ b/spec/unit/resource/windows_task_spec.rb
@@ -271,7 +271,9 @@ describe Chef::Resource::WindowsTask do
 
   context "#validate_idle_time" do
     it "raises error if frequency is not :on_idle" do
-      expect  { resource.send(:validate_idle_time, 5, :hourly) }.to raise_error(ArgumentError, "idle_time property is only valid for tasks that run on_idle")
+      [:minute, :hourly, :daily, :weekly, :monthly, :once, :on_logon, :onstart, :none].each do |frequency|
+        expect  { resource.send(:validate_idle_time, 5, frequency) }.to raise_error(ArgumentError, "idle_time property is only valid for tasks that run on_idle")
+      end
     end
 
     it "raises error if idle_time > 999" do
@@ -285,6 +287,12 @@ describe Chef::Resource::WindowsTask do
     it "raises error if idle_time is not set" do
       expect  { resource.send(:validate_idle_time, nil, :on_idle) }.to raise_error(ArgumentError, "idle_time value should be set for :on_idle frequency.")
     end
+
+    it "does not raises error if idle_time is not set for other frequencies" do
+      [:minute, :hourly, :daily, :weekly, :monthly, :once, :on_logon, :onstart, :none].each do |frequency|
+        expect  { resource.send(:validate_idle_time, nil, frequency) }.not_to raise_error
+      end
+    end
   end
 
   context "#sec_to_dur" do