How deep should unit tests go?

[gargle]

Dear Monks,

How deep into the code should someone test?

I have a base class (of the closure kind, I like my classes like that ;)) with the following protected sub:

# protected add to set credit
sub setCreditAmount {
    my $closure = shift;
    my $amount  = shift;
    caller(0)->isa(__PACKAGE__) || confess "setCreditAmount is protect
+ed";
    &{$closure}( "CREDITAMOUNT", $amount );
}

Another class inherits and calls this protected sub setCreditAmount:

# public method to add to the account
sub addToAccount {
    my $closure   = shift;
    my $addAmount = shift;
    my $amount    = $closure->getCreditAmount() + $addAmount;
    $closure->setCreditAmount($amount);
}

addToAccount is public ofcourse and calls setCreditAmount. setCreditAmount can only be called from classes which inherit from the base class. setCreditAmount cannot be reached outside an inherited class.

So how does one unit test this, if at all?

Do you monks consider it overkill? addToAccount gets tested already with all different parameters. I consider addToAccount as a kind of black-box.

What do you Monks think?

Greetings

[adrianh]

What do you Monks think?

It depends :-)

It it were me I would have started off with a test for addToAccount() before I wrote any code - since that's the public API. Maybe something like (guessing at the rest of the API):

my $account = Account->new;
$account->addToAccount( 10 );
is( $account->getCreditAmount, 10, 'added ten' );

And gone ahead and written the code. I might then have spotted that the &{$closure}( "CREDITAMOUNT", $amount ) wasn't revealing my intention much, and told my subclasses about the implementation decision I made and then factored it out into the base class. If I was confident of the other tests of the methods like addToAccount that callled setCreditAmount then I probably wouldn't add any more.

If I decided to make it protected as you have done I would have written a test to make sure the exception would be thrown in the appropriate context before I added the caller test:

{ package NotASubclass;
  use Test::Exception;
  throws_ok { Account->new-> setCreditAmount( 42 ) } qr/is_protected/;
}

because that's new code and new functionality and I would want to make sure it works.

(although personally I wouldn't have bothered adding the test since I find that adding this sort of thing doesn't seem to solve any problems... I also would use methodNamingThatHurtsMyEyes :-)

[xdg]

Do you use "protected" methods to ease your own implementation, or do you document them for others to do their own subclassing? Generally, I consider a "protected" method to be part of the defined interface and therefore worth testing at the point at which it's documented and if you don't document, you should just make them private (whether with the leading "_" convention or actual checks -- that's up to you).

What I often do to test protected methods is to create a subclass that implementes a public wrapper around the protected method:

package Sub::Class;
use base 'Super::Class';

sub public_setCreditAmount { shift->setCreditAmount(@_) }

Then I test that calling the protected method fails on an instance of the superclass, but that calling the public wrapper succeeds on an instance of the subclass.

That's functionally the same -- and perhaps even a bit more work -- than just setting a package and @ISA for a limited scope in the test script itself:

# in the test script
{
  package Sub::Class;
  our @ISA = 'Super::Class';
  my $got;
  eval { $got = $obj->setCreditAmount( @argument ) };
  is( $@, q{}, "setCreditAmount called as subclass" );
  is( $got, $expected, "setCreditAmount return value correct" );
}

However, since I usually wind up testing other properties of a subclass, too, I tend to just write the subclass object so I can keep adding to it as I need it.

[gargle]

What I often do to test protected methods is to create a subclass that implementes a public wrapper around the protected method:

A great idea!

I'll just to that :)

[rinceWind]

The way of thinking about questions of this nature is in terms of the contract model. If you are delivering a CPAN module, you will be supplying accompanying documentation in the form of POD, documenting the interface. This determines the initial scope for unit tests. Unit tests can be added later for specific bugs that come to light later in the development cycle or after release.

You have chosen to designate setCreditAmount as "protected" - not a concept that Perl has or enforces, though there are probably ways of doing this. Why not make setCreditAmount fully public, documented, and with unit tests?

Alternatively, if you have methods which are "internal", conventionally these are prefixed with an underscore, and are usually not documented in the POD. You could still have unit test calling such methods - sometimes they need to, for simulating aspects of the outside environment.

My criteria for whether unit tests belong with a module distribution are whether they are actually testing something inside the module. The usefulness of unit tests diminishes if they become dependent on code outside the distribution - other modules, and platform or database specific behaviour for example. A better approach is to simulate such external behaviour, using Test::MockObject, DBD::Mock, etc. which will decouple the module from its environment. The DBD::SQLite database is another useful tool here, as you can build cross-platform throwaway databases for running tests against.

Hope this helps

[Anonymous Monk]

Let this module determine if your test goes too deep: Devel::Cover

[gargle]

UPDATE: I've added two more tests to .t/bo/Rekening.t to check the protected setCreditAmount and setDebetAmount subs (and of course changed the number of tests from 13 to 15

Thanks for the comments and answers :)

I think I'll go with the following (sorry for the dutch names, Passief eq Passive Account and Rekening eq Account (the base class):

I'll implement the wrapper class for testing protected subs later on :)

./t/bo/Passief.t

#!/usr/bin/perl

use strict;
use warnings;

use Math::BigInt;

use Test::More tests => 12;
use Test::Exception;

# We need the Rekening and Actief classes
use src::bo::Passief;

# we test the passief account
my $passief = Passief->new("kapitaal");
is( ref $passief, "Passief", "A passief object" );
is( $passief->getName(), "kapitaal", "It's kapitaal" );

lives_ok { $passief->addToAccount() } "addToAccount is overwritten";
$passief->addToAccount( Math::BigInt->new(5) );
is( $passief->getDebetAmount(), Math::BigInt->new(5), "kapitaal has 5 
+credit" );
is( ref $passief->getDebetAmount(),
    "Math::BigInt", "getCreditAmount() returns a BigInt" );

lives_ok { $passief->subtractFromAccount() }
  "subtractFromAccount is overwritten";
$passief->subtractFromAccount( Math::BigInt->new(4) );
is( $passief->getCreditAmount(), Math::BigInt->new(4), "kapitaal has 4
+ debet" );
is( ref $passief->getCreditAmount(),
    "Math::BigInt", "getDebetAmount() returns a BigInt" );

is( $passief->balance(), 1, "kapitaal has a 1 balance" );
is( ref $passief->balance(), "Math::BigInt", "balance() returns a BigI
+nt" );

# we test if we can call setCreditAmount directly
throws_ok { $passief->setCreditAmount(0) } qr/setCreditAmount is prote
+cted/,
  "setCreditAmount is protected";

# we test if we can call setDebetAmount directly
throws_ok { $passief->setDebetAmount(0) } qr/setDebetAmount is protect
+ed/,
  "setDebetAmount is protected";

./t/bo/Rekening.t

#!/usr/bin/perl

use strict;
use warnings;

use Math::BigInt;

use Test::More tests => 15;
use Test::Exception;

# We need the Rekening and Actief classes
use src::bo::Rekening;

# a quick test to see if rekening works
my $rekening = Rekening->new("rekeningnaam");
is( ref $rekening, "Rekening", "A rekening object" );

is( $rekening->getName(), "rekeningnaam", "It's rekeningnaam" );

my $zero = Math::BigInt->new(0);

is( $rekening->getCreditAmount(), $zero, "kas has 0 credit" );
is( ref $rekening->getCreditAmount(),
    "Math::BigInt", "getCreditAmount() returns a BigInt" );
is( $rekening->getDebetAmount(), $zero, "kas has 0 debet" );
is( ref $rekening->getDebetAmount(),
    "Math::BigInt", "getDebetAmount() returns a BigInt" );

# we test a rekening without a name
my $rekening2 = Rekening->new();
is( ref $rekening2, "Rekening", "A rekening object" );
is( $rekening2->getName(), undef, "It's undef" );

# we test if we can call addToAccount directly
throws_ok { $rekening->addToAccount() } qr/Rekening is a base class/,
  "addToAccount is protected";

# we test if we can call subtractFromAccount directly
throws_ok { $rekening->subtractFromAccount() } qr/Rekening is a base c
+lass/,
  "subtractFromAccount is protected";

# we test if we can call balance directly
throws_ok { $rekening->balance() } qr/Rekening is a base class/,
  "balance is protected";

# we test if we can call setCreditAmount directly
throws_ok { $rekening->setCreditAmount(0) } qr/setCreditAmount is prot
+ected/,
  "setCreditAmount is protected";

# we test if we can call setDebetAmount directly
throws_ok { $rekening->setDebetAmount(0) } qr/setDebetAmount is protec
+ted/,
  "setDebetAmount is protected";

# magic, we test the setCredit protected sub
my $setCredit;
my $setDebet;
{
    package Sub::Rekening;
    our @ISA = 'Rekening';
    eval { 
        $rekening->setCreditAmount($two); 
        $rekening->setDebetAmount($one);
        $setCredit = $rekening->getCreditAmount();
        $setDebet = $rekening->getDebetAmount();
    };
}
is( $setCredit, $two, "setCreditAmount returns 2" );
is( $setDebet, $one, "setDebetAmount returns 1" );