Remove unnecessary database access for most AR macro helpers
Reported by Ryan McGeary | June 27th, 2008 @ 11:35 PM
A lot of the Active Record macro helpers look for the first record in the database as the object to perform validations on. I think this should be avoided for two reasons:
1) There might not be any records in the db nor any fixtures
2) Avoiding the database access should speed up tests
In other words, this is a common pattern:
assert object = klass.find(:first), "Can't find first #{klass}"
object.send("#{attribute}=", v)
object.save
# assertions ...
My suggestion is to change this to
object = klass.new
object.send("#{attribute}=", v)
object.valid?
# assertions ...
Please see my "no_db" branch (rmm5t/shoulda no_db):
http://github.com/rmm5t/shoulda/...
Here's the specific commit that matters:
http://github.com/rmm5t/shoulda/...
No tests were added as no functionality was modified.
As part of this code inspection, I found a minor issue with should_ensure_length_at_least. It was ensuring the entire object was okay on valid input of the attribute instead of just checking for no errors on that attribute. I should have made this a separate commit and ticket, but my shortsightedness rolled it into the bigger commit. Here are lines I'm talking about:
Comments and changes to this ticket
-
Chris O'Sullivan July 1st, 2008 @ 05:59 AM
Nice spotting on the should_ensure_length_at_least broken nastiness (I think that one was my fault).
I've created a branch that includes a failing test for this:
-
Ryan McGeary July 1st, 2008 @ 08:41 AM
Chris, Thanks for filling in the testing gap. I pulled your test into my no_db branch.
I had to make Address#zip a string instead of integer to properly support the test though; otherwise, the length was always too short because shoulda's "xxxxx" attribute value would convert to "0". Zips should be stored as strings anyway since they can start with zeros.
(Btw, if anyone already pulled my no_db branch, I mucked my remote branch so I deleted it and rebuilt it from some cherry-picks. Please delete and re-pull it. Sorry for this inconvenience, but you wouldn't have wanted the messy recursive merge I first created. Ugh.)
-
Chris O'Sullivan July 1st, 2008 @ 08:45 AM
Doh! Of course (the perils of making a breaking test that you don't fix yourself!)
Good stuff Ryan
-
-
Ryan McGeary July 30th, 2008 @ 09:08 AM
- → State changed from “new” to “committed”
- → Assigned user changed from “Tammer Saleh” to “Ryan McGeary”
Please Login or create a free account to add a new comment.
You can update this ticket by sending an email to from your email client. (help)
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
Tammer Saleh