Skip to content

Conversation

@huoxito
Copy link

@huoxito huoxito commented Mar 17, 2014

See rails/rails@eaf0d50

Hey guys sorry for the lack of specs here or if this doesn't make sense (will try to look into the specs). Let me know if this is not the way we should be going.

Noticed these kinds of errors while upgrading Spree to rails 4.0.4 and thought it might be handy to have this stub here too. You can find some more context here: spree/spree@2e8fee9#commitcomment-5693277

cheers

huoxito referenced this pull request in spree/spree Mar 17, 2014
Make rails 4.0.4 build happy
@JonRowe
Copy link
Member

JonRowe commented Mar 17, 2014

Thanks for looking into this, can you find source material (ideally a rails commit) proving it's a necessary component for rspec-rails 2-14? We'll also need a spec for this and you should be aware that this feature is being extracted to an outside gem in the upcoming release of RSpec 3, so ideally you'll need to open an issue there too.

In the mean time you can stub this method yourself in spree to make it work without modifying rspec-rails.

@huoxito
Copy link
Author

huoxito commented Mar 17, 2014

Sure thing I'll look into it later today. As pointed above this is the rails commit make that change required rails/rails@eaf0d50 and this is the specific line calling a method not defined / stubbed by mock_model https://github.com/rails/rails/blob/v4.0.4/activerecord/lib/active_record/associations/association.rb#L243

I'll open an issue / PR on the mock gem for rspec 3 too. Thanks!

@JonRowe
Copy link
Member

JonRowe commented Mar 17, 2014

That commit doesn't mention a has_attribute?, further more this should probably be false, as most attributes probably won't exist.

@huoxito
Copy link
Author

huoxito commented Mar 17, 2014

Ah yeah my bad, that's just the commit that started throwing errors. At that point rspec-rails would complain about a missing attributes fields on the mocked model. By rails 4.0.4 release it was changed to has_attribute?. I could point the commit later.

I was not sure whether to return true or false. Just took the valid? method as an example. I'll amend it.

@huoxito
Copy link
Author

huoxito commented Mar 18, 2014

@JonRowe here's the rails commit with has_attribute reference rails/rails@f214896

Just tried but couldn't set up a test to reproduce the issue. I was basically trying to set up a spec where we assign children objects to its parent on a has_many relationship like we do here for example https://github.com/spree/spree/blob/v2.2.0/core/spec/models/spree/promotion_spec.rb#L60-L62

@JonRowe
Copy link
Member

JonRowe commented Mar 18, 2014

Well we'll definitely need a test, just rubber ducking but you did reverse this patch when trying to write a failing test right? ;)

@alindeman
Copy link
Contributor

Thanks for bringing this up. I think we can probably define this as __model_class_has_column?(attribute_name) instead so it responds accurately. I'll wrap this up with some tests and such.

@JonRowe
Copy link
Member

JonRowe commented Mar 22, 2014

Thanks Andy!

On Sunday, 23 March 2014, Andy Lindeman notifications@github.com wrote:

Thanks for bringing this up. I think we can probably define this as
__model_class_has_column?(attribute_name) instead so it responds
accurately. I'll wrap this up with some tests and such.

Reply to this email directly or view it on GitHubhttps://github.com//pull/956#issuecomment-38363837
.

alindeman added a commit that referenced this pull request Mar 22, 2014
@alindeman
Copy link
Contributor

Would appreciate some 👀 over on #965 :)

@JonRowe
Copy link
Member

JonRowe commented Mar 22, 2014

Done

alindeman added a commit that referenced this pull request Mar 22, 2014
@alindeman alindeman closed this Mar 23, 2014
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.

3 participants