7 files changed, 161 insertions, 118 deletions

diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index e57fe5648..d2dbf7a1c 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -318,12 +318,11 @@ class Puppet::Transaction
 
     # Is the resource currently scheduled?
     def scheduled?(resource)
-        self.ignoreschedules or resource.scheduled?
+        self.ignoreschedules or resource_harness.scheduled?(resource)
     end
 
     # Should this resource be skipped?
     def skip?(resource)
-        skip = false
         if missing_tags?(resource)
             resource.debug "Not tagged with %s" % tags.join(", ")
         elsif ! scheduled?(resource)
@@ -356,11 +355,10 @@ class Puppet::Transaction
 
     # Is this resource tagged appropriately?
     def missing_tags?(resource)
-        not appropriately_tagged?(resource)
-    end
+        return false if ignore_tags?
+        return false if tags.empty?
 
-    def appropriately_tagged?(resource)
-        self.ignore_tags? or tags.empty? or resource.tagged?(*tags)
+        not resource.tagged?(*tags)
     end
 end
 
diff --git a/lib/puppet/transaction/resource_harness.rb b/lib/puppet/transaction/resource_harness.rb
index 76148e69f..05e569b92 100644
--- a/lib/puppet/transaction/resource_harness.rb
+++ b/lib/puppet/transaction/resource_harness.rb
@@ -73,6 +73,28 @@ class Puppet::Transaction::ResourceHarness
         @transaction = transaction
     end
 
+    def scheduled?(resource)
+        return true if Puppet[:ignoreschedules]
+        return true unless schedule = schedule(resource)
+
+        # We use 'checked' here instead of 'synced' because otherwise we'll
+        # end up checking most resources most times, because they will generally
+        # have been synced a long time ago (e.g., a file only gets updated
+        # once a month on the server and its schedule is daily; the last sync time
+        # will have been a month ago, so we'd end up checking every run).
+        return schedule.match?(resource.cached(:checked).to_i)
+    end
+
+    def schedule(resource)
+        unless resource.catalog
+            resource.warning "Cannot schedule without a schedule-containing catalog"
+            return nil
+        end
+
+        return nil unless name = resource[:schedule]
+        resource.catalog.resource(:schedule, name) || resource.fail("Could not find schedule #{name}")
+    end
+
     private
 
     def absent_and_not_being_created?(current, param)
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index dde342880..4b5a65e1e 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -1669,45 +1669,6 @@ class Type
         end.flatten.reject { |r| r.nil? }
     end
 
-    ###############################
-    # All of the scheduling code.
-
-    # Look up the schedule and set it appropriately.  This is done after
-    # the instantiation phase, so that the schedule can be anywhere in the
-    # file.
-    def schedule
-         unless catalog
-             warning "Cannot schedule without a schedule-containing catalog"
-             return nil
-         end
-
-        unless defined? @schedule
-            if name = self[:schedule]
-                if sched = catalog.resource(:schedule, name)
-                    @schedule = sched
-                else
-                    self.fail "Could not find schedule %s" % name
-                end
-            else
-                @schedule = nil
-            end
-        end
-        @schedule
-    end
-
-    # Check whether we are scheduled to run right now or not.
-    def scheduled?
-        return true if Puppet[:ignoreschedules]
-        return true unless schedule = self.schedule
-
-        # We use 'checked' here instead of 'synced' because otherwise we'll
-        # end up checking most resources most times, because they will generally
-        # have been synced a long time ago (e.g., a file only gets updated
-        # once a month on the server and its schedule is daily; the last sync time
-        # will have been a month ago, so we'd end up checking every run).
-        return schedule.match?(self.cached(:checked).to_i)
-    end
-
     # Define the initial list of tags.
     def tags=(list)
         tag(self.class.name)
@@ -1895,10 +1856,6 @@ class Type
 
     # Set up all of our autorequires.
     def finish
-        # Scheduling has to be done when the whole config is instantiated, so
-        # that file order doesn't matter in finding them.
-        self.schedule
-
         # Make sure all of our relationships are valid.  Again, must be done
         # when the entire catalog is instantiated.
         self.class.relationship_params.collect do |klass|
diff --git a/spec/integration/transaction.rb b/spec/integration/transaction.rb
index badf5611b..f68287432 100755
--- a/spec/integration/transaction.rb
+++ b/spec/integration/transaction.rb
@@ -181,8 +181,6 @@ describe Puppet::Transaction do
         catalog.apply
         FileTest.should be_exists(fname)
 
-        exec.should_not be_scheduled
-
         # Now remove it, so it can get created again
         File.unlink(fname)
 
@@ -234,4 +232,16 @@ describe Puppet::Transaction do
         FileTest.should_not be_exists(file1[:path])
         FileTest.should_not be_exists(file2[:path])
     end
+
+    # #801 -- resources only checked in noop should be rescheduled immediately.
+    it "should immediately reschedule noop resources" do
+        Puppet::Type.type(:schedule).mkdefaultschedules
+        resource = Puppet::Type.type(:notify).new(:name => "mymessage", :noop => true)
+        catalog = Puppet::Resource::Catalog.new
+        catalog.add_resource resource
+
+        trans = catalog.apply
+
+        trans.resource_harness.should be_scheduled(resource)
+    end
 end
diff --git a/spec/unit/transaction.rb b/spec/unit/transaction.rb
index b315eb124..0c3d901f5 100755
--- a/spec/unit/transaction.rb
+++ b/spec/unit/transaction.rb
@@ -241,39 +241,84 @@ describe Puppet::Transaction do
 
     describe "when skipping a resource" do
         before :each do
-            @resource = stub_everything 'res'
+            @resource = Puppet::Type.type(:notify).new :name => "foo"
             @catalog = Puppet::Resource::Catalog.new
+            @resource.catalog = @catalog
             @transaction = Puppet::Transaction.new(@catalog)
         end
 
         it "should skip resource with missing tags" do
             @transaction.stubs(:missing_tags?).returns(true)
-            @transaction.skip?(@resource).should be_true
+            @transaction.should be_skip(@resource)
         end
 
-        it "should ask the resource if it's tagged with any of the tags" do
-            tags = ['one', 'two']
-            @transaction.stubs(:ignore_tags?).returns(false)
-            @transaction.stubs(:tags).returns(tags)
-
-            @resource.expects(:tagged?).with(*tags).returns(true)
-
-            @transaction.missing_tags?(@resource).should be_false
-        end
-
-        it "should skip not scheduled resources" do
+        it "should skip unscheduled resources" do
             @transaction.stubs(:scheduled?).returns(false)
-            @transaction.skip?(@resource).should be_true
+            @transaction.should be_skip(@resource)
         end
 
         it "should skip resources with failed dependencies" do
-            @transaction.stubs(:failed_dependencies?).returns(false)
-            @transaction.skip?(@resource).should be_true
+            @transaction.stubs(:failed_dependencies?).returns(true)
+            @transaction.should be_skip(@resource)
         end
 
         it "should skip virtual resource" do
             @resource.stubs(:virtual?).returns true
-            @transaction.skip?(@resource).should be_true
+            @transaction.should be_skip(@resource)
+        end
+    end
+
+    describe "when determining if tags are missing" do
+        before :each do
+            @resource = Puppet::Type.type(:notify).new :name => "foo"
+            @catalog = Puppet::Resource::Catalog.new
+            @resource.catalog = @catalog
+            @transaction = Puppet::Transaction.new(@catalog)
+
+            @transaction.stubs(:ignore_tags?).returns false
+        end
+
+        it "should not be missing tags if tags are being ignored" do
+            @transaction.expects(:ignore_tags?).returns true
+
+            @resource.expects(:tagged?).never
+
+            @transaction.should_not be_missing_tags(@resource)
+        end
+
+        it "should not be missing tags if the transaction tags are empty" do
+            @transaction.tags = []
+            @resource.expects(:tagged?).never
+            @transaction.should_not be_missing_tags(@resource)
+        end
+
+        it "should otherwise let the resource determine if it is missing tags" do
+            tags = ['one', 'two']
+            @transaction.tags = tags
+            @resource.expects(:tagged?).with(*tags).returns(false)
+            @transaction.should be_missing_tags(@resource)
+        end
+    end
+
+    describe "when determining if a resource should be scheduled" do
+        before :each do
+            @resource = Puppet::Type.type(:notify).new :name => "foo"
+            @catalog = Puppet::Resource::Catalog.new
+            @resource.catalog = @catalog
+            @transaction = Puppet::Transaction.new(@catalog)
+        end
+
+        it "should always schedule resources if 'ignoreschedules' is set" do
+            @transaction.ignoreschedules = true
+            @transaction.resource_harness.expects(:scheduled?).never
+
+            @transaction.should be_scheduled(@resource)
+        end
+
+        it "should let the resource harness determine whether the resource should be scheduled" do
+            @transaction.resource_harness.expects(:scheduled?).with(@resource).returns "feh"
+
+            @transaction.scheduled?(@resource).should == "feh"
         end
     end
 
diff --git a/spec/unit/transaction/resource_harness.rb b/spec/unit/transaction/resource_harness.rb
index 3b9a42a38..334dbff77 100755
--- a/spec/unit/transaction/resource_harness.rb
+++ b/spec/unit/transaction/resource_harness.rb
@@ -265,4 +265,65 @@ describe Puppet::Transaction::ResourceHarness do
             @harness.should_not be_allow_changes(@resource)
         end
     end
+
+    describe "when finding the schedule" do
+        before do
+            @catalog = Puppet::Resource::Catalog.new
+            @resource.catalog = @catalog
+        end
+
+        it "should warn and return nil if the resource has no catalog" do
+            @resource.catalog = nil
+            @resource.expects(:warning)
+
+            @harness.schedule(@resource).should be_nil
+        end
+
+        it "should return nil if the resource specifies no schedule" do
+            @harness.schedule(@resource).should be_nil
+        end
+
+        it "should fail if the named schedule cannot be found" do
+            @resource[:schedule] = "whatever"
+            @resource.expects(:fail)
+            @harness.schedule(@resource)
+        end
+
+        it "should return the named schedule if it exists" do
+            sched = Puppet::Type.type(:schedule).new(:name => "sched")
+            @catalog.add_resource(sched)
+            @resource[:schedule] = "sched"
+            @harness.schedule(@resource).to_s.should == sched.to_s
+        end
+    end
+
+    describe "when determining if a resource is scheduled" do
+        before do
+            @catalog = Puppet::Resource::Catalog.new
+            @resource.catalog = @catalog
+        end
+
+        it "should return true if 'ignoreschedules' is set" do
+            Puppet[:ignoreschedules] = true
+            @resource[:schedule] = "meh"
+            @harness.should be_scheduled(@resource)
+        end
+
+        it "should return true if the resource has no schedule set" do
+            @harness.should be_scheduled(@resource)
+        end
+
+        it "should return the result of matching the schedule with the cached 'checked' time if a schedule is set" do
+            t = Time.now
+            @resource.expects(:cached).with(:checked).returns(t)
+
+            sched = Puppet::Type.type(:schedule).new(:name => "sched")
+            @catalog.add_resource(sched)
+            @resource[:schedule] = "sched"
+
+            sched.expects(:match?).with(t.to_i).returns "feh"
+
+            @harness.scheduled?(@resource).should == "feh"
+        end
+    end
 end
diff --git a/test/ral/manager/type.rb b/test/ral/manager/type.rb
index 8b146b613..e373cde30 100755
--- a/test/ral/manager/type.rb
+++ b/test/ral/manager/type.rb
@@ -315,54 +315,4 @@ class TestType < Test::Unit::TestCase
         exec = Puppet::Type.newexec(:title => "yay", :command => "/bin/echo yay")
         assert_equal("Exec[yay]", exec.ref)
     end
-
-    # Partially test #704, but also cover the rest of the schedule management bases.
-    def test_schedule
-        schedule = Puppet::Type.type(:schedule).new(:name => "maint")
-        catalog = mk_catalog(schedule)
-
-        {"maint" => true, nil => false, :fail => :fail}.each do |name, should|
-            args = {:name => tempfile, :ensure => :file}
-            if name
-                args[:schedule] = name
-            end
-            resource = Puppet::Type.type(:file).new(args)
-            catalog.add_resource(resource)
-
-            if should == :fail
-                assert_raise(Puppet::Error, "Did not fail on missing schedule") do
-                    resource.schedule
-                end
-            elsif should == false
-                assert_nil(resource.schedule, "Set the schedule tho it is set to nil")
-            else
-                sched = nil
-                assert_nothing_raised("Failed when schedule was %s" % sched) do
-                    sched = resource.schedule
-                end
-
-                assert(sched, "Did not find schedule %s" % sched.inspect)
-
-                if should
-                    assert_equal(name, sched.name, "did not get correct schedule back")
-                end
-            end
-            catalog.remove_resource(resource)
-        end
-    end
-
-    # #801 -- resources only checked in noop should be rescheduled immediately.
-    def test_reschedule_when_noop
-        Puppet::Type.type(:schedule).mkdefaultschedules
-        file = Puppet::Type.type(:file).new(:path => "/tmp/whatever", :mode => "755", :noop => true, :schedule => :daily, :ensure => :file)
-        catalog = Puppet::Resource::Catalog.new
-        catalog.add_resource
-
-        assert(file.noop?, "File not considered in noop")
-        assert(file.scheduled?, "File is not considered scheduled")
-
-        catalog.apply
-
-        assert(file.scheduled?, "File is not considered scheduled even though only a noop run was made")
-    end
 end