0

I am trying to test the output of the following method:

package ASC::Builder::Error;

sub new {

    my ($package, $first_param) = (shift, shift);


    if (ref $first_param eq 'HASH') {
        my %params = @_;
        return bless { message => $first_param->{message}, %params}, $package;
    }
    else {
        my %params = @_; 
        return bless {message => $first_param, %params}, $package;

}   
}

This method is supposed to accept either an error hash or error string. If it accepts a hash it should output the value of the message key from the error hash.

This is the error hash located in ErrorLibrary.pm:

 use constant {

        CABLING_ERROR => {
        code => 561,
        message => "cabling is not correct at T1",
        tt => { template => 'disabled'},
        fatal => 1,
        link =>'http://www.e-solution.com/CABLING_ERROR',
        },
    };

This is the message method along with the other keys of the hash located in Error.pm

package ASC::Builder::Error;

sub message {
        return $_[0]->{message}; 
}

sub tt {
    return {$_[0]->{tt} };
}

sub code {
    return {$_[0]->{code} };
}

This is my current unit test located in error.t

#input value will either be a String or and Error Message Hash


# error hash
my $error_hash = CABLING_ERROR;
# error string
my $error_string = "cabling is not correct at T1.";

# error hash is passed into new and an error object is outputted
my $error_in = ASC::Builder::Error->new($error_hash);

# checks to see if the output object from new is an Error object
isa_ok($error_in, 'ASC::Builder::Error');

# checking that object can call the message() method
can_ok( $error_in, 'message');


# checks to see if the output message matches the message contained in the error hash(correct)
is($error_in->message(),( $error_string || $error_hash->{message} ), 'Returns correct error message');

And finally the results of my test:

#   Failed test 'Returns correct error message'
#   at t/67_error_post.t line 104.
#          got: 'HASH(0x38b393d490)'
#     expected: 'cabling is not correct at T1.'
# 
# '
# Looks like you failed 1 test of 3.
t/67_error_post.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/3 subtests 
Paul Russell
  • 179
  • 10

1 Answers1

4

On my machine

First of, if I run your code I get an error about CABLING_CHECK_TOR_INCORRECT_CABLING_ERROR being not defined. If I replace that with CABLING_ERROR, the test fails with this.

#          got: 'cabling is not correct at T1'
#     expected: 'cabling is not correct at T1.'
# Looks like you failed 1 test of 3.

Two possible outputs at the same time

Now to what you say the output is.

For some reason, your $error_in->message returns a hashref, which gets stringified by is(), because is() doesn't do data structures. You can use Test::Deep to do this.

use Test::Deep;

cmp_deeply(
    $error_in->message,
    any(
        $error_string,
        $error_hash->{message},
    ),
    'Returns correct error message',
);

Here I assumed that your $error_string || $error_hash->{message} is intended to make it check for either one or the other.

But || will just check if $error_string has a true value and return it, or take the value of $error_hash->{message}. It compares the result of that operation to $error_in->message.

Testing clearly

However, this will likely not solve your real problem. Instead of having one test case that checks two possible things, make a dedicated test case for each possible input. That's what unit-testing is all about.

my $error_direct = ASC::Builder::Error->new('foo');
is $error_direct->message, 'foo', 'direct error message gets read correctly';

my $error_indirect = ASC::Builder::Error->new( { message => 'bar' } );
is $error_indirect->message, 'bar', 'indirect error message gets read correctly';

The above code will give you two test cases. One for a direct error string, and another one for an indirect hash.

ok 1 - direct error message gets read correctly
ok 2 - indirect error message gets read correctly
1..2

Don't waste time

At the same time, this also addresses another issue with your approach. In unit tests, you want to test the smallest possible unit. Don't tie them to your other business logic or your business production data.

Your ASC::Builder::Error class doesn't care about the type of error, so don't over-complicate by loading something additonal to give you the exact same error messages you have in real life. Just use simple things that are enough to prove stuff works.

The simpler your unit tests are, the easier it is to maintain them, and the easier it is to add more once you have more cases.

simbabque
  • 53,749
  • 8
  • 73
  • 136
  • Sorry about the first syntax mistake, I copied the code from the wrong version.. I am going to go through your answer and advice now. Thanks. – Paul Russell Apr 04 '16 at 15:27
  • The Perl Test::Deep library is not available and I don't have permission add it. I assume there is a very awkward way of comparing deeply . All that is available is Test::More. Thanks for the advice above I have split them into two separate test cases, it makes more sense they are unit tests after all . haha – Paul Russell Apr 04 '16 at 15:50
  • @Paul splitting it is the best option in my opinion. – simbabque Apr 04 '16 at 15:54
  • so two separate tests, one for `$error_string` and one for `$error_hash` ? – Paul Russell Apr 04 '16 at 16:00
  • @Paul Yes, just as I showed. Of course you can add more test cases. But I would really stick with simple cases that cover everything. Don't make it complex. What I showed is enough to make sure the _message_ gets trough. If you your code doesn't care about the names of the other keys in your hash, don't make tests for each key, just add a `foo` key and check if it's there. If one works, all will work as long as the code doesn't look for specific ones. – simbabque Apr 04 '16 at 16:05
  • Yeah I have done that. But I need to test each value that is returned by each key. I think that will mean I'll have to change the new() method. At the moment I am just receiving hash refs when I try to test the return values. – Paul Russell Apr 04 '16 at 16:21
  • @PaulRussell That's because `return {$_[0]->{code} };` and `return {$_[0]->{tt} };` return hashrefs; drop the extra braces, so `return $_[0]->{code};` and `return $_[0]->{tt};`. – ThisSuitIsBlackNot Apr 04 '16 at 16:25
  • Yeah I removed them and realized that. But I get `undef` now. I don't understand why, I have a feeling it's because of the new() method. Do I have to add more params to new like `tt=> $second_param` . I have attempted to do that. But it doesn't work. – Paul Russell Apr 04 '16 at 16:37
  • @PaulRussell Your `new` method only creates a single attribute, which is `message`. So you need to add more keys to the hash that you `bless`. Currently you have `bless { message => $first_param->{message}, %params}`; to add a `tt` attribute, you would do `bless { message => $first_param->{message}, tt => $first_param->{tt}, %params}` – ThisSuitIsBlackNot Apr 04 '16 at 17:00
  • 1
    @paul you could post this, with a bit more information of what it does, to the codereview stack exchange and tag it with Perl. Then we can give you on-topic feedback about your code. There are a few more issues I'd like to address, but this is not really the place. – simbabque Apr 04 '16 at 17:31