Brush up

8 files changed, 80 insertions, 59 deletions

diff --git a/app/models/ci/trigger_schedule.rb b/app/models/ci/trigger_schedule.rb
index 0df0a03d63e..fc144d3958d 100644
--- a/app/models/ci/trigger_schedule.rb
+++ b/app/models/ci/trigger_schedule.rb
@@ -18,20 +18,22 @@ module Ci
     after_create :schedule_next_run!
 
     def schedule_next_run!
-      puts "cron: #{cron.inspect} | cron_time_zone: #{cron_time_zone.inspect}"
-      next_time = Ci::CronParser.new(cron, cron_time_zone).next_time_from_now
-      if next_time.present?
+      # puts "cron: #{cron.inspect} | cron_time_zone: #{cron_time_zone.inspect}"
+      next_time = Ci::CronParser.new(cron, cron_time_zone).next_time_from(Time.now)
+
+      if next_time.present? && !less_than_1_hour_from_now?(next_time)
         update!(next_run_at: next_time)
       end
     end
 
     def real_next_run(worker_cron: nil, worker_time_zone: nil)
-      puts "worker_cron: #{worker_cron.inspect} | worker_time_zone: #{worker_time_zone.inspect}"
       worker_cron = Settings.cron_jobs['trigger_schedule_worker']['cron'] unless worker_cron.present?
       worker_time_zone = Time.zone.name unless worker_time_zone.present?
-      worker_next_time = Ci::CronParser.new(worker_cron, worker_time_zone).next_time_from_now
+      # puts "real_next_run: worker_cron: #{worker_cron.inspect} | worker_time_zone: #{worker_time_zone.inspect}"
+
+      worker_next_time = Ci::CronParser.new(worker_cron, worker_time_zone).next_time_from(Time.now)
 
-      puts "next_run_at: #{next_run_at.inspect} | worker_next_time: #{worker_next_time.inspect}"
+      # puts "real_next_run: next_run_at: #{next_run_at.inspect} | worker_next_time: #{worker_next_time.inspect}"
       if next_run_at > worker_next_time
         next_run_at
       else
@@ -41,15 +43,20 @@ module Ci
 
     private
 
+    def less_than_1_hour_from_now?(time)
+      ((time - Time.now).abs < 1.hour) ? true : false
+    end
+
     def check_cron
       cron_parser = Ci::CronParser.new(cron, cron_time_zone)
       is_valid_cron, is_valid_cron_time_zone = cron_parser.validation
+      next_time = cron_parser.next_time_from(Time.now)
 
       if !is_valid_cron
         self.errors.add(:cron, " is invalid syntax")
       elsif !is_valid_cron_time_zone
         self.errors.add(:cron_time_zone, " is invalid timezone")
-      elsif (cron_parser.next_time_from_now - Time.now).abs < 1.hour
+      elsif less_than_1_hour_from_now?(next_time)
         self.errors.add(:cron, " can not be less than 1 hour")
       end
     end
diff --git a/app/workers/trigger_schedule_worker.rb b/app/workers/trigger_schedule_worker.rb
index 440c579b99d..9216103b8da 100644
--- a/app/workers/trigger_schedule_worker.rb
+++ b/app/workers/trigger_schedule_worker.rb
@@ -4,11 +4,14 @@ class TriggerScheduleWorker
 
   def perform
     Ci::TriggerSchedule.where("next_run_at < ?", Time.now).find_each do |trigger_schedule|
+      next if Ci::Pipeline.where(project: trigger_schedule.project, ref: trigger_schedule.ref).running_or_pending.count > 0
+
       begin
         Ci::CreateTriggerRequestService.new.execute(trigger_schedule.project,
                                                     trigger_schedule.trigger,
                                                     trigger_schedule.ref)
       rescue => e
+        puts "#{trigger_schedule.id}: Failed to trigger_schedule job: #{e.message}" # TODO: Remove before merge
         Rails.logger.error "#{trigger_schedule.id}: Failed to trigger_schedule job: #{e.message}"
       ensure
         trigger_schedule.schedule_next_run!
diff --git a/lib/ci/cron_parser.rb b/lib/ci/cron_parser.rb
index a569de293b3..2919543bbef 100644
--- a/lib/ci/cron_parser.rb
+++ b/lib/ci/cron_parser.rb
@@ -8,10 +8,13 @@ module Ci
       @cron_time_zone = cron_time_zone
     end
 
-    def next_time_from_now
+    def next_time_from(time)
       cronLine = try_parse_cron(@cron, @cron_time_zone)
-      return nil unless cronLine.present?
-      cronLine.next_time
+      if cronLine.present?
+        cronLine.next_time(time)
+      else
+        nil
+      end
     end
 
     def validation
diff --git a/spec/factories/ci/trigger_schedules.rb b/spec/factories/ci/trigger_schedules.rb
index f572540d9e3..7143db6961c 100644
--- a/spec/factories/ci/trigger_schedules.rb
+++ b/spec/factories/ci/trigger_schedules.rb
@@ -1,11 +1,14 @@
 FactoryGirl.define do
   factory :ci_trigger_schedule, class: Ci::TriggerSchedule do
-    project factory: :project
-    trigger factory: :ci_trigger_with_ref
+    trigger factory: :ci_trigger_for_trigger_schedule
+
+    after(:build) do |trigger_schedule, evaluator|
+      trigger_schedule.update!(project: trigger_schedule.trigger.project)
+    end
 
     trait :force_triggable do
       after(:create) do |trigger_schedule, evaluator|
-        trigger_schedule.next_run_at -= 1.month
+        trigger_schedule.update!(next_run_at: trigger_schedule.next_run_at - 1.year)
       end
     end
 
diff --git a/spec/factories/ci/triggers.rb b/spec/factories/ci/triggers.rb
index c9d2687b28e..c9fedf8a857 100644
--- a/spec/factories/ci/triggers.rb
+++ b/spec/factories/ci/triggers.rb
@@ -3,7 +3,9 @@ FactoryGirl.define do
     factory :ci_trigger do
       token { SecureRandom.hex(15) }
 
-      factory :ci_trigger_with_ref do
+      factory :ci_trigger_for_trigger_schedule do
+        owner factory: :user
+        project factory: :project
         ref 'master'
       end
     end
diff --git a/spec/lib/ci/cron_parser_spec.rb b/spec/lib/ci/cron_parser_spec.rb
index f8c7e88edb3..dd1449b5b02 100644
--- a/spec/lib/ci/cron_parser_spec.rb
+++ b/spec/lib/ci/cron_parser_spec.rb
@@ -2,8 +2,8 @@ require 'spec_helper'
 
 module Ci
   describe CronParser, lib: true do
-    describe '#next_time_from_now' do
-      subject { described_class.new(cron, cron_time_zone).next_time_from_now }
+    describe '#next_time_from' do
+      subject { described_class.new(cron, cron_time_zone).next_time_from(Time.now) }
 
       context 'when cron and cron_time_zone are valid' do
         context 'when specific time' do
@@ -65,8 +65,8 @@ module Ci
       end
 
       context 'when cron and cron_time_zone are invalid' do
-        let(:cron) { 'hack' }
-        let(:cron_time_zone) { 'hack' }
+        let(:cron) { 'invalid_cron' }
+        let(:cron_time_zone) { 'invalid_cron_time_zone' }
 
         it 'returns nil' do
           is_expected.to be_nil
@@ -74,25 +74,23 @@ module Ci
       end
     end
 
-    describe '#valid_syntax?' do
-      subject { described_class.new(cron, cron_time_zone).valid_syntax? }
-
-      context 'when cron and cron_time_zone are valid' do
-        let(:cron) { '* * * * *' }
-        let(:cron_time_zone) { 'Europe/Istanbul' }
-
-        it 'returns true' do
-          is_expected.to eq(true)
-        end 
+    describe '#validation' do
+      it 'returns results' do
+        is_valid_cron, is_valid_cron_time_zone = described_class.new('* * * * *', 'Europe/Istanbul').validation
+        expect(is_valid_cron).to eq(true)
+        expect(is_valid_cron_time_zone).to eq(true)
       end
 
-      context 'when cron and cron_time_zone are invalid' do
-        let(:cron) { 'hack' }
-        let(:cron_time_zone) { 'hack' }
+      it 'returns results' do
+        is_valid_cron, is_valid_cron_time_zone = described_class.new('*********', 'Europe/Istanbul').validation
+        expect(is_valid_cron).to eq(false)
+        expect(is_valid_cron_time_zone).to eq(true)
+      end
 
-        it 'returns false' do
-          is_expected.to eq(false)
-        end 
+      it 'returns results' do
+        is_valid_cron, is_valid_cron_time_zone = described_class.new('* * * * *', 'Invalid-zone').validation
+        expect(is_valid_cron).to eq(true)
+        expect(is_valid_cron_time_zone).to eq(false)
       end
     end
   end
diff --git a/spec/models/ci/trigger_schedule_spec.rb b/spec/models/ci/trigger_schedule_spec.rb
index 11e8083fc86..d47ab529bc0 100644
--- a/spec/models/ci/trigger_schedule_spec.rb
+++ b/spec/models/ci/trigger_schedule_spec.rb
@@ -1,9 +1,6 @@
 require 'spec_helper'
 
 describe Ci::TriggerSchedule, models: true do
-  let(:user) { create(:user) }
-  let(:project) { create(:project) }
-  let(:trigger) { create(:ci_trigger, owner: user, project: project, ref: 'master') }
 
   it { is_expected.to belong_to(:project) }
   it { is_expected.to belong_to(:trigger) }
@@ -11,17 +8,27 @@ describe Ci::TriggerSchedule, models: true do
   # it { is_expected.to validate_presence_of :cron_time_zone }
   it { is_expected.to respond_to :ref }
 
-  # describe '#schedule_next_run!' do
-  #   let(:trigger_schedule) { create(:ci_trigger_schedule, :cron_nightly_build, next_run_at: nil, trigger: trigger) }
+  it 'should validate less_than_1_hour_from_now' do
+    trigger_schedule = create(:ci_trigger_schedule, :cron_nightly_build)
+    trigger_schedule.cron = '* * * * *'
+    trigger_schedule.valid?
+    expect(trigger_schedule.errors[:cron].first).to include('can not be less than 1 hour')
+  end
+
+  describe '#schedule_next_run!' do
+    context 'when more_than_1_hour_from_now' do
+      let(:trigger_schedule) { create(:ci_trigger_schedule, :cron_nightly_build) }
 
-  #   before do
-  #     trigger_schedule.schedule_next_run!
-  #   end
+      before do
+        trigger_schedule.schedule_next_run!
+      end
 
-  #   it 'updates next_run_at' do
-  #     expect(Ci::TriggerSchedule.last.next_run_at).not_to be_nil
-  #   end
-  # end
+      it 'updates next_run_at' do
+        next_time = Ci::CronParser.new(trigger_schedule.cron, trigger_schedule.cron_time_zone).next_time_from(Time.now)
+        expect(Ci::TriggerSchedule.last.next_run_at).to eq(next_time)
+      end
+    end
+  end
 
   describe '#real_next_run' do
     subject { trigger_schedule.real_next_run(worker_cron: worker_cron, worker_time_zone: worker_time_zone) }
diff --git a/spec/workers/trigger_schedule_worker_spec.rb b/spec/workers/trigger_schedule_worker_spec.rb
index 2cf51a31c71..f0c7eeaedae 100644
--- a/spec/workers/trigger_schedule_worker_spec.rb
+++ b/spec/workers/trigger_schedule_worker_spec.rb
@@ -8,38 +8,36 @@ describe TriggerScheduleWorker do
   end
 
   context 'when there is a scheduled trigger within next_run_at' do
-    let(:user) { create(:user) }
-    let(:project) { create(:project) }
-    let(:trigger) { create(:ci_trigger, owner: user, project: project, ref: 'master') }
-    let!(:trigger_schedule) { create(:ci_trigger_schedule, :cron_nightly_build, :force_triggable, trigger: trigger, project: project) }
+    let!(:trigger_schedule) { create(:ci_trigger_schedule, :cron_nightly_build, :force_triggable) }
 
     before do
       worker.perform
     end
 
     it 'creates a new trigger request' do
-      expect(Ci::TriggerRequest.first.trigger_id).to eq(trigger.id)
+      expect(trigger_schedule.trigger.id).to eq(Ci::TriggerRequest.first.trigger_id)
     end
 
     it 'creates a new pipeline' do
       expect(Ci::Pipeline.last.status).to eq('pending')
     end
 
-    it 'schedules next_run_at' do
-      next_time = Ci::CronParser.new('0 1 * * *', 'Europe/Istanbul').next_time_from_now
+    it 'updates next_run_at' do
+      next_time = Ci::CronParser.new(trigger_schedule.cron, trigger_schedule.cron_time_zone).next_time_from(Time.now)
       expect(Ci::TriggerSchedule.last.next_run_at).to eq(next_time)
     end
   end
 
-  context 'when there are no scheduled triggers within next_run_at' do
-    let!(:trigger_schedule) { create(:ci_trigger_schedule, :cron_nightly_build) }
+  context 'when there is a scheduled trigger within next_run_at and a runnign pipeline' do
+    let!(:trigger_schedule) { create(:ci_trigger_schedule, :cron_nightly_build, :force_triggable) }
 
     before do
+      create(:ci_pipeline, project: trigger_schedule.project, ref: trigger_schedule.ref, status: 'running')
       worker.perform
     end
 
     it 'do not create a new pipeline' do
-      expect(Ci::Pipeline.all).to be_empty
+      expect(Ci::Pipeline.count).to eq(1)
     end
 
     it 'do not reschedule next_run_at' do
@@ -47,15 +45,15 @@ describe TriggerScheduleWorker do
     end
   end
 
-  context 'when next_run_at is nil' do
-    let!(:trigger_schedule) { create(:ci_trigger_schedule, :cron_nightly_build, next_run_at: nil) }
+  context 'when there are no scheduled triggers within next_run_at' do
+    let!(:trigger_schedule) { create(:ci_trigger_schedule, :cron_nightly_build) }
 
     before do
       worker.perform
     end
 
     it 'do not create a new pipeline' do
-      expect(Ci::Pipeline.all).to be_empty
+      expect(Ci::Pipeline.count).to eq(0)
     end
 
     it 'do not reschedule next_run_at' do