Add Appraisal for Rails 6.1

[vsppedro]

TODO

[vsppedro]

@mcmire, do you prefer the merger of #1417 to be done with this PR instead of the master? With that we can verify if it solves the error in the CI.

[mcmire]

@vsppedro Yes, exactly. I think that would make the most sense.

[vsppedro]

@vsppedro Yes, exactly. I think that would make the most sense.

Done! If you compare the errors on this PR and the other, #1417, you will see that one of them was solved.

What do you think?

The merge will be with this branch. I will treat every error separately.

[mcmire]

@vsppedro If it helps you to separate each case into its own PR that's fine with me!

[vsppedro]

I will try to describe why the tests are failing for future reference.

Let's go with this first:

  1) Shoulda::Matchers::ActiveModel::Helpers default_error_message if ActiveModel::Errors#generate_message behavior has changed provides the right error message for validate_presence_of
     Failure/Error: expect(record).to validate_presence_of(:attr)

       Expected Example to validate that :attr cannot be empty/falsy, but this
       could not be proved.
         After setting :attr to ‹""›, the matcher expected the Example to be
         invalid and to produce the validation error "Behavior has diverged."
         on :attr. The record was indeed invalid, but it produced these
         validation errors instead:

         * attr: ["can't be blank"]
     # ./spec/unit/shoulda/matchers/active_model/helpers_spec.rb:97:in `assert_presence_validation_has_correct_message'
     # ./spec/unit/shoulda/matchers/active_model/helpers_spec.rb:88:in `block (4 levels) in <main>'

If you try to run the tests with Ruby 6.0 and 6.1 you will notice a diferent values for record.errors in this piece of code:

shoulda-matchers/lib/shoulda/matchers/active_model/validator.rb

Lines 100 to 107 in 3da0956

	validation_error_messages =
	if record.errors.respond_to?(:[])
	record.errors[attribute]
	else
	record.errors.on(attribute)
	end

For Rails 6.0 the value is: #<ActiveModel::Errors:0x0000564199be9ca0 @base=#<Example:0x000056419867da10 id: nil, attr: "">, @details={:attr=>[{:error=>:blank}]}, @messages={:attr=>["Behavior has diverged."]}>

For Rails 6.1 the value is: #<ActiveModel::Errors:0x0000556046460130 @base=#<Example:0x0000556045132720 id: nil, attr: "">, @errors=[#<ActiveModel::Error attribute=attr, type=blank, options={}>]>

In this new version each validation error is now being encapsulated as an ActiveModel::Error object. If you would like to know more about it take a look at this PR:

rails/rails#32313

Now I need to understand why the error message - 'Behavior has diverged'. - that was set up at the beginning of the test is not being used.

[vsppedro]

Hi, @mcmire, I hope you're doing well.

What do you think about removing this test:

shoulda-matchers/spec/unit/shoulda/matchers/active_model/helpers_spec.rb

Lines 82 to 90 in 3da0956

	context 'if ActiveModel::Errors#generate_message behavior has changed' do
	it 'provides the right error message for validate_presence_of' do
	stub_active_model_message_generation(
	type: :blank,
	message: 'Behavior has diverged.',
	)
	assert_presence_validation_has_correct_message
	end
	end

And add an integration test to verify that validate_presence_of matcher is working with a customized message?

[mcmire]

Hi @vsppedro! I looked over the test file you linked to. It seems that the purpose of these tests is to make sure that the validation matchers can be used without specifying with_message even when blank (or wrong_length, or whatever i18n key the matcher is trying to use) is a) set at different levels in en.yml or b) a different value than the typical "can't be blank" message (for whatever reason). Really, what this boils down to is, make sure that when validation matchers are verifying and comparing validation messages, they are always doing so using i18n keys (:blank) and not strings ("can't be blank"). validate_presence_of and validate_length_of are being used here as examples, but they really apply to any validation matcher.

As for the test you highlighted, it's sort of a superfluous test in my opinion. It's also a bit weird. I don't really see how stubbing Rails provides confidence that our code is working correctly. So I would be fine with removing that completely, to be honest — I don't think you need to replicate it anywhere else.

[vsppedro]

Hi @vsppedro! I looked over the test file you linked to. It seems that the purpose of these tests is to make sure that the validation matchers can be used without specifying with_message even when blank (or wrong_length, or whatever i18n key the matcher is trying to use) is a) set at different levels in en.yml or b) a different value than the typical "can't be blank" message (for whatever reason). Really, what this boils down to is, make sure that when validation matchers are verifying and comparing validation messages, they are always doing so using i18n keys (:blank) and not strings ("can't be blank"). validate_presence_of and validate_length_of are being used here as examples, but they really apply to any validation matcher.

Oh thanks! Now I understand.

As for the test you highlighted, it's sort of a superfluous test in my opinion. It's also a bit weird. I don't really see how stubbing Rails provides confidence that our code is working correctly. So I would be fine with removing that completely, to be honest — I don't think you need to replicate it anywhere else.

I was not seeing the value of this test, so I asked if I could remove it. Removing it would make my job of adding support for rails 6.1 much easier. Anyway, now I just need to correct two more tests. Thank you again.

[vsppedro]

Another test fixed. Before:

  1) Shoulda::Matchers::ActiveRecord::AssociationMatcher have_many accepts an association with a valid :source option
     Failure/Error: model.has_many :children, **options
     
     ArgumentError:
       Unknown key: :source. Valid keys are: :class_name, :anonymous_class, :primary_key, :foreign_key, :dependent, :validate, :inverse_of, :strict_loading, :autosave, :before_add, :after_add, :before_remove, :after_remove, :extend, :counter_cache, :join_table, :index_errors, :ensuring_owner_was
       
       rspec ./spec/unit/shoulda/matchers/active_record/association_matcher_spec.rb:887 # Shoulda::Matchers::ActiveRecord::AssociationMatcher have_many accepts an association with a valid :source option

Now is green. To fix this error was necessary to add the option :through before :source. This change makes sense to me, because, quoting the documentation: :source option specifies the source association name for a has_many/has_one :through association.

[vsppedro]

The unit tests are ok. The acceptance tests are not even starting, I'm not sure why. I will keep digging.

[vsppedro]

I noticed that the tests were freezing on this line:

shoulda-matchers/spec/support/acceptance/helpers/step_helpers.rb

Line 121 in 3da0956

run_command_within_bundle!('rails g rspec:install')

After some research, I found this link:

https://stackoverflow.com/questions/31857365/rails-generate-commands-hang-when-trying-to-create-a-model

I tried the solution, adding the command bundle install --binstubs before the rails g, and it worked like a charm. What do you think?

[mcmire]

I don't mind the addition of bundle install --binstubs if it doesn't add much time to the test suite (it seems like that command should be pretty fast). This looks great!