[Bug 19176] New: Dates comparison fails on slow server
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 Bug ID: 19176 Summary: Dates comparison fails on slow server Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: ASSIGNED Severity: normal Priority: P5 - low Component: Test Suite Assignee: jonathan.druart@bugs.koha-community.org Reporter: jonathan.druart@bugs.koha-community.org QA Contact: testopia@bugs.koha-community.org # Failed test 'borrowers.updated_on should have been set to now on creating' # at t/db_dependent/Patrons.t line 74. # got: '2017-08-10T20:53:03' # expected: '2017-08-10T20:53:04' # Looks like you failed 1 test of 17. [20:53:15] t/db_dependent/Patrons.t ..................................... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 66493 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66493&action=edit Bug 19176: Truncate to minutes when comparing dates in tests # Failed test 'borrowers.updated_on should have been set to now on creating' # at t/db_dependent/Patrons.t line 74. # got: '2017-08-10T20:53:03' # expected: '2017-08-10T20:53:04' # Looks like you failed 1 test of 17. [20:53:15] t/db_dependent/Patrons.t ..................................... The plan here is to compare string and/or DateTime objects and truncate the DateTime to minute That way tests will no longer fail when the seconds differ -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |veron@veron.ch --- Comment #2 from Marc Véron <veron@veron.ch> --- Hi Jonathan The patch minifies that false positives happen, but IMO the test will still arbitrarely fail with e.g. following dates: got: '2017-08-10T20:53:59' expected: '2017-08-10T20:54:00' Wouldn't it be safer to calculate the delta and pass the test OK if delta <60 sec? (Or even introduce a param for the precision that defaults to 60.) Marc -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #66493|0 |1 is obsolete| | --- Comment #3 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 66563 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66563&action=edit Bug 19176: Compare the number of seconds when comparing dates in tests # Failed test 'borrowers.updated_on should have been set to now on creating' # at t/db_dependent/Patrons.t line 74. # got: '2017-08-10T20:53:03' # expected: '2017-08-10T20:53:04' # Looks like you failed 1 test of 17. [20:53:15] t/db_dependent/Patrons.t ..................................... The plan here is to compare the number of seconds between two dates. If < 60 the dates are consired as identicals. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 --- Comment #4 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Marc Véron from comment #2)
Wouldn't it be safer to calculate the delta and pass the test OK if delta <60 sec? (Or even introduce a param for the precision that defaults to 60.)
Yes, I though about that but considered as negligible/can be ignored. But you are right, let's fix it correctly :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com --- Comment #5 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Marc Véron from comment #2)
Hi Jonathan
The patch minifies that false positives happen, but IMO the test will still arbitrarely fail with e.g. following dates:
got: '2017-08-10T20:53:59' expected: '2017-08-10T20:54:00'
Wouldn't it be safer to calculate the delta and pass the test OK if delta <60 sec? (Or even introduce a param for the precision that defaults to 60.)
Marc
The datetime should be mocked IMO. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 David Bourgault <david.bourgault@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #66563|0 |1 is obsolete| | --- Comment #6 from David Bourgault <david.bourgault@inlibro.com> --- Created attachment 67322 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=67322&action=edit Bug 19176: Compare the number of seconds when comparing dates in tests # Failed test 'borrowers.updated_on should have been set to now on creating' # at t/db_dependent/Patrons.t line 74. # got: '2017-08-10T20:53:03' # expected: '2017-08-10T20:53:04' # Looks like you failed 1 test of 17. [20:53:15] t/db_dependent/Patrons.t ..................................... The plan here is to compare the number of seconds between two dates. If < 60 the dates are consired as identicals. Signed-off-by: David Bourgault <david.bourgault@inlibro.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 David Bourgault <david.bourgault@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |david.bourgault@inlibro.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |julian.maurice@biblibre.com --- Comment #7 from Julian Maurice <julian.maurice@biblibre.com> --- t::lib::Dates::compare doesn't trigger a fail if dates differ only in hours, days, months or years. For instance, t::lib::Dates::compare('2017-01-01 01:00:00', '2018-02-02 02:00:00') says ok 1 - (in 0 seconds) Also, I think t::lib::Dates::compare shouldn't call ok(), because when a test fails it reports the filename and line where the test failed, and it will always be t/lib/Dates.pm, line 22, which is not very useful. IMO it should return -1, 0 or 1 (like DateTime::compare) and should be called like that: ok(0 == t::lib::Dates::compare($got, $expected), $description) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 --- Comment #8 from Julian Maurice <julian.maurice@biblibre.com> --- ... and it doesn't consider timezone. my $dt = DateTime->new( year => 2017, month => 1, day => 1, hour => 1, minute => 1, second => 0, time_zone => '+0000', ); my $dt2 = DateTime->new( year => 2017, month => 1, day => 1, hour => 1, minute => 1, second => 0, time_zone => '+0400', ); t::lib::Dates::compare($dt, $dt2, 'different timezones'); says 'ok'. Maybe you should compare "epoch values" (DateTime::epoch) instead. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |julian.maurice@biblibre.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #67322|0 |1 is obsolete| | --- Comment #9 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 68082 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=68082&action=edit Bug 19176: Compare the number of seconds when comparing dates in tests # Failed test 'borrowers.updated_on should have been set to now on creating' # at t/db_dependent/Patrons.t line 74. # got: '2017-08-10T20:53:03' # expected: '2017-08-10T20:53:04' # Looks like you failed 1 test of 17. [20:53:15] t/db_dependent/Patrons.t ..................................... The plan here is to compare the number of seconds between two dates. If < 60 the dates are consired as identicals. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Thanks Julian, excellent remarks. I rewrote the patch and add tests for the new module. Switch back to Needs Signoff. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 --- Comment #11 from M. Tompsett <mtompset@hotmail.com> --- Comment on attachment 68082 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=68082 Bug 19176: Compare the number of seconds when comparing dates in tests Review of attachment 68082: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=19176&attachment=68082) ----------------------------------------------------------------- ::: t/Test/Dates.pm @@ +10,5 @@
+ +is( t::lib::Dates::compare( $dt_1, $dt_2 ), -1, '2017 is before 2018' ); +is( t::lib::Dates::compare( $dt_2, $dt_1 ), 1, '2018 is after 2017' ); + +is( t::lib::Dates::compare( $date_1, $date_2 ), -1, '2017 is before 2018 (strings comparaison)' );
Typo: comparison. ::: t/lib/Dates.pm @@ +19,5 @@
+ my $dt_expected = dt_from_string($expected); + $dt_got->set_time_zone('floating'); + $dt_expected->set_time_zone('floating'); + my $diff = $dt_got->epoch - $dt_expected->epoch; + if ( abs($diff) < 60 ) { return 0 }
A slow server isn't going to have a 15+ second gap, I believe this 60 could be smaller. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |magnus@libriotech.no --- Comment #12 from Magnus Enger <magnus@libriotech.no> --- How can this be tested? Just run the tests in t/db_dependent/Patrons.t and t/db_dependent/Virtualshelves.t? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 --- Comment #13 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 68412 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=68412&action=edit Bug 19176: Reduce the number of seconds to 5 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #68082|0 |1 is obsolete| | Attachment #68412|0 |1 is obsolete| | --- Comment #14 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 68413 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=68413&action=edit Bug 19176: Compare the number of seconds when comparing dates in tests # Failed test 'borrowers.updated_on should have been set to now on creating' # at t/db_dependent/Patrons.t line 74. # got: '2017-08-10T20:53:03' # expected: '2017-08-10T20:53:04' # Looks like you failed 1 test of 17. [20:53:15] t/db_dependent/Patrons.t ..................................... The plan here is to compare the number of seconds between two dates. If < 60 the dates are consired as identicals. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 --- Comment #15 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 68414 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=68414&action=edit Bug 19176: Reduce the number of seconds to 5 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 --- Comment #16 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to M. Tompsett from comment #11)
Typo: comparison.
Fixed.
::: t/lib/Dates.pm @@ +19,5 @@
+ my $dt_expected = dt_from_string($expected); + $dt_got->set_time_zone('floating'); + $dt_expected->set_time_zone('floating'); + my $diff = $dt_got->epoch - $dt_expected->epoch; + if ( abs($diff) < 60 ) { return 0 }
A slow server isn't going to have a 15+ second gap, I believe this 60 could be smaller.
Reduced to 5. (In reply to Magnus Enger from comment #12)
How can this be tested? Just run the tests in t/db_dependent/Patrons.t and t/db_dependent/Virtualshelves.t?
yes, and t/Test/Dates.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #68413|0 |1 is obsolete| | Attachment #68414|0 |1 is obsolete| | --- Comment #17 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 68448 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=68448&action=edit Bug 19176: Compare the number of seconds when comparing dates in tests # Failed test 'borrowers.updated_on should have been set to now on creating' # at t/db_dependent/Patrons.t line 74. # got: '2017-08-10T20:53:03' # expected: '2017-08-10T20:53:04' # Looks like you failed 1 test of 17. [20:53:15] t/db_dependent/Patrons.t ..................................... The plan here is to compare the number of seconds between two dates. If < 60 the dates are consired as identicals. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 --- Comment #18 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 68449 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=68449&action=edit Bug 19176: Reduce the number of seconds to 5 Signed-off-by: Josef Moravec <josef.moravec@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 --- Comment #19 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 68450 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=68450&action=edit Bug 19176: followup - fix POD in t/lib/Dates.pm Signed-off-by: Josef Moravec <josef.moravec@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #20 from Julian Maurice <julian.maurice@biblibre.com> --- There's still something wrong with timezones. Try this: $dt_1 = DateTime->new(year => 2001, month => 1, day => 1, hour => 0, minute => 0, second => 0, time_zone => '+0100'); $dt_3 = DateTime->new(year => 2001, month => 1, day => 1, hour => 1, minute => 0, second => 0, time_zone => '+0200'); is( DateTime->compare($dt_1, $dt_3), 0 ); is( t::lib::Dates::compare( $dt_1, $dt_3 ), 0 ); $dt_1 and $dt_3 are the exact same time, but with a different timezone. DateTime->compare returns 0, but t::lib::Dates::Compare returns -1, because it sets timezone of both date to 'floating' before comparing them, which is wrong not only because it produces a false result, but also because comparing DateTime objects shouldn't modify them. Also a note on this line: $dt_3 = $dt_1->clone->set_time_zone('+0400'); It doesn't do what you think. After that line, $dt_3 is still equal to $dt_1 (with a different timezone) because set_time_zone also adjusts the local time (see http://search.cpan.org/~drolsky/DateTime-1.44/lib/DateTime.pm#$dt-%3Eset_tim...) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Hi Julian, Thanks for taking care of this. I am stuck and I do not have more time to spend on this. Do you have something to suggest? If we do not find a way to handle tz easily, we can provide a way to compare dates without handling them. Moreover I am not sure we need it in existing tests. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 --- Comment #22 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 68848 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=68848&action=edit Bug 19176: Fix how t::lib::Dates::compare handle timezone -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 --- Comment #23 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Jonathan Druart from comment #21)
Hi Julian, Thanks for taking care of this. I am stuck and I do not have more time to spend on this. Do you have something to suggest? If we do not find a way to handle tz easily, we can provide a way to compare dates without handling them. Moreover I am not sure we need it in existing tests.
I'm not sure if we need it, but I think it's better to behave like DateTime::compare. The last patch should fix the timezone problem. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA --- Comment #24 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Thanks Julian! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #25 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 17.11, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #26 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Just a few (minor) comments from looking at t::lib::Dates Test::More seems not to be used. $description is not used either, and if I am not mistaken is never passed too. POD says that date is truncated to minutes, but the code compares epoch times (which are seconds) and than rather out of the blue ignores a difference of less than 6 seconds (why 6) ? A comment in the code would be helpful too. The POD is no longer true. If I understood well, Julian asked to remove the floating timezones. It seems to me that you'd better compare dates in floating however. (See similar problem with DST boundaries on earlier pushed report.) And yes, you should clone the dates when changing timezone and comparing. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 --- Comment #27 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 68923 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=68923&action=edit Bug 19176: (QA follow-up) few cosmetic changes -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 --- Comment #28 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Last patch pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |20584 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20584 [Bug 20584] Koha/Patron/Categories.t is on slow servers -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19176 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED Resolution|--- |FIXED CC| |fridolin.somers@biblibre.co | |m -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org