shoulda-matchers 5.0.0

[mcmire]

I know we've been talking about this lately so I figured that it would be better to talk about it in one place.

What do we want to include in 5.0.0? We know that we want to remove support for old Rubies and Rails. So minimally we have:

• Remove support for Rails 4.2
• Remove support for Ruby 2.4
• Remove any code that is specifically aimed for compatibility with these things (code in RailsShim, boolean checks, etc.)

But, given this is a major release, if we've been wanting to make breaking changes, now is the time to do it. So we should think about that too. Something I've been wanting to know is, how many matchers do people actually use? Which matchers are used the most? Which ones are used the least? Given that every matcher we add creates more maintenance cost, are there any matchers that we can outright remove?

Based on that, here are some thoughts on possible breaking changes:

ActionController matchers

• Some of the controller matchers we offer only apply to Minitest. These matchers were originally added as matcher versions of assertions that Rails provided in Test::Unit land. However, rspec-rails has some equivalent matchers, some of which are named the same. These matchers are:

  • redirect_to (also called redirect_to in rspec-rails)
  • render_template (also called render_template)
  • render_with_layout (already captured by rspec-rails's render_template)
  • respond_with (covered by rspec-rails's have_http_status matcher)

  So, ideally, 1) we should not add these matchers to RSpec example groups so that users can make use of rspec-rails's matchers freely without interference. Also, 2) in Minitest, there is no real reason that these matchers shouldn't be equivalent to rspec-rails's matchers. So perhaps there is a way to simply import these matchers from rspec-rails into Minitest test cases rather than reimplement them as we have done. That would save us from having to maintain them.

• Some of our controller matchers are not very helpful. For instance, does set_session and set_flash really provide any more information that cannot be gleaned by saying something like it { expect(controller.flash[:foo]).to eq("bar") } or assert_equal "bar", controller.flash[:foo]? My guess is that it does not.

• I question the value of filter_param, which (aside from not being related to controllers) merely checks that you've added a value to Rails.application.config.filter_parameters. In my opinion, this is not a valuable test. My philosophy is that if you add a change to your code, and that change contains no logic, then it is not worth it to write a test for that code (because that code is deterministic and simple). There is no logic in making a configuration change to your Rails application, so in my opinion, there is no reason to have filter_param.

• I also question the value of the callback matchers. In my opinion, if you want to test a callback — and this applies to models, jobs, or any other thing that has the ability to add a callback — you should not test that the callback gets called, you should the actual behavior of the callback itself. However, the callback matchers don't do that. In fact, the callback matchers (like many of our matchers) rely on a private API in Rails. So not only are we encouraging poor testing practices, but we have a matcher that is liable to break on a future Rails change. We should consider removing this matcher.

• I also question the value of rescue_from, for a similar reason. All this does is check that you've registered a method for a particular class of exceptions. It doesn't actually test what happens when that exception occurs, which to me is a way more valuable test. We should consider also removing this matcher.

ActiveModel matchers

• allow_value — this matcher has a confusing API. In Wording allow_value .. with_message does not read well #1022 I proposed changing this matcher entirely (validate_attribute instead of allow_value). We should consider whether it is worth it to make this change.

ActiveRecord matchers

• have_db_index and have_db_column — these matchers have always been a little weird to me. Similar to my point about filter_param, your database schema is 1) not code, 2) doesn't contain logic and 3) is a form of configuration. So it seems extremely odd that we would give people a way to test their database. Plus, I've never come across a codebase that is not using shoulda-matchers where people consciously test the database in this manner, so it does not match the real world. I would really be curious to know how this is providing value for people. But in my opinion, these are not useful and we should consider removing them to save cost of maintenance.
• serialize — I really don't know how this is helpful either. Again, typically there is no logic around this. This matcher does not test behavior, so if your serialize class doesn't work then you won't get an error just from using the matcher.
• have_readonly_attribute — similar story. I think it's better to test that an attribute cannot be written to rather than that it shows up in MyModel.readonly_attributes.
• implicit_order_column — similar story.
• enum — again, I question the value of this matcher. How does this help anyone if this is (again) using a private API to verify that Rails knows your attribute is an enum attribute AND if the options you give to the matcher are exactly the same as what you're using in your model?
• accept_nested_attributes_for — similar. To be honest, I've never used this matcher.
• validate_uniqueness_of — there is a bit of inconsistency in the examples we give in the docs vs. how the matcher actually works. Namely, it doesn't make use of the subject in a logical way. This bug is documented in validate_uniqueness_of not using given record #1067 and perhaps we should consider changing this behavior. (Not sure if this would require a backward-incompatible change, but noting it in case it does.)

With that in mind, what are y'all's thoughts on these changes? I know that removing matchers would be much larger than merely removing old Rubies/Rails. I don't want to make people angry, and I don't want people to switch away from using this library. But at the same time 1) it's a lot of work to maintain shoulda-matchers, and the less stuff we can have it in it, the better; and 2) as I alluded to above, some of the matchers we have are not based on good testing philosophy or even real-world practice, and I want this library to help people find bugs faster in their code and not give them something that they could have easily done themselves. Anyway, I'm open to alternate opinions.

@vsppedro @KapilSachdev @guialbuk

[vsppedro]

I agree with those thoughts!

It would be nice to get some feedback from the community to answer some of those questions:

• Which matchers do you use the most?
• Which matchers do you use the least?
• Which matchers have you never used?

I think that asking for the job of the developer would be a good idea. Ask if he used it as a Fullstack or a Backend developer or had experience in both cases. Perhaps this information will give us some insight, but I'm still not sure what.

Maybe with a Google Form. What do you think?

[mcmire]

@vsppedro I think making a Google Form makes sense. Going off of your questions, maybe we list each matcher and offer a 0-5 scale (0 means "I've never heard of this matcher", 1 means "I've heard of this matcher but have never used it or would not use it", 5 means "I use it all the time")? Also maybe while we are asking that, we can ask people for some general feedback, i.e., "Is there anything you find confusing/frustrating about using shoulda-matchers?" or "Is there anything you would change about shoulda-matchers that would improve the developer experience?" Something like that.

[vsppedro]

Going off of your questions, maybe we list each matcher and offer a 0-5 scale (0 means "I've never heard of this matcher", 1 means "I've heard of this matcher but have never used it or would not use it", 5 means "I use it all the time")? Also maybe while we are asking that, we can ask people for some general feedback, i.e., "Is there anything you find confusing/frustrating about using shoulda-matchers?" or "Is there anything you would change about shoulda-matchers that would improve the developer experience?"

Sounds like a plan!

As soon as I have some time I will create this form with the questions we talked about and I will share it with your email.

[jonspalmer]

It would be great to see allow_value to be improved. The proposed API elsewhere is:

it { should validate_attribute(:brand).set_to("Suzuki").with_failure_message("is out of business") }

I'd propose a few other additions to this API:

1. Allows asserting the type 'invalid' vs the string message, or both
2. Allows asserting the existing value - set_to is optional

so these combos would be possible

it { should validate_attribute(:brand).set_to("Suzuki").with_failure_type(:out_of_business) }
it { should validate_attribute(:brand).set_to("Suzuki").with_failure_type(:out_of_business).with_failure_message("is out of business") }

context "with brand" do
  before do
    subject.brand = "Suzuki"
  end
  
  it { should validate_attribute(:brand).with_failure_type(:out_of_business) }
end

Note also that the ActiveModel::Errors api changed with Rails 6.1 and currently has deprecation warnings based on internals that will break in 6.2. We'll need a solution for that too.

[mcmire]

@jonspalmer We are working on fixing integration with Rails 6.1 so hopefully we can address that last issue you mentioned at least.

@vsppedro What are your ideas on the remainder of the next release? I don't want to get into a place where it takes a year to finish it and we're holding up other PRs that people are submitting, so I'm wondering if it makes sense to just stick to removing support for EOL'ed Ruby and Rails releases and that's it? Maybe we address some of the other things that I or other people have mentioned in future releases.

[vsppedro]

What are your ideas on the remainder of the next release?

For ruby:

• remove support for 2.4
• remove support for 2.5
• add support for 3.0

For rails:

• remove support for 4.2
• remove support for 5.0
• remove support for 5.1
• add support for 6.1

That is what I would like to achieve.

Yes, I know this is going to take a long time. If you want, I will concentrate only in removing, and in the next version, I’ll try to add all the pending support.

[mcmire]

@vsppedro That sounds good. No, I think it's okay to add support for Ruby 3.0 and Rails 6.1 and 7.0 now. I feel like people are going to be asking for it and based on what I've seen so far, I feel like it may involve making breaking changes to add that support, so I feel like it's worth working through all of that now vs. later.

[mcmire]

@jonspalmer I don't think we'll be able to get to changing allow_value right now due to the items mentioned above. However, what we could do, after we release 5.0, is introduce validate_attribute (or whatever the final API ends up being) as an alternative to allow_value and then slowly phase out allow_value and eventually drop it in the next major release. How does that sound?

[jonspalmer]

@mcmire Sounds very reasonable. Rails 6.x, 7.0 and Ruby 3 would be huge wins and should take priority. Phasing out allow_value with deprecation warnings etc. once there is a replacement sounds perfect. Let me know if there are contributions I could make there.

[mecampbellsoup]

Is it correct to say that Ruby 3 is not currently supported?

Failures:

  1) API::V1::FramesController PATCH #update is expected to (for PATCH #update) restrict parameters on :frame to :job_id, :workflow_id, :status, :started_at, :completed_at, and :pod_id
     Failure/Error:
       should permit(
           :job_id,
           :workflow_id,
           :status,
           :started_at,
           :completed_at,
           :pod_id,
         # :output,
         ).
           for(:update, params: params).

     ArgumentError:
       wrong number of arguments (given 2, expected 1)
     # /usr/local/bundle/gems/shoulda-matchers-4.5.1/lib/shoulda/matchers/rails_shim.rb:68:in `make_controller_request'
     # /usr/local/bundle/gems/shoulda-matchers-4.5.1/lib/shoulda/matchers/action_controller/permit_matcher.rb:254:in `block in matches?'
     # /usr/local/bundle/gems/shoulda-matchers-4.5.1/lib/shoulda/matchers/doublespeak/world.rb:29:in `with_doubles_activated'
     # /usr/local/bundle/gems/shoulda-matchers-4.5.1/lib/shoulda/matchers/action_controller/permit_matcher.rb:253:in `matches?'
     # ./spec/controllers/api/v1/frames_controller_spec.rb:122:in `block (3 levels) in <top (required)>'
     # ./spec/rails_helper.rb:111:in `block (3 levels) in <top (required)>'
     # /usr/local/bundle/gems/database_cleaner-core-2.0.1/lib/database_cleaner/strategy.rb:30:in `cleaning'
     # /usr/local/bundle/gems/database_cleaner-core-2.0.1/lib/database_cleaner/cleaners.rb:34:in `block (2 levels) in cleaning'
     # /usr/local/bundle/gems/database_cleaner-core-2.0.1/lib/database_cleaner/cleaners.rb:35:in `cleaning'
     # ./spec/rails_helper.rb:110:in `block (2 levels) in <top (required)>'

Believe this is related to https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/.

[vsppedro]

I'm sorry, we are not supporting ruby 3.0.0 for now.

I think that this problem that you're pointing out was solved in this PR: #1409. We will release it with ShouldaMatcher 5.0.0.

Unfortunelly, the CI don't work with Ruby 3.0.0 because of this bug: https://bugs.ruby-lang.org/issues/17536.

We are waiting for a new realese with this commit: ruby/ruby#4077. I tried to make it work with ruby-head, but did not work out as you can see in this comment: #1427 (comment)

I will give it another try.

I hope that the ruby 3.0.1 will be released soon. For now, I think you can use this PR if you can't wait: #1410.

I hope that helps.

[vsppedro]

I would love to hear any feedback from anyone using the 5.0.0.rc1. Thanks!

[vsppedro]

It seems to me that we can close this issue and maybe start a new discussion for the next release. 🎉

[mcmire]

That's fine with me :)