Bug 16636 - t/00-load.t warning from C4/External/BakerTaylor.pm
Summary: t/00-load.t warning from C4/External/BakerTaylor.pm
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial (vote)
Assignee: Mark Tompsett
QA Contact: Marcel de Rooy
URL:
Keywords:
: 16342 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-06-02 01:08 UTC by Mark Tompsett
Modified: 2019-06-07 10:29 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 16636: t/00-load.t warning from C4/External/BakerTaylor.pm (1.35 KB, patch)
2016-06-02 01:10 UTC, Mark Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16636: t/00-load.t warning from C4/External/BakerTaylor.pm (1.40 KB, patch)
2016-06-06 05:28 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16636: t/00-load.t warning from C4/External/BakerTaylor.pm (1.55 KB, patch)
2016-06-06 23:58 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 16636: t/00-load.t warning from C4/External/BakerTaylor.pm (1.64 KB, patch)
2016-06-10 08:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16636: [QA Follow-up] Make BakerTaylor plack safe (6.04 KB, patch)
2016-06-10 08:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16636: [QA Follow-up] Move External_BakerTaylor.t to t (944 bytes, patch)
2016-06-10 08:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16636 [QA Followup] - Move tests to t/External (846 bytes, patch)
2016-06-10 18:02 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Tompsett 2016-06-02 01:08:12 UTC
Since the tests are expecting an initialize function, the initialize call was just moved outside of the INIT block.

TEST PLAN
---------
1) prove t/00-load.t
   -- warnings about INIT for hbyymmincr
2) prove `git grep -l BakerTaylor | grep [.]t$`
   -- should all run okay
3) apply patch
4) repeat steps 1 and 2
   -- warning should be gone, and everything else run okay
5) run koha qa test tools
Comment 1 Mark Tompsett 2016-06-02 01:10:43 UTC Comment hidden (obsolete)
Comment 2 Srdjan Jankovic 2016-06-06 05:28:01 UTC Comment hidden (obsolete)
Comment 3 Héctor Eduardo Castro Avalos 2016-06-06 19:27:25 UTC
Srdjan, do you forget to change bug status?
Comment 4 Florent Mara 2016-06-06 23:41:19 UTC
I followed the test plan and got the following. 

Before applying the patch, prove t/00-load.t would output the following
t/00-load.t .. 1/? Too late to run INIT block at /var/koha/Koha/C4/Barcodes/hbyymmincr.pm line 41.
Too late to run INIT block at /var/koha/Koha/C4/External/BakerTaylor.pm line 42.

After applying the patch, it would still output 1 warning while the comment appears to indicate there should be no warnings. 
t/00-load.t .. 7/? Too late to run INIT block at /var/koha/Koha/C4/Barcodes/hbyymmincr.pm line 41.
t/00-load.t .. ok     
All tests successful.

Since the patch focuses on warning from BakerTaylor.pm, I assume it achieves its goal, maybe then the test plan needs to be updated.
Comment 5 Mark Tompsett 2016-06-06 23:50:42 UTC
(In reply to Florent Mara from comment #4)
> I followed the test plan and got the following. 
> 
> Before applying the patch, prove t/00-load.t would output the following
> t/00-load.t .. 1/? Too late to run INIT block at
> /var/koha/Koha/C4/Barcodes/hbyymmincr.pm line 41.
> Too late to run INIT block at /var/koha/Koha/C4/External/BakerTaylor.pm line
> 42.
> 
> After applying the patch, it would still output 1 warning while the comment
> appears to indicate there should be no warnings. 
> t/00-load.t .. 7/? Too late to run INIT block at
> /var/koha/Koha/C4/Barcodes/hbyymmincr.pm line 41.
> t/00-load.t .. ok     
> All tests successful.
> 
> Since the patch focuses on warning from BakerTaylor.pm, I assume it achieves
> its goal, maybe then the test plan needs to be updated.

Yes, the test plan was a cut-and-paste mistake by me. It should be 

TEST PLAN
---------
1) prove t/00-load.t
   -- warnings about INIT for BakerTaylor
2) prove `git grep -l BakerTaylor | grep [.]t$`
   -- should all run okay
3) apply patch
4) repeat steps 1 and 2
   -- warning should be gone, and everything else run okay
5) run koha qa test tools
Comment 6 Florent Mara 2016-06-06 23:52:55 UTC
Then it is good for me. :-)

The patch behaves as described in the test plan and fixes the warning from BakerTaylor.pm
Comment 7 Mark Tompsett 2016-06-06 23:58:59 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2016-06-07 09:38:01 UTC
We will get rid of the warning, but the it won't make it works correctly under Plack. I may be better to keep the warning to tell something is wrong in this area and need to be fixed.
Comment 9 Marcel de Rooy 2016-06-10 08:44:44 UTC
Created attachment 52213 [details] [review]
Bug 16636: t/00-load.t warning from C4/External/BakerTaylor.pm

Since the tests are expecting an initialize function, the
initialize call was just moved outside of the INIT block.

TEST PLAN
---------
1) prove t/00-load.t
   -- warnings about INIT for BakerTaylor
2) prove `git grep -l BakerTaylor | grep [.]t$`
   -- should all run okay
3) apply patch
4) repeat steps 1 and 2
   -- warning should be gone, and everything else run okay
5) run koha qa test tools

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>

Signed-off-by: Florent Mara <florent.mara@gmail.com>

NOTE: Tweaked test plan based on comment #4,
      Added sign off based on comment #6.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2016-06-10 08:44:53 UTC
Created attachment 52214 [details] [review]
Bug 16636: [QA Follow-up] Make BakerTaylor plack safe

Initialize file level lexicals each call. Do not call _initialize
outside the module.
Adjust test by mocking preferences.

Test plan:
Run t/db_dependent/External_BakerTaylor.t.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested module with trivial script under Plack/memcached by toggling
the associated preferences.
Comment 11 Marcel de Rooy 2016-06-10 08:44:57 UTC
Created attachment 52215 [details] [review]
Bug 16636: [QA Follow-up] Move External_BakerTaylor.t to t

When we mock the preferences completely now, we do not need
db access anymore.

Test plan:
Run the test without exporting KOHA_CONF.
The test passes with some warns about koha-conf.xml.
If you point KOHA_CONF to etc/koha-conf.xml, you have no warns.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Kyle M Hall 2016-06-10 18:02:39 UTC
Created attachment 52253 [details] [review]
Bug 16636 [QA Followup] - Move tests to t/External
Comment 13 Kyle M Hall 2016-06-10 18:03:25 UTC
Pushed to master for 16.11, thanks Mark, Marcell!
Comment 14 Frédéric Demians 2016-06-15 07:50:59 UTC
Pushed in 16.05. Will be in 16.05.01.
Comment 15 Julian Maurice 2016-06-16 10:06:44 UTC
Patch pushed to 3.22.x, will be in 3.22.8
Comment 16 Nick Clemens (kidclamp) 2019-06-07 10:29:55 UTC
*** Bug 16342 has been marked as a duplicate of this bug. ***