Re: Fix proposal for comparaison bugs in PostgreSQL::Version - Mailing list pgsql-hackers

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,