use Test::More qw(no_plan);

my @items = map {pin()} 1..1000;
my @bad   = grep { ! /\A\d{4}\z/ } @items;
is_deeply(\@bad,[],"All pins are 4 digits");
[download]

Oops. You're right. Fixed now.

Note that your test doesn't catch the main bug I'm dealing with, namely, that all four digits are used.

Cheers,
Ovid

New address of my CGI Course.

Aye, I was running late for work and indulging myself with a little time on perlmonks.

Your approach to testing for digit usage seems ok to me, as the more condensed ways of doing it might be harder for a maintainer to follow. I would probably do this:

my %unused = map {$_=>1} (0..9);
for my $item (@items) {
    last unless %unused;
    delete $unused{$_} for split '', $item;
}

if (%unused) {
    fail(sprintf "Unused digits: %s", join(',',keys %unused));
}
else {
    ok(1,"All digits used");
}
[download]

My only issue with the original test was that with more tests being added you need to accumulate more stats as you go, which adds complexity to the test script. I prefer to keep tests in neat little compartments when possible.

I think you're approaching it the wrong way. You need to treat it as a black box and control just the inputs -- in this case, rand.

I wrote Test::MockRandom for exactly this kind of testing. Use it to test your boundary conditions on rand, as that's the only thing that will matter to whether you get all 10 digits. It's much better than playing with srand.

PIN length is almost an invariant because it almost doesn't depend on any input data. You're only at risk of a 5 digit pin (or greater) if the maximum rand value happened to give you "10" -- so I would check length at the same time that I check what happens when the random value (before multiplication) is nearly one.

Update: Here's what it would look like to check the boundary conditions:

use Test::MockRandom 'Some::Other::Package';
use Some::Other::Package 'pin'; # exports pin()

# list of values for rand to use
srand( ( (0) x 4, ( oneish() ) x 4 ); # 0,0,0,0,1,1,1,1

is( pin(), '0000', "rand always zero" );
is( pin(), '9999', "rand always almost one");
[download]

You need to treat it as a black box ...

Agreed. But ...

PIN length is almost an invariant because it almost doesn't depend on any input data

I couldn't disagree more.

As pin() has no inputs, your assertion is based upon knowledge of what you know is inside the box, which is wrong. For a black box that takes no inputs and produces variable outputs, then only way to test is compare a representative sample of outputs against the specifications for those outputs.

Assuming pin() is defined to produce a random 4-digit integer; or better pin() is defined to produce a random value of type PIN, which is (currently) defined as a 4-digit integer; then the tests need to be:

• Does it always produce a 4-digit integer.
• Can the full range of integers expected be produced.

  For large ranges, you would rely upon statistics to sample the distribution, but since this is such a small range, and the generation takes so little time, you might as well saturation test it.

If you rely upon knowledge of the implementation to not bother checking for length 4, then a modification by someone not understanding the significance of "%04d" that omitted the zero would not be detected.

---

Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.

Lingua non convalesco, consenesco et abolesco. -- Rule 1 has a caveat! -- Who broke the cabal?

"Science is about questioning the status quo. Questioning authority".

In the absence of evidence, opinion is indistinguishable from prejudice.

As pin() has no inputs

I'll admit to using "black box" in a poorly defined way -- what I meant was that the function in question isn't really random -- it transforms well defined inputs into outputs. In this sense, pin() takes no arguments, but it does have inputs -- four calls to rand. In my view, the right approach is to vary the inputs and examine the outputs -- explicitly test boundary conditions of known inputs rather than trying to evaluate it probabalistically.

If you rely upon knowledge of the implementation to not bother checking for length 4, then a modification by someone not understanding the significance of "%04d" that omitted the zero would not be detected.

I was referring to the original loop version, actually. It will always concatenate 4 integers. That doesn't vary based on the input. So while it needs to be checked, it doesn't need to be checked everywhere, we only need to check a situation where an input might produce an integer of more than one digit.

-xdg

Code written by xdg and posted on PerlMonks is public domain. It is provided as is with no warranties, express or implied, of any kind. Posted code may not have been tested. Use of posted code is at your own risk.

[0] Perl> sub pin{ sprintf '%04d', rand 10000 };;
[0] Perl> $h{ pin() }++ for 1 .. 1e6;;
[0] Perl> print +( 10000 == grep{ length() == 4 } keys %h ) ? 'OK' : '
+not OK';;
OK
[download]

It may be advantageous to modify pin() to take the length as a parameter (you could build in a default length of 4). This would allow you to write tests for the sub that require no knowledge of the implementation, and (to quote BrowserUK) "a modification by someone not understanding the significance of "%04d"" would not be (as much of) a risk because the length would not be hard-coded.

Taking this one step further, pin() could also take the character set to be used for PIN generation (0..9 could be used as a default).

While these ideas may be overkill for your current application, a generalized solution might be a nice snippet that could be reused elsewhere. It would even come with a test suite. :-)

Cheers,

JohnGG

my $pin;
1 while ( $pin = pin() ) =~ /\A\d{4}\z/;

my $digits = "\0" x 10;
do {
    substr $digits, $_, 1, $_ for pin() =~ /\d/g;
    } until $digits =~ /\A\d{10}\z/;
[download]

Cheers,

JohnGG

Sorry about that. Not sure why the link didn't work, but I've fixed it now.

Cheers,
Ovid

New address of my CGI Course.