Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs

Andrew Dunstan <andrew@dunslane.net> writes:

On 2023-06-26 Mo 19:55, Jacob Champion wrote:

Hello,

I was running the test_pg_dump extension suite, and I got annoyed that
I couldn't keep it from deleting its dump artifacts after a successful
run. Here's a patch to make use of PG_TEST_NOCLEAN (which currently
covers the test cluster's base directory) with the Test::Utils
tempdirs too.

(Looks like this idea was also discussed last year [1]; let me know if
I missed any more recent suggestions.)

-        CLEANUP => 1);
+        CLEANUP => not defined $ENV{'PG_TEST_NOCLEAN'});


This doesn't look quite right. If PG_TEST_CLEAN had a value of 0 we
would still do the cleanup. I would probably use something like:

    CLEANUP => $ENV{'PG_TEST_NOCLEAN'} // 1

i.e. if it's not defined at all or has a value of undef, do the cleanup,
otherwise use the value.

If the environment varible were used as a boolean, it should be
	CLEANUP => not $ENV{PG_TEST_NOCLEAN}

since `not undef` returns true with no warning, and the senses of the
two flags are inverted.

However, the docs
(https://www.postgresql.org/docs/16/regress-tap.html#REGRESS-TAP-VARS)
say "If the environment variable PG_TEST_NOCLEAN is set", not "is set to
a true value", and the existing test in PostgreSQL::Test::Cluster's END
block is:
	# skip clean if we are requested to retain the basedir	next if defined $ENV{'PG_TEST_NOCLEAN'};                                  
So the original `not defined` test is consistent with that.

ok, but ...

I think it's unwise to encourage setting environment variables without values. Some years ago I had to work around some ugly warnings in buildfarm logs by removing one such. I guess in the end it's a minor issue, but if someone actually sets it to 0 it would seem to me like a POLA violation still to skip the cleanup.

cheers

andew

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