4
\$\begingroup\$

I guess for some motivation: I ripped this out of a larger library I'm half-working on to parse S-expressions and cleaned it up a bit.

I'm trying to write a simple function that tests whether a string is a valid quoted string literal. Using Test::LectroTest to compare a fast version of a function using a regex against a slow version that uses a Perl for loop.

I have a couple complaints / style questions right off the bat:

  • Is the lib/QuotedString.pm the right place for the accept_string_slow function to live? I don't intend for people to actually use this function, it's just there to support the tests. Is there a way to hide it better?
  • I can't actually figure out how to influence how LectroTest generates strings. Since it picks characters uniformly at random, I decided the clearest thing to do would be to accept the string it produced but add quotations marks to exercise different edge cases.
  • The general style is probably bad. I don't write Perl professionally.

Directory layout:

.
|-- lib
|   `-- QuotedString.pm
`-- t
    `-- test_accept_string_generative.t

Here is lib/QuotedString.pm.

package QuotedString;

BEGIN {
    require Exporter;
    @ISA = qw[Exporter];
    @EXPORT_OK = qw[
        accept_string_regex
        accept_string_slow
    ];
};

use strict;
use warnings;

use Readonly;


Readonly my $string_pattern => qr/["](?:[^\\"]|[\\].|[\\]\n)*["]/;


sub accept_string_regex {
    my $string = shift;
    ($string =~ m/\A$string_pattern\z/) ? 1 : 0;
}


sub accept_string_slow {
    my $string = shift;
    return 0 if length($string) < 2;
    return 0 unless $string =~ /\A"/;
    return 0 unless $string =~ /"\z/;

    my $index = 1;
    my $final_checked_char = length($string) - 2;
    while ($index <= $final_checked_char) {
        my $char = substr($string, $index, 1);
        # if we encounter an unescaped ", we fail
        return 0 if $char eq '"';
        # \ consumes the next character fail if there is no next character before final "
        return 0 if $char eq '\\' and $index == $final_checked_char; 

        # advance the index two positions if the current char is \
        # because the next one is escaped regardless of what it is
        # if the current character is not \, then advance one position
        if ($char eq '\\') {
            $index += 2;
        } else {
            $index += 1;
        }
    }
    return 1;
}

and the test t/test_accept_string_generative.t

#!/usr/bin/env perl
use strict;
use warnings;
use Test::LectroTest (
    trials => 2000
);

use QuotedString qw[accept_string_regex accept_string_slow];

Property {
    ##[ x <- String ]##
    accept_string_slow($x) eq accept_string_regex($x);
};

Property {
    ##[ x <- String ]##
    accept_string_slow('"' . $x) eq accept_string_regex('"' . $x);
};

Property {
    ##[ x <- String ]##
    accept_string_slow('"' . $x . '"') eq accept_string_regex('"' . $x . '"');
};
\$\endgroup\$
  • \$\begingroup\$ You can write '"' . $x . '"' as "\"$x\"" in your tests. \$\endgroup\$ – chicks Dec 23 '16 at 14:31
  • 2
    \$\begingroup\$ @chicks that just makes it even more unreadable. Tests are code as well and should have the same quality. \$\endgroup\$ – simbabque Dec 28 '16 at 12:12
  • \$\begingroup\$ So accept_string_regex is the production function that's been in QuotedString.pm already? \$\endgroup\$ – simbabque Dec 28 '16 at 12:15
  • \$\begingroup\$ You can write '"' . $x . '"' as qq/"$x"/ in your tests \$\endgroup\$ – Borodin Jan 6 '17 at 21:22
3
\$\begingroup\$

Location of your helper function

If you place the accept_string_slow function inside of your lib/QuotedString.pm, it's visible to everyone. That means that someone will use it.

There is no concept of private in Perl. We have a convention to prefix things with an underscore _ so people know it's something they should not be messing with. But that does not stop them.

If this function is really only for testing, then you have a few options. It looks like it's your expected output, which because the input is random, you built into a function. That's a good approach.

The easiest and in my opinion most appropriate way is to put it right into your test .t file. It's part of that very specific test and is not used anywhere else. Just place it under the use QuotedString and remove the name from the use line.

use QuotedString qw[accept_string_regex];

sub accept_string_slow {
    my $string = shift;
    return 0 if length($string) < 2;
    return 0 unless $string =~ /\A"/;
    return 0 unless $string =~ /"\z/;

    # ...
}

Property {
    ##[ x <- String ]##
    accept_string_slow($x) eq accept_string_regex($x);
};

And that's it. Your calls to the two functions inside of the Property now still work, because all that the use does is import the function from the QuotedString package into the local package (which is main). You don't need to worry about how that works.

I would also recommend renaming the function to something that tells the maintainer (which might well be future-you) what the heck is going on here. Maybe go with sub expected_output or similar. Since it's now contained in this test you don't need to adhere to any naming conventions that the project might have. It's your code, make it as readable as possible.

Alternative ways would be to either monkeypatch the function into the QuotedString package, which makes sense if you want to overwrite a certain behavior, or to use a separate library and put it e.g. in t/lib/Helper/QuotedString.pm, which makes sense for larger changes or if you need an entire class to help you out during your tests.

Influencing LectroTest

This is a bit more suited for Stack Overflow, but here's a run-down anyway.

You should first read the docs of Test::LectroTest::Generator. That module gives you everything you need. You can use the Paste combinator with the Unit generator and the String generator. Here's an example that just outputs 20 combinations.

use strict;
use warnings;
use feature 'say';
use Test::LectroTest trials => 20;
use Test::LectroTest::Generator ':all';

Property {
    ##[ x <- Paste( Unit('"'), String( charset=>"A-Z", length=>[1,10] ) ) ]##
    say "x = $x";
};

Unit gives you a fixed value that never changes. Kind of like a constant. String you already know, but it seems we need to be more specific if it's used with Paste. I have not figured out why yet. The arguments charset and length should be self-explanatory. You can use single characters, like abc to allow a, b, and c, character ranges like A-F to create ABCDEF automatically. Those can also be combined as in AB-EFg. The length here means between one and ten.

Paste concatenates them. Actually it's more like a join, but if you omit the last argument glue, it uses an empty string and thus it's really a concat. (Beware, there is a Concat that does something else!)

1..1
x = "YSJAQXSG
x = "WWU
x = "IVKJGLWVV
x = "TWQHU
x = "DVXJDTRTX
x = "CC
x = "SULEP
x = "NFBTM
x = "UR
x = "TKS
x = "IUXIMDLUWX
x = "WHYLRV
x = "CWIQLLGO
x = "N
x = "J
x = "FZOLCBCUZX
x = "FN
x = "ZVAQZJSN
x = "TIYUXJV
x = "SVE
ok 1 - 'Unnamed Test::LectroTest::Property' (20 attempts)

This seems to work. You can also add more than one row of input definitions. That way, you can combine several tests into one Property. This is explained in Test::LectroTest::Property.

Property {
    ##[ x <- Paste( Unit('"'), String( charset=>"A-Z", length=>[1,10] ) ) ],
    [ x <- Paste( String( charset=>"A-Z", length=>[1,10] ), Unit('"') ) ]##
     say "x = $x";
};

I've reduced the trials to 5 to make it easier to read. It will run each line of definitions 5 times.

1..1
x = "CIWNIGSNL
x = "YI
x = "GPCAEZYO
x = "EPLKF
x = "ABUD
x = HS"
x = JGW"
x = QLZJUAZ"
x = NUQEKGL"
x = RSGSVOAXAQ"
ok 1 - 'Unnamed Test::LectroTest::Property' (10 attempts)

This could now also be used to do the "foo" check where the enclosing quotes are intact.

If you want to instead of spelling all of this out, pre-build a generator and use the variable in your definitions, you can do the following. The Test::LectroTest::Generator documentation does this, but fails to explain how to use it with the comment-style syntax.

my $gen = Paste( Unit('"'), String( charset => "A-Z", length => [ 1, 10 ] ) );

Property {
    ##[ x <- $gen ]##
    say $x;
};

To go a bit further, it would also be possible to build a definition that sometimes injects " or \\ or \\" with reduced likelihood.

my $overkill = Paste(
    (
        Frequency(
            [ 95, Char( charset => "a-zA-Z0-9'!?., -" ) ],
            [ 4,  Unit('"') ],
            [ 4,  Unit('\\') ],
            [ 2,  Unit('\\"') ],
        )
    ) x 10, # the x operator needs to be used in list context here
    glue => q{},
);

Property {
    ##[ x <- $overkill ]##
    say $x;
};

__END__
1..1
"Aqgz0,n8x
uBoLq"\",hq
"zx!bfJe0l
YW1u'k5"!b
Q,?9UK\\Jw
ok 1 - 'Unnamed Test::LectroTest::Property' (5 attempts)

Now you could combine all of this together to build intricate tests. If you combine those with the advice I give below at the very end of this post, you should be more than covered.

Looking into the regression testing should also prove useful. Unfortunately the documentation is very lacking as to how this works ifyou want to include manual cases. It does talk about adding failures manually, but never says how to do that. But if you turn it on, it should record it, and from that you should be able to figure out the syntax to add cases yourself.

Also take a look at this document that I found while digging through the distribution. I couldn't find the corresponding PerlMonks post, but there is some stuff about LectroTest on PerlMonks that might help, though it's old.

Although the author's website which is supposed to have more information about the whole thing is down, I did find his blog that might also have some more information. It does seem like the author disappeared from the Perl world in 2013 though.

General style

Test conditions

When I wrote this answer I was looking at the code an saw

accept_string_slow($x) eq accept_string_regex($x);

Because I seem to have a low attention span, I then thought that the function we are testing is accept_string_slow. I copied over the _regex one into the first part of the answer and started writing a whole paragraph about getting rid of Readonly because it's then not needed any more. At some point I realized that earlier you said that you wrote the slow function for control, so I facepalmed and deleted all of that.

What I want to show with that little story is that there is a convention to put the thing that is tested first in conditions.

if ($foo eq 42) { ... }

The same goes for tests written with Test::More.

is $foo, 42, 'Foo is the answer';

Doing it the other way around is called a Yoda condition, and with two function calls it's really not obvious what's going on. While that does not affect anything in the code, your maintenance person might see it the same way I did and then stuff might break later. You should turn them around.

Property {
    ##[ x <- String ]##
    accept_string_regex($x) eq accept_string_slow($x);
};

Putting the quotes into the string

While your '"' . $x . '"' approach is valid and works and is okishly readable, it might be better to use the q{} operator, which constructs strings. q is like single quotes '' without interpolation, and qq is like double quotes with interpolation. You can choose the delimiter yourself. I prefer curly braces {}. I use those for empty strings and strings that contain non-letters or digits, especially with lengthy one. You can read more about them in perlop.

Property {
    ##[ x <- String ]##
    accept_string_slow(q{"} . $x . q{"}) eq accept_string_regex(q{"} . $x . q{"});
};

Of course you can also interpolate your $x variable if you feel that's more readable. Then you need to use qq{}.

Property {
    ##[ x <- String ]##
    accept_string_regex(qq{"$x"}) eq accept_string_slow(qq{"$x"});
};

Testing functions from a library

As I said above, the use Foo qw(bar) syntax imports the bar function from the Foo namespace into the local namespace. Importing means that a new reference to the function is created in the current namespace, so that main::bar() is the same Foo::bar() (and so is bar()).

That's useful, but in your test it might not be what you want to do. While that is a lot more typing in the tests, it might be more expressive to actually not import the function, but instead use the fully qualified name. That way, you know exactly what's going on when you look at the code (did I mention tests are code too?).

use strict;
use warnings;
use Test::LectroTest (
    trials => 2000
);

use QuotedString ""; # this now has an empty string, not an empty list

Property {
    ##[ x <- String ]##
    QuotedString::accept_string_regex($x) eq accept_string_slow($x);
};

This is also what the SYNOPSIS in the Test::LectroTest documentation does.

Name your tests

You should always give meaningful names to your test cases. If I run your tests the way they are, this is the output.

1..3
ok 1 - 'Unnamed Test::LectroTest::Property' (2000 attempts)
ok 2 - 'Unnamed Test::LectroTest::Property' (2000 attempts)
ok 3 - 'Unnamed Test::LectroTest::Property' (2000 attempts)

If one of them breaks, how should we know which one is actually broken? Especially if there are a lot of tests, it gets very confusing quickly. You can name Test::LectroTest Property tests by adding a name parameter like this.

Property {
    ##[ x <- String ]##
    accept_string_slow($x) eq accept_string_regex($x);
}, name => 'String without explicit enclosing quotes';

Try to pick names that describe your test case by explaining the business logic you are testing. A test named try with value foo is a bad name, while passing in foo does not break stuff is a lot better.

Style in your accept_string_slow

This is very good. Your code is clear, readable and well-commented. Your comments make sense, your variable names are speaking and the function fits on one screen. Well done.

The only thing I would say here is actually a moot point because the interface you are mimicking is defined like that. Usually you wouldn't do return 0 (as in return 0 if $foo), but instead just say return if $foo. Without a value, return returns undef. Both 0 and undef are false in Perl, so that's usually fine. But since the interface here explicitly uses 1 and 0, it's perfectly ok to do the same.

Have you verified your code?

Your function has 19 lines of code. That's nicely small, but it's also a lot of space to include bugs. Always remember that tests are also just code. I would include some safety checks that make sure that your function does what it is supposed to do.

The Test::LectroTest approach is a very inclusive, almost all-pairs kind of test. But simply having some predefined test cases should do the trick. At least for your helper.

use Test::More;

is accept_string_slow(q{no quotes here}),          0, 'string without quotes fails';
is accept_string_slow(q{"no quotes at the end}),   0, 'string with no final quote fails';
is accept_string_slow(q{no quotes at the start"}), 0, 'string with no initial quote fails';
is accept_string_slow(q{"too many "quotes"}),      0, 'string with unescaped quote inside fails';
is accept_string_slow(q{"foo \\ bar"}),            0, 'string with stray backslash fails';
is accept_string_slow(q{"foo \\"}),                0, 'string with escaped final quote fails';
is accept_string_slow(q{"happy case"}),            1, 'happy case';

But it seems mixing the Test::Builder (which Test::More is based on) with Test::LectroTest is a bit complicated. It's explained in Test::LectroTest::Compat, but if your whole test suite is based on Test::LectroTest and the output it produces, it's probably not worth using this just for a single function.

So it would make more sense to build your test directly with Test::LectroTest. As far as I understand that's done with the the regression tests file. That seems to be a whole different story though. I suggest you read that doc. It seems to also be the way to go to include your edge-cases.

\$\endgroup\$
  • \$\begingroup\$ It turns out I actually have Test::LectroTest installed, but I don't have any of its reverse dependencies installed directly or ever used. I wonder where I got it from. \$\endgroup\$ – simbabque Dec 28 '16 at 14:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.