Possible regression 4.0.0.beta4 to 4.0.0.rc1: List of ActionMailer deliveries is not cleared between specs

[OtherCroissant]

What Ruby, Rails and RSpec versions are you using?

Ruby version: 2.7.0
Rails version: 6.0.2
RSpec version: 4.0.0.rc1

The following is included for these mailer tests:

RSpec.configure do |config|
config.include ActionMailer::TestHelper
end

Observed behaviour

When running specs, we check the to adresses that are set for the ActionMailer::Base.deliveries, like this:

expect(ActionMailer::Base.deliveries.map(&:to).flatten.include?(leo.email)).to eq true
expect(ActionMailer::Base.deliveries.map(&:to).flatten.sort).to eq([droid.email, leo.email])

This works for version 4.0.0.beta3 and 4.0.0.beta4, but for 4.0.0.rc1, this will break.
It seems that the deliveries array is not cleared between specs. The list of email addresses that are stored in the TO of the mailer, will get larger after each spec. Therefore, running each spec seperately works, but when the fourth or fifth spec is ran, the list of adresses is way bigger than expected:

     expected: ["[email protected]", "[email protected]"]
            got: ["[email protected]", "[email protected]", "[email protected]", "[email protected]", "[email protected]"]

Expected behaviour

The ActionMailer instance is reset between each spec and the TO, CC and BCC attributes of ActionMailer::Base.deliveries should be cleared between each spec.

Can you provide an example app?

I have isolated the issue into an example app:
https://github.com/CUnknown/rspec-rails-bug-example

[benoittgt]

At the moment I see nothing relevant here v4.0.0.beta3...v4.0.0.rc1

Did you try v4.0.0.beta3?

I think that would be possible, please let me know if that is really neccesarry!

Yes please

[OtherCroissant]

Yes, I tried v4.0.0.beta3 and that works fine.

[JonRowe]

Have you tried v4.0.0.beta4? (The narrower the scope the better 😂)

[OtherCroissant]

Ah, I'm sorry, I wasn't aware of a beta 4. v4.0.0.beta4 seems to work fine too!

[pirj]

I couldn't find anything suspicious. Can you help us by bisecting the failure to a specific commit @cunknown ?

[OtherCroissant]

I don't understand why I have this result. But this was my approach:
running bin/rspec --bisect --seed 12345 spec/that/fails.rb
using a local clone of the github repo 'rspec/rspec-rails', pointing to this folder in the Gemfile.
Starting the git bisect and between every bisect step, do a bundle update of rspec-rails, and run the minimal reproduction command for the rspec tests that fail (with the original seed offcourse).

git bisect start
# bad: [69acdc47e5a3f4235a19b88895ba753e3a924275] v4.0.0.rc1
git bisect bad 69acdc47e5a3f4235a19b88895ba753e3a924275
# good: [108a422552fafbccbe4626a0f30e654806c6c3e6] v4.0.0.beta4
git bisect good 108a422552fafbccbe4626a0f30e654806c6c3e6
# bad: [53b955109b73ca16d6a9257c3aa165af4e8d72ce] Changelog for #2217
git bisect bad 53b955109b73ca16d6a9257c3aa165af4e8d72ce
# bad: [15d2c6133856bb4dc91ade5dbe8b43d64a6c7df7] Properly Rename Generator Directory
git bisect bad 15d2c6133856bb4dc91ade5dbe8b43d64a6c7df7
# bad: [5b2dee4756db91b8d056b2ef5783a1648b51876c] Remove IntegrationTest::Behavior from SystemExampleGroup
git bisect bad 5b2dee4756db91b8d056b2ef5783a1648b51876c

[9aee3b6ec4587dbd9d475dab04606205a1d4d949] Merge pull request #2261 from rspec/get-rid-of-redundant-require

I did this procedure twice because I could not understand why removing require 'spec_helper' from the rspec-rails specs would make any difference for my project.

What am I doing wrong?

[JonRowe]

Several of those seem bad :/, if you have the time to investigate further maybe step through the commits in your gemfile manually

[OtherCroissant]

I have isolated the issue into an example app:
https://github.com/CUnknown/rspec-rails-bug-example

When bundling with '4.0.0.beta4', the problem_spec.rb will pass. When bundling with 4.0.0.rc1, the specs will fail.

  mail two
  mail one (FAILED - 1)
  mail four (FAILED - 2)
  mail five (FAILED - 3)
  mail six (FAILED - 4)
  mail three (FAILED - 5)

Failures:

  1) problem mail one
     Failure/Error: expect(ActionMailer::Base.deliveries.map(&:to).flatten.sort).to eq(['[email protected]'])
     
       expected: ["[email protected]"]
            got: ["[email protected]", "[email protected]"]
     
       (compared using ==)
     
     
     
     # ./spec/system/problem_spec.rb:8:in `block (2 levels) in <top (required)>'

  2) problem mail four
     Failure/Error: expect(ActionMailer::Base.deliveries.map(&:to).flatten.sort).to eq(['[email protected]'])
     
       expected: ["[email protected]"]
            got: ["[email protected]", "[email protected]", "[email protected]"]
     
       (compared using ==)
     
     
     
     # ./spec/system/problem_spec.rb:23:in `block (2 levels) in <top (required)>'

  3) problem mail five
     Failure/Error: expect(ActionMailer::Base.deliveries.map(&:to).flatten.sort).to eq(['[email protected]'])
     
       expected: ["[email protected]"]
            got: ["[email protected]", "[email protected]", "[email protected]", "[email protected]"]
     
       (compared using ==)
     
     
     
     # ./spec/system/problem_spec.rb:28:in `block (2 levels) in <top (required)>'

  4) problem mail six
     Failure/Error: expect(ActionMailer::Base.deliveries.map(&:to).flatten.sort).to eq(['[email protected]'])
     
       expected: ["[email protected]"]
            got: ["[email protected]", "[email protected]", "[email protected]", "[email protected]", "[email protected]"]
     
       (compared using ==)
     
     
     
     # ./spec/system/problem_spec.rb:33:in `block (2 levels) in <top (required)>'

  5) problem mail three
     Failure/Error: expect(ActionMailer::Base.deliveries.map(&:to).flatten.sort).to eq(['[email protected]'])
     
       expected: ["[email protected]"]
            got: ["[email protected]", "[email protected]", "[email protected]", "[email protected]", "[email protected]", "[email protected]"]
     
       (compared using ==)

     # ./spec/system/problem_spec.rb:18:in `block (2 levels) in <top (required)>'

[benoittgt]

With your repo it seems the error is because of
3e4c770

3e4c770f529237dbaa5877ff80aa0d0b4594b0d1 is the first bad commit
commit 3e4c770f529237dbaa5877ff80aa0d0b4594b0d1
Author: Jonathan Rochkind
Date:   Wed Dec 18 16:49:05 2019 -0500

    Remove IntegrationTest::Behavior from SystemExampleGroup

    To better mirror current Rails ActionDispatch::SystemTestCase. And resolve problems with inability to set ActiveJob queue type in systems example group.

:040000 040000 d96e70b894f4291376ed17609e29cbecaa3effde 5215240043eafd857e27add269385957e5f25c99 M	features
:040000 040000 6ff301ae967ea1555ccc4ba86770d57d867b0d94 a80447f68f062b63d25f705770398fbd78d2c5dd M	lib

[OtherCroissant]

Looks like that besides the added test, the removal of the following results in reusing the ActionMailer::Base instance between tests?

        other.include ActionDispatch::IntegrationTest::Behavior

[benoittgt]

I am able to still have green system specs and green repo project test suite with this patch:

diff --git a/lib/rspec/rails/example/system_example_group.rb b/lib/rspec/rails/example/system_example_group.rb
index 691b1f0c..e72a6f76 100644
--- a/lib/rspec/rails/example/system_example_group.rb
+++ b/lib/rspec/rails/example/system_example_group.rb
@@ -75,6 +75,7 @@ module RSpec
         original_after_teardown =
           ::ActionDispatch::SystemTesting::TestHelpers::SetupAndTeardown.instance_method(:after_teardown)

+        other.include ::ActionDispatch::IntegrationTest::Behavior
         other.include ::ActionDispatch::SystemTesting::TestHelpers::SetupAndTeardown
         other.include ::ActionDispatch::SystemTesting::TestHelpers::ScreenshotHelper
         other.include BlowAwayTeardownHooks
@@ -102,6 +103,9 @@ module RSpec

         before do
           @routes = ::Rails.application.routes
+          # (ActiveJob::Base.descendants << ActiveJob::Base).each(&:disable_test_adapter)
+          # or
+          ActiveJob::Base.disable_test_adapter
         end

Run after bundle exec rake smoke:app, bundle exec cucumber features/system_specs/system_specs.feature

My question is: Do we want to set ActiveJob::Base.disable_test_adapter or (ActiveJob::Base.descendants << ActiveJob::Base).each(&:disable_test_adapter) in system tests? Or do we want user to set it by themselves?
I get it in a pretty complicated test from Rails but there is an ongoing discussion and probably a fix one day about this subject here rails/rails#37270

When I see shared fixes [1], [2], [3] it seems that we should revert 3e4c770 and maybe disable test adapter ourself. What do you think @JonRowe and @pirj ?

Proposal: #2292

Side note from the doc: https://github.com/rails/rails/blob/f265e0ddb1139a91635b7905aae1be76b22c6db1/guides/source/testing.md#L1667-L1670

[JonRowe]

We don't want to disable the test adapter, we just need to not share the deliveries between tests. IIRC the reason why it got removed was it preventing setting other adapters.. So maybe we have to reset the deliveries between tests?

[benoittgt]

I think if we do this we will have deal to clean queue for other part of rails code. I think removing ActionDispatch::IntegrationTest::Behavior was maybe not a good idea.

I will dig deeper to see what people do.

[benoittgt]

On mailboxer gem they are doing this since 2014:

  # Rspec only clears out ActionMailer::Base#deliveries for mailers specs
  config.after(:each, type: :integration){ ActionMailer::Base.deliveries.clear }

(source)

[benoittgt]

Another proposal could be:

--- a/lib/rspec/rails/example/system_example_group.rb
+++ b/lib/rspec/rails/example/system_example_group.rb
@@ -101,6 +101,7 @@ module RSpec
         end

         before do
+          ActionMailer::Base.deliveries.clear

But system specs are probably not enough.

[pirj]

Might be quite radical, but WDYT of:

if RSpec::Rails::FeatureCheck.has_active_job?
  before { ActionMailer::Base.deliveries.clear }
end

not only for SystemExampleGroup or RailsExampleGroup, but for every example.
I can imagine some "unit of work" test that doesn't fall into any of our existing types that would schedule a delivery.

Side question: does having ActiveJob in place automatically mean we have ActionMailer? We kind of assume that when requiring have_enqueued_mail matcher, but will it work the same way for a before hook when ActiveJob is part of the application, while ActionMailer isn't?

[JonRowe]

I think we just check if we have deliveries, if so clear it each test.

[benoittgt]

Another draft proposal.

--- a/lib/rspec/rails/example/rails_example_group.rb
+++ b/lib/rspec/rails/example/rails_example_group.rb
@@ -12,6 +12,14 @@ module RSpec
       include RSpec::Rails::MinitestLifecycleAdapter
       include RSpec::Rails::MinitestAssertionAdapter
       include RSpec::Rails::FixtureSupport
+
+      included do
+        after do
+          if defined?(ActionMailer) && ActionMailer::Base.deliveries.any?
+            ActionMailer::Base.deliveries.clear
+          end
+        end
+      end
     end
   end
 end

lib/rspec/rails/example/rails_example_group.rb help us to deal with all rspec-rails example group. We will still have issues if we test for example a file in lib/ or a service like:

# spec/services/mailer_service_spec.rb
# frozen_string_literal: true

require 'system_helper'

RSpec.describe MailerService do
  describe '#call' do
    context 'one email' do
      it do
        MailerService.call(to: '[email protected]')

        expect(ActionMailer::Base.deliveries.map(&:to).flatten.sort).to eq(['[email protected]'])
      end
    end

    context 'other email' do
      it do
        MailerService.call(to: '[email protected]')

        expect(ActionMailer::Base.deliveries.map(&:to).flatten.sort).to eq(['[email protected]'])
      end
    end
  end
end

One of the example will fail because it is not cleaned. Is there a better place to put it? I dig into the code but didn't find something like a prepend to spec_helper. You load rspec-rails, all rspec example are cleaned.

[JonRowe]

 # lib/rspec/rails/configuration.rb:138
 if RSpec::Rails::FeatureCheck.has_action_mailer?
   config.include RSpec::Rails::MailerExampleGroup, type: :mailer
+  config.after { ActionMailer::Base.deliveries.clear }
 end

[benoittgt]

That's perfect. Thanks Jon. I will write the test tomorrow.