Bug 17770

Summary: t/db_dependent/Sitemapper.t fails when date changes during test run
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: Test SuiteAssignee: Mark Tompsett <mtompset>
Status: CLOSED FIXED QA Contact: Kyle M Hall <kyle>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, J.P.Knight, jonathan.druart, kyle, m.de.rooy, mtompset, nick
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 17770: Sitemapper.t fails if date changes during test run
Bug 17770: Improve perlcritic level
Bug 17770: Perltidy
[SIGNED-OFF] Bug 17770: Sitemapper.t fails if date changes during test run
[SIGNED-OFF] [SIGNED-OFF] Bug 17770: Improve perlcritic level
[SIGNED-OFF] [SIGNED-OFF] Bug 17770: Perltidy
Bug 17770: Counter patch - sleep till past midnight.
Bug 17770: Perlcritic improvements
Bug 17770: Perltidy
Bug 17770: Sitemapper.t fails if date changes during test run
Bug 17770: Improve perlcritic level
Bug 17770: Perltidy

Description Katrin Fischer 2016-12-13 21:50:05 UTC
(22:48:28) tcohen: we need to fix the test
(22:48:34) tcohen: so it mocks the current date


t_db_dependent_Sitemapper_t.Its content is valid (369)
Failing for the past 1 build (Since Unstable#6 )
Took 0 ms.
Error Message

not ok 12 - Its content is valid

Standard Output

1..16
ok 1 - use Koha::Sitemapper;
ok 2 - use Koha::Sitemapper::Writer;
ok 3 - add fixtures
ok 4 - File sitemapindex.xml created
ok 5 - Its content is valid
ok 6 - File sitemap0001.xml created
ok 7 - Its content is valid
ok 8 - File sitemap0001.xml with short URLs created
ok 9 - Its content is valid
ok 10 - add fixtures
ok 11 - File sitemapindex.xml for 75000 bibs created
not ok 12 - Its content is valid
ok 13 - File sitemap0001.xml created
ok 14 - It contains 50000 URLs
ok 15 - File sitemap0002.xml created
ok 16 - It contains 25000 URLs
Comment 1 Mark Tompsett 2017-11-07 03:27:09 UTC
Created attachment 68994 [details] [review]
Bug 17770: Sitemapper.t fails if date changes during test run

To compensate, DateTime->now is mocked to the current date.
Comment 2 Mark Tompsett 2017-11-07 03:27:12 UTC
Created attachment 68995 [details] [review]
Bug 17770: Improve perlcritic level

before patch: perlcritic -4 noisy
after patch: perlcritic -4 ok
Comment 3 Mark Tompsett 2017-11-07 03:27:14 UTC
Created attachment 68996 [details] [review]
Bug 17770: Perltidy
Comment 4 Jon Knight 2017-12-12 16:37:13 UTC
Created attachment 69741 [details] [review]
[SIGNED-OFF] Bug 17770: Sitemapper.t fails if date changes during test run

To compensate, DateTime->now is mocked to the current date.

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Comment 5 Jon Knight 2017-12-12 16:37:16 UTC
Created attachment 69742 [details] [review]
[SIGNED-OFF] [SIGNED-OFF] Bug 17770: Improve perlcritic level

before patch: perlcritic -4 noisy
after patch: perlcritic -4 ok

Signed-off-by: Your Full Name <your_email>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Comment 6 Jon Knight 2017-12-12 16:37:19 UTC
Created attachment 69743 [details] [review]
[SIGNED-OFF] [SIGNED-OFF] Bug 17770: Perltidy

Signed-off-by: Your Full Name <your_email>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Comment 7 Jon Knight 2017-12-12 16:39:37 UTC
Hmm, I removed the duff "Signed-off-by: Your Full Name <your_email>" in all three patches I thought... that was just signed off by me.  

Incidentally I checked this by running the given test repeatedly whilst changing the date on the machine wildly - all tests completed successfully.
Comment 8 Mark Tompsett 2017-12-12 17:04:20 UTC
The Your Name sign offs are usually a result of failing to set the git setting up:
https://wiki.koha-community.org/wiki/Git_bz_configuration
https://wiki.koha-community.org/wiki/Koha_on_ubuntu_-_git#Download_the_latest_Koha_release
(Some of the things overlap, some don't. That's why I put both)

Also, you'll notice your signed off versions have [SIGNED-OFF] in the title. That means you have to pay attention to which things you need to obsolete when attaching, because different title means different patch. I've obsoleted the non-signed versions. Thanks for the sign off, Jon Knight. :)
Comment 9 Marcel de Rooy 2017-12-15 11:12:27 UTC
You are mocking now while there is only one call left. Why not just call DateTime->now only once to fill $now ? First getting the complete date and then extracting ymd later is unneeded?
Comment 10 Mark Tompsett 2017-12-15 16:29:26 UTC
(In reply to Marcel de Rooy from comment #9)
> You are mocking now while there is only one call left. Why not just call
> DateTime->now only once to fill $now ? First getting the complete date and
> then extracting ymd later is unneeded?

Yes, only 85 directly uses it. However, there are tests on
lines 71, 83, 97, 100, 116, 129, 145, 154, 164, 182, 185, 193, 196, and 204.

Any of these that trigger DateTime->now()->ymd on line 109 of Koha::SiteMapper::Writer needs the mock. Otherwise the end tag won't match.

I added a nice print STDERR "TRIGGERED!\n"; just about it and determined that the tests 4, 8, and 11 use it. This means this mock will fix not just test 12's more typical failure, but even the rarest cases of 5 and 9.

Plus, I was implementing exactly what was suggested in comment #0 by Katrin Fischer.
Comment 11 Katrin Fischer 2017-12-16 11:36:49 UTC
More Tomas, I just filed it :)
Comment 12 Jonathan Druart 2017-12-19 14:44:43 UTC
I guess you are looking for Time::Fake here.
Comment 13 Mark Tompsett 2017-12-19 16:02:44 UTC
(In reply to Jonathan Druart from comment #12)
> I guess you are looking for Time::Fake here.

No, when the server lags, the time expected doesn't match the time generated. We need a mock, not a Time::Fake. Time::Fake does relative shifts in time. This is a matter of holding time still.
Comment 14 Jonathan Druart 2017-12-19 16:30:59 UTC
Sorry, I did not get the problem.
Actually we have a lot of tests that would fail if run at midnight.
I would suggest to sleep at that time and run the test suite in the next morning.
Comment 15 Mark Tompsett 2017-12-19 17:20:38 UTC
(In reply to Jonathan Druart from comment #14)
> Actually we have a lot of tests that would fail if run at midnight.
> I would suggest to sleep at that time and run the test suite in the next
> morning.

Counter-patch coming...
Comment 16 Mark Tompsett 2017-12-19 19:56:25 UTC
Created attachment 69929 [details] [review]
Bug 17770: Counter patch - sleep till past midnight.

As per Jonathan's comment #14, added a wait_till_midnight
function to t::lib::Dates.

Running this test at 23:59 will trigger a sleep until after midnight
accumulated in 60 second intervals.
Comment 17 Mark Tompsett 2017-12-19 19:56:35 UTC
Created attachment 69930 [details] [review]
Bug 17770: Perlcritic improvements

before patch: perlcritic -3 noisy
after patch: perlcritic -3 ok
Comment 18 Mark Tompsett 2017-12-19 19:56:45 UTC
Created attachment 69931 [details] [review]
Bug 17770: Perltidy

differences should only be whitespace
Comment 19 Mark Tompsett 2017-12-19 19:58:34 UTC
Here is the sleep only version that even provides a nice sleep function for other tests which may need it too. As per Jonathan's comment #14
Comment 20 Jonathan Druart 2017-12-19 20:05:29 UTC
(In reply to M. Tompsett from comment #19)
> Here is the sleep only version that even provides a nice sleep function for
> other tests which may need it too. As per Jonathan's comment #14

Well, it meant to be taken as a second degree joke :)
Maybe I forgot a smiley...
Comment 21 Mark Tompsett 2017-12-19 20:30:30 UTC
Restored older version since Jonathan suggested the sleep was a joke in comment #20.
Comment 22 Kyle M Hall 2017-12-21 11:57:20 UTC
Created attachment 70005 [details] [review]
Bug 17770: Sitemapper.t fails if date changes during test run

To compensate, DateTime->now is mocked to the current date.

Signed-off-by: Your Full Name <your_email>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Kyle M Hall 2017-12-21 11:57:27 UTC
Created attachment 70006 [details] [review]
Bug 17770: Improve perlcritic level

before patch: perlcritic -4 noisy
after patch: perlcritic -4 ok

Signed-off-by: Your Full Name <your_email>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Kyle M Hall 2017-12-21 11:57:30 UTC
Created attachment 70007 [details] [review]
Bug 17770: Perltidy

Signed-off-by: Your Full Name <your_email>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Jonathan Druart 2017-12-21 16:06:44 UTC
"Sitemapper.t fails if date changes during test run" is the description of the bug, not what the patch does.
Comment 26 Jonathan Druart 2017-12-21 16:22:38 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 27 Nick Clemens 2018-01-15 13:15:05 UTC
Awesome work all! Pushed to stable for 17.11.02
Comment 28 Fridolin Somers 2018-01-15 15:16:42 UTC
Pushed to 17.05.x for v17.05.08