Originally published in 2007, this article is updated on a semi-regular basis and republished when I add something new...
Rails Idiom
There are a number of small pieces of rails idiom that contribute to better rails code. I've been collecting them at work as they come to me (often by reading other people's code). I know that a lot of this can be gleaned simply by reading the "Agile rails" book... but apparently this would actually take effort, and thus doesn't occur as frequently as I'd like. A lot more is just basic good common sense... but we all know how common that is. In any case, I'll probably put this up on my website soon and keep adding to it.
Note - I don't always have a justification for these things... as with everything - it's up to you to pick out the bits that work for you.
Basic good sense
Clarify your code
This is something that I learned from Joel Spolsky from his article on making wrong code look wrong. He suggests that code smells should stand out a mile off. Doing so will make it much less likely that we'll miss one and leave a hole for a bug to form in.
He further suggests that we should change the way we code to systematically remove all habits (even benign ones) that muddy the waters. If we make it really obvious what is right and what is potentially wrong - then it'll be easier for us to discriminate between the right and wrong bits of the code. This follows from basic psychology. By analogy: it's easier to notice black when all you can see is black or white - but adding many shades of grey makes it harder to detect. If we make a habit of writing only white (or pale-grey)... it's harder for us to miss a black spot.
Joel goes on to discuss a technique for using Hungarian Notation (in the way it was orignally intended) to increase visbility of safe/unsafe strings - which is meant to be just an example of one technique. (So don't take it as the main essence of his message).
Many of the things I bring up in this Idiom-Guide are techniques that draw from this philosophy.
Don't comment-out dead code
Don't comment out code that's no longer needed - just delete it. It'll still be there in the SVN repository if we ever need it again.
Dead code just clutters up the project and slowly makes it less readable/maintainable. :(
Avoid Magic global variables where at all possible
The "don't use magic globals" argument has been around for a while, but here's the rails take on it.
The Rails equivalent of a magic global variable is the "@variable" (though I've also seen some nasty uses of the params hash). I know that instance variables are essential for passing data from the controller to the view. But they should not be relied upon for anything else! eg do not set them and assume they exist when calling methods on a model - or in a library.
From a given piece of code that uses said variable, you cannot tell where the variable originates. If it were simply passed as a parameter to the method, there would be an obvious line of command. Instead, you have to trawl through all the earlier functionality to figure it out or resort to a recursive grep on the rails directory.
This becomes a problem if you have to maintain the code. Given a magic global variable: Can I delete the value? Can I change it without affecting later code (that relies magically on its value)? If I run across a bug whereby it comes in with a value I didn't expect - how do I find which method set the value incorrectly?
On balance - it's much better to simply pass an explicit parameter through the usual method-call interface, so at least you have a chance of tracing the line of command in future.
Don't add ticket/bug comments into the code
I don't wanna have to wade through hideous piles of comments referencing some obscure (fixed) ticket/bug such as:
# DRN: ticket ARC256 -- remove currency symbol
# end ticket ARC256
It wastes time and eyeball-space. This stuff can conveniently go into the SVN log message - where it'll be picked up by the appropriate ticket in the "subversion comments" section. This way it'll only get read by people who actually care about the ticket/bug in question.
Alternatively - what we get is a buildup of comments referencing random tickets that are probably no longer relevant. Especially because the ticket numbers mean nothing when taken outside the context of our ticketing system.
Think about it this way: how long would we keep that comment in the code? when does it get removed? whose job is it to remove the ticket comment or do they all just build up until we have more ticket-comments than code? How unmaintainable!
Please just don't do it.
Ruby idiom
Don't use .nil? unless you need to
There are very few times when you are specifically checking for actual nil results. Mostly you are just checking to see if an object has been populated, in which case use: if foo don't bother with if !foo.nil?
This also brings me to:
Use unless where appropriate
Don't check if !foo instead use: unless foo
This is just the ruby-ish way to do things, and it reads better in many circumstances... given that code is meant to be read by humans, this is a Good Thing.
Do not use and/or - instead use &&/||
The words "and" and "or" mean something quite different to the symbols: &&/||
Unless you understand *exactly* what you are doing - you should use the latter (&& / ||) instead.
Any expression that contains and/or is almost certainly inelegent and should probably be rewritten.
do-end vs {} blocks
{} should be used for single-line blocks and do-end for multi-line blocks... this is a "taste" thing - but it seems to be fairly common practice now.
Use single-line conditionals where possible
if you're only doing one thing based on a conditional, use:
foo_something if x
rather than:
if x
foo_something
end
Obviously this doesn't work if you're doing more than one thing or when you have an else-clause, but it's useful for reducing space eg:
return render(:action => :edit) unless @successful
which also brings me to:
Nested function calls need parentheses
In a nested function call eg :
return render :action => :edit
Rails can make a good guess at whether :action => :edit is the second parameter of the "return" function, or the first parameter of the "render" function... but it's not wise to rely on this and it will generate warnings. It's much better to parenthesise parameters to any nested function eg:
return render(:action => :edit)
However:
Don't parenthesise where unnecessary
When parentheses are not required for the sake of clarity, leave them out.
do_something foo, :bar => :baz
Basic Rails security
h( x-site-scripting )
In your view templates, surround all user-input data with the "h" function. This html-encodes anything within it - which will prevent cross-site scripting attack.
Do this even if the source is trusted!
If you get into the habit of making it part of the <%=h opening, it will just become natural. An example:
<p> Name: <%=h @object.name -%>
<br />Description: <%=h @object.descr -%>
<br /><%= image_tag image_url_for(h @object.image.file_name) -%>
<br /><%= link_to "Edit object", edit_object_path(@object) -%>
</p>
:conditions => ['protect_from = ?', sql_injection]
Most SQL queries are built up by Rails automatically. But adding conditions to your finds or your paginators brings in a slight, but real possibility of introducing sql-injection attacks.
To combat this, always pass everything in as variables. eg:
:conditions => ['name like ? AND manager_id = ? OR is_admin = TRUE',
@user.name, @user.id]
Generators, Plugins and Gems... oh my!
Generators
use scaffold_resource
When you create a new model, generate a scaffold_resource, even if you aren't going to use the view pages.
It generates wonderful tests as well as your model and migration for free.
It's worth it!
It's also worth it to go through the hassle of entering all of the attributes on the command-line. Again because it generates tests, fixtures, migrations, views etc for you.
For usage: script/generate scaffold_resource --help
-x and -c
If you pass the "-c" flag to a generator (script/generate whatever), it will automatically svn add the generated files. This isn't necessary, but it's a really useful thing so you don't have to hand-add every file individually.
There is a similar flag (-x) for when you install plugins. This will automatically add the plugin to the project via svn:externals.
Plugins
Don't use a plugin unless it does exactly what you want
It's so easy to write functionality in Rails that there's no point in using somebody else's plugin unless it does *exactly* what you want it to do.
Especially don't bother using a plugin if you have to hack it to pieces and pull out half the useful functionality. You are just creating a maintenance nightmare in your future.
That being said, if a plugin has half the functionality you need... it's a great place to learn the bits you need to use.
DB and migrations
Use migrations
But you weren't going to hand-hack the db anyway were you?
If you don't use migrations, then capistrano can't deploy the changes automatically. That only means more work for you - and means we can't auto roll-back if something horrible goes wrong.
Don't go back and edit old migrations!
I now have a policy that states "migrations only go forwards". This comes from several bad experiences where a developer (sometimes me) has gone back to edit an old migration and totally locked up the system for everybody else.
You may know that a deployment requires you to "migrate down then back up again". But sometimes that either gets forgotten, or is actually harder to do in practice than in theory eg if you're using capistrano to auto-deploy your site, it's harder to get it to do twisty migrations than just to "move forwards one".
Besides, which, it's a hassle for you to tell everyone on the dev team "before you do an svn up, migrate down to VERSION X, then svn up, then migrate back up again"... what if somebody doesn't want to lose their existing data? what if they're an svn-junkie and have already done an svn up before you get to them? what if somebody is not in today and you have to catch them before they come in tomorrow?
It's way too easy to get your bodgied-up back-hacked migrations into a wedged state where a new person cannot back up or go forward, and often cannot even start from scratch (eg from a fresh copy of a database). This becomes a serious problem for you if you ever have to change servers (or set up a new testing server or whatever) and have to run all the migrations from 0 to get the db ready.
Even if it all runs as you expect, it's three extra steps to remember than just "rake db:migrate", which raises the spectre of Human Error.
It's annoying to make it work, prone to error and bloody awful to fix when it breaks. All in all much easier just to write a new migration to change the existing stuff.
Extra migrations don't actually hurt you - they really don't take up that much more space in your SVN repository - they're worth having them around for the knowledge that anyone, anywhere will have a completely up-to-date system even if they start from complete scratch.
Add messages to data migrations
All of the basic ActiveRecord migrations tend to spit out their own status messages. Data migrations don't, and it's often useful to know what your migration is doing - especially if you've got a long data migration that would otherwise sit there in silence, looking like it's hung your machine.
So drop in a few status messages eg
say "migrated #{orders.size} orders, moved widgets over to new wodget style"
Don't add excessive status messages to migrations
Obviously the above messaging can go too far. Don't write out a line for every piece of data you migrate or you'll get heaps of scrolling text rather than an idea of which migration we're up to - that useful data can get lost in the noise. Just a status message showing how much has been done or what stage we're up to in the migration will do.
Use "say" in migrations
Instead of using "p" use "say" - it gets formatted nicely.
MVC-specific idiom
If you are unsure about what MVC is all about - I suggest you do a bit of basic research on the subject. It's really important to keeping the code clean in a Rails site. Rails is completely founded onthe principle of keeping the MVC layers separate - if you begin tying them together, your run the risk of causing the code to become brittle - losing the flexibility that is the reason why you're using Rails in the first place.
Basic MVC principles
Keep view code out of the model
Your model should be independant of your views. A widget doesn't need to know whether it's being displayed as html, XML or via a screen-reader. It is an independantly-addressable object that contains "data and some knowledge of how to maniupulate that data".
Despite this I consistently see things like methods named "show_edit_link" which apparently tell the html whether this model's edit link should be visible on the html list page. That sort of logic should be in the view only. The model can have methods that, say, determine if they should be "editable" by the current user... and the code that determines if the edit link is displayed could check that method, but should remain inside the view.
If there is common code in your views - then it can be pulled into the helper file for that model. If it's used across multiple models, then it should go in application_helper.rb. It should never appear in the model.
The other common booboo I see are "display" functions that include html (eg "format field X to display nicely"). This is also very wrong! It means that, later, when you want to change html styling across the entire site you have to dig around inside every file in the site to change it... rather than simply changing the views - which is How It Should Be. From having to do just such a nightmareish site-upgrade, I can tell you that the fewer files you need to change - the easier your life will be in the long run.
All view code should remain in the views - with commonality pulled out into helpers.
Data manipulation code should go in the models - not views or controllers
Ok, so you have a report that you want to display that pulls data out of your widget... and your report view pulls it out bit by bit and does some calculations on it - then displays it.
So what happens when your client asks you to now do that report in PDF format... or CSV as well?
If all your data-manipulation methods are on the model itself, then you only need to pass the model object into the view and the view can then pull the data out in the same way for each view style . The alternative is to have a messy set of views/controllers where the same code is duplicated, violating the DRY principle and adding to your future maintenance headaches.
All data-manipulation code should remain on the model that owns the data.
Model idiom
Group relationship commands at top of model
One of the most important and useful things to know about a model is what it is related to - eg the "belongs_to"/"has_many" (etc) relationships.
If these calls get scattered throughout the model object, they will get lost amongst the noise and it will be hard to tell whather a given relationship has been declared...
Thus it's Good Practice to group them all together - and to put them right at the "top" of the class. eg:
class Widget < ActiveRecord::Base
belongs_to :widget_class
has_many :widget_parts
has_one :pricing_plan
#... rest of the model goes here
end
IMO this goes also for composition objects, "acts_as_<whatever>" declarations and auto_scope declarations - all of which declare the "shape" of the model and its relationships with other objects.
Group validations
The second-most important group of information about a model is what consists of valid data for the model. This will determine whether or not the data is accepted or rejected by ActiveRecord - so it's useful to know where to find it. So again, do not scatter it all about the model - put it near the top... preferably just below the "relationship" data. EG:
class Widget < ActiveRecord::Base
belongs_to :widget_class
has_many :widget_parts
has_one :pricing_plan
validates_presence_of :widget_class, :pricing_plan
validates_numericality_of :catalogue_number, :integer_only => true
def validate
errors.add_to_base "should have at least one part" if widget_parts.blank?
end
#... rest of the model goes here
end
Validations should obey the DRY principle...
Please save my eyeballs from having to read the same thing over and over, and obey the DRY principle. You can write:
# please use:
validates_presence_of :field_one, :field_two, :field_three, :field_four
# instead of:
validates_presence_of :field_one
validates_presence_of :field_two
validates_presence_of :field_three
validates_presence_of :field_four
You can also use "validates_each" if you have several things that perform ostensibly the same validation... but it isn't an included one. eg:
# please use:
validates_each [:field_one, :field_two,
:field_three, :field_four] do |model, attr, val|
model.errors.add(attr, "should be a positive number") unless val.nil? || 0 <= val
end
# instead of:
def validate
errors.add(:field_one, " should be a positive number") unless field_one.nil? || 0 <= field_one
errors.add(:field_two, " should be a positive number") unless field_two? || 0 <= field_two
errors.add(:field_three, " should be a positive number") unless field_three? || 0 <= field_three
errors.add(:field_four, " should be a positive number") unless field_four? || 0 <= field_four
end
Read the section on validations in the book!!!
There are lots of really useful standard validations available in Rails... PLEASE check for whether one exists before writing your own hack into "def validate"... you might be surprised at how many are provided absolutely free of charge!
One of the most useful being "validates_each"... so you don't have to repeat the same thing over and over and over... remember that DRY principle?[1]
Another is "validates_format_of" which allows you to perform several validations at once - and give a consistant error message eg checking if a percentage falls within the range (0-100) with an optional percentage sign:[2]
validates_inclusion_of [:my_percentage, :your_percentage], :in => 0..100,
:message => "should be a percentage (0-100)"
validates_format_of [:my_percentage, :your_percentage], :with => /^(\d{1,3})%?/,
:message => "should be a percentage (0-100)"
If a validation doesn't refer to a single field... put it on base.
When it refers to more than one field, feel free to use
errors.add_to_base <whatever>
don't do something silly like:
errors.add(nil, ...)
That being said - if you can squeeze it onto a field, please do - as it really helps the user to quickly find where the problem is in their form.
Give the poor users some useful feedback when validating
Don't tell them "Your weights should add up to 100"... tell them "your weights add up to 82%, your weights need to add up to 100% to be valid". It actually helps them see exactly what's missing.
When you update your validations, update your fixture data
This one has got me a couple of times. The problem is that fixtures are loaded into the database without going through validations - so your test data could be wrong - even when rake test passes flawlessly :(
Name your callbacks and declare them as handlers
If you have a number of callbacks (eg before_save/after_create hooks), it's better if you name them as a method, and just refer to them near the top of the model. eg:
# instead of:
class Widget < ActiveRecord::Base
belongs_to :widget_class
has_many :widget_parts
has_one :pricing_plan
validates_presence_of :widget_class, :pricing_plan
def before_save
# adds default pricing if none was specified
pricing_plan ||= PricingPlan.find_default
end
#rest of model here
end
# try using:
class Widget < ActiveRecord::Base
belongs_to :widget_class
has_many :widget_parts
has_one :pricing_plan
validates_presence_of :widget_class, :pricing_plan
before_save :add_default_pricing
# handler to add default pricing if none specified
def add_default_pricing
pricing_plan ||= PricingPlan.find_default
end
#rest of model here
end
This example is too simple to easily show it, but there are a number of reasons why this is a good idea.
- Firstly, you've named the method, which gives you an idea of what it's for = easier to maintain.
- Secondly, if you need to add multiple of the same hook, you don't need to mash them all into the same callback method - which also aids readability.
- Thirdly, by keeping each call as a single line, you can list them all near the top of the model class without taking up heaps of room (especially annoying in the case that the methods start getting really long). This helps you to quickly see what is and is not being done, without having to read through the details of the methods... or, even worse, seeing the first "after_create" hook and thinking it is the only one, when another is hidden later in the class.
- Fourthly, if the method ever gets more complicated, or has to change, your method is encapsulated - the call to the method is independant of any other callback handlers of the same type.
Use Rails helpers for simple find calls
Rails comes with lots of helpful convenience functions. Read the book and learn about them! The find helpers are a good example. Something like the following is an excellent candidate.
# instead of
thing = Widget.find(:first, :conditions => "name='wodget'")
# use the find_by_<whatever> method
thing = Widget.find_by_name('wodget')
It shortens the code and makes it clear exactly what you're trying to find.
View idiom
Use -%>
Make sure you put a minus sign before closing your %>. This reduces whitespace in the resulting html - which makes it easier for us to read when we need to diagnose a display problem. eg use
<%= render :partial => :foo -%>
<% if some_condition -%>
<div id="bar">
<%= bar_fields -%>
</div>
<% end -%>
h( x-site-scripting )
See this topic in the RailsSecurity
section under XSiteScripting
Templates can have common code too - even across resource-boundaries.
Eg pagination links, or a common header/footer. Common view code should be factored out into a partial template and can be shared by a single resource-type, or across multiple resources.
Shared partials go in /app/views/shared
If a partial is to be used in a common layout, or across multiple resources (eg a common header for all resources reached after login), then the partial should be stored under the "/app/views/shared" directory.
Suppose you have a partial named "_common_footer.rhtml" in the shared directory. You can access it by referring to the partial thus:
<%= render :partial => 'shared/common_footer' -%>
Don't write a string inside a code snippet
This is silly: <%= " | " -%>
In a template, you can just type the pipe (or whatever) without the extra FrouFrou around it.
Controller idiom
Never, never, never use @params or @flash
These were deprecated some time ago. Instead use the similarly named methods: params or flash
Incidentally - you should be reading and taking heed of the warnings that the script/server is giving you about these things...
redirect when moving on
When you are moving on to another stage in the process, you should use redirect. eg you have successfully updated a page and are now moving to the index page.
When you are coming back to the same page, you can use render (eg the user entered an error and you are redisplaying the 'edit' page).
The reason we do this is that a redirect will change the URL in the address bar to the new page. If you just render a different page-action, the user will still have the URL of the previous page, even though they are now looking at a different page. This is Bad because if the previous page was, say, a form-POST... and the user clicks "refresh"... Bad Things will happen. Whereas if you redirect, then the user will have the URL of the new page and it's perfectly fine for them to hit refresh to get the page they're on.
Make private methods obvious!
Private controller methods aren't visible as actions. It can be a problem if somebody doesn't notice your little word "private" and starts to add methods below it without realising - and then wonders why Rails can't find their shiny new action.
So make sure that it's obvious and difficult to make the mistake.
Firstly, put private methods only at the bottom of the file!
Why bother going through spaghetti code with continual calls to private then public then private again... etc and having to figure out exactly what level of visibility we are right now? sort them and put the private ones all together at the bottom.
Secondly, use a line like this:
private ###################################################################################
To make it painfully obvious to all who follow that below this line thar be dragons...
Testing idiom
All testing
Learn your basic assertions
Another "please read the book" request - there are lots of useful assertions already written for you. Using the appropriate ones makes your life easier as the error messages are more appropriate/meaningful for your test case. I especially find it irritating to see things like assert foo == bar when assert_equal not only has a better error message, but also means you're unlikely ever to accidentally use only one '=' - causing the test to silently pass when it should have failed.
Put the expected value first in assert_equals
The error message reads better that way.
Don't cram multiple tests into a single test case
Do not write generic tests with a dozen things to test all in a single test case.
Example shown below. Note - the real code example I took this from actually had another 6 tests crammed into it...
There are (at least) two reasons for not doing this:
Firstly - if the first test here breaks... it doesn't keep running the other tests. If it breaks, you can only tell that the first test isn't working... if you split them out into individual tests, rake will keep running the other tests - this is extremely helpful as you will then know whether all the validations are failing - or just that one.
Secondly - naming tests to mean what they're testing actually helps you find out what's going on when it breaks. A generic "test all sorts of stuff" name doesn't help you much, but a "test that widgets can't be created if the size is negative" does. You won't be thinking about this when you're writing that test case (and are fully immersed in the code you're testing), but six months later when something breaks your test... you will want to know what it's actually breaking and every bit of help (no matter how small) counts.
# don't use:
def test_create_should_not_accept_invalid_parameters
assert_no_difference Widget, :count do
wid = create_widget(:vat_percentage => -1)
assert wid.errors.on(:vat_percentage)
end
# invalid price range
assert_no_difference Widget, :count do
wid = create_widget(:price_range_min => -1)
assert wid.errors.on(:price_range_min)
end
assert_no_difference Widget, :count do
wid = create_widget(:price_range_max => -1)
assert wid.errors.on(:price_range_max)
end
assert_no_difference Widget, :count do
wid = create_widget(:price_range_max => 100, :price_range_min=>200)
assert wid.errors.on(:price_range_max)
end
# invalid size range
assert_no_difference Widget, :count do
wid = create_widget(:size_min => -1)
assert wid.errors.on(:size_min)
end
assert_no_difference Widget, :count do
wid = create_widget(:size_max => -1)
assert wid.errors.on(:size_max)
end
assert_no_difference Widget, :count do
wid = create_widget(:size_max => 100, :size_min=>200)
assert wid.errors.on(:size_max)
end
end
# instead try:
def test_create_should_fail_on_negative_percentage
assert_no_difference Widget, :count do
wid = create_widget(:vat_percentage => -1)
assert wid.errors.on(:vat_percentage)
end
end
def test_create_should_fail_on_negative_min_price
assert_no_difference Widget, :count do
wid = create_widget(:price_range_min => -1)
assert wid.errors.on(:price_range_min)
end
end
def test_create_should_fail_on_negative_max_price
assert_no_difference Widget, :count do
wid = create_widget(:price_range_max => -1)
assert wid.errors.on(:price_range_max)
end
end
def test_create_should_fail_on_backwards_price_range
assert_no_difference Widget, :count do
wid = create_widget(:price_range_max => 100, :price_range_min=>200)
assert wid.errors.on(:price_range_max)
end
end
def test_create_should_fail_on_negative_min_size
assert_no_difference Widget, :count do
wid = create_widget(:size_min => -1)
assert wid.errors.on(:size_min)
end
end
def test_create_should_fail_on_negative_max_size
assert_no_difference Widget, :count do
wid = create_widget(:size_max => -1)
assert wid.errors.on(:size_max)
end
end
def test_create_should_fail_on_backwards_size_range
assert_no_difference Widget, :count do
wid = create_widget(:size_max => 100, :size_min=>200)
assert wid.errors.on(:size_max)
end
end
Group tests in a meaningful way
Test files start out with only a handful of tests... but this rapidly changes. Very soon we get vast numbers of tests covering all sorts of actions.
To reduce the potential maintenance nightmare, group related tests together and put in a comment to indicate why they are grouped together. Any new test should be either put with related groupings or a new grouping created for it.
As an example: the most common set of tests revolves around resource CRUD - in which case it makes sense to group tests with related CRUD actions (eg "edit" and "update") near one another.
Another example is for the "user" object. users go through stages (process of creating a new user, the activation process, CRUD for an ordinary/active user, then destroying/reviving a user), so tests in these stages are grouped together.
If you're using rspec or shoulda, you are provided with a natural grouping mechanism (describe and context respectively). Use that to your advantage and make your future life easier.
Use named fixture records
Don't use bare fixture ids in your tests (eg get :edit :id => 1), instead use named references (eg @quentin.id).
This is a bit of a gotcha as the automatically-generated tests often use bare ids. However, these should be avoided where at all possible.
There are three main benefits:
Firstly, it makes the code more readable, with fewer magic numbers and more meaningful variable names.
Secondly, if we must reorder the fixture records (and thus change the ids) - at least we know which record we really wanted and we don't need to scour the tests looking for the ids to change.
Thirdly - if you're using auto-generated ids (rather than specifically passing them in) you won't know the id - and assuming that it's 1 sets you up to fail.
Similarly, don't use magic strings - especially for things like testing login/authentication. This will eventually bite you. It's much better to specify @quentin.login, @quentin.email (or whatever) so you don't have to go through your tests updating at a later time.
Don't forget to reload
The number of times I've been caught out by this... when you change something and are testing that it got saved into the database correctly, make sure you use @foo.reload. It's especially important if you're checking that you get the right validation errors on your ActiveRecord... you then want to test that it *didn't* change in the database too.
Common functions can be made into methods
If you find yourself performing the same action over and over, don't forget that you can DRY it up by putting it into a common function at the bottom of the test file.
def setup
@default_widget_hash = {:name => 'My widget', :size => 42, :wodget => wodgets(:my_wodget)}
end
def test_should_create_widget
assert_difference Widget, :count do
w = create_widget
assert !w.new_record?, "should not have errors: #{w.errors.full_messages.to_sentence}"
end
end
def test_widget_should_require_name_on_create
assert_no_difference Widget, :count do
w = create_widget(:name => nil)
assert w.errors.on(:name), "Should have required name"
end
end
def create_widget(options = {})
Widget.create(@default_widget_hash.merge(options))
end
Really common tests can be made into assertions
If you find yourself testing the same thing in multiple unit/functional tests, you can DRY that up too it up by putting it into the test helper as a new assertion. Don't forget to name it "assert_X" (where X is descriptive).
Make sure you name it starting with "assert_". This convention allows you to see which methods will have assertions in them (and thus possibly cause your test to fail), as opposed to general methods that simply set-up/tear-down information.
# Assert that the handed object (thing) has no errors on it.
def assert_no_errors(thing, msg = "")
fail("should have passed an object") unless thing
e = thing.errors.full_messages
assert e.empty?, "Should not have error: #{e.to_sentence} #{msg}"
end
Unit tests
Test your CRUD actions
It may seem like overkill to test CRUD here *and* in functional tests, but this is the place to test validations and any things that will affect basic CRUD-actions (eg if you have a method that stops your Widget from being deleted after it's been submitted for approval... then you need to try deleting a submitted one and make sure it fails!
If you only leave it up to your controller tests to check this stuff, then you are getting your data back filtered through one more layer of abstraction and will have to sort out whether it's a model-bug or a controller-bug. It's better to be sure of your model before adding an extra layer on top. It really makes your life easier in the long run!
Sadly scaffold_resource no longer writes these tests for you. :P
Test your validations
Each validation should be tested by trying to create your model object with that one set to nil. Trust me, it's worth it in the long run!
Functional tests
Test your validations
Create a functional test for each validation - again, try to create an object, nulling out the attribute-under-test. I know this seems to double-up on the unit tests - but what you're testing here is that the model is correctly getting back an error on the required attribute. This will be what is displayed on the user's screen - so they can fix the error before moving on. You also need to test that the template is still on the "edit" page. If it's not, then all the ActiveRecord Errors will be in vain as your user will never get to see them.
Test authentication-levels
If you have different levels of authentication (eg administrators get to see everyone's stuff, but you can only see your own stuff) then you must test both ways... test that you can see what you should see, but also test that you can't see stuff that you shouldn't see! You don't want your users peeking into your admin accounts just because you missed the "unhappy path" tests.
...and don't forget to test what happens when you're not logged on too.
Tests need more than just "assert_response :success"
Unfortunately, "assert_response :success" only checks that *something* happened - not that it was the right something.
To be more precise, "assert_response :success" asserts that rails successfully returned an "HTTP 200" response (as opposed to a 404 or a redirect). Unfortunately it does not check that you received the response you were expecting... (though I wish it were that simple) - sadly you do have to actually check that it not only returned a response - but that it was the correct response with the correct variables etc.
In the case of a "get" request - you should check that we are rendering the correct template, and that the "assigns" variables have been correctly populated.
In the case of a request that does something - check all the variables afterwards to make sure they changed in the way that you expected (eg that the variables actually were updated).
Notes
[1] Why is it that the DRY principle is the one I seem to have to harp on the most? Repeated endlessly reaching a pinnacle of irony.
[2] check my earlier post on "validates_percentage_of" for this example in context.
