Horton

Thread

Commits

  1. Replace Test::More plans with done_testing

  1. Replacing TAP test planning with done_testing()

    Daniel Gustafsson <daniel@yesql.se> — 2022-02-09T14:01:36Z

    Whether or not to explicitly plan the number of TAP tests per suite has been
discussed a number of times on this list, often as a side-note in an unrelated
thread which adds/modifies a test.  The concensus has so far weighed towards
not doing manual bookkeeping of test plans but to let Test::More deal with it.
So far, no concrete patch to make that happen has been presented though.

The attached patch removes all Test::More planning and instead ensures that all
tests conclude with a done_testing() call.  While there, I also removed a few
exit(0) calls from individual tests making them more consistent.

Thoughts?

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

  2. Re: Replacing TAP test planning with done_testing()

    Julien Rouhaud <rjuju123@gmail.com> — 2022-02-09T14:16:11Z

    Hi,

On Wed, Feb 09, 2022 at 03:01:36PM +0100, Daniel Gustafsson wrote:
> Whether or not to explicitly plan the number of TAP tests per suite has been
> discussed a number of times on this list, often as a side-note in an unrelated
> thread which adds/modifies a test.  The concensus has so far weighed towards
> not doing manual bookkeeping of test plans but to let Test::More deal with it.
> So far, no concrete patch to make that happen has been presented though.
> 
> The attached patch removes all Test::More planning and instead ensures that all
> tests conclude with a done_testing() call.  While there, I also removed a few
> exit(0) calls from individual tests making them more consistent.
> 
> Thoughts?

+1, I don't think it adds much value compared to the extra work it requires.
Not having the current total also doesn't seem much a problem, as it was said
on the other thread no tests is long enough to really care, at least on a
development box.

I still remember the last time I wanted to add some TAP tests to pg_dump, and
it wasn't pleasant.  In any case we should at least get rid of this one.

The patch itself looks good to me.

  3. Re: Replacing TAP test planning with done_testing()

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2022-02-09T14:34:08Z

    Daniel Gustafsson <daniel@yesql.se> writes:

> Whether or not to explicitly plan the number of TAP tests per suite has been
> discussed a number of times on this list, often as a side-note in an unrelated
> thread which adds/modifies a test.  The concensus has so far weighed towards
> not doing manual bookkeeping of test plans but to let Test::More deal with it.
> So far, no concrete patch to make that happen has been presented though.
>
> The attached patch removes all Test::More planning and instead ensures that all
> tests conclude with a done_testing() call.  While there, I also removed a few
> exit(0) calls from individual tests making them more consistent.
>
> Thoughts?

LGTM, +1.

I have grepped the patched code and can verify that there are no
occurrences of `tests => N` (with or without quotes around `tests`) left
in any test scripts, and `make check-world` passes.

I'm especially pleased by the removal of the big chunks of test count
calculating code in the pg_dump tests.

- ilmari

  4. Re: Replacing TAP test planning with done_testing()

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-09T19:49:47Z

    =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
> Daniel Gustafsson <daniel@yesql.se> writes:
>> The attached patch removes all Test::More planning and instead ensures that all
>> tests conclude with a done_testing() call.  While there, I also removed a few
>> exit(0) calls from individual tests making them more consistent.

> LGTM, +1.

LGTM too.

			regards, tom lane

  5. Re: Replacing TAP test planning with done_testing()

    Michael Paquier <michael@paquier.xyz> — 2022-02-10T00:58:27Z

    On Wed, Feb 09, 2022 at 02:49:47PM -0500, Tom Lane wrote:
> =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
>> Daniel Gustafsson <daniel@yesql.se> writes:
>>> The attached patch removes all Test::More planning and instead ensures that all
>>> tests conclude with a done_testing() call.  While there, I also removed a few
>>> exit(0) calls from individual tests making them more consistent.
> 
>> LGTM, +1.
> 
> LGTM too.

Not tested, but +1.  Could it be possible to backpatch that even if
this could be qualified as only cosmetic?  Each time a test is
backpatched we need to tweak the number of tests planned, and that may
change slightly depending on the branch dealt with.
--
Michael

  6. Re: Replacing TAP test planning with done_testing()

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-02-10T06:26:44Z

    At Thu, 10 Feb 2022 09:58:27 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Wed, Feb 09, 2022 at 02:49:47PM -0500, Tom Lane wrote:
> > =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
> >> Daniel Gustafsson <daniel@yesql.se> writes:
> >>> The attached patch removes all Test::More planning and instead ensures that all
> >>> tests conclude with a done_testing() call.  While there, I also removed a few
> >>> exit(0) calls from individual tests making them more consistent.
> > 
> >> LGTM, +1.
> > 
> > LGTM too.

+1. I was anoyed by the definitions especially when adding Non-windows
only tests.

> Not tested, but +1.  Could it be possible to backpatch that even if
> this could be qualified as only cosmetic?  Each time a test is
> backpatched we need to tweak the number of tests planned, and that may
> change slightly depending on the branch dealt with.

+1. I think that makes backpatching easier.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  7. Re: Replacing TAP test planning with done_testing()

    Daniel Gustafsson <daniel@yesql.se> — 2022-02-11T20:04:53Z

    > On 10 Feb 2022, at 01:58, Michael Paquier <michael@paquier.xyz> wrote:
>>> Daniel Gustafsson <daniel@yesql.se> writes:

>>>> The attached patch removes all Test::More planning and instead ensures that all
>>>> tests conclude with a done_testing() call.

Pushed to master now with a few more additional hunks fixing test changes that
happened between posting this and now.

> Could it be possible to backpatch that even if
> this could be qualified as only cosmetic?  Each time a test is
> backpatched we need to tweak the number of tests planned, and that may
> change slightly depending on the branch dealt with.

I opted out of backpatching for now, to solicit more comments on that.  It's
not a bugfix, but it's also not affecting the compiled bits that we ship, so I
think there's a case to be made both for and against a backpatch.  Looking at
the oldest branch we support, it seems we've done roughly 25 changes to the
test plans in REL_10_STABLE over the years, so it's neither insignificant nor
an everyday activity.  Personally I don't have strong opinions, what do others
think?

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

  8. Re: Replacing TAP test planning with done_testing()

    Andres Freund <andres@anarazel.de> — 2022-02-11T20:16:22Z

    On 2022-02-11 21:04:53 +0100, Daniel Gustafsson wrote:
> I opted out of backpatching for now, to solicit more comments on that.  It's
> not a bugfix, but it's also not affecting the compiled bits that we ship, so I
> think there's a case to be made both for and against a backpatch.  Looking at
> the oldest branch we support, it seems we've done roughly 25 changes to the
> test plans in REL_10_STABLE over the years, so it's neither insignificant nor
> an everyday activity.  Personally I don't have strong opinions, what do others
> think?

+1 on backpatching. Backpatching tests now is less likely to cause conflicts,
but more likely to fail during tests.

  9. Re: Replacing TAP test planning with done_testing()

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-11T20:22:41Z

    Andres Freund <andres@anarazel.de> writes:
> On 2022-02-11 21:04:53 +0100, Daniel Gustafsson wrote:
>> I opted out of backpatching for now, to solicit more comments on that.  It's
>> not a bugfix, but it's also not affecting the compiled bits that we ship, so I
>> think there's a case to be made both for and against a backpatch.  Looking at
>> the oldest branch we support, it seems we've done roughly 25 changes to the
>> test plans in REL_10_STABLE over the years, so it's neither insignificant nor
>> an everyday activity.  Personally I don't have strong opinions, what do others
>> think?

> +1 on backpatching. Backpatching tests now is less likely to cause conflicts,
> but more likely to fail during tests.

If you've got the energy to do it, +1 for backpatching.  I agree
with Michael's opinion that doing so will probably save us
future annoyance.

			regards, tom lane

  10. Re: Replacing TAP test planning with done_testing()

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-02-11T20:28:00Z

    On 2022-Feb-11, Tom Lane wrote:

> Andres Freund <andres@anarazel.de> writes:

> > +1 on backpatching. Backpatching tests now is less likely to cause conflicts,
> > but more likely to fail during tests.
> 
> If you've got the energy to do it, +1 for backpatching.  I agree
> with Michael's opinion that doing so will probably save us
> future annoyance.

+1

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

  11. Re: Replacing TAP test planning with done_testing()

    Julien Rouhaud <rjuju123@gmail.com> — 2022-02-12T02:06:03Z

    Le sam. 12 févr. 2022 à 04:28, Alvaro Herrera <alvherre@alvh.no-ip.org> a
écrit :

> On 2022-Feb-11, Tom Lane wrote:
>
> > Andres Freund <andres@anarazel.de> writes:
>
> > > +1 on backpatching. Backpatching tests now is less likely to cause
> conflicts,
> > > but more likely to fail during tests.
> >
> > If you've got the energy to do it, +1 for backpatching.  I agree
> > with Michael's opinion that doing so will probably save us
> > future annoyance.
>
> +1
>

+1

>