Thread: pgsql: Add more $Test::Builder::Level in the TAP tests
Add more $Test::Builder::Level in the TAP tests Incrementing the level of the call stack reported is useful for debugging purposes as it allows to control which part of the test is exactly failing, especially if a test is structured with subroutines that call routines from Test::More. This adds more incrementations of $Test::Builder::Level where debugging gets improved (for example it does not make sense for some paths like pg_rewind where long subroutines are used). A note is added to src/test/perl/README about that, based on a suggestion from Andrew Dunstan and a wording coming from both of us. Usage of Test::Builder::Level has spread in 12, so a backpatch down to this version is done. Reviewed-by: Andrew Dunstan, Peter Eisentraut, Daniel Gustafsson Discussion: https://postgr.es/m/YV1CCFwgM1RV1LeS@paquier.xyz Backpatch-through: 12 Branch ------ REL_14_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/d834ebcf23208b3ae2109c0cae9af077202a27a4 Modified Files -------------- contrib/amcheck/t/001_verify_heapam.pl | 8 ++++++++ contrib/test_decoding/t/001_repl_stats.pl | 2 ++ src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl | 2 ++ src/bin/pg_verifybackup/t/005_bad_manifest.pl | 6 ++++++ src/bin/psql/t/010_tab_completion.pl | 4 ++++ src/test/kerberos/t/001_auth.pl | 2 ++ src/test/perl/README | 10 +++++++++- src/test/recovery/t/001_stream_rep.pl | 2 ++ src/test/recovery/t/003_recovery_targets.pl | 2 ++ src/test/recovery/t/007_sync_rep.pl | 2 ++ src/test/recovery/t/009_twophase.pl | 2 ++ src/test/recovery/t/018_wal_optimize.pl | 2 ++ 12 files changed, 43 insertions(+), 1 deletion(-)
On Mon, Oct 11, 2021 at 7:17 PM Michael Paquier <michael@paquier.xyz> wrote: > Add more $Test::Builder::Level in the TAP tests I saw an issue just now that I suspect is linked with this commit. I ran my parallel "make check-world" recipe, and saw this failure: # Failed test 'handling of unexpected PQresultStatus: matches' # at t/001_basic.pl line 43. # 'psql:/home/pg/.psqlrc:56: ERROR: syntax error # psql:/home/pg/.psqlrc:57: ERROR: syntax error # LOG: received replication command: START_REPLICATION 0/0 # unexpected PQresultStatus: 8 # ' # doesn't match '(?^:^unexpected PQresultStatus: 8$)' # Looks like you failed 1 test of 25. t/001_basic.pl ........... Dubious, test returned 1 (wstat 256, 0x100) Failed 1/25 subtests t/010_tab_completion.pl .. ok t/020_cancel.pl .......... ok -- Peter Geoghegan
On Wed, Oct 13, 2021 at 08:14:20PM -0700, Peter Geoghegan wrote: > On Mon, Oct 11, 2021 at 7:17 PM Michael Paquier <michael@paquier.xyz> wrote: > > Add more $Test::Builder::Level in the TAP tests > > I saw an issue just now that I suspect is linked with this commit. I > ran my parallel "make check-world" recipe, and saw this failure: This commit has not touched 001_basic.pl, nor does it touch any of the modules we have in src/test/perl/, so that's unrelated. In the worst case, what you would get after this commit is a better report by incrementing the level stack. The problem is that this test script assumes that it is fine to not use -X/--no-psqlrc in the commands of psql run, making this test prone to fail depending on the local setup. Please note that we pass down -X through PostgresNode::psql, to keep the tests isolated from the external world, so this could be fixed either by using -X but I think that it would be better to switch to psql() with a comparison done to the outputs of stdout, stderr and the error code returned. The failure can be reproduced easily once one adds some junk in their local psqlrc. Adding Peter E in CC, as the author of c0280bc & co. > # Failed test 'handling of unexpected PQresultStatus: matches' > # at t/001_basic.pl line 43. > # 'psql:/home/pg/.psqlrc:56: ERROR: syntax error > # psql:/home/pg/.psqlrc:57: ERROR: syntax error > # LOG: received replication command: START_REPLICATION 0/0 > # unexpected PQresultStatus: 8 > # ' You may also want to fix your local .psqlrc, while on it. -- Michael
Attachment
On Wed, Oct 13, 2021 at 8:51 PM Michael Paquier <michael@paquier.xyz> wrote: > > # Failed test 'handling of unexpected PQresultStatus: matches' > > # at t/001_basic.pl line 43. > > # 'psql:/home/pg/.psqlrc:56: ERROR: syntax error > > # psql:/home/pg/.psqlrc:57: ERROR: syntax error > > # LOG: received replication command: START_REPLICATION 0/0 > > # unexpected PQresultStatus: 8 > > # ' > > You may also want to fix your local .psqlrc, while on it. Why, though? It doesn't do this in any other context. -- Peter Geoghegan
On Wed, Oct 13, 2021 at 08:54:16PM -0700, Peter Geoghegan wrote: > On Wed, Oct 13, 2021 at 8:51 PM Michael Paquier <michael@paquier.xyz> wrote: >>> # Failed test 'handling of unexpected PQresultStatus: matches' >>> # at t/001_basic.pl line 43. >>> # 'psql:/home/pg/.psqlrc:56: ERROR: syntax error >>> # psql:/home/pg/.psqlrc:57: ERROR: syntax error >>> # LOG: received replication command: START_REPLICATION 0/0 >>> # unexpected PQresultStatus: 8 >>> # ' >> >> You may also want to fix your local .psqlrc, while on it. > > Why, though? It doesn't do this in any other context. Even for single psql commands in your environment? That would be surprising that this does not show the same error about lines 56/57 in /home/pg/.psqlrc, but I guess that's up to you for this part.. It does not change the fact that the test needs to be fixed. TAP tests should be designed to not be influenced by the external world. -- Michael
Attachment
On Wed, Oct 13, 2021 at 9:00 PM Michael Paquier <michael@paquier.xyz> wrote: > Even for single psql commands in your environment? That would be > surprising that this does not show the same error about lines 56/57 in > /home/pg/.psqlrc, but I guess that's up to you for this part.. It's doing this: ------------------------------------------------- -- Source custom variables from dedicated file -- ------------------------------------------------- \set queries `cat $HOME/dotfiles/psql-rc-queries.sql` \o /dev/null :queries -- this is line 56 \gset I've been doing this for many years now. It's nothing new. > It does not change the fact that the test needs to be fixed. TAP > tests should be designed to not be influenced by the external world. Right. -- Peter Geoghegan
On Wed, Oct 13, 2021 at 09:04:11PM -0700, Peter Geoghegan wrote: > On Wed, Oct 13, 2021 at 9:00 PM Michael Paquier <michael@paquier.xyz> wrote: >> It does not change the fact that the test needs to be fixed. TAP >> tests should be designed to not be influenced by the external world. > > Right. Hearing nothing, I have just poked at this problem. And I'd rather make all the tests go through PostgresNode::psql from which it is possible to grab stdout, stderr and the result code, as we could miss local changes in this test if we need a centralized fix, for problems like the one with SIGPIPE where we don't use -c in a psql command. Peter, does it take care of your problem? -- Michael
Attachment
On Fri, Oct 15, 2021 at 11:22 PM Michael Paquier <michael@paquier.xyz> wrote: > Hearing nothing, I have just poked at this problem. > Peter, does it take care of your problem? Yes, it does. Thank you for taking care of this. -- Peter Geoghegan
On Sat, Oct 16, 2021 at 08:08:28PM -0700, Peter Geoghegan wrote: > Thank you for taking care of this. Okay. Done, then. -- Michael