Upgrade sidekiq to newer version

Fix mail tests

Gemfile

@@ -122,7 +122,7 @@ gem 'acts-as-taggable-on', '~> 3.4'
 # Background jobs
 gem 'sinatra', '~> 1.4.4', require: nil
-gem 'sidekiq', '3.3.0'
+gem 'sidekiq', '~> 3.5.0'
 gem 'sidetiq', '~> 0.6.3'
 # HTTP requests
@@ -216,7 +216,7 @@ group :development do
   gem "letter_opener", '~> 1.1.2'
   gem 'quiet_assets', '~> 1.0.2'
   gem 'rack-mini-profiler', '~> 0.9.0', require: false
-  gem 'rerun', '~> 0.10.0'
+  gem 'rerun', '~> 0.11.0'
   gem 'bullet', require: false
   gem 'active_record_query_trace', require: false
   gem 'rack-lineprof', platform: :mri

Gemfile.lock

@@ -106,8 +106,23 @@ GEM
       activemodel (>= 3.2.0)
       activesupport (>= 3.2.0)
       json (>= 1.7)
-    celluloid (0.16.0)
-      timers (~> 4.0.0)
+    celluloid (0.17.2)
+      celluloid-essentials
+      celluloid-extras
+      celluloid-fsm
+      celluloid-pool
+      celluloid-supervision
+      timers (>= 4.1.1)
+    celluloid-essentials (0.20.5)
+      timers (>= 4.1.1)
+    celluloid-extras (0.20.5)
+      timers (>= 4.1.1)
+    celluloid-fsm (0.20.5)
+      timers (>= 4.1.1)
+    celluloid-pool (0.20.5)
+      timers (>= 4.1.1)
+    celluloid-supervision (0.20.5)
+      timers (>= 4.1.1)
     charlock_holmes (0.6.9.4)
     chunky_png (1.3.4)
     cliver (0.3.2)
@@ -375,8 +390,7 @@ GEM
       addressable (~> 2.3)
     letter_opener (1.1.2)
       launchy (~> 2.2)
-    listen (2.10.1)
-      celluloid (~> 0.16.0)
+    listen (3.0.3)
       rb-fsevent (>= 0.9.3)
       rb-inotify (>= 0.9)
     macaddr (1.7.1)
@@ -530,7 +544,7 @@ GEM
     raindrops (0.15.0)
     rake (10.4.2)
     raphael-rails (2.1.2)
-    rb-fsevent (0.9.5)
+    rb-fsevent (0.9.6)
     rb-inotify (0.9.5)
       ffi (>= 0.5.0)
     rblineprof (0.3.6)
@@ -562,8 +576,8 @@ GEM
     redis-store (1.1.6)
       redis (>= 2.2)
     request_store (1.2.0)
-    rerun (0.10.0)
-      listen (~> 2.7, >= 2.7.3)
+    rerun (0.11.0)
+      listen (~> 3.0)
     responders (1.1.2)
       railties (>= 3.2, < 4.2)
     rest-client (1.8.0)
@@ -644,12 +658,12 @@ GEM
       rack
     shoulda-matchers (2.8.0)
       activesupport (>= 3.0.0)
-    sidekiq (3.3.0)
-      celluloid (>= 0.16.0)
-      connection_pool (>= 2.0.0)
-      json
-      redis (>= 3.0.6)
-      redis-namespace (>= 1.3.1)
+    sidekiq (3.5.3)
+      celluloid (~> 0.17.2)
+      connection_pool (~> 2.2, >= 2.2.0)
+      json (~> 1.0)
+      redis (~> 3.2, >= 3.2.1)
+      redis-namespace (~> 1.5, >= 1.5.2)
     sidetiq (0.6.3)
       celluloid (>= 0.14.1)
       ice_cube (= 0.11.1)
@@ -713,7 +727,7 @@ GEM
     thor (0.19.1)
     thread_safe (0.3.5)
     tilt (1.4.1)
-    timers (4.0.4)
+    timers (4.1.1)
       hitimes
     timfel-krb5-auth (0.8.3)
     tinder (1.10.1)
@@ -887,7 +901,7 @@ DEPENDENCIES
   redcarpet (~> 3.3.3)
   redis-rails (~> 4.0.0)
   request_store (~> 1.2.0)
-  rerun (~> 0.10.0)
+  rerun (~> 0.11.0)
   rqrcode-rails3 (~> 0.1.7)
   rspec-rails (~> 3.3.0)
   rubocop (~> 0.28.0)
@@ -900,7 +914,7 @@ DEPENDENCIES
   settingslogic (~> 2.0.9)
   sham_rack
   shoulda-matchers (~> 2.8.0)
-  sidekiq (= 3.3.0)
+  sidekiq (~> 3.5.0)
   sidetiq (~> 0.6.3)
   simplecov (~> 0.10.0)
   sinatra (~> 1.4.4)

spec/models/ci/project_services/mail_service_spec.rb

@@ -15,6 +15,12 @@
 require 'spec_helper'
 describe Ci::MailService do
+  let(:deliveries) { Ci::Notify.deliveries }
+
+  before do
+    deliveries.clear
+  end
+
   describe "Associations" do
     it { is_expected.to belong_to :project }
   end
@@ -44,13 +50,13 @@ describe Ci::MailService do
       end
       it do
-        should_email("git@example.com")
         mail.execute(build)
+        should_email("git@example.com")
       end
       def should_email(email)
-        expect(Ci::Notify).to receive(:build_fail_email).with(build.id, email)
-        expect(Ci::Notify).not_to receive(:build_success_email).with(build.id, email)
+        expect(sent?(build, 'failed', email)).to be_truthy
+        expect(sent?(build, 'success', email)).to be_falsey
       end
     end
@@ -67,13 +73,13 @@ describe Ci::MailService do
       end
       it do
-        should_email("git@example.com")
         mail.execute(build)
+        should_email("git@example.com")
       end
       def should_email(email)
-        expect(Ci::Notify).to receive(:build_success_email).with(build.id, email)
-        expect(Ci::Notify).not_to receive(:build_fail_email).with(build.id, email)
+        expect(sent?(build, 'success', email)).to be_truthy
+        expect(sent?(build, 'failed', email)).to be_falsey
       end
     end
@@ -95,14 +101,14 @@ describe Ci::MailService do
       end
       it do
+        mail.execute(build)
         should_email("git@example.com")
         should_email("jeroen@example.com")
-        mail.execute(build)
       end
       def should_email(email)
-        expect(Ci::Notify).to receive(:build_success_email).with(build.id, email)
-        expect(Ci::Notify).not_to receive(:build_fail_email).with(build.id, email)
+        expect(sent?(build, 'success', email)).to be_truthy
+        expect(sent?(build, 'failed', email)).to be_falsey
       end
     end
@@ -124,14 +130,14 @@ describe Ci::MailService do
       end
       it do
+        mail.execute(build) if mail.can_execute?(build)
         should_email(commit.git_author_email)
         should_email("jeroen@example.com")
-        mail.execute(build) if mail.can_execute?(build)
       end
       def should_email(email)
-        expect(Ci::Notify).not_to receive(:build_success_email).with(build.id, email)
-        expect(Ci::Notify).not_to receive(:build_fail_email).with(build.id, email)
+        expect(sent?(build, 'success', email)).to be_falsey
+        expect(sent?(build, 'failed', email)).to be_falsey
       end
     end
@@ -177,15 +183,22 @@ describe Ci::MailService do
       it do
         Ci::Build.retry(build)
+        mail.execute(build) if mail.can_execute?(build)
         should_email(commit.git_author_email)
         should_email("jeroen@example.com")
-        mail.execute(build) if mail.can_execute?(build)
       end
       def should_email(email)
-        expect(Ci::Notify).not_to receive(:build_success_email).with(build.id, email)
-        expect(Ci::Notify).not_to receive(:build_fail_email).with(build.id, email)
+        expect(sent?(build, 'success', email)).to be_falsey
+        expect(sent?(build, 'failed', email)).to be_falsey
       end
     end
   end
+
+  def sent?(build, status, email)
+    deliveries.any? do |delivery|
+      delivery.subject.include?("Build #{status} for #{build.gl_project.name_with_namespace}") &&
+        delivery.to.include?(email)
+    end
+  end
 end

spec/services/notification_service_spec.rb

@@ -2,6 +2,11 @@ require 'spec_helper'
 describe NotificationService do
   let(:notification) { NotificationService.new }
+  let(:deliveries) { Notify.deliveries }
+
+  before(:each) do
+    deliveries.clear
+  end
   describe 'Keys' do
     describe :new_key do
@@ -10,8 +15,8 @@ describe NotificationService do
       it { expect(notification.new_key(key)).to be_truthy }
       it 'should sent email to key owner' do
-        expect(Notify).to receive(:new_ssh_key_email).with(key.id)
         notification.new_key(key)
+        expect(sent?('SSH key was added to your account', key.user_id)).to be_truthy
       end
     end
   end
@@ -23,8 +28,8 @@ describe NotificationService do
       it { expect(notification.new_email(email)).to be_truthy }
       it 'should send email to email owner' do
-        expect(Notify).to receive(:new_email_email).with(email.id)
         notification.new_email(email)
+        expect(sent?('Email was added to your account', email.user_id)).to be_truthy
       end
     end
   end
@@ -47,6 +52,8 @@ describe NotificationService do
         it do
           add_users_with_subscription(note.project, issue)
+          notification.new_note(note)
+
           should_email(@u_watcher.id)
           should_email(note.noteable.author_id)
           should_email(note.noteable.assignee_id)
@@ -57,15 +64,13 @@ describe NotificationService do
           should_not_email(@u_disabled.id)
           should_not_email(@unsubscriber.id)
           should_not_email(@u_outsider_mentioned)
-
-          notification.new_note(note)
         end
         it 'filters out "mentioned in" notes' do
           mentioned_note = SystemNoteService.cross_reference(mentioned_issue, issue, issue.author)
-          expect(Notify).not_to receive(:note_issue_email)
           notification.new_note(mentioned_note)
+          expect(deliveries.length).to eq(0)
         end
       end
@@ -85,6 +90,8 @@ describe NotificationService do
         end
         it do
+          notification.new_note(note)
+
           should_email(note.noteable.author_id)
           should_email(note.noteable.assignee_id)
           should_email(@u_mentioned.id)
@@ -92,16 +99,15 @@ describe NotificationService do
           should_not_email(note.author_id)
           should_not_email(@u_participating.id)
           should_not_email(@u_disabled.id)
-          notification.new_note(note)
         end
       end
       def should_email(user_id)
-        expect(Notify).to receive(:note_issue_email).with(user_id, note.id)
+        expect(sent?(note.noteable.title, user_id)).to be_truthy
       end
       def should_not_email(user_id)
-        expect(Notify).not_to receive(:note_issue_email).with(user_id, note.id)
+        expect(sent?(note.noteable.title, user_id)).to be_falsey
       end
     end
@@ -117,6 +123,8 @@ describe NotificationService do
       describe :new_note do
         it do
+          notification.new_note(note)
+
           # Notify all team members
           note.project.team.members.each do |member|
             # User with disabled notification should not be notified
@@ -130,23 +138,22 @@ describe NotificationService do
           should_not_email(@u_mentioned.id)
           should_not_email(@u_disabled.id)
           should_not_email(@u_not_mentioned.id)
-          notification.new_note(note)
         end
         it 'filters out "mentioned in" notes' do
           mentioned_note = SystemNoteService.cross_reference(mentioned_issue, issue, issue.author)
-          expect(Notify).not_to receive(:note_issue_email)
           notification.new_note(mentioned_note)
+          expect(deliveries.length).to eq(0)
         end
       end
       def should_email(user_id)
-        expect(Notify).to receive(:note_issue_email).with(user_id, note.id)
+        expect(sent?(note.noteable.title, user_id)).to be_truthy
       end
       def should_not_email(user_id)
-        expect(Notify).not_to receive(:note_issue_email).with(user_id, note.id)
+        expect(sent?(note.noteable.title, user_id)).to be_falsey
       end
     end
@@ -161,38 +168,41 @@ describe NotificationService do
       describe :new_note do
         it do
+          notification.new_note(note)
+
           should_email(@u_committer.id, note)
           should_email(@u_watcher.id, note)
           should_not_email(@u_mentioned.id, note)
           should_not_email(note.author_id, note)
           should_not_email(@u_participating.id, note)
           should_not_email(@u_disabled.id, note)
-          notification.new_note(note)
         end
         it do
           note.update_attribute(:note, '@mention referenced')
+          notification.new_note(note)
+
           should_email(@u_committer.id, note)
           should_email(@u_watcher.id, note)
           should_email(@u_mentioned.id, note)
           should_not_email(note.author_id, note)
           should_not_email(@u_participating.id, note)
           should_not_email(@u_disabled.id, note)
-          notification.new_note(note)
         end
         it do
           @u_committer.update_attributes(notification_level: Notification::N_MENTION)
-          should_not_email(@u_committer.id, note)
           notification.new_note(note)
+
+          should_not_email(@u_committer.id, note)
         end
         def should_email(user_id, n)
-          expect(Notify).to receive(:note_commit_email).with(user_id, n.id)
+          expect(sent?(n.noteable.title, user_id)).to be_truthy
         end
         def should_not_email(user_id, n)
-          expect(Notify).not_to receive(:note_commit_email).with(user_id, n.id)
+          expect(sent?(n.noteable.title, user_id)).to be_falsey
         end
       end
     end
@@ -209,32 +219,36 @@ describe NotificationService do
     describe :new_issue do
       it do
+        notification.new_issue(issue, @u_disabled)
+
         should_email(issue.assignee_id)
         should_email(@u_watcher.id)
         should_email(@u_participant_mentioned.id)
         should_not_email(@u_mentioned.id)
         should_not_email(@u_participating.id)
         should_not_email(@u_disabled.id)
-        notification.new_issue(issue, @u_disabled)
       end
       it do
         issue.assignee.update_attributes(notification_level: Notification::N_MENTION)
-        should_not_email(issue.assignee_id)
         notification.new_issue(issue, @u_disabled)
+
+        should_not_email(issue.assignee_id)
       end
       def should_email(user_id)
-        expect(Notify).to receive(:new_issue_email).with(user_id, issue.id)
+        expect(sent?(issue.title, user_id)).to be_truthy
       end
       def should_not_email(user_id)
-        expect(Notify).not_to receive(:new_issue_email).with(user_id, issue.id)
+        expect(sent?(issue.title, user_id)).to be_falsey
       end
     end
     describe :reassigned_issue do
       it 'should email new assignee' do
+        notification.reassigned_issue(issue, @u_disabled)
+
         should_email(issue.assignee_id)
         should_email(@u_watcher.id)
         should_email(@u_participant_mentioned.id)
@@ -242,21 +256,21 @@ describe NotificationService do
         should_not_email(@unsubscriber.id)
         should_not_email(@u_participating.id)
         should_not_email(@u_disabled.id)
-
-        notification.reassigned_issue(issue, @u_disabled)
       end
       def should_email(user_id)
-        expect(Notify).to receive(:reassigned_issue_email).with(user_id, issue.id, nil, @u_disabled.id)
+        expect(sent?(issue.title, user_id)).to be_truthy
       end
       def should_not_email(user_id)
-        expect(Notify).not_to receive(:reassigned_issue_email).with(user_id, issue.id, issue.assignee_id, @u_disabled.id)
+        expect(sent?(issue.title, user_id)).to be_falsey
       end
     end
     describe :close_issue do
       it 'should sent email to issue assignee and issue author' do
+        notification.close_issue(issue, @u_disabled)
+
         should_email(issue.assignee_id)
         should_email(issue.author_id)
         should_email(@u_watcher.id)
@@ -265,21 +279,21 @@ describe NotificationService do
         should_not_email(@unsubscriber.id)
         should_not_email(@u_participating.id)
         should_not_email(@u_disabled.id)
-
-        notification.close_issue(issue, @u_disabled)
       end
       def should_email(user_id)
-        expect(Notify).to receive(:closed_issue_email).with(user_id, issue.id, @u_disabled.id)
+        expect(sent?(issue.title, user_id)).to be_truthy
       end
       def should_not_email(user_id)
-        expect(Notify).not_to receive(:closed_issue_email).with(user_id, issue.id, @u_disabled.id)
+        expect(sent?(issue.title, user_id)).to be_falsey
       end
     end
     describe :reopen_issue do
       it 'should send email to issue assignee and issue author' do
+        notification.reopen_issue(issue, @u_disabled)
+
         should_email(issue.assignee_id)
         should_email(issue.author_id)
         should_email(@u_watcher.id)
@@ -288,16 +302,14 @@ describe NotificationService do
         should_not_email(@unsubscriber.id)
         should_not_email(@u_participating.id)
         should_not_email(@u_disabled.id)
-
-        notification.reopen_issue(issue, @u_disabled)
       end
       def should_email(user_id)
-        expect(Notify).to receive(:issue_status_changed_email).with(user_id, issue.id, 'reopened', @u_disabled.id)
+        expect(sent?(issue.title, user_id)).to be_truthy
       end
       def should_not_email(user_id)
-        expect(Notify).not_to receive(:issue_status_changed_email).with(user_id, issue.id, 'reopened', @u_disabled.id)
+        expect(sent?(issue.title, user_id)).to be_falsey
       end
     end
   end
@@ -313,25 +325,28 @@ describe NotificationService do
     describe :new_merge_request do
       it do
+        notification.new_merge_request(merge_request, @u_disabled)
+
         should_email(merge_request.assignee_id)
         should_email(@u_watcher.id)
         should_email(@u_participant_mentioned.id)
         should_not_email(@u_participating.id)
         should_not_email(@u_disabled.id)
-        notification.new_merge_request(merge_request, @u_disabled)
       end
       def should_email(user_id)
-        expect(Notify).to receive(:new_merge_request_email).with(user_id, merge_request.id)
+        expect(sent?(merge_request.title, user_id)).to be_truthy
       end
       def should_not_email(user_id)
-        expect(Notify).not_to receive(:new_merge_request_email).with(user_id, merge_request.id)
+        expect(sent?(merge_request.title, user_id)).to be_falsey
       end
     end
     describe :reassigned_merge_request do
       it do
+        notification.reassigned_merge_request(merge_request, merge_request.author)
+
         should_email(merge_request.assignee_id)
         should_email(@u_watcher.id)
         should_email(@u_participant_mentioned.id)
@@ -339,20 +354,20 @@ describe NotificationService do
         should_not_email(@unsubscriber.id)
         should_not_email(@u_participating.id)
         should_not_email(@u_disabled.id)
-        notification.reassigned_merge_request(merge_request, merge_request.author)
       end
       def should_email(user_id)
-        expect(Notify).to receive(:reassigned_merge_request_email).with(user_id, merge_request.id, nil, merge_request.author_id)
+        expect(sent?(merge_request.title, user_id)).to be_truthy
       end
       def should_not_email(user_id)
-        expect(Notify).not_to receive(:reassigned_merge_request_email).with(user_id, merge_request.id, merge_request.assignee_id, merge_request.author_id)
+        expect(sent?(merge_request.title, user_id)).to be_falsey
       end
     end
     describe :closed_merge_request do
       it do
+        notification.close_mr(merge_request, @u_disabled)
         should_email(merge_request.assignee_id)
         should_email(@u_watcher.id)
         should_email(@u_participant_mentioned.id)
@@ -360,20 +375,20 @@ describe NotificationService do
         should_not_email(@unsubscriber.id)
         should_not_email(@u_participating.id)
         should_not_email(@u_disabled.id)
-        notification.close_mr(merge_request, @u_disabled)
       end
       def should_email(user_id)
-        expect(Notify).to receive(:closed_merge_request_email).with(user_id, merge_request.id, @u_disabled.id)
+        expect(sent?(merge_request.title, user_id)).to be_truthy
       end
       def should_not_email(user_id)
-        expect(Notify).not_to receive(:closed_merge_request_email).with(user_id, merge_request.id, @u_disabled.id)
+        expect(sent?(merge_request.title, user_id)).to be_falsey
       end
     end
     describe :merged_merge_request do
       it do
+        notification.merge_mr(merge_request, @u_disabled)
         should_email(merge_request.assignee_id)
         should_email(@u_watcher.id)
         should_email(@u_participant_mentioned.id)
@@ -381,20 +396,20 @@ describe NotificationService do
         should_not_email(@unsubscriber.id)
         should_not_email(@u_participating.id)
         should_not_email(@u_disabled.id)
-        notification.merge_mr(merge_request, @u_disabled)
       end
       def should_email(user_id)
-        expect(Notify).to receive(:merged_merge_request_email).with(user_id, merge_request.id, @u_disabled.id)
+        expect(sent?(merge_request.title, user_id)).to be_truthy
       end
       def should_not_email(user_id)
-        expect(Notify).not_to receive(:merged_merge_request_email).with(user_id, merge_request.id, @u_disabled.id)
+        expect(sent?(merge_request.title, user_id)).to be_falsey
       end
     end
     describe :reopen_merge_request do
       it do
+        notification.reopen_mr(merge_request, @u_disabled)
         should_email(merge_request.assignee_id)
         should_email(@u_watcher.id)
         should_email(@u_participant_mentioned.id)
@@ -402,15 +417,14 @@ describe NotificationService do
         should_not_email(@unsubscriber.id)
         should_not_email(@u_participating.id)
         should_not_email(@u_disabled.id)
-        notification.reopen_mr(merge_request, @u_disabled)
       end
       def should_email(user_id)
-        expect(Notify).to receive(:merge_request_status_email).with(user_id, merge_request.id, 'reopened', @u_disabled.id)
+        expect(sent?(merge_request.title, user_id)).to be_truthy
       end
       def should_not_email(user_id)
-        expect(Notify).not_to receive(:merge_request_status_email).with(user_id, merge_request.id, 'reopened', @u_disabled.id)
+        expect(sent?(merge_request.title, user_id)).to be_falsey
       end
     end
   end
@@ -424,18 +438,18 @@ describe NotificationService do
     describe :project_was_moved do
       it do
+        notification.project_was_moved(project, "gitlab/gitlab")
         should_email(@u_watcher.id)
         should_email(@u_participating.id)
         should_not_email(@u_disabled.id)
-        notification.project_was_moved(project, "gitlab/gitlab")
       end
       def should_email(user_id)
-        expect(Notify).to receive(:project_was_moved_email).with(project.id, user_id, "gitlab/gitlab")
+        expect(sent?('Project was moved', user_id)).to be_truthy
       end
       def should_not_email(user_id)
-        expect(Notify).not_to receive(:project_was_moved_email).with(project.id, user_id, "gitlab/gitlab")
+        expect(sent?('Project was moved', user_id)).to be_falsey
       end
     end
   end
@@ -469,4 +483,13 @@ describe NotificationService do
     issuable.subscriptions.create(user: @subscriber, subscribed: true)
     issuable.subscriptions.create(user: @unsubscriber, subscribed: false)
   end
+
+  def sent?(subject, recipient_id)
+    recipient = User.find(recipient_id)
+
+    deliveries.any? do |delivery|
+      delivery.subject.include?(subject) &&
+        delivery.to.include?(recipient.notification_email)
+    end
+  end
 end