develooper Front page | | Postings from September 2016

Re: CPANifying our test framework - or parts of it

Thread Previous
Sam Kington
September 10, 2016 15:39
Re: CPANifying our test framework - or parts of it
Message ID:
Thanks for the detailed response!

On 10 Sep 2016, at 15:42, James E Keenan <> wrote:
> Let me state my overall impression at the output.  It's apparent that many (hundreds?) of hours of work have been put into the development of this testing apparatus, but if it were put on CPAN I doubt I would use it.  It appears to be too heavily designed toward your specific use cases and, in certain respects, is designed in a way that does not sit well with me.

Yes, there will inevitably have to be a fair bit of work done to make it more general-purpose. (From our purposes, a general-purpose “test” method was fine because we have no other code that has a conflicting method, for instance, but obviously this would need revamping for the CPAN.) If nothing else regarding documentation, as I don’t think I managed to explain very well what exactly this test framework does.

> On 09/09/2016 09:34 PM, Sam Kington wrote:
>> At its most elaborate, our current test code lets you say e.g.
>> # Assume $self->state('location_id') has been set previously
>> $self->test(
> Of what class is $self an instance?  The class of the object being tested (as inferred from the existence of a 'state' method)?  Or a specialized testing class (as inferred from the existence of a 'test' method)?
> It seems that you are mixing both real object data with testing data in the same object.  Speaking for myself, I don't think that's a clean design.

It’s a test class. The state method is to remember data returned by previous test results, so e.g. you can inline them in URLs and/or include them in subsequent tests.

>>    expect => {
>>        http_code => HTTP_CREATED,
>>        sql_count => 12,
>>        data      => {
>>            location_id => $self->state('location_id'),
>>            build_id    => qr{^ (?<build_id> BUILD \d+ ) $}x,
>>            built       => $self->true,
>>            author     => $self->meh,    # It's OK if they call him Idiot.
>>            structures => [
>>                {
>>                    _hashref_contains => {
>>                        structure_id => qr{^ (?<structure_id> STRUCT \d+ ) $}x,
>>                        type         => 'dolmen',
>>                        material     => 'concrete',
>>                        # There's probably stuff about where the dolmen
>>                        # was erected but we ignore that for the purpose
>>                        # of this test.
>>                    }
>>                }
>>            ]
>>        }
>>    }
>> );
> More important is the fact that you must have a lot of code under the hood to generate the hash reference which is the value of the 'expect' key. That code is inquiring about many different attributes of the object and it has to get *every one* of those attributes right for the unit test to pass.  You're making *one, big assertion* about the state of the object -- and you're probably making a strong assumption about the internal structure of the object.

There’s no objects involved here. The test says “if I call this particular URL, I expect to get back a hashref of data which matches this description”. The description can be as complicated or as simple as the test requires.

I should add that when the code runs, it generates (in a subtest) individual tests for each component of the data structure, so if one of the keys didn’t match, but the rest of the data structure was fine, you’d get plenty of passing tests and then one failure, e.g. “not ok 123 data{built} is true”, and a Test::Differences side-by-side comparison of the expected data structure and what we actually got. So it’s not like this is one big monolithic test that either succeeds or fails.

> An alternative approach would be to create the object, then write method calls which focus on individual attributes expected within that object:
> #####
> my $obj = $self->call( [
>         POST => '/location/:location_id/build',
>         {
>             author     => 'Wally Wallington',
>             structures => [
>                 {
>                     type     => 'dolmen',
>                     material => 'concrete',
>                 }
>             ]
>         }
>     ]
> );
> is($obj->http_code, HTTP_CREATED, "Got expected HTTP response code");
> ok($obj->built, "'built' set to true value");
> ok(length($obj->author),
>  "I don't care what was set for 'author' as long as it's a non-zero-length string");
> #####

That’s exactly what I don’t want to do. Writing individual tests for each element of the data structure gets old very quickly, and it’s easy to decide you don’t want to do that and just skip some of the tests. (I’ve seen that happen in a project at $WORK which didn’t use this test framework, incidentally; this isn’t just supposition.)

The complicated expect structure is explicitly designed to be as concise and readable as possible, so e.g. you don’t have to write test titles for every comparison (semi-decent ones are generated for you), and your test can look like the data structure you’re getting back.

> One more specific objection:
> #####
> >     expect => {
> ...
> >             build_id    => qr{^ (?<build_id> BUILD \d+ ) $}x,
> ...
> >     }
> ...
> > # $self->state('build_id') got set by the named capture in the regex above.
> #####
> Is the 'build_id' something set by the process of creation of a new object and intrinsically part of that new object?  Or is it an artifact of testing that object?
> And if it is (as I suspect) generated in the creation of a new object, is its value predictable in advance?  If not predictable in advance -- if, say, it's partly composed of an epoch timestamp --  then you can't make any assertion about what its value ought to be.  That, in turn, means that it has no place in the 'expect' part of a unit test.

I’m not making an assertion about its exact value, though - I’m just saying “we should get a build_id that looks like BUILD \d+”, and if the resulting value doesn’t look like that, it’s a test failure. But if it does look reasonable, then that’s fine, and let’s remember it for later so we can e.g. test that subsequent calls return the exact same build ID.

> I would raise the same objection with respect to 'structure_id'.  I'd be much more inclined to make method calls on the new object and store the return values in variables for later use:
> #####
> my $build_id = $obj->build_id();
> my $structure_id = $obj->build_id();
> my $newobj = $self->call( [
>         PATCH => "/location/$location_id/build/$build_id/structure/$structure_id",
>         { material => 'gold' }
>     ]
> );
> #####

Again, that’s too cumbersome. Without the magic of named captures in a regex creating a state variable, I’d have to say:

my $data = $self->test(…);
my $build_id = $data->{build_id};
my $structure_id = $data->{structures}[0]{structure_id};

which is (a) two and a bit extra lines of typing, and (b) potentially the source of an error.

But with the named capture magic, I can say in the same place “this value should look like this, and I want to remember it for later”, no matter how deep in a data structure that value happens to be.

> Now, I concede that some of my objections are a matter of taste. TIMTOWTDI.  But I've written a lot of testing code over the years -- probably more testing code than "production" code -- and my gut feeling is that your testing apparatus is over-engineered for general use -- and perhaps even over-engineered for your own use.

I’ll happily admit that it’s not suitable for everything - it’s only really suitable for testing complicated data structures. But all of the complicated stuff - which has indeed built up over time, as you suspected - is there for a reason, and to make writing tests easier and nicer.


Thread Previous Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at | Group listing | About