Thread: 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/
Attachment
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@yesql.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@alvh.no-ip.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/
Attachment
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@sss.pgh.pa.us> writes: > Daniel Gustafsson <daniel@yesql.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@sss.pgh.pa.us> writes: > >> Daniel Gustafsson <daniel@yesql.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
Attachment
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
Attachment
> On 6 Feb 2022, at 08:00, Michael Paquier <michael@paquier.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/