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

On Tue, 5 Jul 2022 09:59:42 -0400
Andrew Dunstan <andrew@dunslane.net> wrote:

> On 2022-07-03 Su 16:12, Jehan-Guillaume de Rorthais wrote:
> > 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.  
> 
> 
> There is not a single TAP test in our source code that is aimed at
> testing our test infrastructure as opposed to testing what we are
> actually in the business of building, and I'm not about to add one.

Whatever, it helped me during the dev process of fixing this bug. Remove
them if you are uncomfortable with them.

> Every added test consumes buildfarm cycles and space on the buildfarm
> server for the report, be it ever so small.

They were not supposed to enter the buildfarm cycles. I wrote it earlier, they
do not interfere with day-to-day dev activity.

> Every added test needs maintenance, be it ever so small. There's no such
> thing as a free test (apologies to Heinlein and others).

This is the first argument I can understand.

> > ...
> > 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.
> 
> I think we'll just have to agree to disagree about it.

Noted.

Cheers,