Prevent internal ActiveSupport test helpers to call assert_nothing_raised

[benoittgt]

This method has been moved in rails/rails@3cece0b
We do not include ActiveSupport::Testing::Assertion at the moment.

To avoid having to deal with complicated scenarios like Minitest errors. We redefine our own methods in rspec-rails.

---

I am really not sure about the approach.

Few questions:

• Do we want to include all of ActiveSupport::Testing::Assertions? I was not tempted because:
  1. Minitest error to handle like https://github.com/rails/rails/blob/master/activesupport/lib/active_support/testing/assertions.rb#L36
  2. Also I do not want to encourage people to use ActiveSupport assertions methods when they can use rspec ones.
• Is it the right way do to it. I dig a little bit into https://github.com/rspec/rspec-rails/blob/main/lib/rspec/rails/adapters.rb#L30-L36 but found it was not probably the best place to put the code.
• I do not understand why it does not work on previous versions ATM. I should have access to ActiveSupport::TestCase and "assert_not_raised"

---

As always feel free to push on this branch and amend my commit.

Fix: #2410

[benoittgt]

Still in draft because for the CI but open for review.

Sorry I am very tired.

[JonRowe]

@benoittgt I would just include the new module from Rails, my reasoning is that their test helpers are built on their assertions, and by implementing them ourselves we will always lag behind. Take care of yourself!

[JonRowe]

Do we need to check if this is defined? / Can we pull it into one of our existing modules?

[benoittgt]

I was very curious about the CI.
I am pretty confident. This class exist since at least Rails 4.2 https://github.com/rails/rails/blob/4-2-stable/activesupport/lib/active_support/testing/assertions.rb

I tried in RailsExampleGroup but it was not working, I still had the same error. I will keep digging.

[JonRowe]

Ok! I'll wait for your experiments to finish :)

[benoittgt]

I added a check on version. It's pretty basic but I think enough for our needs.

[JonRowe]

👍

[benoittgt]

The commit that probably impact us is https://github.com/rails/rails/pull/26293/files
But I don't understand because activesupport/lib/active_support/testing/assertions.rb is included in activesupport/lib/active_support/test_case.rb.

[benoittgt]

Ok. It fails on 5.0.7.2.

Even with

--- a/lib/rspec/rails/configuration.rb
+++ b/lib/rspec/rails/configuration.rb
@@ -72,7 +72,9 @@ module RSpec
       config.include RSpec::Rails::FixtureSupport, :use_fixtures

       # Prevent test from failing on undefined method used in ActiveSupport testing helpers
-      config.include ActiveSupport::Testing::Assertions
+      if ::Rails::VERSION::STRING.to_f >= 6.1
+        config.include ActiveSupport::Testing::Assertions
+      end

I have a failure locally like the CI. How people can say it was working before? Maybe active_support/test_case is loaded into specific type of specs and we should validate our patch using example spec?

I am wondering if the best approach is to require ActiveSupport::Testing::Assertion into our different example like here

rspec-rails/lib/rspec/rails/example/mailer_example_group.rb

Line 20 in 9217638

include ActionMailer::TestCase::Behavior

[pirj]

Way to go! 👍
Thanks

[JonRowe]

I think this implementation needs to go in lib/rspec/rails/adapters.rb or lib/rspec/rails/example/rails_example_group.rb

[benoittgt]

I tried this initially but was not able to make it work.

For lib/rspec/rails/example/rails_example_group.rb with:

--- a/lib/rspec/rails/example/rails_example_group.rb
+++ b/lib/rspec/rails/example/rails_example_group.rb
@@ -12,6 +12,11 @@ module RSpec
       include RSpec::Rails::MinitestLifecycleAdapter
       include RSpec::Rails::MinitestAssertionAdapter
       include RSpec::Rails::FixtureSupport
+
+      # Prevent test from failing on undefined method used in ActiveSupport testing helpers
+      if ::Rails::VERSION::STRING.to_f >= 6.0
+        include ActiveSupport::Testing::Assertions
+      end

From a Rails app that use rspec-rails with a test that call assert_nothing_raised, I get:

NameError:
  uninitialized constant ActiveSupport::Testing::Assertion

For lib/rspec/rails/adapters.rb this is pretty complicated inside. I tried to put in in multiple places but I am having error like from my Rails app:

NameError:
  uninitialized constant ActiveSupport::Testing

The other place were we properly load an active support testing class is in
https://github.com/rspec/rspec-rails/blob/rails-6-1-dev/lib/rspec/rails/fixture_file_upload_support.rb#L42
and then it is loaded in configuration

rspec-rails/lib/rspec/rails/configuration.rb

Line 94 in 0e5d606

config.include RSpec::Rails::FixtureFileUploadSupport

I am stuck to provide you another solution ATM. 😬

[doits]

This is fixed in Rails 6.1.1. rails/rails#40780, nothing to do here 🎉

[benoittgt]

I can confirm that the regression is no longer visible in 6.1.1 but my spec here do not reflect that change, it is still failing.

I do not see a place where we handle this kind of regression check at a Rails level.

For the Rails level spec, we have feature spec, but they are also used for documentation. What we could do, could be to make the small app snippet as in #2423 ? Or last option, do not test for this regression that is more related to Rails itself. We never use assert_nothing_raise in our code base. WDYT @JonRowe and @pirj.

[pirj]

Removed the fix, only kept the regression test as per #2410 (comment).

Fails on 6.1.1, and 6.0 as well.

Snippets don't help much, there's still the same error.

rails/rails#40780 brought this method back to ActiveJob::TestHelper, and I struggle to understand how it can help us to get rid of the error. According to features/job_specs/job_spec.feature:

Job specs provide alternative assertions to those available in ActiveJob::TestHelper and help assert behaviour of the jobs themselves and that other entities correctly enqueue them.

Sorry for ruining the branch (I've also switched the target PR branch to rails-6-1-dev). Please feel free to push-force your local.

[pirj]

I'm not sure what is the real use case where assert_nothing_raised { ... } could be preferable over expect { ... }.not_to raise_error.
Is there a real use case?

[benoittgt]

I am not sure the fix on Rails it the good solution because the initial move was rails/rails@3cece0b.

And for your last comment I have the same question. There is an interesting discussion on it here: minitest/minitest#809

[benoittgt]

This should normally work, but it is not:

if __FILE__ =~ /^snippets/
  fail "Snippets are supposed to be run from their own directory to avoid side " \
       "effects as e.g. the root `Gemfile`, or `spec/spec_helpers.rb` to be " \
       "loaded by the root `.rspec`."
end

# We opt-out from using RubyGems, but `bundler/inline` requires it
require 'rubygems'

require "bundler/inline"

# We pass `false` to `gemfile` to skip the installation of gems,
# because it may install versions that would conflict with versions
# from the main `Gemfile.lock`.
gemfile(false) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Those Gemfiles carefully pick the right versions depending on
  # settings in the ENV, `.rails-version` and `maintenance-branch`.
  Dir.chdir('..') do
    # This Gemfile expects `maintenance-branch` file to be present
    # in the current directory.
    eval_gemfile 'Gemfile-rspec-dependencies'
    # This Gemfile expects `.rails-version` file
    eval_gemfile 'Gemfile-rails-dependencies'
  end

  gem "rspec-rails", path: "../"
end

# Run specs at exit
require "rspec/autorun"

# Initialization
require "active_job/railtie"

def skip_reason
  if Gem::Version.new(Rails.version) >= Gem::Version.new('6.1.1')
    false
  else
    "Skipping because Rails version is bellow 6.0 (Rails version: #{Rails.version})"
  end
end

# Rails project specs
RSpec.describe 'ActiveSupport::Testing::Assertions requirements', skip: skip_reason do
  it 'does not raise that "assert_nothing_raised" is undefined' do
    expect {
      assert_nothing_raised do
        :foo
      end
    }.to_not raise_error
  end
end

When we "puts caller" from where the new TestingAssertion has been added, we can see

/Users/bti/code/rails/activejob/lib/active_job/test_helper.rb:7:in `<module:ActiveJob>'
/Users/bti/code/rails/activejob/lib/active_job/test_helper.rb:5:in `<main>'
/Users/bti/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/zeitwerk-2.4.2/lib/zeitwerk/kernel.rb:34:in `require'
/Users/bti/code/rails/activejob/lib/active_job/railtie.rb:35:in `block (2 levels) in <class:Railtie>'

[pirj]

I suggest we just close this and if questions arise, recommend including a module from Rails that provides this method. It would be an arguable situation for us to provide access to a method that is an extension of Minitest syntax.

[benoittgt]

Thanks Phil for the followup. Yep I agree with you.