Description
Katrin Fischer
2016-12-13 21:50:05 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. Created attachment 68995 [details] [review] Bug 17770: Improve perlcritic level before patch: perlcritic -4 noisy after patch: perlcritic -4 ok Created attachment 68996 [details] [review] Bug 17770: Perltidy 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> 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> 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> 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. 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. :) 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? (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. More Tomas, I just filed it :) I guess you are looking for Time::Fake here. (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. 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. (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... 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. Created attachment 69930 [details] [review] Bug 17770: Perlcritic improvements before patch: perlcritic -3 noisy after patch: perlcritic -3 ok Created attachment 69931 [details] [review] Bug 17770: Perltidy differences should only be whitespace 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 (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... Restored older version since Jonathan suggested the sleep was a joke in comment #20. 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> 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> 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> "Sitemapper.t fails if date changes during test run" is the description of the bug, not what the patch does. Pushed to master for 18.05, thanks to everybody involved! Awesome work all! Pushed to stable for 17.11.02 Pushed to 17.05.x for v17.05.08 |