Re: test_pg_dump missing cleanup actions

On Tue, Sep 04, 2018 at 06:02:51PM -0400, Stephen Frost wrote:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> Michael Paquier <michael(at)paquier(dot)xyz> writes:
>>> While hacking another patch, I have noticed that triggerring multiple
>>> times in a row installcheck on test_pg_dump results in a failure because
>>> it is missing clean up actions on the role regress_dump_test_role.
>>> Roles are shared objects, so I think that we ought to not let traces of
>>> it when doing any regression tests on a running instance.
>>
>>> Attached is a patch to clean up things.
>>
>> I'm confused. Isn't the point of that script exactly to create a modified
>> extension for testing pg_dump with?

Not really, the regression tests run for pg_regress and the TAP test
suite are two completely isolated things and share no dependencies.
e54f757 has actually changed test_pg_dump.sql so as it adds regression
tests for pg_init_privs for ALTER EXTENSION ADD/DROP, and visibly those
have been added to test_pg_dump because they were easier to add there,
and this has no interactions with pg_dump. What I think should have
been done initially is to add those new tests in test_extensions
instead.

>> What I'd do is leave the final state as-is and add a "drop role if exists"
>> at the start, similar to what some of the core regression tests do.
>
> I've not followed this thread but based on Tom's response, I agree with
> his suggestion of what to do here.

Not as far as I can see.. Please note that using installcheck on the
main regression test suite does not leave around any extra roles. I can
understand the role of having a DROP ROLE IF EXISTS though: if you get a
crash while testing, then the beginning of the tests are repeatable, so
independently of the root issue Tom's suggestion makes sense to me.
--
Michael