Bug #99747 for DBD-SQLite: New test t/virtual_table/21_perldata

Thu Oct 23 04:19:07 2014 ribasushi [...] leporine.io - Ticket created

CC:	laurent.dami [...] free.fr
Subject:	New test t/virtual_table/21_perldata_charinfo.t relies on volatile data

* This bug is currently of unimportant severity, but this may change * I am so sorry I didn't catch this earlier, I am in the process of adjusting my CI to make sure I do not miss this kind of stuff in the future. The main issue is that the test relies on Unicode::UCD::charinfo to always return the same data. Unicode is an evolving standard. It *does* change (more often than one would want). In the interest on of backward and more importantly forward compatibility, changing this test to use a stable data set before 1.46 around Christmas is very strongly advised. rabbit@Ahasver:~$ perlbrew use 5.8.1 rabbit@Ahasver:~$ perl -MDevel::Dwarn -MUnicode::UCD=charinfo -e 'Dwarn { ver_perl => $], ver_UCD => Unicode::UCD->VERSION, cmap => { map { $_, (charinfo($_)||{})->{script} } (890..900) } }' { cmap => { 890 => "Greek", 891 => undef, 892 => undef, 893 => undef, 894 => undef, 895 => undef, 896 => undef, 897 => undef, 898 => undef, 899 => undef, 900 => undef }, ver_UCD => "0.21", ver_perl => "5.008001" } rabbit@Ahasver:~$ perlbrew use 5.8.5 rabbit@Ahasver:~$ perl -MDevel::Dwarn -MUnicode::UCD=charinfo -e 'Dwarn { ver_perl => $], ver_UCD => Unicode::UCD->VERSION, cmap => { map { $_, (charinfo($_)||{})->{script} } (890..900) } }' { cmap => { 890 => "Greek", 891 => undef, 892 => undef, 893 => undef, 894 => "Common", 895 => undef, 896 => undef, 897 => undef, 898 => undef, 899 => undef, 900 => "Greek" }, ver_UCD => "0.22", ver_perl => "5.008005" } rabbit@Ahasver:~$ perlbrew use 5.16.2 rabbit@Ahasver:~$ perl -MDevel::Dwarn -MUnicode::UCD=charinfo -e 'Dwarn { ver_perl => $], ver_UCD => Unicode::UCD->VERSION, cmap => { map { $_, (charinfo($_)||{})->{script} } (890..900) } }' { cmap => { 890 => "Greek", 891 => "Greek", 892 => "Greek", 893 => "Greek", 894 => "Common", 895 => undef, 896 => undef, 897 => undef, 898 => undef, 899 => undef, 900 => "Greek" }, ver_UCD => "0.43", ver_perl => "5.016002" }

Thu Oct 23 21:01:29 2014 ishigaki [...] cpan.org - Correspondence added

RT-Send-CC:

laurent.dami [...] free.fr

On Thu Oct 23 17:19:07 2014, RIBASUSHI wrote: Show quoted text

> * This bug is currently of unimportant severity, but this may change * > > I am so sorry I didn't catch this earlier, I am in the process of > adjusting my CI to make sure I do not miss this kind of stuff in the > future. > > The main issue is that the test relies on Unicode::UCD::charinfo to > always return the same data. Unicode is an evolving standard. It > *does* change (more often than one would want). In the interest on of > backward and more importantly forward compatibility, changing this > test to use a stable data set before 1.46 around Christmas is very > strongly advised.

This test is to see if an example in the pod works correctly. So changing U::UCD into a stable data doesn't make much sense. Besides, the test is fairly loose and it's unlikely to break unless Unicode standard changes so drastically that "Greek and Coptic" block itself moves into a totally different block. One thing I do care is it assumes U::UCD is always installed in the system. The assumption is fairly right as U::UCD is a core module as of now, but it's not promised, and I do not want to add it as an explicit dependency of DBD::SQLite, either. So, if we need to make the test more robust, it should test the existence of U::UCD first and skip the test if it's not installed. It might also be useful to add a perl code to see if U::UCD has actually a greek sigma in the testing block. It's also an option to factor out ::VirtualTable and make it another distribution so that it can depend on U::UCD (and a non-dev version of DBD::SQLite) freely. What do you think, Laurent? Show quoted text

> > rabbit@Ahasver:~$ perlbrew use 5.8.1 > rabbit@Ahasver:~$ perl -MDevel::Dwarn -MUnicode::UCD=charinfo -e > 'Dwarn { ver_perl => $], ver_UCD => Unicode::UCD->VERSION, cmap => { > map { $_, (charinfo($_)||{})->{script} } (890..900) } }' > { > cmap => { > 890 => "Greek", > 891 => undef, > 892 => undef, > 893 => undef, > 894 => undef, > 895 => undef, > 896 => undef, > 897 => undef, > 898 => undef, > 899 => undef, > 900 => undef > }, > ver_UCD => "0.21", > ver_perl => "5.008001" > } > rabbit@Ahasver:~$ perlbrew use 5.8.5 > rabbit@Ahasver:~$ perl -MDevel::Dwarn -MUnicode::UCD=charinfo -e > 'Dwarn { ver_perl => $], ver_UCD => Unicode::UCD->VERSION, cmap => { > map { $_, (charinfo($_)||{})->{script} } (890..900) } }' > { > cmap => { > 890 => "Greek", > 891 => undef, > 892 => undef, > 893 => undef, > 894 => "Common", > 895 => undef, > 896 => undef, > 897 => undef, > 898 => undef, > 899 => undef, > 900 => "Greek" > }, > ver_UCD => "0.22", > ver_perl => "5.008005" > } > rabbit@Ahasver:~$ perlbrew use 5.16.2 > rabbit@Ahasver:~$ perl -MDevel::Dwarn -MUnicode::UCD=charinfo -e > 'Dwarn { ver_perl => $], ver_UCD => Unicode::UCD->VERSION, cmap => { > map { $_, (charinfo($_)||{})->{script} } (890..900) } }' > { > cmap => { > 890 => "Greek", > 891 => "Greek", > 892 => "Greek", > 893 => "Greek", > 894 => "Common", > 895 => undef, > 896 => undef, > 897 => undef, > 898 => undef, > 899 => undef, > 900 => "Greek" > }, > ver_UCD => "0.43", > ver_perl => "5.016002" > }

Thu Oct 23 21:01:30 2014 The RT System itself - Status changed from 'new' to 'open'

Fri Oct 24 04:17:46 2014 laurent.dami [...] free.fr - Correspondence added

Subject:	Re: [rt.cpan.org #99747] New test t/virtual_table/21_perldata_charinfo.t relies on volatile data
Date:	Fri, 24 Oct 2014 10:17:31 +0200
To:	bug-DBD-SQLite [...] rt.cpan.org, ribasushi [...] cpan.org
From:	Laurent Dami <laurent.dami [...] free.fr>

Le 24.10.2014 03:01, Kenichi Ishigaki via RT a écrit : Show quoted text

> [...] > One thing I do care is it assumes U::UCD is always installed in the system. The assumption is fairly right as U::UCD is a core module as of now, but it's not promised, and I do not want to add it as an explicit dependency of DBD::SQLite, either. So, if we need to make the test more robust, it should test the existence of U::UCD first and skip the test if it's not installed. It might also be useful to add a perl code to see if U::UCD has actually a greek sigma in the testing block.

That's an excellent compromise. I'm currently on holydays but I can volunteer to adapt the test in this way within a couple of days. Show quoted text

> It's also an option to factor out ::VirtualTable and make it another distribution so that it can depend on U::UCD (and a non-dev version of DBD::SQLite) freely. > > What do you think, Laurent? >

Separating VirtualTable would be good in principle, because it will only be used by a small subset of SQLite users. But the C code is tightly integrated with dbdimp.c , so I don't see how this code could possibly separated. Of course we could just keep the abstract class in the distribution, and publish the example concrete subclasses separately, but then the problem is how to write meaningful tests without having the subclasses. Cheers, Laurent

Fri Oct 24 04:53:31 2014 ribasushi [...] leporine.io - Correspondence added

Subject:	Re: [rt.cpan.org #99747] New test t/virtual_table/21_perldata_charinfo.t relies on volatile data
Date:	Fri, 24 Oct 2014 10:53:15 +0200
To:	bug-DBD-SQLite [...] rt.cpan.org
From:	Peter Rabbitson <ribasushi [...] cpan.org>

On 10/24/2014 10:17 AM, laurent.dami@free.fr via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=99747 > > > Le 24.10.2014 03:01, Kenichi Ishigaki via RT a écrit :

>> [...] >> One thing I do care is it assumes U::UCD is always installed in the system. The assumption is fairly right as U::UCD is a core module as of now, but it's not promised, and I do not want to add it as an explicit dependency of DBD::SQLite, either. So, if we need to make the test more robust, it should test the existence of U::UCD first and skip the test if it's not installed. It might also be useful to add a perl code to see if U::UCD has actually a greek sigma in the testing block.

> That's an excellent compromise. I'm currently on holydays but I can > volunteer to adapt the test in this way within a couple of days.

The easiest is to check the version of U::UCD and to skip unless found. I still think this is a bad idea for "down the road", but am happy with whatever solution that will let me pass tests on 5.8.1+ (currently DBD::SQLite is effectively 5.8.4+) Btw Laurent have you had a chance to look at https://rt.cpan.org/Public/Bug/Display.html?id=99748 ? Cheers

Fri Oct 24 05:09:13 2014 ishigaki [...] cpan.org - Correspondence added

On Fri Oct 24 17:17:46 2014, laurent.dami@free.fr wrote: Show quoted text

> Le 24.10.2014 03:01, Kenichi Ishigaki via RT a écrit :

> > [...] > > One thing I do care is it assumes U::UCD is always installed in the > > system. The assumption is fairly right as U::UCD is a core module as > > of now, but it's not promised, and I do not want to add it as an > > explicit dependency of DBD::SQLite, either. So, if we need to make > > the test more robust, it should test the existence of U::UCD first > > and skip the test if it's not installed. It might also be useful to > > add a perl code to see if U::UCD has actually a greek sigma in the > > testing block.

> That's an excellent compromise. I'm currently on holydays but I can > volunteer to adapt the test in this way within a couple of days. >

> > It's also an option to factor out ::VirtualTable and make it another > > distribution so that it can depend on U::UCD (and a non-dev version > > of DBD::SQLite) freely. > > > > What do you think, Laurent? > >

> Separating VirtualTable would be good in principle, because it will > only > be used by a small subset of SQLite users. But the C code is tightly > integrated with dbdimp.c , so I don't see how this code could possibly > separated. Of course we could just keep the abstract class in the > distribution, and publish the example concrete subclasses separately, > but then the problem is how to write meaningful tests without having > the > subclasses.

Maybe we can move everything except C code into a separate distribution, and eval require ::VirtualTable(::SubClasses) (and skip if not available) in some basic tests in DBD::SQLite. Or, maybe we can remove even those basic tests from DBD::SQLite itself, and let ::VirtualTable distribution do thorough testing with an arbitrary version of DBD::SQLite (so that future DBD::SQLite should not fail accidentally by old/incompatible ::VirtualTable). I think the latter is a bit more robust wrt DBD::SQLite (if I need to choose between those two), but whatever you feel comfortable is fine with me :) Show quoted text

> Cheers, Laurent

Fri Oct 24 23:42:25 2014 laurent.dami [...] free.fr - Correspondence added

Subject:	Re: [rt.cpan.org #99747] New test t/virtual_table/21_perldata_charinfo.t relies on volatile data
Date:	Sat, 25 Oct 2014 05:42:14 +0200
To:	bug-DBD-SQLite [...] rt.cpan.org
From:	Laurent Dami <laurent.dami [...] free.fr>

Le 24.10.2014 11:09, Kenichi Ishigaki via RT a écrit : [...] Show quoted text

> So, if we need to make > the test more robust, it should test the existence of U::UCD first > and skip the test if it's not installed. It might also be useful to > add a perl code to see if U::UCD has actually a greek sigma in the > testing block.

Patched. Show quoted text

> Maybe we can move everything except C code into a separate > distribution, and eval require ::VirtualTable(::SubClasses) (and skip > if not available) in some basic tests in DBD::SQLite.

This would make it very difficult to propagate any later changes. The .pm code and C code go tightly together, so any change is likely to affect both parts, and this should go in one single commit. Even worse, if they belonged to separate distributions, this would mean that CPAN publications of both distributions would have to be synchronized ... a nightmare ! By the way, the python version also has vtable bundled within the main distribution : http://rogerbinns.github.io/apsw/vtable.html

Fri Oct 24 23:43:43 2014 DAMI [...] cpan.org - Status changed from 'open' to 'patched'

Sat Oct 25 00:03:26 2014 ishigaki [...] cpan.org - Correspondence added

RT-Send-CC:

laurent.dami [...] free.fr

On Sat Oct 25 12:42:25 2014, laurent.dami@free.fr wrote: Show quoted text

> Le 24.10.2014 11:09, Kenichi Ishigaki via RT a écrit : > [...]

> > So, if we need to make > > the test more robust, it should test the existence of U::UCD first > > and skip the test if it's not installed. It might also be useful to > > add a perl code to see if U::UCD has actually a greek sigma in the > > testing block.

> Patched.

Thanks! Show quoted text

>

> > Maybe we can move everything except C code into a separate > > distribution, and eval require ::VirtualTable(::SubClasses) (and skip > > if not available) in some basic tests in DBD::SQLite.

> This would make it very difficult to propagate any later changes. The > .pm code and C code go tightly together, so any change is likely to > affect both parts, and this should go in one single commit. Even worse, > if they belonged to separate distributions, this would mean that CPAN > publications of both distributions would have to be synchronized ... a > nightmare ! > > By the way, the python version also has vtable bundled within the main > distribution : > http://rogerbinns.github.io/apsw/vtable.html >

OK. Let's keep them in the DBD::SQLite distribution then. Show quoted text

> >

Thu Dec 11 23:48:34 2014 ishigaki [...] cpan.org - Correspondence added

Closed as DBD::SQLite 1.46 was released. Thanks!

Thu Dec 11 23:48:34 2014 ishigaki [...] cpan.org - Status changed from 'patched' to 'resolved'

Bug #99747 for DBD-SQLite: New test t/virtual_table/21_perldata_charinfo.t relies on volatile data