# 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 .....................................
Created attachment 66493 [details] [review] 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
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
Created attachment 66563 [details] [review] 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.
(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 :)
(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.
Created attachment 67322 [details] [review] 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>
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)
... 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.
Created attachment 68082 [details] [review] 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.
Thanks Julian, excellent remarks. I rewrote the patch and add tests for the new module. Switch back to Needs Signoff.
Comment on attachment 68082 [details] [review] Bug 19176: Compare the number of seconds when comparing dates in tests Review of attachment 68082 [details] [review]: ----------------------------------------------------------------- ::: 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.
How can this be tested? Just run the tests in t/db_dependent/Patrons.t and t/db_dependent/Virtualshelves.t?
Created attachment 68412 [details] [review] Bug 19176: Reduce the number of seconds to 5
Created attachment 68413 [details] [review] 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.
Created attachment 68414 [details] [review] Bug 19176: Reduce the number of seconds to 5
(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
Created attachment 68448 [details] [review] 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>
Created attachment 68449 [details] [review] Bug 19176: Reduce the number of seconds to 5 Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 68450 [details] [review] Bug 19176: followup - fix POD in t/lib/Dates.pm Signed-off-by: Josef Moravec <josef.moravec@gmail.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_time_zone%28_$tz_%29)
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.
Created attachment 68848 [details] [review] Bug 19176: Fix how t::lib::Dates::compare handle timezone
(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.
Thanks Julian!
Pushed to master for 17.11, thanks to everybody involved!
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.
Created attachment 68923 [details] [review] Bug 19176: (QA follow-up) few cosmetic changes
Last patch pushed to master