Moved polyamorous from gem to a module #1105
Conversation
|
Thanks @shadabmalik1310 I will review this later today. Just some initial notes before the review:
Many thanks! |
| .to_not raise_error | ||
| end | ||
| end | ||
| # describe '#join_associations', if: AR_version <= '4.0' do |
There was a problem hiding this comment.
As we are not supporting AR <= 4 this test can be removed.
| .to match(%r{LEFT OUTER JOIN articles ON (\('default_scope' = 'default_scope'\) AND )?articles.person_id = people.id}) | ||
| expect(real_query) | ||
| .to match(%r{LEFT OUTER JOIN articles articles_people ON (\('default_scope' = 'default_scope'\) AND )?articles_people.person_id = parents_people.id}) | ||
| if ::Gem::Version.new(::ActiveRecord::VERSION::STRING) > ::Gem::Version.new(Constants::RAILS_6_0) |
There was a problem hiding this comment.
Can we also add an a test where the data for this condition is created, and we check that the output is the same across each version of AR.
There was a problem hiding this comment.
I have tried but i couldn't find a way to change the dynamically change the version of AR. But i have tested with different versions without spec, it's fine.
There was a problem hiding this comment.
Thanks @shadabmalik1310 the way to do this would to:
- set up some data
- check for an expected output of a Ransack query as data (not the SQL generated)
- run it in CI for each different version of Rails
There was a problem hiding this comment.
Hi @shadabmalik1310 this look good so far, great job 🥇
A few points:
-
Ensuring Ransack does not break across versions is important. It is OK to test output SQL, but we need to also set up some sample data scenarios and ensure they return the same result across versions.
-
we need to also include the tests from Polyamorous that were previously missed. Convert Polyamorous into a module #1099
-
all the tests will need to pass. This might be dependent on CI against Rails 6-0-stable broken #1081
| @@ -1,4 +1,5 @@ | |||
| require 'machinist/active_record' | |||
| require 'polyamorous/polyamorous.rb' | |||
There was a problem hiding this comment.
We need the specs from the Polyamorous gem added and passing.
There was a problem hiding this comment.
Hi @seanfcarroll - All files are covered with the specs and coverage is 100%, let me know the file and line which is not covered with the spec.
Thanks
There was a problem hiding this comment.
Could you please add in the code missing from here https://github.com/activerecord-hackery/polyamorous/blob/master/spec/polyamorous/join_association_spec.rb#L20-L37
Also, please move the Polyamorous tests under their own folder (at the same level as Ransack).
Thank you. :-)
|
Thanks for the work @shadabmalik1310 If the review points are all now resolved, would it be possible to make a note and then resolve the conversation? Let's get this merged! |
|
bump @shadabmalik1310 |
1 similar comment
|
bump @shadabmalik1310 |
|
Closed as superceeded by #1113 |
Closes #1099