From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: Andrew Dunstan <andrew@dunslane.net>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Fix proposal for comparaison bugs in PostgreSQL::Version
Date: Sun, 3 Jul 2022 22:12:13 +0200
Message-ID: <20220703221213.3bee4790@karst> (raw)
In-Reply-To: <b7772fde-9c65-f56a-cff4-2ef34fc72ccf@dunslane.net>
References: <20220628225325.53d97b8d@karst>
<ea18cfc3-7f8e-8975-b095-226b1865e2e7@dunslane.net>
<20220629110937.19fb2ca9@karst>
<b7772fde-9c65-f56a-cff4-2ef34fc72ccf@dunslane.net>
On Sun, 3 Jul 2022 10:40:21 -0400
Andrew Dunstan <andrew@dunslane.net> wrote:
> On 2022-06-29 We 05:09, Jehan-Guillaume de Rorthais wrote:
> > On Tue, 28 Jun 2022 18:17:40 -0400
> > Andrew Dunstan <andrew@dunslane.net> wrote:
> >
> >> On 2022-06-28 Tu 16:53, Jehan-Guillaume de Rorthais wrote:
> >>> ...
> >>> A better fix would be to store the version internally as version_num that
> >>> are trivial to compute and compare. Please, find in attachment an
> >>> implementation of this.
> >>>
> >>> The patch is a bit bigger because it improved the devel version to support
> >>> rc/beta/alpha comparison like 14rc2 > 14rc1.
> >>>
> >>> Moreover, it adds a bunch of TAP tests to check various use cases.
> >>
> >> Nice catch, but this looks like massive overkill. I think we can very
> >> simply fix the test in just a few lines of code, instead of a 190 line
> >> fix and a 130 line TAP test.
> > I explained why the patch was a little bit larger than required: it fixes
> > the bugs and do a little bit more. The _version_cmp sub is shorter and
> > easier to understand, I use multi-line code where I could probably fold
> > them in a one-liner, added some comments... Anyway, I don't feel the number
> > of line changed is "massive". But I can probably remove some code and
> > shrink some other if it is really important...
> >
> > Moreover, to be honest, I don't mind the number of additional lines of TAP
> > tests. Especially since it runs really, really fast and doesn't hurt
> > day-to-day devs as it is independent from other TAP tests anyway. It could
> > be 1k, if it runs fast, is meaningful and helps avoiding futur regressions,
> > I would welcome the addition.
>
>
> I don't see the point of having a TAP test at all. We have TAP tests for
> testing the substantive products we test, not for the test suite
> infrastructure. Otherwise, where will we stop? Shall we have tests for
> the things that test the test suite?
Tons of perl module have regression tests. When questioning where testing
should stop, it seems the Test::More module itself is not the last frontier:
https://github.com/Test-More/test-more/tree/master/t
Moreover, the PostgreSQL::Version is not a TAP test module, but a module to
deal with PostgreSQL versions and compare them.
Testing makes development faster as well when it comes to test the code.
Instead of testing vaguely manually, you can test a whole bunch of situations
and add accumulate some more when you think about a new one or when a bug is
reported. Having TAP test helps to make sure the code work as expected.
It helped me when creating my patch. With all due respect, I just don't
understand your arguments against them. The number of lines or questioning when
testing should stop doesn't hold much.
> > If we really want to save some bytes, I have a two lines worth of code fix
> > that looks more readable to me than fixing _version_cmp:
> >
> > +++ b/src/test/perl/PostgreSQL/Version.pm
> > @@ -92,9 +92,13 @@ sub new
> > # Split into an array
> > my @numbers = split(/\./, $arg);
> >
> > + # make sure all digit of the array-represented version are set so
> > we can
> > + # keep _version_cmp code as a "simple" digit-to-digit comparison
> > loop
> > + $numbers[$_] += 0 for 0..3;
> > +
> > # Treat development versions as having a minor/micro version one
> > less than # the first released version of that branch.
> > - push @numbers, -1 if ($devel);
> > + $numbers[3] = -1 if $devel;
> >
> > $devel ||= "";
>
> I don't see why this is any more readable.
The _version_cmp is much more readable.
But anyway, this is not the point. Using an array to compare versions where we
can use version_num seems like useless and buggy convolutions to me.
Regards,
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: jgdr@dalibo.com, andrew@dunslane.net
Subject: Re: Fix proposal for comparaison bugs in PostgreSQL::Version
In-Reply-To: <20220703221213.3bee4790@karst>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox