How does a person test private methods? Is there a way to declare them as private, but retain any tests that were written during the initial development? Thanks. -- Posted via http://www.ruby-forum.com/.
On 8 Jan 2008, at 18:39, Chris Olsen wrote:> How does a person test private methods? > > Is there a way to declare them as private, but retain any tests that > were written during the initial development?I''m rather liking Jay Field''s approach (described at http:// blog.jayfields.com/2007/11/ruby-testing-private-methods.html) In which he basically redeclares the private methods public for the duration of the test only, with a bit of block-scoped evaling goodness. Matt -- Matt Patterson | Design & Code <matt at reprocessed org> | http://www.reprocessed.org/
Might be a personal thing, but my approach is that I try to test the public behaviour of the object. Testing private methods is, imho, getting dangerously close to specifying how the object does its business, rather than what it does. I would just spec the externally visible behaviour, where it occurs, and let the object implement it as it wants (using private methods or any other mechanism). Daniel On 8 Jan 2008, at 19:06 8 Jan 2008, Matt Patterson wrote:> On 8 Jan 2008, at 18:39, Chris Olsen wrote: > >> How does a person test private methods? >> >> Is there a way to declare them as private, but retain any tests that >> were written during the initial development? > > I''m rather liking Jay Field''s approach (described at http:// > blog.jayfields.com/2007/11/ruby-testing-private-methods.html) > > In which he basically redeclares the private methods public for the > duration of the test only, with a bit of block-scoped evaling > goodness. > > Matt > > -- > Matt Patterson | Design & Code > <matt at reprocessed org> | http://www.reprocessed.org/ > > > _______________________________________________ > rspec-users mailing list > rspec-users at rubyforge.org > http://rubyforge.org/mailman/listinfo/rspec-users
Chris Olsen wrote:> How does a person test private methods? > > Is there a way to declare them as private, but retain any tests that > were written during the initial development? > > Thanks. >When I need to do this I just use the send method. -Ben
On 8 Jan 2008, at 19:14, Daniel Tenner wrote:> Might be a personal thing, but my approach is that I try to test the > public behaviour of the object. Testing private methods is, imho, > getting dangerously close to specifying how the object does its > business, rather than what it does. >If you''re a proponent of many-skinny-methods then you wind up with a lot of public methods which should never need to be called by another object, so making them private can be a good thing for general users of the object.> I would just spec the externally visible behaviour, where it occurs, > and let the object implement it as it wants (using private methods or > any other mechanism). >If the object were to implement it itself, that would be great ;-) Unfortunately, I have to implement the innards, and I''m rubbish so I like to test things... Matt -- Matt Patterson | Design & Code <matt at reprocessed org> | http://www.reprocessed.org/
On Jan 8, 2008, at 2:20 PM, Ben Mabey wrote:> Chris Olsen wrote: >> How does a person test private methods? >> >> Is there a way to declare them as private, but retain any tests that >> were written during the initial development? >> >> Thanks. >> > When I need to do this I just use the send method.But it will break in 1.9 and on! (That''s why Jay Fields didn''t do it in his post). Scott
Doesn''t 1.9 allow you to call obj.send!, which will allow you to access private methods? Tim On Jan 8, 2008, at 12:27 PM, Scott Taylor wrote:> > On Jan 8, 2008, at 2:20 PM, Ben Mabey wrote: > >> Chris Olsen wrote: >>> How does a person test private methods? >>> >>> Is there a way to declare them as private, but retain any tests that >>> were written during the initial development? >>> >>> Thanks. >>> >> When I need to do this I just use the send method. > > But it will break in 1.9 and on! (That''s why Jay Fields didn''t do it > in his post). > > Scott > > _______________________________________________ > rspec-users mailing list > rspec-users at rubyforge.org > http://rubyforge.org/mailman/listinfo/rspec-users
Yes. http://eigenclass.org/hiki/Changes+in+Ruby+1.9#l24 Tim Harper wrote:> Doesn''t 1.9 allow you to call obj.send!, which will allow you to > access private methods? > > Tim > > On Jan 8, 2008, at 12:27 PM, Scott Taylor wrote: > > >> On Jan 8, 2008, at 2:20 PM, Ben Mabey wrote: >> >> >>> Chris Olsen wrote: >>> >>>> How does a person test private methods? >>>> >>>> Is there a way to declare them as private, but retain any tests that >>>> were written during the initial development? >>>> >>>> Thanks. >>>> >>>> >>> When I need to do this I just use the send method. >>> >> But it will break in 1.9 and on! (That''s why Jay Fields didn''t do it >> in his post). >> >> Scott >> >> _______________________________________________ >> rspec-users mailing list >> rspec-users at rubyforge.org >> http://rubyforge.org/mailman/listinfo/rspec-users >> > > _______________________________________________ > rspec-users mailing list > rspec-users at rubyforge.org > http://rubyforge.org/mailman/listinfo/rspec-users >
Well there we go, Ben''s method still stands :) Tim On Jan 8, 2008, at 12:36 PM, Ben Mabey wrote:> Yes. http://eigenclass.org/hiki/Changes+in+Ruby+1.9#l24 > > Tim Harper wrote: >> Doesn''t 1.9 allow you to call obj.send!, which will allow you to >> access private methods? >> >> Tim >> >> On Jan 8, 2008, at 12:27 PM, Scott Taylor wrote: >> >> >>> On Jan 8, 2008, at 2:20 PM, Ben Mabey wrote: >>> >>> >>>> Chris Olsen wrote: >>>> >>>>> How does a person test private methods? >>>>> >>>>> Is there a way to declare them as private, but retain any tests >>>>> that >>>>> were written during the initial development? >>>>> >>>>> Thanks. >>>>> >>>>> >>>> When I need to do this I just use the send method. >>>> >>> But it will break in 1.9 and on! (That''s why Jay Fields didn''t do >>> it >>> in his post). >>> >>> Scott >>> >>> _______________________________________________ >>> rspec-users mailing list >>> rspec-users at rubyforge.org >>> http://rubyforge.org/mailman/listinfo/rspec-users >>> >> >> _______________________________________________ >> rspec-users mailing list >> rspec-users at rubyforge.org >> http://rubyforge.org/mailman/listinfo/rspec-users >> > > _______________________________________________ > rspec-users mailing list > rspec-users at rubyforge.org > http://rubyforge.org/mailman/listinfo/rspec-users
Will obj.send(:method) work in 1.9 or is it saying that the send call requires 2 params, the method and the object reference? -- Posted via http://www.ruby-forum.com/.
obj.send(:method) will work for non-private methods and send! works for private methods. additionally there is send() without a receiving object. that is the only of those methods requiring two parameters. Chris Olsen schrieb:> Will obj.send(:method) work in 1.9 or is it saying that the send call > requires 2 params, the method and the object reference? > >
The send call never expects an object reference as a parameter. Either it is called on an object or without an object context (i.e. kernel). In the first case the parameter should contain one of the receiving object''s methods. In the second case the parameter is a kernel method. But... that''s not really an RSpec topic anymore. Chris Olsen schrieb:> Will obj.send(:method) work in 1.9 or is it saying that the send call > requires 2 params, the method and the object reference? > >
On Jan 8, 2008 2:47 PM, Chris Olsen <lists at ruby-forum.com> wrote:> Will obj.send(:method) work in 1.9 or is it saying that the send call > requires 2 params, the method and the object reference?In 1.9 you can say obj.send!(:method) even if :method is private. obj.send(:method) will not work for private methods.
On Jan 8, 2008 1:25 PM, Matt Patterson <matt-lists at reprocessed.org> wrote:> On 8 Jan 2008, at 19:14, Daniel Tenner wrote: > > > > Might be a personal thing, but my approach is that I try to test the > > public behaviour of the object. Testing private methods is, imho, > > getting dangerously close to specifying how the object does its > > business, rather than what it does. > > > > If you''re a proponent of many-skinny-methods then you wind up with a > lot of public methods which should never need to be called by another > object, so making them private can be a good thing for general users > of the object.Keep in mind that if you''re specifying object behaviour with the prescribed granular red/green/refactor cycle, then the examples are all against public methods. Private methods should only ever appear as a result of refactoring. Those are the many-skinny that you are talking about.> > > I would just spec the externally visible behaviour, where it occurs, > > and let the object implement it as it wants (using private methods or > > any other mechanism). > > > > If the object were to implement it itself, that would be great ;-) > Unfortunately, I have to implement the innards, and I''m rubbish so I > like to test things...You should check out the bowling kata (http://butunclebob.com/ArticleS.UncleBob.TheBowlingGameKata) if you haven''t. At the end there are just a few tests and they all touch only 2 public methods, but there are many, many smaller methods that appear through refactoring. They are all thoroughly tested, though not directly. Cheers, David
I totally agree with you, David! For quite a while I was testing all my methods (even had to declare them protected/package scope in java!), but I realized that I was getting into a lot of trouble. Now I''ve shifted to testing functionality in stead of methods. Now, sometimes you might end up having small methods (typically a result of refactoring) that are being used by several clients. In that case you should start testing those methods, since they actually represent real business logic. I talked to uncle Bob about this issue just a few months ago, and as far as I understood, he uses a similar approach. I think it might make sense to think of the facade pattern when you do your testing - do you really care what happens behind the facade? Stefan 2008/1/9, David Chelimsky <dchelimsky at gmail.com>:> > On Jan 8, 2008 1:25 PM, Matt Patterson <matt-lists at reprocessed.org> wrote: > > You should check out the bowling kata > (http://butunclebob.com/ArticleS.UncleBob.TheBowlingGameKata) if you > haven''t. At the end there are just a few tests and they all touch only > 2 public methods, but there are many, many smaller methods that appear > through refactoring. They are all thoroughly tested, though not > directly. > > Cheers, > David > ______________-- Bekk Open Source http://boss.bekk.no -------------- next part -------------- An HTML attachment was scrubbed... URL: http://rubyforge.org/pipermail/rspec-users/attachments/20080109/786c9b45/attachment.html
Just to clarify, this is what I meant in my original email :-) Most of my methods are very small - in Ruby any method longer than 5 lines is, imho, a code smell that''s waiting to be fixed. However, no matter how many methods are used to implement the functionality, I test the public behaviour of the object rather than the methods themselves. Sometimes, this maps directly to a public method. Sometimes, a spec will actually use several public methods in concert. With this approach, I never spec private methods. Daniel On 9 Jan 2008, at 10:01 9 Jan 2008, Stefan Magnus Landr? wrote:> I totally agree with you, David! > > For quite a while I was testing all my methods (even had to declare > them protected/package scope in java!), but I realized that I was > getting into a lot of trouble. Now I''ve shifted to testing > functionality in stead of methods. > > Now, sometimes you might end up having small methods (typically a > result of refactoring) that are being used by several clients. In > that case you should start testing those methods, since they > actually represent real business logic. > I talked to uncle Bob about this issue just a few months ago, and > as far as I understood, he uses a similar approach. > > I think it might make sense to think of the facade pattern when you > do your testing - do you really care what happens behind the facade? > > Stefan > > 2008/1/9, David Chelimsky <dchelimsky at gmail.com>: > On Jan 8, 2008 1:25 PM, Matt Patterson <matt-lists at reprocessed.org> > wrote: > > You should check out the bowling kata > ( http://butunclebob.com/ArticleS.UncleBob.TheBowlingGameKata) if you > haven''t. At the end there are just a few tests and they all touch only > 2 public methods, but there are many, many smaller methods that appear > through refactoring. They are all thoroughly tested, though not > directly. > > Cheers, > David > ______________ > > -- > Bekk Open Source > http://boss.bekk.no > _______________________________________________ > rspec-users mailing list > rspec-users at rubyforge.org > http://rubyforge.org/mailman/listinfo/rspec-users-------------- next part -------------- An HTML attachment was scrubbed... URL: http://rubyforge.org/pipermail/rspec-users/attachments/20080109/eb7c2f0a/attachment.html
On Jan 9, 2008 10:01 AM, Stefan Magnus Landr? <stefan.landro at gmail.com> wrote:> I totally agree with you, David! > > For quite a while I was testing all my methods (even had to declare them > protected/package scope in java!), but I realized that I was getting into a > lot of trouble. Now I''ve shifted to testing functionality in stead of > methods. > > Now, sometimes you might end up having small methods (typically a result of > refactoring) that are being used by several clients. In that case you should > start testing those methods, since they actually represent real business > logic.I wonder whether that is a smell indicating that the functionality in those methods really belongs in its own class? Kerry -- http://www.kerrybuckley.com/
Well, I think it all depends on the scenario - but in a lot of cases it should absolutely be considered a code-smell. Stefan 2008/1/9, Kerry Buckley <kerry at kerrybuckley.com>:> > On Jan 9, 2008 10:01 AM, Stefan Magnus Landr? <stefan.landro at gmail.com> > wrote: > > I totally agree with you, David! > > > > For quite a while I was testing all my methods (even had to declare them > > protected/package scope in java!), but I realized that I was getting > into a > > lot of trouble. Now I''ve shifted to testing functionality in stead of > > methods. > > > > Now, sometimes you might end up having small methods (typically a result > of > > refactoring) that are being used by several clients. In that case you > should > > start testing those methods, since they actually represent real business > > logic. > > I wonder whether that is a smell indicating that the functionality in > those methods really belongs in its own class? > > Kerry > -- > http://www.kerrybuckley.com/ > _______________________________________________ > rspec-users mailing list > rspec-users at rubyforge.org > http://rubyforge.org/mailman/listinfo/rspec-users >-- Bekk Open Source http://boss.bekk.no -------------- next part -------------- An HTML attachment was scrubbed... URL: http://rubyforge.org/pipermail/rspec-users/attachments/20080109/c03ae384/attachment.html
On Jan 9, 2008 6:09 AM, Kerry Buckley <kerry at kerrybuckley.com> wrote:> On Jan 9, 2008 10:01 AM, Stefan Magnus Landr? <stefan.landro at gmail.com> wrote: > > I totally agree with you, David!Then you agree with the majority of the TDD community.> > > > For quite a while I was testing all my methods (even had to declare them > > protected/package scope in java!), but I realized that I was getting into a > > lot of trouble. Now I''ve shifted to testing functionality in stead of > > methods. > > > > Now, sometimes you might end up having small methods (typically a result of > > refactoring) that are being used by several clients. In that case you should > > start testing those methods, since they actually represent real business > > logic.Again, if they appear through refactoring, then they ARE tested through the public methods. The only time I would test them directly would be in the process of trying to locate the source of a bug, or if I wanted to move the method to another class because it represented a fundamentally different concept from the one represented by its current class. This is all TDD 101 stuff. Maybe we should have required reading on this list ;) Here are a few suggestions: http://www.amazon.com/Software-Development-Principles-Patterns-Practices/dp/0135974445/ http://www.amazon.com/Refactoring-Improving-Existing-Addison-Wesley-Technology/dp/0201485672 http://www.amazon.com/Test-Driven-Development-Practical-Guide-Coad/dp/0131016490/ http://www.amazon.com/Test-Driven-Development-Addison-Wesley-Signature/dp/0321146530/ These sorts of questions are explored in great detail in these books. Cheers, David> > I wonder whether that is a smell indicating that the functionality in > those methods really belongs in its own class? > > Kerry > -- > http://www.kerrybuckley.com/ > > _______________________________________________ > rspec-users mailing list > rspec-users at rubyforge.org > http://rubyforge.org/mailman/listinfo/rspec-users >
On Jan 9, 2008 7:12 AM, Stefan Magnus Landr? <stefan.landro at gmail.com> wrote:> > 2008/1/9, Kerry Buckley < kerry at kerrybuckley.com>: > > > On Jan 9, 2008 10:01 AM, Stefan Magnus Landr? < stefan.landro at gmail.com> > wrote: > > > I totally agree with you, David! > > > > > > For quite a while I was testing all my methods (even had to declare them > > > protected/package scope in java!), but I realized that I was getting > into a > > > lot of trouble. Now I''ve shifted to testing functionality in stead of > > > methods. > > > > > > Now, sometimes you might end up having small methods (typically a result > of > > > refactoring) that are being used by several clients. In that case you > should > > > start testing those methods, since they actually represent real business > > > logic. > > > > I wonder whether that is a smell indicating that the functionality in > > those methods really belongs in its own class?> Well, I think it all depends on the scenario - but in a lot of cases it > should absolutely be considered a code-smell.Keep in mind that a code smell is an indicator, but not a dictator. It is supposed to draw your attention but you still have to consider the pros and cons and make a reasoned decision. Cheers, David
On Jan 8, 2008 7:14 PM, Daniel Tenner <daniel.ruby at tenner.org> wrote:> Might be a personal thing, but my approach is that I try to test the > public behaviour of the object. Testing private methods is, imho, > getting dangerously close to specifying how the object does its > business, rather than what it does. > > I would just spec the externally visible behaviour, where it occurs, > and let the object implement it as it wants (using private methods or > any other mechanism).The trouble is that this makes it very difficult to get effective coverage. There is usually a lot more setup involved. If your tests are more expensive to write, because you are unit testing indirectly through public methods, less testing will get done, and worse tests can end up getting very brittle. I do like the idea though of during the unit test itself, you unseal the method. You get the best of both worlds.
On Jan 9, 2008 7:26 AM, Richard Conroy <richard.conroy at gmail.com> wrote:> On Jan 8, 2008 7:14 PM, Daniel Tenner <daniel.ruby at tenner.org> wrote: > > Might be a personal thing, but my approach is that I try to test the > > public behaviour of the object. Testing private methods is, imho, > > getting dangerously close to specifying how the object does its > > business, rather than what it does. > > > > I would just spec the externally visible behaviour, where it occurs, > > and let the object implement it as it wants (using private methods or > > any other mechanism). > > The trouble is that this makes it very difficult to get effective coverage. > There is usually a lot more setup involved. If your tests are more expensive > to write, because you are unit testing indirectly through public methods, > less testing will get done,If you live by the rule of "never write a line of code except to get a failing example to pass," then you should always have 100% coverage implicitly. If you''re back-filling that''s a different story, but then we''re not talking about BDD anymore. I''m not saying that I will never test private methods, but it''s something that should be rare and avoided. FWIW, David> and worse tests can end up getting very brittle. > > I do like the idea though of during the unit test itself, you unseal the method. > You get the best of both worlds. > > _______________________________________________ > rspec-users mailing list > rspec-users at rubyforge.org > http://rubyforge.org/mailman/listinfo/rspec-users >
This is also worth checking out: http://xunitpatterns.com/ -- Bekk Open Source http://boss.bekk.no -------------- next part -------------- An HTML attachment was scrubbed... URL: http://rubyforge.org/pipermail/rspec-users/attachments/20080109/a02523a1/attachment.html
On Jan 8, 2008 3:56 PM, Francois Wurmus <francois.wurmus at infopark.de> wrote:> Chris Olsen schrieb: > > Will obj.send(:method) work in 1.9 or is it saying that the send call > > requires 2 params, the method and the object reference? > > > obj.send(:method) will work for non-private methods and send! works for > private methods.I''m pretty sure that Matz backed this difference out between 1.8 and 1.9, he took out some of these things before the Christmas release of 1.9.0 Object#send will work for private methods as it always did, and Object#send! is no longer there.> additionally there is send() without a receiving object. that is the > only of those methods requiring two parameters.There''s no such thing as a method call without a receiving object in Ruby send(:foo) is the same as self.send(:foo) Object#send takes an arbitrary number of arguments, the first argument is the method selector, the rest are used as arguments to the method being called. -- Rick DeNatale My blog on Ruby http://talklikeaduck.denhaven2.com/
On Jan 8, 2008 11:25 AM, Matt Patterson <matt-lists at reprocessed.org> wrote:> On 8 Jan 2008, at 19:14, Daniel Tenner wrote: > > > > Might be a personal thing, but my approach is that I try to test the > > public behaviour of the object. Testing private methods is, imho, > > getting dangerously close to specifying how the object does its > > business, rather than what it does. > > > > If you''re a proponent of many-skinny-methods then you wind up with a > lot of public methods which should never need to be called by another > object, so making them private can be a good thing for general users > of the object.There are several problems with this thinking: 1. It separates refactoring from BDD. Having several private helper methods is a result of refactoring. Refactoring is not only a design tool, but a central part of BDD. 2. It couples your specs too tightly to the implementation. If you rename/delete/add a private method, you need to update the spec, even though the object''s public behavior didn''t change. 3. It eliminates the effectiveness of specs as documentation. Reading the specs is one of the first things I do when examining some code. If I see examples of how to use it, except not really, because they''re private methods, I''m going to be seriously confused or seriously pissed off. 4. It leads to violations of the "one defect, one broken test" guideline. Some spec that exercises public behavior covers this private method, as does its individual spec, so if it breaks, then you have two broken specs. Whether that''s important is a matter of personal preference. There is no 1:1 mapping of specification example to method. You don''t specify a method. You specify a behavior. There are no rules dictating the specification''s granularity, as long as it helps you design your code in the beginning, and localize defects as your code base persists. Specifying private methods is, in my opinion, slightly less pleasant than week-old trout in my pillow. It means one of two things. At a code level, your method''s visibility could just be off. At a conceptual level, you may be missing the point of BDD entirely. Pat
On Jan 9, 2008 5:26 AM, Richard Conroy <richard.conroy at gmail.com> wrote:> On Jan 8, 2008 7:14 PM, Daniel Tenner <daniel.ruby at tenner.org> wrote: > > Might be a personal thing, but my approach is that I try to test the > > public behaviour of the object. Testing private methods is, imho, > > getting dangerously close to specifying how the object does its > > business, rather than what it does. > > > > I would just spec the externally visible behaviour, where it occurs, > > and let the object implement it as it wants (using private methods or > > any other mechanism). > > The trouble is that this makes it very difficult to get effective coverage. > There is usually a lot more setup involved. If your tests are more expensive > to write, because you are unit testing indirectly through public methods, > less testing will get done, and worse tests can end up getting very brittle.I''ve found that this is symptomatic of a bad design. Specifically, it smells of an SRP violation or tight coupling. One thing I think people don''t realize is that a bit of code should be easiest to use in a test. We test objects in isolation, they don''t depend on expensive external resources, etc. If you can''t use the code within a test harness - that is, write a test for it - then how can you possibly expect to use it in production? In practice, of course, this can all be very difficult. I recommend reading "Working Effectively with Legacy Code" by Michael Feathers for some excellent strategies on how to make existing gnarly code easier to test.> I do like the idea though of during the unit test itself, you unseal the method. > You get the best of both worlds.Not really. You get test coverage, which is good, but at the expense of carefully thinking why your design might be wrong. Pat
It''s interesting that this thread has started because I just ran into
this problem.
The error I got was:
NoMethodError in ''Signup she be a valid mac address''
protected method `normalize_mac'' called for #<Signup:0x408c0434>
./spec/models/signup_spec.rb:10:
Here''s the spec:
describe Signup do
before(:each) do
@signup = Signup.new
end
it "she be a valid mac address" do
@signup.mac_address = "00-11-22-33-44-55-66"
normalized = @signup.normalize_mac(@signup.mac_address)
@signup.mac_address.should == normalized
end
end
I have a model that has mac_address attribute. In the before
validation, I wanted to make the mac address have colons(:) instead of
whatever the user typed in which could have spaces between, dashes,
nothing at all.
My thought was to just remove all of those special characters validate
it against a regex then if it passed the regex check produce the mac
address with the colons included.
And, if I understand it properly, Pat''s statement is saying that I
really shouldn''t be protecting that method...which would fix my
problem.
So my question is then, how do you know when to use protected and
private or do I just do what Rick suggested and use send?
I just realized this isn''t really an rspec question...so I''ll
just move along.
Mike B.
----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.
Apart from private or public methods I see a problem in your test case.
You don''t make sure that your mac_address is returned in exactly the
way
you want - you are merely saying it should look like the return value of
some (protected) method you call (normalize_mac).
Let''s assume this method was implemented wrong and just returns nil.
Let''s further assume that @signup.mac_address calls a reader method
returning nil, too. Such an implementation, though wrong, would satisfy
your spec!
A second problem is, that the parameter you are passing to the
normalize-method will already have been transformed before your method
gets its hands on it. You are not passing the init value but the value
that is returned be a read operation on the mac_address field.
What you really should specify is your concrete mac address format:
it "should return a valid mac address" do
@signup.mac_address = "00-11-22-33-44-55"
@signup.mac_address.should == "00:11:22:33:44:55"
end
Now you are not only implementation independent but your spec is also
saying more.
Fran?ois
barsalou schrieb:> It''s interesting that this thread has started because I just ran
into
> this problem.
>
> The error I got was:
>
> NoMethodError in ''Signup she be a valid mac address''
> protected method `normalize_mac'' called for
#<Signup:0x408c0434>
> ./spec/models/signup_spec.rb:10:
>
>
> Here''s the spec:
>
> describe Signup do
> before(:each) do
> @signup = Signup.new
> end
>
> it "she be a valid mac address" do
> @signup.mac_address = "00-11-22-33-44-55-66"
> normalized = @signup.normalize_mac(@signup.mac_address)
> @signup.mac_address.should == normalized
> end
> end
>
>
> I have a model that has mac_address attribute. In the before
> validation, I wanted to make the mac address have colons(:) instead of
> whatever the user typed in which could have spaces between, dashes,
> nothing at all.
>
> My thought was to just remove all of those special characters validate
> it against a regex then if it passed the regex check produce the mac
> address with the colons included.
>
> And, if I understand it properly, Pat''s statement is saying that I
> really shouldn''t be protecting that method...which would fix my
problem.
>
> So my question is then, how do you know when to use protected and
> private or do I just do what Rick suggested and use send?
>
> I just realized this isn''t really an rspec question...so
I''ll just move along.
>
> Mike B.
>
> ----------------------------------------------------------------
> This message was sent using IMP, the Internet Messaging Program.
>
> _______________________________________________
> rspec-users mailing list
> rspec-users at rubyforge.org
> http://rubyforge.org/mailman/listinfo/rspec-users
>
>
On 10.1.2008, at 22.21, Francois Wurmus wrote:> Apart from private or public methods I see a problem in your test > case. > You don''t make sure that your mac_address is returned in exactly the > way > you want - you are merely saying it should look like the return > value of > some (protected) method you call (normalize_mac). > > Let''s assume this method was implemented wrong and just returns nil. > Let''s further assume that @signup.mac_address calls a reader method > returning nil, too. Such an implementation, though wrong, would > satisfy > your spec! > > A second problem is, that the parameter you are passing to the > normalize-method will already have been transformed before your method > gets its hands on it. You are not passing the init value but the value > that is returned be a read operation on the mac_address field. > > What you really should specify is your concrete mac address format: > > it "should return a valid mac address" do > @signup.mac_address = "00-11-22-33-44-55" > @signup.mac_address.should == "00:11:22:33:44:55" > end > > Now you are not only implementation independent but your spec is also > saying more.Also, this line of code is a bit smelly:>> normalized = @signup.normalize_mac(@signup.mac_address)Since normalize_mac is an instance method in the Signup class, there''s no point passing the mac_address as a parameter; the method can simply call the mac_address method directly. If you want to make it more general-purpose, it doesn''t sound like it should be an instance method anymore. //jarkko -- Jarkko Laine http://jlaine.net http://dotherightthing.com http://www.railsecommerce.com http://odesign.fi
Even my suggestion wouldn''t be a sufficient spec. What about completely wrong mac addresses? Empty addresses, too many characters, invalid characters? It is not for nothing that the rspec test cases are called examples - you may define any number of example inputs and expected outputs that are required to describe the behavior of the object to be specified. barsalou schrieb:> It''s interesting that this thread has started because I just ran into > this problem. > > The error I got was: > > NoMethodError in ''Signup she be a valid mac address'' > protected method `normalize_mac'' called for #<Signup:0x408c0434> > ./spec/models/signup_spec.rb:10: > > > Here''s the spec: > > describe Signup do > before(:each) do > @signup = Signup.new > end > > it "she be a valid mac address" do > @signup.mac_address = "00-11-22-33-44-55-66" > normalized = @signup.normalize_mac(@signup.mac_address) > @signup.mac_address.should == normalized > end > end > > > I have a model that has mac_address attribute. In the before > validation, I wanted to make the mac address have colons(:) instead of > whatever the user typed in which could have spaces between, dashes, > nothing at all. > > My thought was to just remove all of those special characters validate > it against a regex then if it passed the regex check produce the mac > address with the colons included. > > And, if I understand it properly, Pat''s statement is saying that I > really shouldn''t be protecting that method...which would fix my problem. > > So my question is then, how do you know when to use protected and > private or do I just do what Rick suggested and use send? > > I just realized this isn''t really an rspec question...so I''ll just move along. > > Mike B. > > ---------------------------------------------------------------- > This message was sent using IMP, the Internet Messaging Program. > > _______________________________________________ > rspec-users mailing list > rspec-users at rubyforge.org > http://rubyforge.org/mailman/listinfo/rspec-users > >
On Jan 10, 2008 12:25 PM, Jarkko Laine <jarkko at jlaine.net> wrote:> Also, this line of code is a bit smelly: > > >> normalized = @signup.normalize_mac(@signup.mac_address) > > Since normalize_mac is an instance method in the Signup class, there''s > no point passing the mac_address as a parameter; the method can simply > call the mac_address method directly. If you want to make it more > general-purpose, it doesn''t sound like it should be an instance method > anymore.Right, you could pull this into a MacAddress class, and use that: describe MacAddress, " for 00-11-22-33-44-55" do it "should normalize to 00:11:22:33:44:55" do MacAddress.new("00-11-22-33-44-55").to_s.should == "00:11:22:33:44:55" end end Then you can just do @signup.mac_address = MacAddress.new "00-11-22-33-44-55" and you don''t really need to test that, of course, since it''s just a setter. Pat
On Jan 10, 2008 12:34 PM, Francois Wurmus <francois.wurmus at infopark.de> wrote:> Even my suggestion wouldn''t be a sufficient spec. What about completely > wrong mac addresses? Empty addresses, too many characters, invalid > characters?I just sent a reply that crossed wires with yours...anyway, introducing a MacAddress class here would be great for solving these sorts of problems. Mac address validation is something that the @signup object should not be concerned with. Pat
Yes, you''re absolutely right. I somehow ignored the class name in my previous answers, but that would have been the next point of attack. ;-) Pat Maddox schrieb:> On Jan 10, 2008 12:34 PM, Francois Wurmus <francois.wurmus at infopark.de> wrote: >> Even my suggestion wouldn''t be a sufficient spec. What about completely >> wrong mac addresses? Empty addresses, too many characters, invalid >> characters? > > I just sent a reply that crossed wires with yours...anyway, > introducing a MacAddress class here would be great for solving these > sorts of problems. Mac address validation is something that the > @signup object should not be concerned with. > > Pat > _______________________________________________ > rspec-users mailing list > rspec-users at rubyforge.org > http://rubyforge.org/mailman/listinfo/rspec-users > >
Daniel Tenner:> Might be a personal thing, but my approach is that I try to test the > public behaviour of the object. Testing private methods is, imho, > getting dangerously close to specifying how the object does its > business, rather than what it does.I agree on principle, but I ran into the following case in my PhD: There?s a Decomposition class that decomposes an FSM to a given architecture. Its public methods should be new() and decompose!(). Now, decompose!() works by running a private method by_input_sets!() many times with different parameters. One run of by_input_sets!() takes a couple of seconds, so can be tested; one run of decompose!() takes much longer, so to test decompose!() I should stub by_input_sets!() so it returns canned data (right?). In this situation, I think I do need to test/spec the by_input_sets!() private method ? otherwise there would be no code that would check on the way it works. -- Shot -- A school in the UK is using RFID chips in school uniforms to track attendance. So now it''s easy to cut class; just ask someone to carry your shirt around the building while you''re elsewhere. -- Bruce Schneier -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 191 bytes Desc: not available Url : http://rubyforge.org/pipermail/rspec-users/attachments/20080111/66c1481b/attachment.bin
On Jan 11, 2008 3:49 AM, Shot (Piotr Szotkowski) <shot at hot.pl> wrote:> Daniel Tenner: > > > Might be a personal thing, but my approach is that I try to test the > > public behaviour of the object. Testing private methods is, imho, > > getting dangerously close to specifying how the object does its > > business, rather than what it does. > > I agree on principle, but I ran into the following case in my PhD: > > There''s a Decomposition class that decomposes an FSM to a given > architecture. Its public methods should be new() and decompose!(). > Now, decompose!() works by running a private method by_input_sets!() > many times with different parameters. > > One run of by_input_sets!() takes a couple of seconds, so can be tested; > one run of decompose!() takes much longer, so to test decompose!() > I should stub by_input_sets!() so it returns canned data (right?). > > In this situation, I think I do need to test/spec the by_input_sets!() > private method ? otherwise there would be no code that would check on > the way it works.In TDD there is a rule of thumb that says don''t stub a method in the same class as the method you''re testing. The risk is that as the real implementation of by_input_sets!() changes over time, it has access to internal state that could impact the behaviour of decompose!(). It also sounds like by_input_sets!() has enough independent responsibility that it should actually be public. The standard approach here, which is as much a result of OO principles as TDD, is to extract a class with the by_input_sets!() exposed as a public method. You mock that object in your Decomposition examples and then you have public access to by_input_sets!() on the new class. Cheers, David> > -- Shot > -- > A school in the UK is using RFID chips in school uniforms to track > attendance. So now it''s easy to cut class; just ask someone to carry > your shirt around the building while you''re elsewhere. -- Bruce Schneier > > _______________________________________________ > rspec-users mailing list > rspec-users at rubyforge.org > http://rubyforge.org/mailman/listinfo/rspec-users >
David Chelimsky:> On Jan 11, 2008 3:49 AM, Shot (Piotr Szotkowski) <shot at hot.pl> wrote:>> One run of by_input_sets!() takes a couple of seconds, so can >> be tested; one run of decompose!() takes much longer, so to test >> decompose!() I should stub by_input_sets!() so it returns canned >> data (right?).> In TDD there is a rule of thumb that says don''t stub a method in the > same class as the method you''re testing. The risk is that as the real > implementation of by_input_sets!() changes over time, it has access to > internal state that could impact the behaviour of decompose!().Ah, well pointed.> It also sounds like by_input_sets!() has enough independent > responsibility that it should actually be public.It actually started out as a public method and only this very thread made me consider switching it to private. :)> The standard approach here, which is as much a result of OO principles > as TDD, is to extract a class with the by_input_sets!() exposed as > a public method. You mock that object in your Decomposition examples > and then you have public access to by_input_sets!() on the new class.Ok, makes sense, thanks ? this definitely made me think about the design. What about the general issue of a slow method that is fast enough to be testable in isolation, but some other method from the same class calls it so many times that it needs to be stubbed for that method to be spec?d in a sane manner? -- Shot -- Fail hard. Fail with motherfucking gusto. Succeeding, like flying, is throwing yourself to the ground and missing. -- Jeff Hodges -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 191 bytes Desc: not available Url : http://rubyforge.org/pipermail/rspec-users/attachments/20080111/f1b42214/attachment.bin
On Jan 11, 2008 8:04 AM, Shot (Piotr Szotkowski) <shot at hot.pl> wrote:> David Chelimsky: > > > On Jan 11, 2008 3:49 AM, Shot (Piotr Szotkowski) <shot at hot.pl> wrote: > > >> One run of by_input_sets!() takes a couple of seconds, so can > >> be tested; one run of decompose!() takes much longer, so to test > >> decompose!() I should stub by_input_sets!() so it returns canned > >> data (right?). > > > In TDD there is a rule of thumb that says don''t stub a method in the > > same class as the method you''re testing. The risk is that as the real > > implementation of by_input_sets!() changes over time, it has access to > > internal state that could impact the behaviour of decompose!(). > > Ah, well pointed. > > > It also sounds like by_input_sets!() has enough independent > > responsibility that it should actually be public. > > It actually started out as a public method and only this > very thread made me consider switching it to private. :) > > > The standard approach here, which is as much a result of OO principles > > as TDD, is to extract a class with the by_input_sets!() exposed as > > a public method. You mock that object in your Decomposition examples > > and then you have public access to by_input_sets!() on the new class. > > Ok, makes sense, thanks ? this definitely > made me think about the design. > > What about the general issue of a slow method that is fast enough to be > testable in isolation, but some other method from the same class calls > it so many times that it needs to be stubbed for that method to be > spec''d in a sane manner?Every case is different. In this case, the method is slow because it calls one other method repeatedly, so the solution we''ve discussed may make sense. Without seeing the actual code, it''s difficult to assess. In other cases there might be other design changes that could be made to not only improve the speed, but clarify intent. HTH, David> > -- Shot > -- > Fail hard. Fail with motherfucking gusto. Succeeding, like flying, > is throwing yourself to the ground and missing. -- Jeff Hodges > > _______________________________________________ > rspec-users mailing list > rspec-users at rubyforge.org > http://rubyforge.org/mailman/listinfo/rspec-users >
On Jan 11, 2008 5:23 AM, David Chelimsky <dchelimsky at gmail.com> wrote:> On Jan 11, 2008 3:49 AM, Shot (Piotr Szotkowski) <shot at hot.pl> wrote: > > Daniel Tenner: > > > > > Might be a personal thing, but my approach is that I try to test the > > > public behaviour of the object. Testing private methods is, imho, > > > getting dangerously close to specifying how the object does its > > > business, rather than what it does. > > > > I agree on principle, but I ran into the following case in my PhD: > > > > There''s a Decomposition class that decomposes an FSM to a given > > architecture. Its public methods should be new() and decompose!(). > > Now, decompose!() works by running a private method by_input_sets!() > > many times with different parameters. > > > > One run of by_input_sets!() takes a couple of seconds, so can be tested; > > one run of decompose!() takes much longer, so to test decompose!() > > I should stub by_input_sets!() so it returns canned data (right?). > > > > In this situation, I think I do need to test/spec the by_input_sets!() > > private method ? otherwise there would be no code that would check on > > the way it works. > > In TDD there is a rule of thumb that says don''t stub a method in the > same class as the method you''re testing.A simple rule I follow: Don''t stub or mock the object under test. On Jan 11, 2008 9:04 AM, Shot (Piotr Szotkowski) <shot at hot.pl> wrote:> > What about the general issue of a slow method that is fast enough to be > testable in isolation, but some other method from the same class calls > it so many times that it needs to be stubbed for that method to be > spec''d in a sane manner?In addition to David''s response to this, have you evaluated moving this method and its responsibility to its own object that could be tested in isolation, allowing you to not break the "don''t stub or mock the object under test" guideline? -- Zach Dennis http://www.continuousthinking.com
David Chelimsky wrote:> > > In TDD there is a rule of thumb that says don''t stub a method in the > same class as the method you''re testing. The risk is that as the real > implementation of by_input_sets!() changes over time, it has access to > internal state that could impact the behaviour of decompose!(). >So, stubbing a current_user method on a rails controller would be considered bad practice? I suppose stubbing the find on User would be just as easy but I have always just stubbed controller.current_user. -Ben
On Jan 11, 2008 9:54 AM, Ben Mabey <ben at benmabey.com> wrote:> David Chelimsky wrote: > > > > > > In TDD there is a rule of thumb that says don''t stub a method in the > > same class as the method you''re testing. The risk is that as the real > > implementation of by_input_sets!() changes over time, it has access to > > internal state that could impact the behaviour of decompose!(). > > > > > So, stubbing a current_user method on a rails controller would be > considered bad practice? > I suppose stubbing the find on User would be just as easy but I have > always just stubbed controller.current_user.Rails is tricky. These rules are stem from situations in which you are in complete control of the design. Clearly, Rails makes it easy to work with if you follow its conventions, but the resulting design is far from Object Oriented. This is not an inherently bad thing - don''t get me wrong. I use Rails and it''s a delight in terms of development. But it''s a challenge in terms of this kind of testing. That said, the User class object is a different object than a user instance, so I have no issue w/ stubbing find on it. As for controller.current_user, a purist TDD view would have you move that behaviour elsewhere. I break the rule and just stub it directly. This general advice I learned from Uncle Bob Martin: sometimes you have to break the rules, but when you do you should do it consciously and feel dirty about it ;)> > -Ben > > _______________________________________________ > rspec-users mailing list > rspec-users at rubyforge.org > http://rubyforge.org/mailman/listinfo/rspec-users >
On Jan 11, 2008 11:56 AM, David Chelimsky <dchelimsky at gmail.com> wrote:> On Jan 11, 2008 9:54 AM, Ben Mabey <ben at benmabey.com> wrote: > > David Chelimsky wrote: > > > > > > > > > In TDD there is a rule of thumb that says don''t stub a method in the > > > same class as the method you''re testing. The risk is that as the real > > > implementation of by_input_sets!() changes over time, it has access to > > > internal state that could impact the behaviour of decompose!(). > > > > > > > > > So, stubbing a current_user method on a rails controller would be > > considered bad practice? > > I suppose stubbing the find on User would be just as easy but I have > > always just stubbed controller.current_user. > > Rails is tricky. These rules are stem from situations in which you are > in complete control of the design. Clearly, Rails makes it easy to > work with if you follow its conventions, but the resulting design is > far from Object Oriented. This is not an inherently bad thing - don''t > get me wrong. I use Rails and it''s a delight in terms of development. > But it''s a challenge in terms of this kind of testing. > > That said, the User class object is a different object than a user > instance, so I have no issue w/ stubbing find on it. > > As for controller.current_user, a purist TDD view would have you move > that behaviour elsewhere. I break the rule and just stub it directly. > This general advice I learned from Uncle Bob Martin: sometimes you > have to break the rules, but when you do you should do it consciously > and feel dirty about it ;)On the current project we''ve quit moved all authentication into a LoginManager. This has worked out so nicely as we have simple methods for: login_from_cookie, login_from_session, login_from_user_credentials, etc. This cleans up a lot of the hairy code sprinkled throughout controllers and before filters which were trying to do some form of authentication based on peeking at the sessions themselves or validating users. -- Zach Dennis http://www.continuousthinking.com
On Jan 11, 2008 11:40 AM, Zach Dennis <zach.dennis at gmail.com> wrote:> On Jan 11, 2008 11:56 AM, David Chelimsky <dchelimsky at gmail.com> wrote: > > On Jan 11, 2008 9:54 AM, Ben Mabey <ben at benmabey.com> wrote: > > > David Chelimsky wrote: > > > > > > > > > > > > In TDD there is a rule of thumb that says don''t stub a method in the > > > > same class as the method you''re testing. The risk is that as the real > > > > implementation of by_input_sets!() changes over time, it has access to > > > > internal state that could impact the behaviour of decompose!(). > > > > > > > > > > > > > So, stubbing a current_user method on a rails controller would be > > > considered bad practice? > > > I suppose stubbing the find on User would be just as easy but I have > > > always just stubbed controller.current_user. > > > > Rails is tricky. These rules are stem from situations in which you are > > in complete control of the design. Clearly, Rails makes it easy to > > work with if you follow its conventions, but the resulting design is > > far from Object Oriented. This is not an inherently bad thing - don''t > > get me wrong. I use Rails and it''s a delight in terms of development. > > But it''s a challenge in terms of this kind of testing. > > > > That said, the User class object is a different object than a user > > instance, so I have no issue w/ stubbing find on it. > > > > As for controller.current_user, a purist TDD view would have you move > > that behaviour elsewhere. I break the rule and just stub it directly. > > This general advice I learned from Uncle Bob Martin: sometimes you > > have to break the rules, but when you do you should do it consciously > > and feel dirty about it ;) > > On the current project we''ve quit moved all authentication into a > LoginManager. This has worked out so nicely as we have simple methods > for: login_from_cookie, login_from_session, > login_from_user_credentials, etc. > > This cleans up a lot of the hairy code sprinkled throughout > controllers and before filters which were trying to do some form of > authentication based on peeking at the sessions themselves or > validating users.Cool. That sounds like the right way to go. I guess I''ve just been lazy - not wanting to have to stray too much from the code generated by restful_authentication on each project. Guess it''s time for my own internal plugin/engine :)
Zach Dennis wrote:> On Jan 11, 2008 11:56 AM, David Chelimsky <dchelimsky at gmail.com> wrote: > >> On Jan 11, 2008 9:54 AM, Ben Mabey <ben at benmabey.com> wrote: >> >>> David Chelimsky wrote: >>> >>>> In TDD there is a rule of thumb that says don''t stub a method in the >>>> same class as the method you''re testing. The risk is that as the real >>>> implementation of by_input_sets!() changes over time, it has access to >>>> internal state that could impact the behaviour of decompose!(). >>>> >>>> >>> So, stubbing a current_user method on a rails controller would be >>> considered bad practice? >>> I suppose stubbing the find on User would be just as easy but I have >>> always just stubbed controller.current_user. >>> >> Rails is tricky. These rules are stem from situations in which you are >> in complete control of the design. Clearly, Rails makes it easy to >> work with if you follow its conventions, but the resulting design is >> far from Object Oriented. This is not an inherently bad thing - don''t >> get me wrong. I use Rails and it''s a delight in terms of development. >> But it''s a challenge in terms of this kind of testing. >> >> That said, the User class object is a different object than a user >> instance, so I have no issue w/ stubbing find on it. >> >> As for controller.current_user, a purist TDD view would have you move >> that behaviour elsewhere. I break the rule and just stub it directly. >> This general advice I learned from Uncle Bob Martin: sometimes you >> have to break the rules, but when you do you should do it consciously >> and feel dirty about it ;) >> > > On the current project we''ve quit moved all authentication into a > LoginManager. This has worked out so nicely as we have simple methods > for: login_from_cookie, login_from_session, > login_from_user_credentials, etc. > > This cleans up a lot of the hairy code sprinkled throughout > controllers and before filters which were trying to do some form of > authentication based on peeking at the sessions themselves or > validating users. > >Interesting, do you pass in the session in the constructor or how do you get access to the session data? -Ben
We pass the required items in as method arguments. In the spirit of
sharing code and getting people to review code. Here is our current
LoginManager:
class LoginManager
include Injection
inject :invitation_manager
def login_from_cookie(cookies, session)
CookieLoginManager.new(
:cookies => cookies,
:session => session,
:invitation_manager => @invitation_manager
).login
end
def login_from_session(session)
SessionLoginManager.new(
:session => session,
:invitation_manager => @invitation_manager
).login
end
def login_with_credentials(options, session, cookies)
UserCredentialsLoginManager.new(
:options => options,
:session => session,
:cookies => cookies,
:invitation_manager => @invitation_manager
).login
end
def login_from_password_reset(user, session)
PasswordResetLoginManager.new(
:user => user,
:session => session
).login
end
def login_from_signup(user, session)
SignupLoginManager.new(
:user => user,
:session => session,
:invitation_manager => @invitation_manager
).login
end
end
The reason we did this in the first place was that we needed to be
able to add functionality (accepting invitations) to the login process
and it seemed to get ugly without having it isolated in it''s own
object. We use additional login managers behind the scenes to have
simple testable objects for each type of login we do.
The "Injection" module just lets us pull out existing objects from a
global app content and assign them as instance variables. That is how
we are getting reference to invitation manager.
Zach
On Jan 11, 2008 12:45 PM, Ben Mabey <ben at benmabey.com>
wrote:>
> Zach Dennis wrote:
> > On Jan 11, 2008 11:56 AM, David Chelimsky <dchelimsky at
gmail.com> wrote:
> >
> >> On Jan 11, 2008 9:54 AM, Ben Mabey <ben at benmabey.com>
wrote:
> >>
> >>> David Chelimsky wrote:
> >>>
> >>>> In TDD there is a rule of thumb that says don''t
stub a method in the
> >>>> same class as the method you''re testing. The risk
is that as the real
> >>>> implementation of by_input_sets!() changes over time, it
has access to
> >>>> internal state that could impact the behaviour of
decompose!().
> >>>>
> >>>>
> >>> So, stubbing a current_user method on a rails controller would
be
> >>> considered bad practice?
> >>> I suppose stubbing the find on User would be just as easy but
I have
> >>> always just stubbed controller.current_user.
> >>>
> >> Rails is tricky. These rules are stem from situations in which you
are
> >> in complete control of the design. Clearly, Rails makes it easy to
> >> work with if you follow its conventions, but the resulting design
is
> >> far from Object Oriented. This is not an inherently bad thing -
don''t
> >> get me wrong. I use Rails and it''s a delight in terms of
development.
> >> But it''s a challenge in terms of this kind of testing.
> >>
> >> That said, the User class object is a different object than a user
> >> instance, so I have no issue w/ stubbing find on it.
> >>
> >> As for controller.current_user, a purist TDD view would have you
move
> >> that behaviour elsewhere. I break the rule and just stub it
directly.
> >> This general advice I learned from Uncle Bob Martin: sometimes you
> >> have to break the rules, but when you do you should do it
consciously
> >> and feel dirty about it ;)
> >>
> >
> > On the current project we''ve quit moved all authentication
into a
> > LoginManager. This has worked out so nicely as we have simple methods
> > for: login_from_cookie, login_from_session,
> > login_from_user_credentials, etc.
> >
> > This cleans up a lot of the hairy code sprinkled throughout
> > controllers and before filters which were trying to do some form of
> > authentication based on peeking at the sessions themselves or
> > validating users.
> >
> >
> Interesting, do you pass in the session in the constructor or how do you
> get access to the session data?
>
> -Ben
>
--
Zach Dennis
http://www.continuousthinking.com
To add, all of our managers return LoginResult objects which contain methods like: - successful? - user - message In the controller our code will look like: if login.successful? self.current_user = login.user else flash[:error] = login.message end This has worked well because it allows each our types of logins to generate an accurate login message based on the type of login. For example when a user logs in from a session you don''t need a message, but after someone logs in with their credentials you may want to say "You''ve successfully logged in". I like this approach because it removes responsibility away from the controller, and it makes things much easier to test (and to understand IMO). And you have a very readable API, Zach On Jan 11, 2008 12:49 PM, Zach Dennis <zach.dennis at gmail.com> wrote:> We pass the required items in as method arguments. In the spirit of > sharing code and getting people to review code. Here is our current > LoginManager: > > class LoginManager > include Injection > inject :invitation_manager > > def login_from_cookie(cookies, session) > CookieLoginManager.new( > :cookies => cookies, > :session => session, > :invitation_manager => @invitation_manager > ).login > end > > def login_from_session(session) > SessionLoginManager.new( > :session => session, > :invitation_manager => @invitation_manager > ).login > end > > def login_with_credentials(options, session, cookies) > UserCredentialsLoginManager.new( > :options => options, > :session => session, > :cookies => cookies, > :invitation_manager => @invitation_manager > ).login > end > > def login_from_password_reset(user, session) > PasswordResetLoginManager.new( > :user => user, > :session => session > ).login > end > > def login_from_signup(user, session) > SignupLoginManager.new( > :user => user, > :session => session, > :invitation_manager => @invitation_manager > ).login > end > > end > > > The reason we did this in the first place was that we needed to be > able to add functionality (accepting invitations) to the login process > and it seemed to get ugly without having it isolated in it''s own > object. We use additional login managers behind the scenes to have > simple testable objects for each type of login we do. > > The "Injection" module just lets us pull out existing objects from a > global app content and assign them as instance variables. That is how > we are getting reference to invitation manager. > > Zach > > > On Jan 11, 2008 12:45 PM, Ben Mabey <ben at benmabey.com> wrote: > > > > Zach Dennis wrote: > > > On Jan 11, 2008 11:56 AM, David Chelimsky <dchelimsky at gmail.com> wrote: > > > > > >> On Jan 11, 2008 9:54 AM, Ben Mabey <ben at benmabey.com> wrote: > > >> > > >>> David Chelimsky wrote: > > >>> > > >>>> In TDD there is a rule of thumb that says don''t stub a method in the > > >>>> same class as the method you''re testing. The risk is that as the real > > >>>> implementation of by_input_sets!() changes over time, it has access to > > >>>> internal state that could impact the behaviour of decompose!(). > > >>>> > > >>>> > > >>> So, stubbing a current_user method on a rails controller would be > > >>> considered bad practice? > > >>> I suppose stubbing the find on User would be just as easy but I have > > >>> always just stubbed controller.current_user. > > >>> > > >> Rails is tricky. These rules are stem from situations in which you are > > >> in complete control of the design. Clearly, Rails makes it easy to > > >> work with if you follow its conventions, but the resulting design is > > >> far from Object Oriented. This is not an inherently bad thing - don''t > > >> get me wrong. I use Rails and it''s a delight in terms of development. > > >> But it''s a challenge in terms of this kind of testing. > > >> > > >> That said, the User class object is a different object than a user > > >> instance, so I have no issue w/ stubbing find on it. > > >> > > >> As for controller.current_user, a purist TDD view would have you move > > >> that behaviour elsewhere. I break the rule and just stub it directly. > > >> This general advice I learned from Uncle Bob Martin: sometimes you > > >> have to break the rules, but when you do you should do it consciously > > >> and feel dirty about it ;) > > >> > > > > > > On the current project we''ve quit moved all authentication into a > > > LoginManager. This has worked out so nicely as we have simple methods > > > for: login_from_cookie, login_from_session, > > > login_from_user_credentials, etc. > > > > > > This cleans up a lot of the hairy code sprinkled throughout > > > controllers and before filters which were trying to do some form of > > > authentication based on peeking at the sessions themselves or > > > validating users. > > > > > > > > Interesting, do you pass in the session in the constructor or how do you > > get access to the session data? > > > > -Ben > > > > > > > -- > Zach Dennis > http://www.continuousthinking.com >-- Zach Dennis http://www.continuousthinking.com
Thank you Zach. I was just about to ask about this. I''m just getting started with restful_authentication and have missed the context of your point. restful_authentication is such a huge improvement over what I''m use to. Could you elaborate just a little on the use context in controllers? Is this called from the "authenticate" method in the controller? Cody Zach Dennis wrote:> We pass the required items in as method arguments. In the spirit of > sharing code and getting people to review code. Here is our current > LoginManager: > > class LoginManager > include Injection > inject :invitation_manager > > def login_from_cookie(cookies, session) > CookieLoginManager.new( > :cookies => cookies, > :session => session, > :invitation_manager => @invitation_manager > ).login > end > > def login_from_session(session) > SessionLoginManager.new( > :session => session, > :invitation_manager => @invitation_manager > ).login > end > > def login_with_credentials(options, session, cookies) > UserCredentialsLoginManager.new( > :options => options, > :session => session, > :cookies => cookies, > :invitation_manager => @invitation_manager > ).login > end > > def login_from_password_reset(user, session) > PasswordResetLoginManager.new( > :user => user, > :session => session > ).login > end > > def login_from_signup(user, session) > SignupLoginManager.new( > :user => user, > :session => session, > :invitation_manager => @invitation_manager > ).login > end > > end > > > The reason we did this in the first place was that we needed to be > able to add functionality (accepting invitations) to the login process > and it seemed to get ugly without having it isolated in it''s own > object. We use additional login managers behind the scenes to have > simple testable objects for each type of login we do. > > The "Injection" module just lets us pull out existing objects from a > global app content and assign them as instance variables. That is how > we are getting reference to invitation manager. > > Zach > > On Jan 11, 2008 12:45 PM, Ben Mabey <ben at benmabey.com> wrote: >> >> Zach Dennis wrote: >> > On Jan 11, 2008 11:56 AM, David Chelimsky <dchelimsky at gmail.com> >> wrote: >> > >> >> On Jan 11, 2008 9:54 AM, Ben Mabey <ben at benmabey.com> wrote: >> >> >> >>> David Chelimsky wrote: >> >>> >> >>>> In TDD there is a rule of thumb that says don''t stub a method in >> the >> >>>> same class as the method you''re testing. The risk is that as the >> real >> >>>> implementation of by_input_sets!() changes over time, it has access >> to >> >>>> internal state that could impact the behaviour of decompose!(). >> >>>> >> >>>> >> >>> So, stubbing a current_user method on a rails controller would be >> >>> considered bad practice? >> >>> I suppose stubbing the find on User would be just as easy but I have >> >>> always just stubbed controller.current_user. >> >>> >> >> Rails is tricky. These rules are stem from situations in which you >> are >> >> in complete control of the design. Clearly, Rails makes it easy to >> >> work with if you follow its conventions, but the resulting design is >> >> far from Object Oriented. This is not an inherently bad thing - don''t >> >> get me wrong. I use Rails and it''s a delight in terms of development. >> >> But it''s a challenge in terms of this kind of testing. >> >> >> >> That said, the User class object is a different object than a user >> >> instance, so I have no issue w/ stubbing find on it. >> >> >> >> As for controller.current_user, a purist TDD view would have you move >> >> that behaviour elsewhere. I break the rule and just stub it directly. >> >> This general advice I learned from Uncle Bob Martin: sometimes you >> >> have to break the rules, but when you do you should do it consciously >> >> and feel dirty about it ;) >> >> >> > >> > On the current project we''ve quit moved all authentication into a >> > LoginManager. This has worked out so nicely as we have simple methods >> > for: login_from_cookie, login_from_session, >> > login_from_user_credentials, etc. >> > >> > This cleans up a lot of the hairy code sprinkled throughout >> > controllers and before filters which were trying to do some form of >> > authentication based on peeking at the sessions themselves or >> > validating users. >> > >> > >> Interesting, do you pass in the session in the constructor or how do you >> get access to the session data? >> >> -Ben >> > > > > -- > Zach Dennis > http://www.continuousthinking.com > _______________________________________________ > rspec-users mailing list > rspec-users at rubyforge.org > http://rubyforge.org/mailman/listinfo/rspec-users >With Regards, // Signed // Cody P. Skidmore
On Jan 11, 2008 12:56 PM, Cody P. Skidmore <cody at skidmore.us> wrote:> Thank you Zach. I was just about to ask about this. I''m just getting > started with restful_authentication and have missed the context of your > point. restful_authentication is such a huge improvement over what I''m > use to. > > Could you elaborate just a little on the use context in controllers? Is > this called from the "authenticate" method in the controller? >Here is a before filter in our ApplicationController for attempting to log the user in from a cookie: Here is what we have in our application controller as a before filter for logging in from a controller: def login_from_cookie session[:invitation_code] = params[:code] if params[:code] unless logged_in? login = @login_manager.login_from_cookie(cookies, session) if login.successful? flash[:notice] = login.message self.current_user = login.user unless login.return_to.blank? redirect_to login.return_to return false end end end end And here is our SessionsController#create method for how to log in a user from their credentials: def create login = @login_manager.login_with_credentials(params[:login], session, cookies) if login.successful? self.current_user = login.user flash[:notice] = login.message redirect_to(login.return_to || home_path) session[:return_to] = nil else flash[:error] = login.message redirect_to signup_path(:login => {:email => params[:login][:email]}) end end You''ll notice that we already have a @login_manager instance variable accessible in both cases. This is because of our use of the Injection plugin to automatically assign one for us from a global application context. You don''t have to do that (but we like it because it decouples our controllers from implementation). You could construct a LoginManager where you need it, ie: LoginManager.new.login_with_credentials(....) We didn''t set the methods up as class methods on LoginManager (ie: LoginManager.login_with_credentials) because we like to work with instances. We feel they are easier to test and refactor and it helps us avoid the temptation of adding class methods and instance methods which often times can lead to mixing multiple responsibilities onto an object. We''re big fan of single responsibility. HTH, -- Zach Dennis http://www.continuousthinking.com
On Jan 11, 2008, at 5:54 PM, Ben Mabey <ben at benmabey.com> wrote:> David Chelimsky wrote: >> >> >> In TDD there is a rule of thumb that says don''t stub a method in the >> same class as the method you''re testing. The risk is that as the real >> implementation of by_input_sets!() changes over time, it has access >> to >> internal state that could impact the behaviour of decompose!(). >> > > > So, stubbing a current_user method on a rails controller would be > considered bad practice? > I suppose stubbing the find on User would be just as easy but I have > always just stubbed controller.current_user. >In my login_as helper, I just call controller.send(:current_user=, user). No need to stub anything. //jarkko> -Ben > _______________________________________________ > rspec-users mailing list > rspec-users at rubyforge.org > http://rubyforge.org/mailman/listinfo/rspec-users