Date: Sun, 3 Jul 2022 22:12:13 +0200
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
Message-ID: <20220703221213.3bee4790@karst>
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>
Organization: Dalibo
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20220703221213.3bee4790%40karst>
Precedence: bulk

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,