2

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 documentation (see above), but still the coverage report shows only 66% conditional coverage:

~$ HARNESS_PERL_SWITCHES=-MDevel::Cover prove -Ilib t/ && cover

...

----------- ------ ------ ------ ------ ------ ------ ------
File          stmt   bran   cond    sub    pod   time  total
----------- ------ ------ ------ ------ ------ ------ ------
lib/Mcve.pm  100.0    n/a   66.6  100.0    0.0    0.5   90.4
t/mcve.t     100.0    n/a    n/a  100.0    n/a   99.4  100.0
Total        100.0    n/a   66.6  100.0    0.0  100.0   96.3
----------- ------ ------ ------ ------ ------ ------ ------

The html report shows that Devel::Cover thinks that both values being false is not covered:

HTML coverage report showing missing condition coverage

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

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

Robert
  • 7,394
  • 40
  • 45
  • 64
  • 1
    I don't know whether Devel::Cover handles it specially, but you could try using a constant rather than an readonly-valued variable: `use constant CONST => 123; ...; $args{'CONST'} || CONST` – Dave Mitchell Jun 23 '22 at 07:04

1 Answers1

2

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.

brian d foy
  • 129,424
  • 31
  • 207
  • 592
  • 1
    Re "*If you use a value that can never be false as a conditional, do you need the conditional?*", Look again. It's the RHS of the `||` that can't be false, and it's not use as a conditional. You made the same mistake as D::C, expecting the result of `||` to only be used as a condition. /// Also, I find your alternatives a lot hard to read. You literally replaced `$args{CONST} || $CONST` with multiples lines of code. And there's the fact that the second adds the copying of two hashes. Finally, the second alternative isn't even equivalent, but that might actually be good. – ikegami Jun 23 '22 at 15:15
  • Replacing one line in a toy example with a couple more lines that work better in larger, real-world examples is a good thing. Your "multiple" is that I have a single extra line in an expanded example of more robust code. Neither of my examples are equivalent, which is why I have the extra paragraph about validation and notice. – brian d foy Jun 23 '22 at 16:40
  • Re "*that work better in larger, real-world examples*", Except it's doesn't. The first alternative doesn't simply anything, in larger example or not. And the second alternative doesn't work in practice since things are never that uniform. There's always exceptions (differences in the handling of different parameters). You assume an existence check is always what's needed, but I can't even remember the last time I used an existence check. – ikegami Jun 23 '22 at 17:10
  • 1
    It's working fine for me. You're free to supply your own answer. But, This is why I don't provide full examples; it's an idea that anyone can use if it works for them. If it doesn't fit their situation, they can ignore it. It often fits my situation. – brian d foy Jun 24 '22 at 15:06
  • You still didn't address the actual issue I raised, though. You're still claiming the OP is using the constant as a conditional. /// Re "*It's working fine for me.*", Never said it didn't. Rewriting the code does circumvent D::C's bug/incompleteness. /// Re "*This is why I don't provide full examples*", Am I reading this right? you didn't provide the full example, because it "doesn't work in practice since things are never that uniform"? – ikegami Jun 24 '22 at 17:55
  • No, you are misreading that. I don't provide a full example for the reason I just stated. I use this technique quite a bit. I think you need to simply move on. You disagree, and you've made that clear. – brian d foy Jun 24 '22 at 22:08
  • Re "*for the reason I just stated*", No reason was given. You simple said "This is why I don't provide full examples". But you didn't say what "this" is. – ikegami Jun 24 '22 at 22:13
  • Anyway, not sure why you're ignoring the relevant part of my comment still. You still have this incorrect statement in your answer: "If you use a value that can never be false as a conditional, do you need the conditional?" in your answer. The OP clearly does use a false value in the conditional. – ikegami Jun 24 '22 at 22:16