rspec users - Oct 2007 - Example for attr_accessible?

Is anyone out there writing specs to check attr_accessible fields? I had
originally written my spec to check for allowing the desired fields, and
then none of the other regular db fields. Unfortunately this isn''t
satisfactory, because attr_protected could have been used instead, which
of course wouldn''t prevent mass assignment to any whatever=(val)
method.

I''m thinking of maybe some internal Rails variable that holds an array
of
accessible fields maybe? Does anyone know if such a thing exists? Just to
make it clear, I don''t want to test Rails, I want to check that the
accessible fields have been set properly. Would it be possible to mock or
some such to check for a call to attr_accessible?

Thanks,
Steve

El 16/10/2007, a las 6:50, Steve <vertebrate at gmail.com> escribi?:
> Is anyone out there writing specs to check attr_accessible fields?  
> I had
> originally written my spec to check for allowing the desired  
> fields, and
> then none of the other regular db fields. Unfortunately this isn''t
> satisfactory, because attr_protected could have been used instead,  
> which
> of course wouldn''t prevent mass assignment to any whatever=(val)  
> method.
>
> I''m thinking of maybe some internal Rails variable that holds an  
> array of
> accessible fields maybe? Does anyone know if such a thing exists?  
> Just to
> make it clear, I don''t want to test Rails, I want to check that
the
> accessible fields have been set properly. Would it be possible to  
> mock or
> some such to check for a call to attr_accessible?
Yes, I spec this kind of thing.

   describe User, ''accessible attributes'' do
     it ''should allow mass-assignment to the login name'' do
       lambda { new_user.update_attributes(:login_name =>  
String.random) }.should_not raise_error
     end
     # many other examples here, one for each accessible attribute  
that I care about
   end

   describe User, ''protected attributes'' do
     it ''should deny mass-assignment to the passphrase hash''
do
       lambda { new_user.update_attributes(:passphrase_hash =>  
String.random) }.should raise_error
     end
     # many other examples here, one for each protected attribute  
that I care about
   end

I do it this way because:

1. I am most interested in the *behaviour*; does this let me mass- 
assign to this attribute (or not)?

2. I don''t want my specs to depend on internal knowledge of the Rails  
attribute protection/access implementation.

The style of spec shown above seems the best way to me of both  
documenting the desired behaviour and confirming that it exists; I  
don''t view these as tests of Rails'' internals, but as tests
that I''ve
used attr_accessible and attr_protected correctly. I tried other ways  
in the past, but this is the one I like best.

Another thing to note that helps the readability of the specs: the  
new_user and String.random methods are provided by FixtureReplacement  
(<http://replacefixtures.rubyforge.org/>). Note that by default  
FixtureReplacement used mass-assignment under the covers, so it can''t  
be used with protected attributes, but I''ve sent a patch in to the  
maintainer which changes that:

Index: lib/fixture_replacement/fixture_replacement.rb
==================================================================---
lib/fixture_replacement/fixture_replacement.rb      (revision 31)
+++ lib/fixture_replacement/fixture_replacement.rb      (working copy)
@@ -70,7 +70,9 @@
            hash_given = args[0] || Hash.new
            merged_hash = self.send(attributes_method).merge(hash_given)
            evaluated_hash = Generator.merge_unevaluated_method 
(self, :create, merged_hash)
-          obj = class_name.create!(evaluated_hash)
+          obj = class_name.new
+          evaluated_hash.each { |k, v| obj.send("#{k}=", v) }
+          obj.save!
            obj
          end
        end
@@ -86,7 +88,9 @@
            hash_given = args[0] || Hash.new
            merged_hash = self.send(attributes_method).merge(hash_given)
            evaluated_hash = Generator.merge_unevaluated_method 
(self, :create, merged_hash)
-          class_name.new(evaluated_hash)
+          obj = class_name.new
+          evaluated_hash.each { |k, v| obj.send("#{k}=", v) }
+          obj
          end
        end
      end

Hopefully this will be included soon.

One more thing to note: the documentation for FixtureReplacement is  
pretty thin at this stage, so I threw together this cheatsheet:

<http://wincent.com/knowledge-base/FixtureReplacement_cheatsheet>

This is a bit easier than consulting the (excellent) screencast  
provided by the author:

<http://http://replacefixtures.rubyforge.org/>

Cheers,
Wincent

On Tue, 16 Oct 2007 10:39:32 +0200, Wincent Colaiuta wrote:
> Yes, I spec this kind of thing.
> 
>    describe User, ''accessible attributes'' do
>      it ''should allow mass-assignment to the login name''
do
>        lambda { new_user.update_attributes(:login_name =>  
> String.random) }.should_not raise_error
>      end
>      # many other examples here, one for each accessible attribute  
> that I care about
>    end
> 
>    describe User, ''protected attributes'' do
>      it ''should deny mass-assignment to the passphrase
hash'' do
>        lambda { new_user.update_attributes(:passphrase_hash =>  
> String.random) }.should raise_error
>      end
>      # many other examples here, one for each protected attribute  
> that I care about
>    end
> 
> I do it this way because:
> 
> 1. I am most interested in the *behaviour*; does this let me mass- 
> assign to this attribute (or not)?
> 
> 2. I don''t want my specs to depend on internal knowledge of the
Rails
> attribute protection/access implementation.
> 
> The style of spec shown above seems the best way to me of both  
> documenting the desired behaviour and confirming that it exists; I  
> don''t view these as tests of Rails'' internals, but as
tests that I''ve
> used attr_accessible and attr_protected correctly. I tried other ways  
> in the past, but this is the one I like best.
> 
> Another thing to note that helps the readability of the specs: the  
> new_user and String.random methods are provided by FixtureReplacement  
> (<http://replacefixtures.rubyforge.org/>). Note that by default  
> FixtureReplacement used mass-assignment under the covers, so it
can''t
> be used with protected attributes, but I''ve sent a patch in to the
> maintainer which changes that:
I''m all for avoiding rails internal behavior, but attr_accessible deals
on a share least model. If you can verify that only
attrs x, y, z are accessible, you know that any other field
that rails can come up with, or you add later is covered.

I just saw FixtureReplacement the other day, and plan on giving it a go.
Looks like it does a bunch of what I already do manually.

On 10/15/07, Steve <vertebrate at gmail.com>
wrote:
> Is anyone out there writing specs to check attr_accessible fields? I had
> originally written my spec to check for allowing the desired fields, and
> then none of the other regular db fields. Unfortunately this isn''t
> satisfactory, because attr_protected could have been used instead, which
> of course wouldn''t prevent mass assignment to any whatever=(val)
method.
>
> I''m thinking of maybe some internal Rails variable that holds an
array of
> accessible fields maybe? Does anyone know if such a thing exists? Just to
> make it clear, I don''t want to test Rails, I want to check that
the
> accessible fields have been set properly. Would it be possible to mock or
> some such to check for a call to attr_accessible?
>
> Thanks,
> Steve
>
> _______________________________________________
> rspec-users mailing list
> rspec-users at rubyforge.org
> http://rubyforge.org/mailman/listinfo/rspec-users
>
You could expect a call to attr_accessible and then load your model file.

Or, loop through all the column names in your model, and make sure
that only the ones that are supposed to be accessible get set.

Pat

> 
> You could expect a call to attr_accessible and then load your model file.
> 
> Or, loop through all the column names in your model, and make sure
> that only the ones that are supposed to be accessible get set.
> 
> Pat
Expecting the call is what I had originally thought of, but couldn''t
figure out how to write that test. Isn''t attr_accessible called when
you
create a new instance, so there wouldn''t be time to setup an
expectation
for the call?

On 10/16/07, Steve <vertebrate at gmail.com>
wrote:
> >
> > You could expect a call to attr_accessible and then load your model
file.
> >
> > Or, loop through all the column names in your model, and make sure
> > that only the ones that are supposed to be accessible get set.
> >
> > Pat
>
> Expecting the call is what I had originally thought of, but
couldn''t
> figure out how to write that test. Isn''t attr_accessible called
when you
> create a new instance, so there wouldn''t be time to setup an
expectation
> for the call?
describe Chicken do
  it "should make only :name and :age attr_accessible" do
    Chicken.should_receive(:attr_accessible).with(:name, :age)
    load "#{RAILS_ROOT}/app/models/chicken.rb"
  end
end

I first saw this technique described by David Chelimsky at
http://rubyforge.org/pipermail/rspec-users/2007-September/002965.html

Pat

On 10/16/07, Pat Maddox <pergesu at gmail.com>
wrote:
> describe Chicken do
>   it "should make only :name and :age attr_accessible" do
>     Chicken.should_receive(:attr_accessible).with(:name, :age)
>     load "#{RAILS_ROOT}/app/models/chicken.rb"
>   end
> end
>
> I first saw this technique described by David Chelimsky at
> http://rubyforge.org/pipermail/rspec-users/2007-September/002965.html
Which I first saw described by Jay Fields at
http://blog.jayfields.com/2006/12/rails-unit-testing-activerecord.html

On Tue, 16 Oct 2007 13:43:43 -0500, David Chelimsky wrote:
> On 10/16/07, Pat Maddox <pergesu at gmail.com> wrote:
>> describe Chicken do
>>   it "should make only :name and :age attr_accessible" do
>>     Chicken.should_receive(:attr_accessible).with(:name, :age)
>>     load "#{RAILS_ROOT}/app/models/chicken.rb"
>>   end
>> end
>>
>> I first saw this technique described by David Chelimsky at
>> http://rubyforge.org/pipermail/rspec-users/2007-September/002965.html
> 
> Which I first saw described by Jay Fields at
> http://blog.jayfields.com/2006/12/rails-unit-testing-activerecord.html
Sweet, thanks guys. That''s exactly what I was looking for.

On Tue, 16 Oct 2007 13:43:43 -0500, David Chelimsky wrote:
> On 10/16/07, Pat Maddox <pergesu at gmail.com> wrote:
>> describe Chicken do
>>   it "should make only :name and :age attr_accessible" do
>>     Chicken.should_receive(:attr_accessible).with(:name, :age)
>>     load "#{RAILS_ROOT}/app/models/chicken.rb"
>>   end
>> end
>>
>> I first saw this technique described by David Chelimsky at
>> http://rubyforge.org/pipermail/rspec-users/2007-September/002965.html
> 
> Which I first saw described by Jay Fields at
> http://blog.jayfields.com/2006/12/rails-unit-testing-activerecord.html
I''ve been thinking about this throughout the day, and is this a pattern
that should maybe be implemented in a more widespread pattern? For
example, check that the model makes the requisite calls to the various
validation functions? It would seem that unless you have some very custom
validation methods, that you would be testing rails implementation of its
validators, by running through all of the various checks manually. 

I guess maybe it depends on if you view the testing as mostly testing a
black box, or if you assume some knowledge of the code internals? Thoughts?

Steve

On 10/16/07, Steve <vertebrate at gmail.com>
wrote:
> On Tue, 16 Oct 2007 13:43:43 -0500, David Chelimsky wrote:
>
> > On 10/16/07, Pat Maddox <pergesu at gmail.com> wrote:
> >> describe Chicken do
> >>   it "should make only :name and :age attr_accessible"
do
> >>     Chicken.should_receive(:attr_accessible).with(:name, :age)
> >>     load "#{RAILS_ROOT}/app/models/chicken.rb"
> >>   end
> >> end
> >>
> >> I first saw this technique described by David Chelimsky at
> >>
http://rubyforge.org/pipermail/rspec-users/2007-September/002965.html
> >
> > Which I first saw described by Jay Fields at
> > http://blog.jayfields.com/2006/12/rails-unit-testing-activerecord.html
>
> I''ve been thinking about this throughout the day, and is this a
pattern
> that should maybe be implemented in a more widespread pattern? For
> example, check that the model makes the requisite calls to the various
> validation functions? It would seem that unless you have some very custom
> validation methods, that you would be testing rails implementation of its
> validators, by running through all of the various checks manually.
>
> I guess maybe it depends on if you view the testing as mostly testing a
> black box, or if you assume some knowledge of the code internals? Thoughts?
Well, you''ll find differing opinions on this.  Every time I''ve
done
validations I''ve written a spec that actually exercises the behavior.
For example

class Person < ActiveRecord::Base
  validates_presence_of :name
end

describe Person do
  it "should not be valid without a name" do
    p = Person.new
    p.should_not be_valid
    p.errors.on(:name).should == "can''t be blank"
  end
end

As far as I''m concerned, that''s specifying the behavior.  A
person
without a name is not valid, and it results in an error message on
name.  Doing it the way we''ve discussed in this thread is ugly and
stupid, imo.

However, I had never used attr_accessible up to this point, and in
this case I thought it made sense.  It''s just a lot easier to expect
the call itself than to try to test every single column that''s not
accessible.  You end up looping through the columns, writing helper
methods to provide default values...it all gets ugly.  So, I thought
this works.

An interesting thing is that since I decided it''s okay for
attr_accessible, I''m starting to consider whether it may be okay for
other validations.  afaik David C doesn''t have a firm opinion on this,
and I have to admit that mine is softening up a bit.

Personally, if I wanted to extract this pattern, I would use a custom
expectation matcher.

Person.should require_field(:name) =>
  p = Person.new
  p.should_not be_valid
  p.errors.on(:name).should == "can''t be blank"

or to make it mirror AR a bit more
Person.should validate_presence_of(:name)

I would be happy with a nice little library of expectation matchers
that handle all the validations like that.  It may seem silly since
the name is essentially the same as the method call itself, but I
prefer it because you''re using the object''s actual behavior.
Generally I want to exercise the behavior as much as possible and mock
out as little as I can.  The exceptions are when I haven''t implemented
an interaction yet, so I mock it out, or when I''m dealing with code
from a lower layer in which case I keep the mocks for speed/dependency
purposes.

Pat

On 10/16/07, Pat Maddox <pergesu at gmail.com>
wrote:
> On 10/16/07, Steve <vertebrate at gmail.com> wrote:
> > I''ve been thinking about this throughout the day, and is this
a pattern
> > that should maybe be implemented in a more widespread pattern? For
> > example, check that the model makes the requisite calls to the various
> > validation functions? It would seem that unless you have some very
custom
> > validation methods, that you would be testing rails implementation of
its
> > validators, by running through all of the various checks manually.
> >
> > I guess maybe it depends on if you view the testing as mostly testing
a
> > black box, or if you assume some knowledge of the code internals?
Thoughts?
>
> Well, you''ll find differing opinions on this.
Yes, you will. This gets very gray. Message expectations (mocks)
implicitly know about internals, but they also allow you to spec more
accurately WHERE behaviour gets implemented.

The argument in favor of loading up the file and expecting an AR
declaration (like attr_accessible) is that the behaivour is actually
implemented in AR, and it''s really not any different from a Message
Expectation except it''s happening on file load. It also means you
don''t have to hit the database, which means it runs faster.

The argument against is that the declaration is an implementation
detail and therefore brittle.
> Every time I''ve done
> validations I''ve written a spec that actually exercises the
behavior.
> For example
>
> class Person < ActiveRecord::Base
>   validates_presence_of :name
> end
>
> describe Person do
>   it "should not be valid without a name" do
>     p = Person.new
>     p.should_not be_valid
>     p.errors.on(:name).should == "can''t be blank"
>   end
> end
I''ve got one project where I do this - it comes from
somebody''s post
on this list (though I''m at a loss for who right now):

person.should reject(nil).for(:name).with("can''t be blank")
person.should reject("").for(:name).with("can''t be
blank")
person.should accept("a name").for(:name)

<snip>
> An interesting thing is that since I decided it''s okay for
> attr_accessible, I''m starting to consider whether it may be okay
for
> other validations.  afaik David C doesn''t have a firm opinion on
this,
> and I have to admit that mine is softening up a bit.
>
> Personally, if I wanted to extract this pattern, I would use a custom
> expectation matcher.
... which would probably be implemented like this under the hood:

def matches?(target)
  target.should_receive(:validates_presence_of).with(@expected)
  load "#{RAILS_ROOT}/app/models/#{target.humanize}.rb"
end

No?

On 10/16/07, David Chelimsky <dchelimsky at gmail.com>
wrote:
> On 10/16/07, Pat Maddox <pergesu at gmail.com> wrote:
> > Personally, if I wanted to extract this pattern, I would use a custom
> > expectation matcher.
>
> ... which would probably be implemented like this under the hood:
>
> def matches?(target)
>   target.should_receive(:validates_presence_of).with(@expected)
>   load "#{RAILS_ROOT}/app/models/#{target.humanize}.rb"
> end
>
> No?
No.  I actually showed it in my post, though I suppose I did it in a
slightly confusing format.  I would implement it like so:

def matches?(target)
  instance = Target.new
  instance.should_not be_valid
  instance.error_messages.on(@expected).should == "can''t be
blank"
end

A couple key points:
1. AR defines validates_presence_of, but your subclass is responsible
for doing the actual validation
2. This particular code is lightweight enough that it''s okay to use
the real implementation.  You don''t get a big performance hit like you
do when you''re creating/updating objects in the db and their
associations
3. When you call MyModel.new when spec''ing business logic, it hits the
db to get the column info.  You already have a coupling to the DB
simply by using AR, so I don''t think that''s a good reason to
prefer
expecting the validates_* method call.  If perf does become an issue,
you can stub out the columns and remove the db dependency all
together.  Jay Fields mentions that in a blog, and I think he''s insane
(at least the way he showed), but it should be easy enough to preload
all the column info from schema.rb.  In fact I think I heard of that
technique from someone on this list.

Pat

El 17/10/2007, a las 7:26, "Pat Maddox" <pergesu at gmail.com>
escribi?:
> Well, you''ll find differing opinions on this.  Every time
I''ve done
> validations I''ve written a spec that actually exercises the
behavior.
> For example
>
> class Person < ActiveRecord::Base
>   validates_presence_of :name
> end
>
> describe Person do
>   it "should not be valid without a name" do
>     p = Person.new
>     p.should_not be_valid
>     p.errors.on(:name).should == "can''t be blank"
>   end
> end
>
> As far as I''m concerned, that''s specifying the behavior. 
A person
> without a name is not valid, and it results in an error message on
> name.  Doing it the way we''ve discussed in this thread is ugly and
> stupid, imo.
I agree that you''ll find many opinions on this. My personal take is  
that you should be testing the behaviour, not testing that the  
"validates..." method was called.

Why? Because while this is fine for a simple validation like  
validates_presence_of, it can fall down for complex validations  
with :on, :if, and other clauses. The problem is that it is fairly  
easy to write a complex validation that doesn''t actually do what you  
think it does. Testing that you set it up gives you very little in  
that case; you should be testing that it *does* what you think it  
does. You are not testing ActiveRecord in this case (it''s well  
tested); you are testing that your use of ActiveRecord gives you the  
desired behaviour. And as has already been pointed out, you can write  
a custom matcher to make such oft-used specs easy to write. My own  
take on this is here:

<http://wincent.com/a/about/wincent/weblog/archives/2007/10/ 
custom_validation_matcher.php>

Compare this with attr_accesible. That''s much simpler; it takes a  
list of symbols. It is much harder to make a mistake. In that case I  
think it''s fine to mock the call and just check that your code does  
the attr_accessible call. But having said that, seeing as testing the  
behaviour directly is so simple (just try mass assigning and see if  
it worked or not) I just test the behaviour anyway and forget about  
the mock+load trick.

Cheers,
Wincent