Skip to content

Conversation

cldwalker
Copy link
Contributor

This time rebased off master as requested in #2194

hmcfletch and others added 2 commits July 24, 2011 00:31
@@ -0,0 +1,9 @@
class WholesaleProduct < ActiveRecord::Base
Copy link
Member

Choose a reason for hiding this comment

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

Actually the ActiveRecord maintainers would prefer that you use existing models for new tests instead of add new models

Copy link
Contributor

Choose a reason for hiding this comment

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

I poked around existing models to see if there was one I could use for this test, but I didn't see one that I could easily repurpose. I didn't want to mess with one that looked they were being used for specific test cases, which most of the ones I looked at seemed to be. I could have put it at the top of the test file, but none of the other classes up there had method declarations. It's my first submitted patch, so I just tried to not mess with things that were already in place too much.

@cldwalker
Copy link
Contributor Author

@guilleiguaran Updated tests to use existing model. Personally, I don't see it as a good testing practice to be using one model for multiple unrelated tests. Leads to unnecessary coupling and hard to find bugs

@guilleiguaran
Copy link
Member

I agree with you but we must try to avoid defining extra models since we already have tonnes of them and if every patch add a new model it would quickly get out of hand.

Your code and tests looks great, thanks!!!

spastorino added a commit that referenced this pull request Jul 25, 2011
Fix after_initialize and Base.create edge case
@spastorino spastorino merged commit 64affc9 into rails:master Jul 25, 2011
@vijaydev vijaydev mentioned this pull request Nov 22, 2011
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.

5 participants