Thanks for the detailed response!
On 10 Sep 2016, at 15:42, James E Keenan <jk...@verizon.net> 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.
Sam
--
Website: http://www.illuminated.co.uk/
