Improve performance of 'spec/services/notification_service_spec.rb'

Signed-off-by: Rémy Coutable <remy@rymai.me>

spec/services/notification_service_spec.rb

@@ -10,10 +10,12 @@ describe NotificationService, :mailer do
   let(:notification) { described_class.new }
   let(:assignee) { create(:user) }
 
-  around do |example|
-    perform_enqueued_jobs do
-      example.run
-    end
+  around(:example, :deliver_mails_inline) do |example|
+    # This is a temporary `around` hook until all the examples check the
+    # background jobs queue instead of the delivered emails array.
+    # `perform_enqueued_jobs` makes the ActiveJob jobs (e.g. mailer jobs) run inline
+    # compared to `Sidekiq::Testing.inline!` which makes the Sidekiq jobs run inline.
+    perform_enqueued_jobs { example.run }
   end
 
   shared_examples 'altered milestone notification on issue' do
@@ -187,26 +189,41 @@ describe NotificationService, :mailer do
   describe 'Keys' do
     describe '#new_key' do
       let(:key_options) { {} }
-      let!(:key) { create(:personal_key, key_options) }
+      let!(:key) { build_stubbed(:personal_key, key_options) }
+
+      subject { notification.new_key(key) }
 
-      it { expect(notification.new_key(key)).to be_truthy }
+      it "sends email to key owner" do
+        expect { subject }.to have_enqueued_email(key.id, mail: "new_ssh_key_email")
+      end
 
-      describe 'never emails the ghost user' do
+      describe "never emails the ghost user" do
         let(:key_options) { { user: User.ghost } }
 
-        it { should_not_email_anyone }
+        it "does not send email to key owner" do
+          expect { subject }.not_to have_enqueued_email(key.id, mail: "new_ssh_key_email")
+        end
       end
     end
   end
 
   describe 'GpgKeys' do
     describe '#new_gpg_key' do
-      let!(:key) { create(:gpg_key) }
+      let(:key_options) { {} }
+      let(:key) { create(:gpg_key, key_options) }
+
+      subject { notification.new_gpg_key(key) }
+
+      it "sends email to key owner" do
+        expect { subject }.to have_enqueued_email(key.id, mail: "new_gpg_key_email")
+      end
 
-      it { expect(notification.new_gpg_key(key)).to be_truthy }
+      describe "never emails the ghost user" do
+        let(:key_options) { { user: User.ghost } }
 
-      it 'sends email to key owner' do
-        expect { notification.new_gpg_key(key) }.to change { ActionMailer::Base.deliveries.size }.by(1)
+        it "does not send email to key owner" do
+          expect { subject }.not_to have_enqueued_email(key.id, mail: "new_gpg_key_email")
+        end
       end
     end
   end
@@ -215,10 +232,10 @@ describe NotificationService, :mailer do
     describe '#access_token_about_to_expire' do
       let_it_be(:user) { create(:user) }
 
-      it 'sends email to the token owner' do
-        expect(notification.access_token_about_to_expire(user)).to be_truthy
+      subject { notification.access_token_about_to_expire(user) }
 
-        should_email user
+      it 'sends email to the token owner' do
+        expect { subject }.to have_enqueued_email(user, mail: "access_token_about_to_expire_email")
       end
     end
   end
@@ -231,6 +248,8 @@ describe NotificationService, :mailer do
       let(:author) { create(:user) }
       let(:note) { create(:note_on_issue, author: author, noteable: issue, project_id: issue.project_id, note: '@mention referenced, @unsubscribed_mentioned and @outsider also') }
 
+      subject { notification.new_note(note) }
+
       before do
         build_team(project)
         project.add_maintainer(issue.author)
@@ -260,32 +279,23 @@ describe NotificationService, :mailer do
             reset_delivered_emails!
           end
 
-          it do
-            expect(SentNotification).to receive(:record).with(issue, any_args).exactly(10).times
-
-            notification.new_note(note)
-
-            should_email(@u_watcher)
-            should_email(note.noteable.author)
-            should_email(note.noteable.assignees.first)
-            should_email(@u_custom_global)
-            should_email(@u_mentioned)
-            should_email(@subscriber)
-            should_email(@watcher_and_subscriber)
-            should_email(@subscribed_participant)
-            should_email(@u_custom_off)
-            should_email(@unsubscribed_mentioned)
-            should_not_email(@u_guest_custom)
-            should_not_email(@u_guest_watcher)
-            should_not_email(note.author)
-            should_not_email(@u_participating)
-            should_not_email(@u_disabled)
-            should_not_email(@unsubscriber)
-            should_not_email(@u_outsider_mentioned)
-            should_not_email(@u_lazy_participant)
+          it 'sends emails to recipients' do
+            subject
+
+            expect_delivery_jobs_count(10)
+            expect_enqueud_email(@u_watcher.id, note.id, nil, mail: "note_issue_email")
+            expect_enqueud_email(note.noteable.author.id, note.id, nil, mail: "note_issue_email")
+            expect_enqueud_email(note.noteable.assignees.first.id, note.id, nil, mail: "note_issue_email")
+            expect_enqueud_email(@u_custom_global.id, note.id, nil, mail: "note_issue_email")
+            expect_enqueud_email(@u_mentioned.id, note.id, "mentioned", mail: "note_issue_email")
+            expect_enqueud_email(@subscriber.id, note.id, "subscribed", mail: "note_issue_email")
+            expect_enqueud_email(@watcher_and_subscriber.id, note.id, "subscribed", mail: "note_issue_email")
+            expect_enqueud_email(@subscribed_participant.id, note.id, "subscribed", mail: "note_issue_email")
+            expect_enqueud_email(@u_custom_off.id, note.id, nil, mail: "note_issue_email")
+            expect_enqueud_email(@unsubscribed_mentioned.id, note.id, "mentioned", mail: "note_issue_email")
           end
 
-          it "emails the note author if they've opted into notifications about their activity" do
+          it "emails the note author if they've opted into notifications about their activity", :deliver_mails_inline do
             note.author.notified_of_own_activity = true
 
             notification.new_note(note)
@@ -294,7 +304,7 @@ describe NotificationService, :mailer do
             expect(find_email_for(note.author)).to have_header('X-GitLab-NotificationReason', 'own_activity')
           end
 
-          it_behaves_like 'project emails are disabled' do
+          it_behaves_like 'project emails are disabled', check_delivery_jobs_queue: true do
             let(:notification_target)  { note }
             let(:notification_trigger) { notification.new_note(note) }
           end
@@ -302,21 +312,21 @@ describe NotificationService, :mailer do
 
         it 'filters out "mentioned in" notes' do
           mentioned_note = SystemNoteService.cross_reference(mentioned_issue, issue, issue.author)
+          reset_delivered_emails!
 
-          expect(Notify).not_to receive(:note_issue_email)
           notification.new_note(mentioned_note)
+
+          expect_no_delivery_jobs
         end
 
         context 'participating' do
           context 'by note' do
             before do
-              reset_delivered_emails!
               note.author = @u_lazy_participant
               note.save
-              notification.new_note(note)
             end
 
-            it { should_not_email(@u_lazy_participant) }
+            it { expect { subject }.not_to have_enqueued_email(@u_lazy_participant.id, note.id, mail: "note_issue_email") }
           end
         end
       end
@@ -335,7 +345,7 @@ describe NotificationService, :mailer do
         end
 
         shared_examples 'new note notifications' do
-          it do
+          it 'sends notifications', :deliver_mails_inline do
             notification.new_note(note)
 
             should_email(note.noteable.author)
@@ -359,7 +369,7 @@ describe NotificationService, :mailer do
 
         it_behaves_like 'new note notifications'
 
-        it_behaves_like 'project emails are disabled' do
+        it_behaves_like 'project emails are disabled', check_delivery_jobs_queue: true do
           let(:notification_target)  { note }
           let(:notification_trigger) { notification.new_note(note) }
         end
@@ -378,13 +388,13 @@ describe NotificationService, :mailer do
 
             notification.new_note(note)
 
-            should_email(user)
+            expect_enqueud_email(user.id, note.id, nil, mail: "note_issue_email")
           end
         end
       end
     end
 
-    context 'confidential issue note' do
+    context 'confidential issue note', :deliver_mails_inline do
       let(:project) { create(:project, :public) }
       let(:author) { create(:user) }
       let(:assignee) { create(:user) }
@@ -441,7 +451,7 @@ describe NotificationService, :mailer do
       end
     end
 
-    context 'issue note mention' do
+    context 'issue note mention', :deliver_mails_inline do
       let(:project) { create(:project, :public) }
       let(:issue) { create(:issue, project: project, assignees: [assignee]) }
       let(:mentioned_issue) { create(:issue, assignees: issue.assignees) }
@@ -507,7 +517,7 @@ describe NotificationService, :mailer do
       end
     end
 
-    context 'project snippet note' do
+    context 'project snippet note', :deliver_mails_inline do
       let!(:project) { create(:project, :public) }
       let(:snippet) { create(:project_snippet, project: project, author: create(:user)) }
       let(:author) { create(:user) }
@@ -551,7 +561,7 @@ describe NotificationService, :mailer do
       end
     end
 
-    context 'personal snippet note' do
+    context 'personal snippet note', :deliver_mails_inline do
       let(:snippet) { create(:personal_snippet, :public, author: @u_snippet_author) }
       let(:note)    { create(:note_on_personal_snippet, noteable: snippet, note: '@mentioned note', author: @u_note_author) }
 
@@ -600,7 +610,7 @@ describe NotificationService, :mailer do
       end
     end
 
-    context 'commit note' do
+    context 'commit note', :deliver_mails_inline do
       let(:project) { create(:project, :public, :repository) }
       let(:note) { create(:note_on_commit, project: project) }
 
@@ -659,7 +669,7 @@ describe NotificationService, :mailer do
       end
     end
 
-    context "merge request diff note" do
+    context "merge request diff note", :deliver_mails_inline do
       let(:project) { create(:project, :repository) }
       let(:user) { create(:user) }
       let(:merge_request) { create(:merge_request, source_project: project, assignees: [user], author: create(:user)) }
@@ -691,11 +701,11 @@ describe NotificationService, :mailer do
     end
   end
 
-  describe '#send_new_release_notifications' do
+  describe '#send_new_release_notifications', :deliver_mails_inline, :sidekiq_inline do
     context 'when recipients for a new release exist' do
       let(:release) { create(:release) }
 
-      it 'calls new_release_email for each relevant recipient', :sidekiq_might_not_need_inline do
+      it 'calls new_release_email for each relevant recipient' do
         user_1 = create(:user)
         user_2 = create(:user)
         user_3 = create(:user)
@@ -712,7 +722,7 @@ describe NotificationService, :mailer do
     end
   end
 
-  describe 'Participating project notification settings have priority over group and global settings if available' do
+  describe 'Participating project notification settings have priority over group and global settings if available', :deliver_mails_inline do
     let!(:group) { create(:group) }
     let!(:maintainer) { group.add_owner(create(:user, username: 'maintainer')).user }
     let!(:user1) { group.add_developer(create(:user, username: 'user_with_project_and_custom_setting')).user }
@@ -770,7 +780,7 @@ describe NotificationService, :mailer do
     end
   end
 
-  describe 'Issues' do
+  describe 'Issues', :deliver_mails_inline do
     let(:group) { create(:group) }
     let(:project) { create(:project, :public, namespace: group) }
     let(:another_project) { create(:project, :public, namespace: group) }
@@ -1423,7 +1433,7 @@ describe NotificationService, :mailer do
     end
   end
 
-  describe 'Merge Requests' do
+  describe 'Merge Requests', :deliver_mails_inline do
     let(:group) { create(:group) }
     let(:project) { create(:project, :public, :repository, namespace: group) }
     let(:another_project) { create(:project, :public, namespace: group) }
@@ -1898,7 +1908,7 @@ describe NotificationService, :mailer do
     end
   end
 
-  describe 'Projects' do
+  describe 'Projects', :deliver_mails_inline do
     let(:project) { create(:project) }
 
     before do
@@ -1989,7 +1999,7 @@ describe NotificationService, :mailer do
     end
   end
 
-  describe 'GroupMember' do
+  describe 'GroupMember', :deliver_mails_inline do
     let(:added_user) { create(:user) }
 
     describe '#new_access_request' do
@@ -2075,7 +2085,7 @@ describe NotificationService, :mailer do
     end
   end
 
-  describe 'ProjectMember' do
+  describe 'ProjectMember', :deliver_mails_inline do
     let(:project) { create(:project) }
     let(:added_user) { create(:user) }
 
@@ -2236,7 +2246,7 @@ describe NotificationService, :mailer do
     end
   end
 
-  context 'guest user in private project' do
+  context 'guest user in private project', :deliver_mails_inline do
     let(:private_project) { create(:project, :private) }
     let(:guest) { create(:user) }
     let(:developer) { create(:user) }
@@ -2291,7 +2301,7 @@ describe NotificationService, :mailer do
     end
   end
 
-  describe 'Pipelines' do
+  describe 'Pipelines', :deliver_mails_inline do
     describe '#pipeline_finished' do
       let(:project) { create(:project, :public, :repository) }
       let(:u_member) { create(:user) }
@@ -2507,7 +2517,7 @@ describe NotificationService, :mailer do
     end
   end
 
-  describe 'Pages domains' do
+  describe 'Pages domains', :deliver_mails_inline do
     let_it_be(:project, reload: true) { create(:project) }
     let_it_be(:domain, reload: true) { create(:pages_domain, project: project) }
     let_it_be(:u_blocked) { create(:user, :blocked) }
@@ -2560,7 +2570,7 @@ describe NotificationService, :mailer do
     end
   end
 
-  context 'Auto DevOps notifications' do
+  context 'Auto DevOps notifications', :deliver_mails_inline do
     describe '#autodevops_disabled' do
       let(:owner) { create(:user) }
       let(:namespace) { create(:namespace, owner: owner) }
@@ -2584,7 +2594,7 @@ describe NotificationService, :mailer do
     end
   end
 
-  describe 'Repository cleanup' do
+  describe 'Repository cleanup', :deliver_mails_inline do
     let(:user) { create(:user) }
     let(:project) { create(:project) }
 
@@ -2615,7 +2625,7 @@ describe NotificationService, :mailer do
     end
   end
 
-  context 'Remote mirror notifications' do
+  context 'Remote mirror notifications', :deliver_mails_inline do
     describe '#remote_mirror_update_failed' do
       let(:project) { create(:project) }
       let(:remote_mirror) { create(:remote_mirror, project: project) }
@@ -2653,7 +2663,7 @@ describe NotificationService, :mailer do
     end
   end
 
-  context 'with external authorization service' do
+  context 'with external authorization service', :deliver_mails_inline do
     let(:issue) { create(:issue) }
     let(:project) { issue.project }
     let(:note) { create(:note, noteable: issue, project: project) }

spec/support/helpers/email_helpers.rb

@@ -6,7 +6,12 @@ module EmailHelpers
   end
 
   def reset_delivered_emails!
+    # We shouldn't actually send the emails, but we keep the following line for
+    # back-compatibility until we only check the mailer jobs enqueued in Sidekiq
     ActionMailer::Base.deliveries.clear
+    # We should only check that the mailer jobs are enqueued in Sidekiq, hence
+    # clearing the background jobs queue
+    ActiveJob::Base.queue_adapter.enqueued_jobs.clear
   end
 
   def should_only_email(*users, kind: :to)

spec/support/helpers/notification_helpers.rb

@@ -36,4 +36,28 @@ module NotificationHelpers
     setting = user.notification_settings_for(resource)
     setting.update!(event => value)
   end
+
+  def expect_delivery_jobs_count(count)
+    expect(ActionMailer::DeliveryJob).to have_been_enqueued.exactly(count).times
+  end
+
+  def expect_no_delivery_jobs
+    expect(ActionMailer::DeliveryJob).not_to have_been_enqueued
+  end
+
+  def expect_any_delivery_jobs
+    expect(ActionMailer::DeliveryJob).to have_been_enqueued.at_least(:once)
+  end
+
+  def have_enqueued_email(*args, mailer: "Notify", mail: "", delivery: "deliver_now")
+    have_enqueued_job(ActionMailer::DeliveryJob).with(mailer, mail, delivery, *args)
+  end
+
+  def expect_enqueud_email(*args, mailer: "Notify", mail: "", delivery: "deliver_now")
+    expect(ActionMailer::DeliveryJob).to have_been_enqueued.with(mailer, mail, delivery, *args)
+  end
+
+  def expect_not_enqueud_email(*args, mailer: "Notify", mail: "", delivery: "deliver_now")
+    expect(ActionMailer::DeliveryJob).not_to have_been_enqueued.with(mailer, mail, *args, any_args)
+  end
 end

spec/support/shared_examples/services/notification_service_shared_examples.rb

@@ -3,7 +3,7 @@
 # Note that we actually update the attribute on the target_project/group, rather than
 # using `allow`.  This is because there are some specs where, based on how the notification
 # is done, using an `allow` doesn't change the correct object.
-RSpec.shared_examples 'project emails are disabled' do
+RSpec.shared_examples 'project emails are disabled' do |check_delivery_jobs_queue: false|
   let(:target_project) { notification_target.is_a?(Project) ? notification_target : notification_target.project }
 
   before do
@@ -16,7 +16,13 @@ RSpec.shared_examples 'project emails are disabled' do
 
     notification_trigger
 
-    should_not_email_anyone
+    if check_delivery_jobs_queue
+      # Only check enqueud jobs, not delivered emails
+      expect_no_delivery_jobs
+    else
+      # Deprecated: Check actual delivered emails
+      should_not_email_anyone
+    end
   end
 
   it 'sends emails to someone' do
@@ -24,7 +30,13 @@ RSpec.shared_examples 'project emails are disabled' do
 
     notification_trigger
 
-    should_email_anyone
+    if check_delivery_jobs_queue
+      # Only check enqueud jobs, not delivered emails
+      expect_any_delivery_jobs
+    else
+      # Deprecated: Check actual delivered emails
+      should_email_anyone
+    end
   end
 end