How to tell Perl's Devel::Cover that a Readonly value can never be false?-CodePudding

I have this module which has a Readonly constant for a default value that is used if the caller does not specify a value.

package Mcve;

use strict;
use warnings;
use Readonly;

Readonly our $CONST => 123;

sub new {
    my ($class, %args) = @_;
    my $self = bless{}, $class;

    # uncoverable condition right
    # uncoverable condition false
    # uncoverable branch false
    $self->{'CONST'} = $args{'CONST'} || $CONST;
    return $self;
}

And a test case:

use strict;
use warnings;
use base 'Test::Class';
use Test::More;
use Mcve;

__PACKAGE__->runtests() unless caller;

sub uses_default_arg: Test {
    my $mcve = Mcve->new();
    is($mcve->{'CONST'}, 123);
}

sub overrides_default_arg: Test {
    my $mcve = Mcve->new('CONST' => 456);
    is($mcve->{'CONST'}, 456);
}

When I collect test coverage with Devel::Cover, it does not see that $COVER is always defined, so the condition "both values are false" can never be covered. I tried to add the # uncoverable ... comments as per the

How do I tell Devel::Cover that this condition is uncoverable?

(This is with Perl 5.34 and Devel::Cover version 1.36)

CodePudding user response：

If you use a value that can never be false as a conditional, do you need the conditional? Typically, when I find a hard-to-cover situation like this, I realize that it's my code that is the problem, and not the tool.

Your code says to the programmer that you want a default, because we are used to this (although I'd ask about this is a code review because I'd want to know why 0 can't be a valid value). However, the code actually says to logically combine two, two-valued (true/false) values to come up with a value:

$self->{'CONST'} = $args{'CONST'} || $CONST;

But, what you really want it a set of defaults. Instead of fixing up the object (especially by direct access to its data structure), perhaps you fix up the incoming arguments so it has the default values before you create the object. I find this a bit cleaner because the object doesn't think about two paths:

sub new {
    my ($class, %args) = @_;
    $args{'CONST'} = $CONST unless $args{'CONST'};
    my $self = bless { }, $class;
    ...
    return $self;
}

That may also show up as something like this, where you get rid of the conditional by letting later key-value pairs overwrite the ones from $defaults:

sub new {
    state $defaults = { CONST => $CONST };
    my ($class, %args) = @_;
    %args = ( %$defaults, %args );
    my $self = bless { }, $class;
    ...
    return $self;
}

After this, you might want a step to validate the arguments, where you can catch someone passing in a bad value. If you truly don't want someone to use 0 as a CONST value, I'd really like a warning (or exception, whatever) that the explicit value I passed in was not valid. Your original code silently replaces it, so I don't find out that the value I gave you was ignored. In moving around the code to stop using a logical operator, you're led to a better situation again.

This has often turned out much better for my code because I now have an easy way to specify and assign defaults. I merely add new defaults in one place that is clearly marked as default values. If I have another default value, the structure of my code does not change.

I much prefer for the structure to be stable (there's this thing called the 0,1,Infinity rule) than to make weird adjustments to external tools, which you then must pass on as lore to future people who have to deal with the code.