Description
Jonathan Druart
2020-04-16 14:08:45 UTC
Created attachment 103088 [details] [review] Bug 25172: Make Koha::Logger explode if init went wrong We do not want to fail silently for the logger. Created attachment 103089 [details] [review] Bug 25172: Remove _recheck_logfile I don't understand how that could be useful. We do not want to test if the logfile is writable every time we log something! Created attachment 103090 [details] [review] Bug 25172: Adjust tests Created attachment 103091 [details] [review] Bug 25172: Identify and display possible problems on the about page Applying: Bug 25172: Make Koha::Logger explode if init went wrong error: sha1 information is lacking or useless (Koha/Logger.pm). error: could not build fake ancestor Patch failed at 0001 Bug 25172: Make Koha::Logger explode if init went wrong I think this patch set needs a rebase. Created attachment 103300 [details] [review] Bug 25172: Make Koha::Logger explode if init went wrong We do not want to fail silently for the logger. Created attachment 103301 [details] [review] Bug 25172: Remove _recheck_logfile I don't understand how that could be useful. We do not want to test if the logfile is writable every time we log something! Created attachment 103302 [details] [review] Bug 25172: Adjust tests Created attachment 103303 [details] [review] Bug 25172: Identify and display possible problems on the about page Patches apply now, taking a look. Created attachment 103304 [details] [review] Bug 25172: Make Koha::Logger explode if init went wrong We do not want to fail silently for the logger. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 103305 [details] [review] Bug 25172: Remove _recheck_logfile I don't understand how that could be useful. We do not want to test if the logfile is writable every time we log something! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 103306 [details] [review] Bug 25172: Adjust tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 103307 [details] [review] Bug 25172: Identify and display possible problems on the about page Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> This all appears to work as expected for me.. and offers a genuine improvement.. I didn't like that we just carried on regardless either.. I'm sure Kyle has also done similar changes in a SIP bug at some point? (In reply to Martin Renvoize from comment #15) > This all appears to work as expected for me.. and offers a genuine > improvement.. I didn't like that we just carried on regardless either.. I'm > sure Kyle has also done similar changes in a SIP bug at some point? The SIP logging bug uses Koha::Logger so its' behavior should be fixed by this as well. I love everything about this patch set, but I'm getting one unit test failure: root@6b3a1a3db7b8:koha(bug25172-qa)$ prove t/Logger.t t/Logger.t .. # Failed test 'Logger did not init correctly without permission' # at t/Logger.t line 52. # Looks like you failed 1 test of 6. t/Logger.t .. 1/1 # Failed test 'Test01 -- Simple tests for Koha::Logger' # at t/Logger.t line 63. # Looks like you failed 1 test of 1. t/Logger.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/1 subtests Test Summary Report ------------------- t/Logger.t (Wstat: 256 Tests: 1 Failed: 1) Failed test: 1 Non-zero exit status: 1 Files=1, Tests=1, 1 wallclock secs ( 0.01 usr 0.01 sys + 0.36 cusr 0.06 csys = 0.44 CPU) Result: FAIL Jonathan, does this test fail for you as well? Created attachment 103310 [details] [review] Bug 25172: Add missing filter (In reply to Kyle M Hall from comment #17) > I love everything about this patch set, but I'm getting one unit test > failure: > > root@6b3a1a3db7b8:koha(bug25172-qa)$ prove t/Logger.t > t/Logger.t .. > # Failed test 'Logger did not init correctly without permission' > # at t/Logger.t line 52. > # Looks like you failed 1 test of 6. > t/Logger.t .. 1/1 > # Failed test 'Test01 -- Simple tests for Koha::Logger' > # at t/Logger.t line 63. > # Looks like you failed 1 test of 1. > t/Logger.t .. Dubious, test returned 1 (wstat 256, 0x100) > Failed 1/1 subtests > > Test Summary Report > ------------------- > t/Logger.t (Wstat: 256 Tests: 1 Failed: 1) > Failed test: 1 > Non-zero exit status: 1 > Files=1, Tests=1, 1 wallclock secs ( 0.01 usr 0.01 sys + 0.36 cusr 0.06 > csys = 0.44 CPU) > Result: FAIL > > > Jonathan, does this test fail for you as well? Thanks for testing Kyle. No, it passes for me. But I have a weird behavior with this permission test, I had to move it up before the other test (when the log file is 700). Hmm, I can't get the test to fail either. (In reply to Martin Renvoize from comment #20) > Hmm, I can't get the test to fail either. Thanks for the confirmations. It's probably something odd with my setup. Created attachment 103391 [details] [review] Bug 25172: Make Koha::Logger explode if init went wrong We do not want to fail silently for the logger. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 103392 [details] [review] Bug 25172: Remove _recheck_logfile I don't understand how that could be useful. We do not want to test if the logfile is writable every time we log something! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 103393 [details] [review] Bug 25172: Adjust tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 103394 [details] [review] Bug 25172: Identify and display possible problems on the about page Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 103395 [details] [review] Bug 25172: Add missing filter Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Nice work everyone! Pushed to master for 20.05 Created attachment 103485 [details] [review] Bug 25172: Fix Auth_with_ldap.t # Failed test 'checkpw_ldap tests' # at t/db_dependent/Auth_with_ldap.t line 324. {UNKNOWN}: Configuration not defined at /usr/share/perl5/Log/Log4perl/Config.pm line 579. at /kohadevbox/koha/Koha/Patron.pm line 395 Nice work everyone! Pushed to master for 20.05 Could bug 25284 be related to this? (In reply to Katrin Fischer from comment #30) > Could bug 25284 be related to this? Testing strongly suggest it does... linking. not backported to 19.11.x |