Ensure that STDERR is empty during connect_ok

As part of the NSS patchset, quite a few bugs (and NSS quirks) were found by
inspecting STDERR in connect_ok and require it to be empty. This is not really
NSS specific, and could help find issues in other libraries as well so I
propose to apply it regardless of the fate of the NSS patchset.

(The change in the SCRAM tests stems from this now making all testruns have the
same number of tests. While I prefer to not plan at all and instead run
done_testing(), doing that consistently is for another patch, keeping this with
the remainder of the suites.)

--
Daniel Gustafsson https://vmware.com/

On 2022-Feb-02, Daniel Gustafsson wrote:

> As part of the NSS patchset, quite a few bugs (and NSS quirks) were found by
> inspecting STDERR in connect_ok and require it to be empty. This is not really
> NSS specific, and could help find issues in other libraries as well so I
> propose to apply it regardless of the fate of the NSS patchset.
>
> (The change in the SCRAM tests stems from this now making all testruns have the
> same number of tests. While I prefer to not plan at all and instead run
> done_testing(), doing that consistently is for another patch, keeping this with
> the remainder of the suites.)

Since commit 405f32fc4960 we can rely on subtests for this, so perhaps
we should group all the tests in connect_ok() (including your new one)
into a subtest; then each connect_ok() calls count as a single test, and
we can add more tests in it without having to change the callers.

--
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
Al principio era UNIX, y UNIX habló y dijo: "Hello world\n".
No dijo "Hello New Jersey\n", ni "Hello USA\n".

Daniel Gustafsson <daniel(at)yesql(dot)se> writes:
> As part of the NSS patchset, quite a few bugs (and NSS quirks) were found by
> inspecting STDERR in connect_ok and require it to be empty. This is not really
> NSS specific, and could help find issues in other libraries as well so I
> propose to apply it regardless of the fate of the NSS patchset.

+1

> (The change in the SCRAM tests stems from this now making all testruns have the
> same number of tests. While I prefer to not plan at all and instead run
> done_testing(), doing that consistently is for another patch, keeping this with
> the remainder of the suites.)

+1 to that too, counting the tests is a pretty useless exercise.

regards, tom lane

> On 2 Feb 2022, at 16:01, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> On 2022-Feb-02, Daniel Gustafsson wrote:
>
>> As part of the NSS patchset, quite a few bugs (and NSS quirks) were found by
>> inspecting STDERR in connect_ok and require it to be empty. This is not really
>> NSS specific, and could help find issues in other libraries as well so I
>> propose to apply it regardless of the fate of the NSS patchset.
>>
>> (The change in the SCRAM tests stems from this now making all testruns have the
>> same number of tests. While I prefer to not plan at all and instead run
>> done_testing(), doing that consistently is for another patch, keeping this with
>> the remainder of the suites.)
>
> Since commit 405f32fc4960 we can rely on subtests for this, so perhaps
> we should group all the tests in connect_ok() (including your new one)
> into a subtest; then each connect_ok() calls count as a single test, and
> we can add more tests in it without having to change the callers.

Disclaimer: longer term I would prefer to remove test plan counting like above
and Toms comment downthread. This is just to make this patch more palatable on
the way there.

Making this a subtest in order to not having to change the callers, turns the
patch into the attached. For this we must group the new test with one already
existing test, if we group more into it (which would make more sense) then we
need to change callers as that reduce the testcount across the tree.

This makes the patch smaller, but not more readable IMO (given that we can't
make it fully use subtests), so my preference is still the v1 patch.

Or did I misunderstand you?

--
Daniel Gustafsson https://vmware.com/

On 2022-Feb-02, Daniel Gustafsson wrote:

> Making this a subtest in order to not having to change the callers, turns the
> patch into the attached. For this we must group the new test with one already
> existing test, if we group more into it (which would make more sense) then we
> need to change callers as that reduce the testcount across the tree.

Well, it wasn't my intention that this patch would not have to change
the callers. What I was thinking about is making connect_ok() have a
subtest for *all* the tests it contains (and changing the callers to
match), so that *in the future* we can add more tests there without
having to change the callers *again*.

> Or did I misunderstand you?

I think you did.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Cómo ponemos nuestros dedos en la arcilla del otro. Eso es la amistad; jugar
al alfarero y ver qué formas se pueden sacar del otro" (C. Halloway en
La Feria de las Tinieblas, R. Bradbury)

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> Daniel Gustafsson <daniel(at)yesql(dot)se> writes:
>
>> While I prefer to not plan at all and instead run done_testing(),
>> doing that consistently is for another patch, keeping this with the
>> remainder of the suites.
>
> +1 to that too, counting the tests is a pretty useless exercise.

Rather than waiting for Someone™ to find a suitably-shaped tuit to do a
whole sweep converting everything to done_testing(), I think we should
make a habit of converting individual scripts whenever a change breaks
the count.

- ilmari

On 2/2/22 11:01, Dagfinn Ilmari Mannsåker wrote:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>
>> Daniel Gustafsson <daniel(at)yesql(dot)se> writes:
>>
>>> While I prefer to not plan at all and instead run done_testing(),
>>> doing that consistently is for another patch, keeping this with the
>>> remainder of the suites.
>> +1 to that too, counting the tests is a pretty useless exercise.
> Rather than waiting for Someone™ to find a suitably-shaped tuit to do a
> whole sweep converting everything to done_testing(), I think we should
> make a habit of converting individual scripts whenever a change breaks
> the count.

Or when anyone edits a script, even if the number of tests doesn't get
broken.

cheers

andrew

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

On 2022-Feb-02, Andrew Dunstan wrote:

> On 2/2/22 11:01, Dagfinn Ilmari Mannsåker wrote:

> > Rather than waiting for Someone™ to find a suitably-shaped tuit to do a
> > whole sweep converting everything to done_testing(), I think we should
> > make a habit of converting individual scripts whenever a change breaks
> > the count.

+1.

> Or when anyone edits a script, even if the number of tests doesn't get
> broken.

Sure, if the committer is up to doing it, but let's not make that a hard
requirement for any commit that touches a test script.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Ni aún el genio muy grande llegaría muy lejos
si tuviera que sacarlo todo de su propio interior" (Goethe)

On Wed, Feb 02, 2022 at 04:42:09PM +0100, Daniel Gustafsson wrote:
> Disclaimer: longer term I would prefer to remove test plan counting like above
> and Toms comment downthread.

That would be really nice.
--
Michael

On Wed, Feb 02, 2022 at 03:40:39PM +0100, Daniel Gustafsson wrote:
> As part of the NSS patchset, quite a few bugs (and NSS quirks) were found by
> inspecting STDERR in connect_ok and require it to be empty. This is not really
> NSS specific, and could help find issues in other libraries as well so I
> propose to apply it regardless of the fate of the NSS patchset.

Sounds sensible from here. Thanks!

All the code paths seem to be covered, at quick glance.
--
Michael

> On 6 Feb 2022, at 08:00, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Wed, Feb 02, 2022 at 03:40:39PM +0100, Daniel Gustafsson wrote:
>> As part of the NSS patchset, quite a few bugs (and NSS quirks) were found by
>> inspecting STDERR in connect_ok and require it to be empty. This is not really
>> NSS specific, and could help find issues in other libraries as well so I
>> propose to apply it regardless of the fate of the NSS patchset.
>
> Sounds sensible from here. Thanks!
>
> All the code paths seem to be covered, at quick glance.

Applied, minus the changes to the test plans which are no longer required after
549ec201d6.

--
Daniel Gustafsson https://vmware.com/