|
Description
Tomás Cohen Arazi (tcohen)
2020-04-10 19:00:54 UTC
Created attachment 102757 [details] [review] Bug 25109: Add lockdir configuration entry Created attachment 102758 [details] [review] Bug 25109: Add entry in about.pl about the configuration lockdir entry Created attachment 102759 [details] [review] Bug 25109: Add ->new and ->lock_exec to Koha::Script Created attachment 102760 [details] [review] Bug 25109: Add lockdir configuration entry Created attachment 102761 [details] [review] Bug 25109: Add entry in about.pl about the configuration lockdir entry Created attachment 102762 [details] [review] Bug 25109: Add ->new and ->lock_exec to Koha::Script 1. <zebra_lockdir>/var/lock/koha/__KOHASITE__</zebra_lockdir> + <lockdir>/var/lock/koha/__KOHASITE__</lockdir> So should we remove zebra_lockdir (and replace it with $lockdir/zebra)? 2. + "The 'script' parameter is mandatory. You should usually pass \$0" $0? Should be "script", right? (In reply to Jonathan Druart from comment #7) > 1. > <zebra_lockdir>/var/lock/koha/__KOHASITE__</zebra_lockdir> > + <lockdir>/var/lock/koha/__KOHASITE__</lockdir> > > So should we remove zebra_lockdir (and replace it with $lockdir/zebra)? Yes, but on a separate bug. If we agree on this, I can work on getting rid of zebra_lockdir of course. > 2. > + "The 'script' parameter is mandatory. You should usually pass \$0" > > $0? Should be "script", right? I tried to say that the 'script' parameter should generally be passed $0. This comes from my try to write the tests for this, in which Script.t was always picked if I used $0. Taking care of some QA concerns. Created attachment 102814 [details] [review] Bug 25109: Add lockdir configuration entry Created attachment 102815 [details] [review] Bug 25109: Add entry in about.pl about the configuration lockdir entry Created attachment 102816 [details] [review] Bug 25109: Add ->new and ->lock_exec to Koha::Script Created attachment 102817 [details] [review] Bug 25109: Add 'wait' tests This simple patch introduces a test for the lock waiting scenario. It replicates the previous tests, but calls a script that passes the wait => 1 parameter to ->lock_exec. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Thanks for the feedback, Jonathan! Created attachment 102840 [details] [review] Bug 25109: Add lockdir configuration entry Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 102841 [details] [review] Bug 25109: Add entry in about.pl about the configuration lockdir entry Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 102842 [details] [review] Bug 25109: Add ->new and ->lock_exec to Koha::Script Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 102843 [details] [review] Bug 25109: Add 'wait' tests This simple patch introduces a test for the lock waiting scenario. It replicates the previous tests, but calls a script that passes the wait => 1 parameter to ->lock_exec. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> lockdir vs lock_dir, please fix. Once fixed, I guess we will have a problem if the dir is not writable, but we need a -w test for the about warning? (In reply to Jonathan Druart from comment #19) > lockdir vs lock_dir, please fix. > > Once fixed, I guess we will have a problem if the dir is not writable, but > we need a -w test for the about warning? Oops. If it is not writeable it will raise a relevant exception and report back. Created attachment 102856 [details] [review] Bug 25109: (QA follow-up) Fix typo in configuration entry name This patch fixes an error in the configuration retrieving line. Created attachment 102887 [details] [review] Bug 25109: (QA follow-up) Add a test for lockdir writable in about.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> (In reply to Tomás Cohen Arazi from comment #8) > (In reply to Jonathan Druart from comment #7) > > 1. > > <zebra_lockdir>/var/lock/koha/__KOHASITE__</zebra_lockdir> > > + <lockdir>/var/lock/koha/__KOHASITE__</lockdir> > > > > So should we remove zebra_lockdir (and replace it with $lockdir/zebra)? > > Yes, but on a separate bug. If we agree on this, I can work on getting rid > of zebra_lockdir of course. I opened bug 25141. Created attachment 102904 [details] [review] Bug 25109: Add lockdir configuration entry Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 102905 [details] [review] Bug 25109: Add entry in about.pl about the configuration lockdir entry Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 102906 [details] [review] Bug 25109: Add ->new and ->lock_exec to Koha::Script Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 102907 [details] [review] Bug 25109: Add 'wait' tests This simple patch introduces a test for the lock waiting scenario. It replicates the previous tests, but calls a script that passes the wait => 1 parameter to ->lock_exec. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 102908 [details] [review] Bug 25109: (QA follow-up) Fix typo in configuration entry name This patch fixes an error in the configuration retrieving line. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 102909 [details] [review] Bug 25109: (QA follow-up) Add a test for lockdir writable in about.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Nice work everyone! Pushed to master for 20.05 enhancement not backported to 19.11.x (In reply to Jonathan Druart from comment #23) > (In reply to Tomás Cohen Arazi from comment #8) > > Yes, but on a separate bug. If we agree on this, I can work on getting rid > > of zebra_lockdir of course. > > I opened bug 25141. Still waiting for a patch there. Once again we have the proof that using sleep in tests is VERY bad. + sleep 1; # Make sure we start after the fork Tests are failing randomly on Jenkins, quite often (at least twice a week). Example for Koha_Master_D10 (#467) 15:01:46 koha_1 | # Failed test 'Exception found' 15:01:46 koha_1 | # at t/Koha/Script.t line 69. 15:01:46 koha_1 | # '' 15:01:46 koha_1 | # doesn't match '(?^u:Unable to acquire the lock.*)' 15:01:46 koha_1 | # Looks like you failed 1 test of 3. 15:01:46 koha_1 | 15:01:46 koha_1 | # Failed test 'lock_exec() tests' 15:01:46 koha_1 | # at t/Koha/Script.t line 87. 15:01:46 koha_1 | # Looks like you failed 1 test of 4. |