Обсуждение: More business with $Test::Builder::Level in the TAP tests

Hi all,

Following up with Peter E's recent commit 73aa5e0 to add some
forgotten level incrementations, I got to look again at what I did
wrong and why this stuff is useful.

I have gone through all the TAP tests and any code paths using
subroutines, to note that we could improve the locations of the
reports we get by adding more $Test::Builder::Level.  The context is
important, as some code paths use rather-long routines and also
argument values that allow to track easily which test path is being
taken (like pg_rewind), so there is no need to add anything in such
places.  The attached patch adds incrementations for the tests where
the debugging becomes much easier if there is a failure.

Thoughts?
--
Michael

On 06.10.21 08:28, Michael Paquier wrote:
> Following up with Peter E's recent commit 73aa5e0 to add some
> forgotten level incrementations, I got to look again at what I did
> wrong and why this stuff is useful.
> 
> I have gone through all the TAP tests and any code paths using
> subroutines, to note that we could improve the locations of the
> reports we get by adding more $Test::Builder::Level.  The context is
> important, as some code paths use rather-long routines and also
> argument values that allow to track easily which test path is being
> taken (like pg_rewind), so there is no need to add anything in such
> places.  The attached patch adds incrementations for the tests where
> the debugging becomes much easier if there is a failure.

These look correct to me.

On 10/6/21 2:28 AM, Michael Paquier wrote:
> Hi all,
>
> Following up with Peter E's recent commit 73aa5e0 to add some
> forgotten level incrementations, I got to look again at what I did
> wrong and why this stuff is useful.
>
> I have gone through all the TAP tests and any code paths using
> subroutines, to note that we could improve the locations of the
> reports we get by adding more $Test::Builder::Level.  The context is
> important, as some code paths use rather-long routines and also
> argument values that allow to track easily which test path is being
> taken (like pg_rewind), so there is no need to add anything in such
> places.  The attached patch adds incrementations for the tests where
> the debugging becomes much easier if there is a failure.
>
> Thoughts?



We should probably state a requirement for this somewhere. Maybe in
src/test/perl/README. AIUI, the general rule is that any subroutine that
directly or indirectly calls ok() and friends should increase the level.
Such subroutines that don't increase it should probably contain a
comment stating why, so we can know in future that it's not just an
oversight.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

> On 6 Oct 2021, at 13:33, Andrew Dunstan <andrew@dunslane.net> wrote:

> We should probably state a requirement for this somewhere. Maybe in
> src/test/perl/README.

+1, I think that sounds like a very good idea.

--
Daniel Gustafsson        https://vmware.com/

On Wed, Oct 06, 2021 at 07:33:22AM -0400, Andrew Dunstan wrote:
> We should probably state a requirement for this somewhere. Maybe in
> src/test/perl/README. AIUI, the general rule is that any subroutine that
> directly or indirectly calls ok() and friends should increase the level.
> Such subroutines that don't increase it should probably contain a
> comment stating why, so we can know in future that it's not just an
> oversight.

That makes sense.  How about something like that after the part about
Test::More::like and qr// in the section about writing tests?  Here it
is:
+Test::Builder::Level controls how far up in the call stack a test will look
+at when reporting a failure.  This should be incremented by any subroutine
+calling test routines from Test::More, like ok() or is():
+
+    local $Test::Builder::Level = $Test::Builder::Level + 1;
--
Michael

> On 7 Oct 2021, at 03:53, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Oct 06, 2021 at 07:33:22AM -0400, Andrew Dunstan wrote:
>> We should probably state a requirement for this somewhere. Maybe in
>> src/test/perl/README. AIUI, the general rule is that any subroutine that
>> directly or indirectly calls ok() and friends should increase the level.
>> Such subroutines that don't increase it should probably contain a
>> comment stating why, so we can know in future that it's not just an
>> oversight.
>
> That makes sense.  How about something like that after the part about
> Test::More::like and qr// in the section about writing tests?  Here it
> is:
> +Test::Builder::Level controls how far up in the call stack a test will look
> +at when reporting a failure.  This should be incremented by any subroutine
> +calling test routines from Test::More, like ok() or is():
> +
> +    local $Test::Builder::Level = $Test::Builder::Level + 1;

LGTM.  Maybe it should be added that it *must* be called before any Test::More
function is called, it's sort of self-explanatory but not everyone writing TAP
tests will be deeply familiar with Perl.

--
Daniel Gustafsson        https://vmware.com/

On Fri, Oct 08, 2021 at 09:28:04AM +0200, Daniel Gustafsson wrote:
> LGTM.  Maybe it should be added that it *must* be called before any Test::More
> function is called, it's sort of self-explanatory but not everyone writing TAP
> tests will be deeply familiar with Perl.

I think that "must" is too strong in this context, as in some cases it
does not really make sense to increment the level, when using for
example a rather long routine that's labelled with one of the
routine arguments like for pg_rewind.  So I would stick with
"should".
--
Michael

> On 8 Oct 2021, at 09:51, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Oct 08, 2021 at 09:28:04AM +0200, Daniel Gustafsson wrote:
>> LGTM.  Maybe it should be added that it *must* be called before any Test::More
>> function is called, it's sort of self-explanatory but not everyone writing TAP
>> tests will be deeply familiar with Perl.
>
> I think that "must" is too strong in this context, as in some cases it
> does not really make sense to increment the level, when using for
> example a rather long routine that's labelled with one of the
> routine arguments like for pg_rewind.  So I would stick with
> "should".

Fair enough.

--
Daniel Gustafsson        https://vmware.com/

On 10/6/21 9:53 PM, Michael Paquier wrote:
> On Wed, Oct 06, 2021 at 07:33:22AM -0400, Andrew Dunstan wrote:
>> We should probably state a requirement for this somewhere. Maybe in
>> src/test/perl/README. AIUI, the general rule is that any subroutine that
>> directly or indirectly calls ok() and friends should increase the level.
>> Such subroutines that don't increase it should probably contain a
>> comment stating why, so we can know in future that it's not just an
>> oversight.
> That makes sense.  How about something like that after the part about
> Test::More::like and qr// in the section about writing tests?  Here it
> is:
> +Test::Builder::Level controls how far up in the call stack a test will look
> +at when reporting a failure.  This should be incremented by any subroutine
> +calling test routines from Test::More, like ok() or is():
> +
> +    local $Test::Builder::Level = $Test::Builder::Level + 1;


I think we need to be more explicit about it, especially w.r.t. indirect
calls. Every subroutine in the call stack below where you want to error
reported as coming from should contain this line.

Suppose we have


sub a { b();  }

sub b { c();  }

sub c { local $Test::Builder::Level = $Test::Builder::Level + 1;
ok(0,"should succeed"); }


AIUI a call to a() will show the call in b() as the error source. If we
want the error source to be the call to a() we need to add that
increment to both b() and a();


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Fri, Oct 08, 2021 at 12:14:57PM -0400, Andrew Dunstan wrote:
> I think we need to be more explicit about it, especially w.r.t. indirect
> calls. Every subroutine in the call stack below where you want to error
> reported as coming from should contain this line.

Hmm.  I got to think about that for a couple of days, and the
simplest, still the cleanest, phrasing I can come up with is that:
This should be incremented by any subroutine part of a stack calling
test routines from Test::More, like ok() or is().

Perhaps you have a better suggestion?
--
Michael

On 10/10/21 7:18 AM, Michael Paquier wrote:
> On Fri, Oct 08, 2021 at 12:14:57PM -0400, Andrew Dunstan wrote:
>> I think we need to be more explicit about it, especially w.r.t. indirect
>> calls. Every subroutine in the call stack below where you want to error
>> reported as coming from should contain this line.
> Hmm.  I got to think about that for a couple of days, and the
> simplest, still the cleanest, phrasing I can come up with is that:
> This should be incremented by any subroutine part of a stack calling
> test routines from Test::More, like ok() or is().
>
> Perhaps you have a better suggestion?


I would say:

    This should be incremented by any subroutine which directly or indirectly calls test routines from Test::More, such
asok() or is().
 


cheers



andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Mon, Oct 11, 2021 at 10:48:54AM -0400, Andrew Dunstan wrote:
> I would say:
>
>     This should be incremented by any subroutine which directly or
>     indirectly calls test routines from Test::More, such as ok() or
>     is().

Indeed, that looks better.  I have just used that and applied the
change down to 12 where we have begun playing with level
incrementations.
--
Michael