WIP #547

mrageh · 2014-06-25T22:10:58Z

Change join_table on matcher to join_table_name
For issue NoMethodError when testing with join_table #476

* Change `join_table` on matcher to `join_table_name` * For issue #476

houndci-bot · 2014-06-25T22:11:02Z

lib/shoulda/matchers/active_record/association_matcher.rb

@@ -747,7 +747,7 @@ def have_and_belong_to_many(name)
      # @private
      class AssociationMatcher
        delegate :reflection, :model_class, :associated_class, :through?,
-          :join_table, :polymorphic?, to: :reflector
+          :join_table_name, :polymorphic?, to: :reflector


Align the parameters of a method call if they span more than one line.

mcmire · 2014-06-27T20:41:08Z

Thanks @Adam89, do you want to rename the commit message?

mrageh · 2014-06-27T21:21:13Z

@mcmire this is a work in progress commit. I think I should rename it as soon as it's ready to be reviewed.

mrageh · 2014-06-27T21:43:51Z

This is taking me a really long time and am sorry about that. Just having trouble figuring out how to test all the different cases for join_table.

Now `has_and_belongs_to_many` has a `join_table` option that matches correctly on existing join tables.

houndci-bot · 2014-06-27T21:51:26Z

spec/shoulda/matchers/active_record/association_matcher_spec.rb

    end

+


Extra blank line detected.

mcmire · 2014-06-27T22:02:14Z

@Adam89 Ah ok. No worries then. Take your time :)

mrageh · 2014-06-27T22:07:51Z

@mcmire could you suggest some other cases I should test, like rejecting a invalid join_table name with the correct error message?
The problem am now having is am not sure on what to test for moving forward and I have done the bulk of the work. Its now getting the error correct messages to display and adding a few finishing touches.

mcmire · 2014-06-27T22:10:44Z

spec/shoulda/matchers/active_record/association_matcher_spec.rb

@@ -830,8 +830,24 @@ def having_one_non_existent(model_name, assoc_name, options = {})
        expect(having_and_belonging_to_many_relatives).
          to have_and_belong_to_many(:relatives).validate(false)
      end
+
+      it 'accepts a valid association with a join_table' do
+        create_table :developers_relatives, id: false do |t|


Would it be a good idea to choose a different name for this table than the default, so as to prevent false positives?

That makes sense, thank you

mcmire · 2014-06-27T22:20:27Z

Hmm... I've never tried to test for this before. Let's see...

You could check that the reflection options hash contains a join_table key whose value is the join table in question (using OptionVerifier, we have other code that checks options like this, you may have run across it).
In addition, you could also check that the join table exists by using ActiveRecord::Base.connection.table_exists?.

Instead of using the default name of the join_table choose a different one to prevent false positives.

mrageh · 2014-06-27T22:24:11Z

Thanks man this has been really interesting.

mrageh · 2014-06-28T12:40:03Z

Any idea why the build failed, seems irrelevant to the work I did in this pr

Could not find gem 'jquery-rails (>= 0) ruby' in the gems available on this
machine.

mcmire · 2014-06-30T23:49:23Z

@Adam89 I think I broke master again :( I'll get it fixed shortly.

mrageh · 2014-07-01T10:20:08Z

No problem @mcmire I thought I broke the build and it only worked on my machine, nothing worse than that type of failure.

simonoff · 2014-07-04T14:37:46Z

Any updates?

simonoff · 2014-07-04T14:40:45Z

And what about association_foreign_key ? It's will be added or better to do separate PR?

mrageh · 2014-07-04T15:16:10Z

Hi @simonoff

I hope to work on this over the weekend and try to finish the pr off.

As for association_foreign_key it seems like that could be done in a separate PR if am not mistaken.

houndci-bot · 2014-07-18T20:04:32Z

lib/shoulda/matchers/active_record/association_matchers/join_table_matcher.rb

+          def initialize(join_table, name)
+            @join_table = join_table
+            @name = name
+            @missing_option = ''


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

mrageh · 2014-07-18T20:04:40Z

One of my specs fails when I run the version below;

BUNDLE_GEMFILE=/Users/Adam/code/projects/shoulda-matchers/gemfiles/4.1.gemfile bundle exec rspec spec/shoulda/matchers/active_record/association_matcher_spec.rb:834

Failures:
  1) Shoulda::Matchers::ActiveRecord::AssociationMatcher have_and_belong_to_many validate accepts a valid association with a join_table
     Failure/Error: expect(developer.new).to have_and_belong_to_many(:relatives).
       Expected Developer to have a has_and_belongs_to_many association called relatives (Expected to find join table called random_name)
     # ./spec/shoulda/matchers/active_record/association_matcher_spec.rb:847:in `block (4 levels) in <top (required)>'

Finished in 0.04423 seconds
1 example, 1 failure

Failed examples:

rspec ./spec/shoulda/matchers/active_record/association_matcher_spec.rb:834 # Shoulda::Matchers::ActiveRecord::AssociationMatcher have_and_belong_to_many validate accepts a valid association with a join_table

The interesting thing is no other version of the gemfiles fail.

After some investigating it turns out that in rails 4.1.0 .reflect_on_association behaves differently. When we call Developer.reflect_on_association(:relatives) it returns an instance of ThroughReflection;

#<ActiveRecord::Reflection::ThroughReflection:0x007fcd429cab50 @macro=:has_many, @name=:relatives, @scope=nil, @options={:through=>:developers_relatives, :source=>:relative}, @active_record=Developer(id: integer), @klass=Relative(id: integer), @plural_name="relatives", @collection=true, @automatic_inverse_of=nil, @type=nil, @foreign_type="relatives_type", @constructable=true, @source_reflection_name=:relative, @class_name="Relative">

Whereas when we run rails 4.0.1 Developer.reflect_on_association returns an instance of AssociationReflection.

#<ActiveRecord::Reflection::AssociationReflection:0x007f9d5fc15738 @macro=:has_and_belongs_to_many, @name=:relatives, @scope=nil, @options={:join_table=>:random_name}, @active_record=Developer(id: integer), @plural_name="relatives", @collection=true>

Am not sure what change in rails 4.1 caused this issue, maybe this issue has something to do with it rails/rails#15671 ?

cc @mcmire

mcmire · 2014-07-18T21:15:27Z

If I had to guess, I'd say it's a result of changing has_and_belongs_to_many to be implemented in terms of has_many :through.

Maybe we need a convenience method in ModelReflection to work around this?

mcmire · 2014-07-18T21:23:41Z

Also it looks like I misunderstood what you meant before -- you were trying to figure out what tests to write.

Given Model.has_and_belongs_to_many(:foos).join_table(:bars), and the bars join table exists, I expect(model).to have_and_belong_to_many(:foos).join_table(:bars)
Given Model.has_and_belongs_to_many(:foos).join_table(:bars), and the bars join table exists, I expect(model).not_to have_and_belong_to_many(:foos).join_table(:some_other_table)
Given Model.has_and_belongs_to_many(:foos).join_table(:bars), and the bars join table does not exist, I expect(model).not_to have_and_belong_to_many(:foos).join_table(:bars)
Given Model.has_and_belongs_to_many(:foos).join_table(:bars), and the bars join table exists, I expect(model).not_to have_and_belong_to_many(:some_other_association).join_table(:bars)

mrageh · 2014-07-18T21:30:29Z

Why do we have to make a change ModelReflection, when the failure is caused by a change in the way reflect_on_association works in rails 4.1?

The tests pass for every other version of rails but for rails 4.1 reflect_on_association returns a instance of ThroughMatcher.

On 18 Jul 2014, at 22:15, Elliot Winkler [email protected] wrote:

If I had to guess, I'd say it's a result of changing has_and_belongs_to_many to be implemented in terms of has_many :through.

Maybe we need a convenience method in ModelReflection to work around this?

—
Reply to this email directly or view it on GitHub.

mrageh · 2014-07-19T03:46:07Z

Its a bug in rails 4.1.0 that causes the spec to fail and it was addressed in rails/rails#15753, so by updating to rails 4.1.4 all the specs pass.

What should I do @mcmire, should I update the version in the gem file to a specific patch?

mrageh · 2014-07-19T03:52:54Z

Great now another spec fails after upgrading to rails 4.1.4, this is slowly starting to get more and more complex.


  1) Shoulda::Matchers::ActiveRecord::AssociationMatcher have_and_belong_to_many accepts an association with a valid :conditions option
     Failure/Error: args << lambda { where(conditions) }
     ArgumentError:
       wrong number of arguments (1 for 0)
     # ./spec/shoulda/matchers/active_record/association_matcher_spec.rb:875:in `block in define_association_with_conditions'
     # ./lib/shoulda/matchers/active_record/association_matchers/model_reflection.rb:57:in `instance_exec'
     # ./lib/shoulda/matchers/active_record/association_matchers/model_reflection.rb:57:in `convert_scope_to_relation'
     # ./lib/shoulda/matchers/active_record/association_matchers/model_reflection.rb:42:in `association_relation'
     # ./lib/shoulda/matchers/active_record/association_matchers/model_reflector.rb:7:in `association_relation'
     # ./lib/shoulda/matchers/active_record/association_matchers/option_verifier.rb:84:in `actual_value_for_relation_clause'
     # ./lib/shoulda/matchers/active_record/association_matchers/option_verifier.rb:35:in `actual_value_for'
     # ./lib/shoulda/matchers/active_record/association_matchers/option_verifier.rb:56:in `correct_for?'
     # ./lib/shoulda/matchers/active_record/association_matchers/option_verifier.rb:30:in `correct_for_relation_clause?'
     # ./lib/shoulda/matchers/active_record/association_matcher.rb:978:in `conditions_correct?'
     # ./lib/shoulda/matchers/active_record/association_matcher.rb:857:in `matches?'
     # ./spec/shoulda/matchers/active_record/association_matcher_spec.rb:735:in `block (3 levels) in <top (required)>'

Finished in 4.27 seconds
120 examples, 1 failure

Failed examples:

rspec ./spec/shoulda/matchers/active_record/association_matcher_spec.rb:727 # Shoulda::Matchers::ActiveRecord::AssociationMatcher have_and_belong_to_many accepts an association with a valid :conditions option

mcmire · 2014-07-20T06:33:27Z

I don't see how this has to do with rails/rails#15753. If it did, then you'd be getting nil instead of an instance of ThroughReflection.

In Rails 4.1, has_and_belongs_to_many associations were rewritten in terms of has_many :through. In other words, this:

class Person < ActiveRecord::Base
  has_and_belongs_to_many :relatives
end

is like saying:

class HABTM_Relatives
  belongs_to :left_side, class: Person
  belongs_to :relative
end

class Person < ActiveRecord::Base
  has_many :people_relatives, class: HABTM_Relatives, source: :left_side
  has_many :relatives, through: :people_relatives, source: :relative
end

So when you access the reflection for the association, you're actually going to get the reflection for the has_many :through association. A reflection for a has_many :through association, regardless of how that association was created, is an instance of ThroughReflection, not AssociationReflection.

But let's stay focused. I don't see how the tests magically pass under 4.1.4. Are you sure you're running the test under the proper gemset? i.e.:

appraisal 4.1 rspec spec/shoulda/matchers/active_record/association_matcher_spec.rb

mrageh · 2014-07-20T21:45:41Z

Thanks for the explanation on how has_and_belongs_to_many has been reimplemented in rails 4.1

In order to make sure that tests did not pass on rails 4.1.4, I deleted my local version of shoulda-matchers and cloned it again. Then I ran the following command
appraisal 4.1 rspec spec/shoulda/matchers/active_record/association_matcher_spec.rb and as expected I got my original failure from earlier. I then updated the rails version for 4.1 by running the following command; BUNDLE_GEMFILE=/Users/Adam/code/projects/shoulda-matchers/gemfiles/4.1.gemfile bundle update rails. After doing this I again ran my test suite with this command
appraisal 4.1 rspec spec/shoulda/matchers/active_record/association_matcher_spec.rb
and the original failing spec was no longer failing. However another spec was failing and the stack trace for it can be found in the comment above.

mcmire · 2014-07-21T00:10:32Z

Thanks for the update. That's interesting... let me play with your branch and get back to you. I don't want to upgrade to 4.1.4 in this PR, that should be done in another PR, so maybe there is a way to get this to work without upgrading just yet.

Some developers might want to pass a :join_table option to their HABTM association declarations (e.g., `has_and_belongs_to_many :posts, join_table: :users_and_their_posts). Those same developers, may want to explicitly test the existence of the join table. Make this possible.

jacobsimeon · 2014-07-23T23:47:03Z

Well, it looks like me and @Adam89 have been duplicating efforts a little bit. I was working on implementation a join table matcher as well, and I think it's been merged into master.

mcmire · 2014-07-24T02:10:50Z

Ahhh... I see. I guess this does foil your efforts @Adam89, doesn't it...

What do you think about letting @jacobsimeon take this over, Adam?

mrageh · 2014-07-24T08:37:49Z

Yeah that's fine @mcmire, it was a great learning experience and I hope to be able to solve issues more swiftly next time. Thanks for informing @jacobsimeon

Some developers might want to pass a :join_table option to their HABTM association declarations (e.g., `has_and_belongs_to_many :posts, join_table: :users_and_their_posts). Those same developers, may want to explicitly test the existence of the join table. Make this possible.

WIP

1b20b84

* Change `join_table` on matcher to `join_table_name` * For issue #476

houndci-bot reviewed Jun 25, 2014
View reviewed changes

Adam89 added 2 commits June 27, 2014 13:12

Add test for accepting valid join_table

d4d52a5

Create join_table_matcher

f97a8f8

mcmire added this to the 2.7.0 milestone Jun 27, 2014

Add matches? to join_table_matcher

b9ba408

Now `has_and_belongs_to_many` has a `join_table` option that matches correctly on existing join tables.

houndci-bot reviewed Jun 27, 2014
View reviewed changes

spec/shoulda/matchers/active_record/association_matcher_spec.rb

end

Copy link

houndci-bot Jun 27, 2014

Choose a reason for hiding this comment

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

Extra blank line detected.

mcmire reviewed Jun 27, 2014
View reviewed changes

Test class instance returned by define_model

0763535

Choose different name for join_table

80d1347

Instead of using the default name of the join_table choose a different one to prevent false positives.

mcmire added the PR: In Progress label Jun 30, 2014

Add option verifier to check join_table

07572dc

houndci-bot reviewed Jul 18, 2014
View reviewed changes

jacobsimeon mentioned this pull request Jul 23, 2014

Add support for HBTM :join_table option #556

Closed

mrageh closed this Jul 24, 2014

mrageh deleted the ma-add-join-option branch August 2, 2014 14:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP #547

WIP #547

mrageh commented Jun 25, 2014

houndci-bot Jun 25, 2014

mcmire commented Jun 27, 2014

mrageh commented Jun 27, 2014

mrageh commented Jun 27, 2014

houndci-bot Jun 27, 2014

mcmire commented Jun 27, 2014

mrageh commented Jun 27, 2014

mcmire Jun 27, 2014

mrageh Jun 27, 2014

mcmire commented Jun 27, 2014

mrageh commented Jun 27, 2014

mrageh commented Jun 28, 2014

mcmire commented Jun 30, 2014

mrageh commented Jul 1, 2014

simonoff commented Jul 4, 2014

simonoff commented Jul 4, 2014

mrageh commented Jul 4, 2014

houndci-bot Jul 18, 2014

mrageh commented Jul 18, 2014

mcmire commented Jul 18, 2014

mcmire commented Jul 18, 2014

mrageh commented Jul 18, 2014

mrageh commented Jul 19, 2014

mrageh commented Jul 19, 2014

mcmire commented Jul 20, 2014

mrageh commented Jul 20, 2014

mcmire commented Jul 21, 2014

jacobsimeon commented Jul 23, 2014

mcmire commented Jul 24, 2014

mrageh commented Jul 24, 2014

WIP #547

WIP #547

Conversation

mrageh commented Jun 25, 2014

houndci-bot Jun 25, 2014

Choose a reason for hiding this comment

mcmire commented Jun 27, 2014

mrageh commented Jun 27, 2014

mrageh commented Jun 27, 2014

houndci-bot Jun 27, 2014

Choose a reason for hiding this comment

mcmire commented Jun 27, 2014

mrageh commented Jun 27, 2014

mcmire Jun 27, 2014

Choose a reason for hiding this comment

mrageh Jun 27, 2014

Choose a reason for hiding this comment

mcmire commented Jun 27, 2014

mrageh commented Jun 27, 2014

mrageh commented Jun 28, 2014

mcmire commented Jun 30, 2014

mrageh commented Jul 1, 2014

simonoff commented Jul 4, 2014

simonoff commented Jul 4, 2014

mrageh commented Jul 4, 2014

houndci-bot Jul 18, 2014

Choose a reason for hiding this comment

mrageh commented Jul 18, 2014

mcmire commented Jul 18, 2014

mcmire commented Jul 18, 2014

mrageh commented Jul 18, 2014

mrageh commented Jul 19, 2014

mrageh commented Jul 19, 2014

mcmire commented Jul 20, 2014

mrageh commented Jul 20, 2014

mcmire commented Jul 21, 2014

jacobsimeon commented Jul 23, 2014

mcmire commented Jul 24, 2014

mrageh commented Jul 24, 2014