Replacing TAP test planning with done_testing()

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.

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

=?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

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

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

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/

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.

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

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/

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