Re: CPANifying our test framework - or parts of it

Thanks for the detailed response!

On 10 Sep 2016, at 15:42, James E Keenan <[email protected]> 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. (=46rom our purposes, a general-purpose =E2=80=9Ctes=
t=E2=80=9D 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=E2=80=99t 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.
>>=20
>> # Assume $self->state('location_id') has been set previously
>> $self->test(
>=20
> 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)?
>=20
> 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=E2=80=99s 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 =3D> {
>>        http_code =3D> HTTP_CREATED,
>>        sql_count =3D> 12,
>>        data      =3D> {
>>            location_id =3D> $self->state('location_id'),
>>            build_id    =3D> qr{^ (?<build_id> BUILD \d+ ) $}x,
>>            built       =3D> $self->true,
>>            author     =3D> $self->meh,    # It's OK if they call him =
Idiot.
>>            structures =3D> [
>>                {
>>                    _hashref_contains =3D> {
>>                        structure_id =3D> qr{^ (?<structure_id> STRUCT =
\d+ ) $}x,
>>                        type         =3D> 'dolmen',
>>                        material     =3D> 'concrete',
>>=20
>>                        # There's probably stuff about where the =
dolmen
>>                        # was erected but we ignore that for the =
purpose
>>                        # of this test.
>>                    }
>>                }
>>            ]
>>        }
>>    }
>> );
>=20
> 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=E2=80=99s no objects involved here. The test says =E2=80=9Cif I =
call this particular URL, I expect to get back a hashref of data which =
matches this description=E2=80=9D. 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=E2=80=99t match, but the rest of the data structure was =
fine, you=E2=80=99d get plenty of passing tests and then one failure, =
e.g. =E2=80=9Cnot ok 123 data{built} is true=E2=80=9D, and a =
Test::Differences side-by-side comparison of the expected data structure =
and what we actually got. So it=E2=80=99s 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:
>=20
> #####
> my $obj =3D $self->call( [
>         POST =3D> '/location/:location_id/build',
>         {
>             author     =3D> 'Wally Wallington',
>             structures =3D> [
>                 {
>                     type     =3D> 'dolmen',
>                     material =3D> '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=E2=80=99s exactly what I don=E2=80=99t want to do. Writing =
individual tests for each element of the data structure gets old very =
quickly, and it=E2=80=99s easy to decide you don=E2=80=99t want to do =
that and just skip some of the tests. (I=E2=80=99ve seen that happen in =
a project at $WORK which didn=E2=80=99t use this test framework, =
incidentally; this isn=E2=80=99t just supposition.)

The complicated expect structure is explicitly designed to be as concise =
and readable as possible, so e.g. you don=E2=80=99t 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=E2=80=99re getting =
back.

> One more specific objection:
>=20
> #####
> >     expect =3D> {
> ...
> >             build_id    =3D> qr{^ (?<build_id> BUILD \d+ ) $}x,
> ...
> >     }
> ...
> > # $self->state('build_id') got set by the named capture in the regex =
above.
> #####
>=20
> 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?
>=20
> 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=E2=80=99m not making an assertion about its exact value, though - =
I=E2=80=99m just saying =E2=80=9Cwe should get a build_id that looks =
like BUILD \d+=E2=80=9D, and if the resulting value doesn=E2=80=99t look =
like that, it=E2=80=99s a test failure. But if it does look reasonable, =
then that=E2=80=99s fine, and let=E2=80=99s 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:
>=20
> #####
> my $build_id =3D $obj->build_id();
> my $structure_id =3D $obj->build_id();
> my $newobj =3D $self->call( [
>         PATCH =3D> =
"/location/$location_id/build/$build_id/structure/$structure_id",
>         { material =3D> 'gold' }
>     ]
> );
> #####

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

my $data =3D $self->test(=E2=80=A6);
my $build_id =3D $data->{build_id};
my $structure_id =3D $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 =E2=80=9Cthi=
s value should look like this, and I want to remember it for later=E2=80=9D=
, 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=E2=80=99ll happily admit that it=E2=80=99s not suitable for everything =
- it=E2=80=99s 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
--=20
Website: http://www.illuminated.co.uk/