Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standardize error message of association matcher #1417

Merged
merged 1 commit into from Apr 3, 2021

Conversation

vsppedro
Copy link
Collaborator

@vsppedro vsppedro commented Feb 17, 2021

I noticed that the association matcher was returning a slightly different message between the tests below:

expected_message = 'Expected Person to have a has_one association called detail ' \
'(PersonDetail does not have a ["company_id", "person_detail_id"] foreign key.)'

expected_message = 'Expected Person to have a has_one association called detail ' \
'(PersonDetail does not have a [:foo_id, :bar_id] foreign key.)'

Pay attention to the foreign_keys, their types are different. This PR standardized the error message - in both cases the foreign_keys will be symbols. Besides that, these modifications fix one of the four tests that are failing with Rails 6.1.

Copy link
Collaborator

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just had two comments. This error message is not the greatest anyways, IMO I don't know if it always points users to what the problem actually is or what the solution is, but that is not your fault and seems out of scope for your change here, so I'm okay with letting it slide.

Out of curiosity, which test failed on Rails 6.1 and why?

lib/shoulda/matchers/active_record/association_matcher.rb Outdated Show resolved Hide resolved
lib/shoulda/matchers/active_record/association_matcher.rb Outdated Show resolved Hide resolved
@vsppedro
Copy link
Collaborator Author

vsppedro commented Feb 17, 2021

I'm opening this PR, #1418, just to show the errors that are happening with Rails 6.1.

One of them is:

3) Shoulda::Matchers::ActiveRecord::AssociationMatcher have_one rejects an association with a valid :class_name and :foreign_key option (CPK), but no columns
     Failure/Error:
       expect do
         have_one(:detail).class_name('PersonDetail').
           with_foreign_key(%i[company_id person_detail_id])
       end.not_to match_against(Person.new).and_fail_with(expected_message)
     
       Expected the positive version of the matcher not to match and for the failure
       message to be:
     
       ------------------------------------------------------------------------
       Expected Person to have a has_one association called detail (PersonDetail does not have a ["company_id", "person_detail_id"] foreign key.)
       ------------------------------------------------------------------------
     
       However, it was:
     
       ------------------------------------------------------------------------
       Expected Person to have a has_one association called detail (PersonDetail does not have a [:company_id, :person_detail_id] foreign key.)

Before Rails 6.1 this lines was returning an array of symbols:

But now is returning a string. Example: "[:company_id, :person_detail_id]". The logic on the association matcher checks if this is an array:

if key.is_a?(Array)
key.map(&:to_s)
else
key.to_s
end

This logic was supposed to update that value to "["company_id", "person_detail_id"], but it's not working because it's not returning an array anymore.

I think this was the commit that made this impact: rails/rails@c0750fe

Standardizing the error message to be symbols solves this error. What do you think?

@vsppedro vsppedro changed the base branch from master to add-rails-6-1 February 19, 2021 03:06
@mcmire
Copy link
Collaborator

mcmire commented Mar 1, 2021

@vsppedro As long as we don't change what the developer is passing to with_foreign_key when printing out the failure message, this makes sense to me. I say that because if the developer passes ["company_id", "person_detail_id"] but the failure message seems to indicate they passed [:company_id, :person_detail_id] then they might get confused (or, it would give us confusing information if we attempted to help them). It doesn't seem like that that's what you're doing here, though, so I'm good with what you've done so far here.

@vsppedro
Copy link
Collaborator Author

vsppedro commented Mar 7, 2021

I'll go back to this as soon as I finished dropping rails 4.2, 5.0 and 5.1.

@vsppedro
Copy link
Collaborator Author

vsppedro commented Apr 2, 2021

Done!

@vsppedro vsppedro merged commit f0f1801 into add-rails-6-1 Apr 3, 2021
@vsppedro vsppedro deleted the standardize-error-message branch April 3, 2021 00:30
@vsppedro
Copy link
Collaborator Author

vsppedro commented Apr 3, 2021

I decided to merge this PR into #1418. I think it will be simpler to work with just one branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants