Skip Menu |
 

This queue is for tickets about the DBD-SQLite CPAN distribution.

Report information
The Basics
Id: 99747
Status: resolved
Priority: 0/
Queue: DBD-SQLite

People
Owner: Nobody in particular
Requestors: ribasushi [...] leporine.io
Cc: laurent.dami [...] free.fr
AdminCc:

Bug Information
Severity: (no value)
Broken in: 1.44
Fixed in: (no value)



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" }
RT-Send-CC: laurent.dami [...] free.fr
Download (untitled) / with headers
text/plain 3.1k
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" > }
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>
Download (untitled) / with headers
text/plain 1.3k
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
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>
Download (untitled) / with headers
text/plain 1.1k
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
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
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>
Download (untitled) / with headers
text/plain 1017b
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
RT-Send-CC: laurent.dami [...] free.fr
Download (untitled) / with headers
text/plain 1.1k
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
> >
Closed as DBD::SQLite 1.46 was released. Thanks!


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.