Should implicit default values have valid expectations?

[rhannequin]

Description

We can't expect for the presence of an option with value false if it's against a modified value of a Rails default.

Reproduction Steps

By default, the some options such as touch are set as false by the framework. shoulda-matchers supports having expectations about these default values:

# app/models/author.rb

class Author < ApplicationRecord
  has_many :articles
end

# spec/models/author_spec.rb

RSpec.describe Author do
  it { is_expected.to have_many(:articles).touch(false) }
  # => this expectation passes
end

My first point is that there are arguments not to provide such an expectation from shoulda-matchers. The default value for touch is a framework feature, not a business code one. In the same way it is considered bad practice to test frawework features in business code specs, the expectation in the previous example is testing that the framework is adding implicitly the value touch: false to associations.

My second point is that that the current way shoulda-matchers deals with boolean values can mess with explicit false values. We recently created the PR #1607 (in code review as I write this issue) to support a strict_loading option for associations. strict_loading can be enabled by default at the configuration level with the following instruction:

# config/application.rb
# or, config/environments/<environment>.rb

config.active_record.strict_loading_by_default = true
# In current versions of Rails, this is set to false by default

As business code, a developer could decide to have every single association with a strict_loading constraint, except one:

# app/models/author.rb

class Author < ApplicationRecord
  has_many :articles, strict_loading: false
end

It would make sense, in that case, to cover this exception:

# spec/models/author_spec.rb

RSpec.describe Author do
  it 'rejects an association missing explicit strict_loading option to false with the correct message' do
    message = [
      'Expected Author to have a has_many association called articles ',
      '(articles should have strict_loading set to false)',
    ].join

    expect {
      expect(described_class.new).
        to have_many(:articles).
        strict_loading(false) # strict_loading matcher is yet to be approved.
    }.to fail_with_message(message)
  end
end

In the current state, the expectation of the last example would not pass if the strict_loading option was missing from the model's association declaration, because of how shoulda-matchers deals with boolean casting and validation. In lib/shoulda/matchers/active_record/association_matchers/option_verifier.rb #type_cast, booleans are casted with !!value. This means a missing value for strict_loading is considered as a false value.

I understand that changing this behaviour would be a breaking change, as many current users of the gem have probably introduced spec expectations based on Rails default values. This issue is a first step: discussing the situation and figure out what we want to do with it.

If it happens that we agree on a breaking change, I'm happy to work on the deprecation process and change itself.

Expected behavior

# app/models/author.rb

class Author < ApplicationRecord
  has_many :articles, strict_loading: false
end

# spec/models/author_spec.rb

RSpec.describe Author do
  it 'rejects an association missing explicit strict_loading option to false with the correct message' do
    message = [
      'Expected Author to have a has_many association called articles ',
      '(articles should have strict_loading set to false)',
    ].join

    expect {
      expect(described_class.new).
        to have_many(:articles).
        strict_loading(false) # strict_loading matcher is yet to be approved.
    }.to fail_with_message(message)
  end
end

Expected: Test passes, matcher fails with message.

Actual behavior

Test fails, the matcher is valid and no failing message is raised.

System configuration

shoulda_matchers version: 6.1.0 rails version: 7.1.3 ruby version: 3.3.0

[matsales28]

We're not enabling users to make expectations based on the default values of the framework. The test you provided, with touch(false) as a modifier, as an example, passes because of the way we are dealing with boolean missing values as you mentioned, and that might not be the best way to do it, I agree with you on that point. However, I don't think making this change will be worth it, as it will be a breaking change that will add little value to the gem, as we don't often check the default values of configurations.

But I think, especially for newly introduced matchers or qualifiers, that we should make expectations based on the qualifiers used in the spec, even if those qualifiers are defined or not in the framework defaults because they can be overridden at the application level.

Allowing such expectations to pass even without explicit definitions in the association is valuable. It provides flexibility and aligns with the expectation that users can customize and override these settings at the application level.

# app/models/author.rb

class Author < ApplicationRecord
  has_many :articles
end

# config/...

config.active_record.strict_loading_by_default = true

RSpec.describe Author do
  it { is_expected.to have_many(:articles).strict_loading(true) }
  # => this expectation will fail.
end

I'd love to hear your thoughts on this @VSPPedro, as you have more experience maintaining the gem and might have great insights to share.

[rhannequin]

Thank you @matsales28, this makes complete sense. I might have been confused with the fact that strict loading can be configured on several levels on the application, as you mentioned here, and I thought my example was passing because we wanted to support for implicit framework defaults. But now I understand better, especially what you mentioned about flexibility. I was conscious such a change would be a very breaking one, and that we don't want to introduce a breaking change for any reason.

I will let @VSPPedro share their point of view to gain experience and insights, and then I'm happy to either change this issue or close it, and get back to improving #1607.

[vsppedro]

I don't believe it's worth the trouble, but I'm more than happy to review any PRs aiming to make this change.

So, I don't have a strong opinion on this. Sorry.

With that said, excellent review, @matsales28. I couldn't have conducted a more comprehensive one myself.

[matsales28]

With that said, we should not make the way we typecast the Boolean values a priority; it's really up to you. Feel free to do so if you are very interested in working on this. Otherwise, I'd recommend continuing the work on #1607 as it's a valuable addition to gem.