Re: More business with $Test::Builder::Level in the TAP tests
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 signature.asc Description: PGP signature
Re: More business with $Test::Builder::Level in the TAP tests
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 as ok() or is(). cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: More business with $Test::Builder::Level in the TAP tests
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 signature.asc Description: PGP signature
Re: More business with $Test::Builder::Level in the TAP tests
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
Re: More business with $Test::Builder::Level in the TAP tests
> On 8 Oct 2021, at 09:51, Michael Paquier 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/
Re: More business with $Test::Builder::Level in the TAP tests
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 signature.asc Description: PGP signature
Re: More business with $Test::Builder::Level in the TAP tests
> On 7 Oct 2021, at 03:53, 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; 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/
Re: More business with $Test::Builder::Level in the TAP tests
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
diff --git a/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl b/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl
index 8134c2a62e..8d689b9601 100644
--- a/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl
+++ b/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl
@@ -72,6 +72,8 @@ command_fails_like(
sub run_check
{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
my ($suffix, $test_name) = @_;
create_files();
diff --git a/src/bin/pg_ctl/t/004_logrotate.pl b/src/bin/pg_ctl/t/004_logrotate.pl
index aa0d64a4f7..13e91f3bc9 100644
--- a/src/bin/pg_ctl/t/004_logrotate.pl
+++ b/src/bin/pg_ctl/t/004_logrotate.pl
@@ -31,6 +31,8 @@ sub fetch_file_name
# Check for a pattern in the logs associated to one format.
sub check_log_pattern
{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
my $format = shift;
my $logfiles = shift;
my $pattern = shift;
diff --git a/src/bin/pg_verifybackup/t/005_bad_manifest.pl b/src/bin/pg_verifybackup/t/005_bad_manifest.pl
index 4f5b8f5a49..1420cfb352 100644
--- a/src/bin/pg_verifybackup/t/005_bad_manifest.pl
+++ b/src/bin/pg_verifybackup/t/005_bad_manifest.pl
@@ -176,6 +176,8 @@ EOM
sub test_parse_error
{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
my ($test_name, $manifest_contents) = @_;
test_bad_manifest($test_name,
@@ -186,6 +188,8 @@ sub test_parse_error
sub test_fatal_error
{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
my ($test_name, $manifest_contents) = @_;
test_bad_manifest($test_name, qr/fatal: $test_name/, $manifest_contents);
@@ -194,6 +198,8 @@ sub test_fatal_error
sub test_bad_manifest
{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
my ($test_name, $regexp, $manifest_contents) = @_;
open(my $fh, '>', "$tempdir/backup_manifest") || die "open: $!";
diff --git a/src/bin/psql/t/010_tab_completion.pl b/src/bin/psql/t/010_tab_completion.pl
index 8695d22545..dbca56afad 100644
--- a/src/bin/psql/t/010_tab_completion.pl
+++ b/src/bin/psql/t/010_tab_completion.pl
@@ -127,6 +127,8 @@ sub check_completion
# (won't work if we are inside a string literal!)
sub clear_query
{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
check_completion("\\r\n", qr/postgres=# /, "\\r works");
return;
}
@@ -136,6 +138,8 @@ sub clear_query
# than clear_query because we lose evidence in the history file)
sub clear_line
{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
check_completion("\025\n", qr/postgres=# /, "control-U works");
return;
}
diff --git a/src/test/kerberos/t/001_auth.pl b/src/test/kerberos/t/001_auth.pl
index c484237d07..968be3952f 100644
--- a/src/test/kerberos/t/001_auth.pl
+++ b/src/test/kerberos/t/001_auth.pl
@@ -221,6 +221,8 @@ sub test_access
# As above, but test for an arbitrary query result.
sub test_query
{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
my ($node, $role, $query, $expected, $gssencmode, $test_name) = @_;
# need to connect over TCP/IP for Kerberos
diff --git a/src/test/perl/README b/src/test/perl/README
index f04b2a2ea4..45de7661ff 100644
--- a/src/test/perl/README
+++ b/src/test/perl/README
@@ -61,9 +61,16 @@ Test::More::like entails use of the qr// operator. Avoid Perl 5.8.8 bug
#39185 by not using the "$" regular expression metacharacter in qr// when also
using the "/m" modifier. Instead of "$", use "\n" or "(?=\n|\z)".
-Read the Test::More documentation for more on how to write tests:
+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;
+
+Read the documentation for more on how to write tests:
perldoc Test::More
+perldoc Test::Builder
For available PostgreSQL-specific test methods and some example tests read the
perldoc for the test modules, e.g.:
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index ac581c1c07..9916a36012 100644
---
Re: More business with $Test::Builder::Level in the TAP tests
> On 6 Oct 2021, at 13:33, Andrew Dunstan 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/
Re: More business with $Test::Builder::Level in the TAP tests
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
Re: More business with $Test::Builder::Level in the TAP tests
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.
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
diff --git a/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl b/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl
index 8134c2a62e..8d689b9601 100644
--- a/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl
+++ b/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl
@@ -72,6 +72,8 @@ command_fails_like(
sub run_check
{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
my ($suffix, $test_name) = @_;
create_files();
diff --git a/src/bin/pg_ctl/t/004_logrotate.pl b/src/bin/pg_ctl/t/004_logrotate.pl
index aa0d64a4f7..13e91f3bc9 100644
--- a/src/bin/pg_ctl/t/004_logrotate.pl
+++ b/src/bin/pg_ctl/t/004_logrotate.pl
@@ -31,6 +31,8 @@ sub fetch_file_name
# Check for a pattern in the logs associated to one format.
sub check_log_pattern
{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
my $format = shift;
my $logfiles = shift;
my $pattern = shift;
diff --git a/src/bin/pg_verifybackup/t/005_bad_manifest.pl b/src/bin/pg_verifybackup/t/005_bad_manifest.pl
index 4f5b8f5a49..1420cfb352 100644
--- a/src/bin/pg_verifybackup/t/005_bad_manifest.pl
+++ b/src/bin/pg_verifybackup/t/005_bad_manifest.pl
@@ -176,6 +176,8 @@ EOM
sub test_parse_error
{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
my ($test_name, $manifest_contents) = @_;
test_bad_manifest($test_name,
@@ -186,6 +188,8 @@ sub test_parse_error
sub test_fatal_error
{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
my ($test_name, $manifest_contents) = @_;
test_bad_manifest($test_name, qr/fatal: $test_name/, $manifest_contents);
@@ -194,6 +198,8 @@ sub test_fatal_error
sub test_bad_manifest
{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
my ($test_name, $regexp, $manifest_contents) = @_;
open(my $fh, '>', "$tempdir/backup_manifest") || die "open: $!";
diff --git a/src/bin/psql/t/010_tab_completion.pl b/src/bin/psql/t/010_tab_completion.pl
index 8695d22545..dbca56afad 100644
--- a/src/bin/psql/t/010_tab_completion.pl
+++ b/src/bin/psql/t/010_tab_completion.pl
@@ -127,6 +127,8 @@ sub check_completion
# (won't work if we are inside a string literal!)
sub clear_query
{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
check_completion("\\r\n", qr/postgres=# /, "\\r works");
return;
}
@@ -136,6 +138,8 @@ sub clear_query
# than clear_query because we lose evidence in the history file)
sub clear_line
{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
check_completion("\025\n", qr/postgres=# /, "control-U works");
return;
}
diff --git a/src/test/kerberos/t/001_auth.pl b/src/test/kerberos/t/001_auth.pl
index c484237d07..968be3952f 100644
--- a/src/test/kerberos/t/001_auth.pl
+++ b/src/test/kerberos/t/001_auth.pl
@@ -221,6 +221,8 @@ sub test_access
# As above, but test for an arbitrary query result.
sub test_query
{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
my ($node, $role, $query, $expected, $gssencmode, $test_name) = @_;
# need to connect over TCP/IP for Kerberos
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index ac581c1c07..9916a36012 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -75,6 +75,8 @@ note "testing connection parameter \"target_session_attrs\"";
# Expect to connect to $target_node (undef for failure) with given $status.
sub test_target_session_attrs
{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
my $node1 = shift;
my $node2 = shift;
my $target_node = shift;
diff --git a/src/test/recovery/t/003_recovery_targets.pl b/src/test/recovery/t/003_recovery_targets.pl
index 7bd500ed95..78ef60d3b2 100644
--- a/src/test/recovery/t/003_recovery_targets.pl
+++ b/src/test/recovery/t/003_recovery_targets.pl
@@ -14,6 +14,8 @@ use Time::HiRes qw(usleep);
# count to reach $num_rows, yet not later than the recovery target.
sub test_recovery_standby
{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
my $test_name = shift;
my $node_name = shift;
my $node_primary= shift;
diff --git a/src/test/recovery/t/007_sync_rep.pl b/src/test/recovery/t/007_sync_rep.pl
index 9d00e17f9f..3b031addf7 100644
---
