Cop idea: discourage the use of `assert_`/`refute_` Minitest matchers

[pirj]

Suggested by @Darhazer

RSpec Expectations allows using assert_/refute_ Minitest matchers However, RSpec comes with its matchers that pretty much cover the same:

- assert_equal a, b
+ expect(a).to eq(b)

A notable difference is that Minitest allows for an optional failure message. It's a lesser known RSpec feature, but it allows it, too:

- assert_equal a, b, "must be equal"
+ expect(a).to(eq(b), "must be equal")

[bquorning]

Do we know why rspec-expectations provide this feature? Is it going to be deprecated?

One of the non-goals of rubocop-rspec is

Enforcing should vs. expect syntax

and I wonder if the suggested cop doesn’t fall under this category.

[pirj]

Fair enough. RSpeс has a сonfiguration option for that:

RSpec.configure do |config|
  config.expect_with :minitest
end

which is exclusively using rspec-expectations in the default project spec/spec_helper.rb initializer (and most of the projects, I assume):

config.expect_with :rspec do |expectations|

If with this it's still possible to use Minitest's assertions, there's probably something wrong.

However, rspec-rails is using Rails' Minitests assertions internally, and I can't tell off the top of my head if those can be encapsulated and unintentionally leak, or there's just no simple way to encapsulate them and use rspec-expectations' matchers exclusively.

[pirj]

Indeed, it's coming from rspec-rails:

module RSpec
  module Rails
    # @api public
    # Common rails example functionality.
    module RailsExampleGroup
      # ...
      include RSpec::Rails::MinitestAssertionAdapter

I'll give it a shot to only include it if expect_with is configured to also allow :minitest.

[pirj]

https://github.com/rspec/rspec-rails/pull/2639

[pirj]

Fails, e.g.:

1) UploadsController DELETE #destroy redirects to the uploads list
     Failure/Error: expect(response).to redirect_to(uploads_url)

     NoMethodError:
       undefined method `assert' for #<RSpec::ExampleGroups::UploadsController::DELETEDestroy "redirects to the uploads list" (./spec/controllers/uploads_controller_spec.rb:138)>
     # ./spec/controllers/uploads_controller_spec.rb:141:in `block (3 levels) in <top (required)>'

If it's impossible to restrict this on RSpec level, I suppose we're left with static analysis. Unless I misunderstood your argument, @bquorning

[pirj]

There are also a few aliases that Rails defines here. I guess the cop could make this list configurable, and rubocop-rspec-rails would amend the list supplying those Rails ones? But how would auto-correction work? Or is it better to somehow make the cop extensible from other extensions?

[bquorning]

I am still not sure which problem this PR fixes. Minitest matchers are not loaded by default. This example:

❯ ruby -r rspec/autorun -e 'describe "foo" do; it "bar" do; assert_equal(1, 2); end; end'

gives me the error undefined method `assert_equal' for ...

[pirj]

They are if you use rspec-rails.

It makes sense to re-target this cop to be RSpec/Rails/MinitestAssertions and eventually moving it to rubocop-rspec-rails under RSpecRails/ namespace.

[pirj]

Let's keep this open, since there are a few more matchers than can be covered.

[mvz]

Please note that the order of the arguments of assert_equals is reversed from the expect syntax: The actual is the second argument. See http://docs.seattlerb.org/minitest/Minitest/Assertions.html#method-i-assert_equal

[pirj]

test-unit assertions, for completeness https://www.rubydoc.info/gems/test-unit/2.3.2/Test/Unit/Assertions

[G-Rath]

Also here's the Rails aliased assertion methods: https://github.com/rails/rails/blob/main/activesupport/lib/active_support/test_case.rb#L153