Specs on private methods

On Jan 8, 2008 7:14 PM, Daniel T. [email protected] 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 C. [email protected] wrote:

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

This is also worth checking out:

http://xunitpatterns.com/

On Jan 8, 2008 3:56 PM, Francois W. [email protected]
wrote:

Chris O. 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 9, 2008 5:26 AM, Richard C. [email protected] wrote:

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

On Jan 8, 2008 11:25 AM, Matt P. [email protected]
wrote:

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

Daniel T.:

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

On Jan 11, 2008 3:49 AM, Shot (Piotr S.) [email protected] wrote:

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

On Jan 11, 2008 8:04 AM, Shot (Piotr S.) [email protected] wrote:

same class as the method you’re testing. The risk is that as the real

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

David C.:

On Jan 11, 2008 3:49 AM, Shot (Piotr S.) [email protected] 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. :slight_smile:

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

David C. 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 5:23 AM, David C. [email protected] wrote:

There’s a Decomposition class that decomposes an FSM to a given
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 S.) [email protected] 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?

On Jan 11, 2008 9:54 AM, Ben M. [email protected] wrote:

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 :wink:

On Jan 11, 2008 11:40 AM, Zach D. [email protected] wrote:

get me wrong. I use Rails and it’s a delight in terms of development.

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 :slight_smile:

Zach D. wrote:

get me wrong. I use Rails and it’s a delight in terms of development.

Interesting, do you pass in the session in the constructor or how do you
get access to the session data?

-Ben

On Jan 11, 2008 11:56 AM, David C. [email protected] wrote:

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 :wink:

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.

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

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

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 D. wrote:

  :cookies => cookies,

end
def login_from_password_reset(user, session)
:invitation_manager => @invitation_manager
simple testable objects for each type of login we do.

On Jan 11, 2008 11:56 AM, David C. [email protected]

implementation of by_input_sets!() changes over time, it has access
are

that behaviour elsewhere. I break the rule and just stub it directly.
This cleans up a lot of the hairy code sprinkled throughout


Zach D.
http://www.continuousthinking.com


rspec-users mailing list
[email protected]
http://rubyforge.org/mailman/listinfo/rspec-users

With Regards,

// Signed //

Cody P. Skidmore

On Jan 11, 2008 12:56 PM, Cody P. Skidmore [email protected] 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,

This forum is not affiliated to the Ruby language, Ruby on Rails framework, nor any Ruby applications discussed here.

| Privacy Policy | Terms of Service | Remote Ruby Jobs