Re: [PATCH] Honor PG_TEST_NOCLEAN for tempdirs

Andrew Dunstan <andrew(at)dunslane(dot)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.

Tangentially, even though the above line contradicts it, the general
perl style is to not unnecessarily quote hash keys or words before `=>`:

~/src/postgresql $ rg -P -t perl '\{\s*\w+\s*\}' | wc -l
1662
~/src/postgresql $ rg -P -t perl '\{\s*(["'\''])\w+\1\s*\}' | wc -l
155
~/src/postgresql $ rg -P -t perl '\w+\s*=>' | wc -l
3842
~/src/postgresql $ rg -P -t perl '(["'\''])\w+\1\s*=>' | wc -l
310

- ilmari