Bug 17539

Summary: t/db_dependent/Reserves.t is failing
Product: Koha Reporter: Lari Taskula <lari.taskula>
Component: Test SuiteAssignee: Lari Taskula <lari.taskula>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, kyle, m.de.rooy, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21133
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 14610    
Bug Blocks:    
Attachments: Bug 17539: Fix Undefined subroutine &C4::Circulation::GetItem error
Bug 17539: Fix Already in a transaction error
Bug 17539: Fix failing Reserves.t tests if category enrolmentfee > 0
Bug 17539: Fix Undefined subroutine &C4::Circulation::GetItem error
Bug 17539: Fix Already in a transaction error
Bug 17539: Fix failing Reserves.t tests if category enrolmentfee > 0
Bug 17539: Fix Undefined subroutine &C4::Circulation::GetItem error
Bug 17539: Fix Already in a transaction error
Bug 17539: Fix failing Reserves.t tests if category enrolmentfee > 0
Bug 17539: (QA followup) Silence warnings
Bug 17539: Fix Undefined subroutine &C4::Circulation::GetItem error
Bug 17539: Fix Already in a transaction error
Bug 17539: Fix failing Reserves.t tests if category enrolmentfee > 0
Bug 17539: (QA followup) Silence warnings
Bug 17539: [QA Follow-up] Move handle after schema

Description Lari Taskula 2016-11-02 11:23:52 UTC
1..72
ok 1 - use C4::Reserves;
ok 2 - CheckReserves Test 1
ok 3 - CheckReserves() include reserve_id in its response
ok 4 - CheckReserves Test 2
ok 5 - CheckReserves Test 3
ok 6 - GetReservesControlBranch returns item home branch when set to ItemHomeLibrary
ok 7 - GetReservesControlBranch returns patron home branch when set to PatronLibrary
ok 8 - Item is correctly waiting
ok 9 - Item is correctly priority 1
ok 10 - Item is correctly priority 2
ok 11 - GetWaiting got only the waiting reserve
ok 12 - GetWaiting got the reserve for the correct borrower
Undefined subroutine &C4::Circulation::GetItem called at /home/ubuntu/kohaclone/C4/Circulation.pm line 1852.
# Looks like you planned 72 tests but ran 12.
# Looks like your test exited with 255 just after 12.

I went back in the version history until the test passed this point, and it seems issues began after commit bf1563e60b31244f4ea977eb84954fb8501ed59a from Bug 14610; this commit on top looks to be failing with error Undefined subroutine &C4::Circulation::GetItemnumberFromBarcode called at /home/ubuntu/kohaclone/C4/Circulation.pm line 1853.
Comment 1 Lari Taskula 2016-11-02 13:31:30 UTC
Created attachment 57094 [details] [review]
Bug 17539: Fix Undefined subroutine &C4::Circulation::GetItem error

perl t/db_dependent/Reserves.t

1..72
ok 1 - use C4::Reserves;
ok 2 - CheckReserves Test 1
ok 3 - CheckReserves() include reserve_id in its response
ok 4 - CheckReserves Test 2
ok 5 - CheckReserves Test 3
ok 6 - GetReservesControlBranch returns item home branch when set to ItemHomeLibrary
ok 7 - GetReservesControlBranch returns patron home branch when set to PatronLibrary
ok 8 - Item is correctly waiting
ok 9 - Item is correctly priority 1
ok 10 - Item is correctly priority 2
ok 11 - GetWaiting got only the waiting reserve
ok 12 - GetWaiting got the reserve for the correct borrower
Undefined subroutine &C4::Circulation::GetItem called at /home/ubuntu/kohaclone/C4/Circulation.pm line 1853.

This patch fixes this error. The solution was to use C4::Reserves before C4::Items.
Comment 2 Lari Taskula 2016-11-02 13:32:05 UTC
Created attachment 57095 [details] [review]
Bug 17539: Fix Already in a transaction error

perl t/db_dependent/Reserves.t

1..72
ok 1 - use C4::Reserves;
ok 2 - CheckReserves Test 1
...
ok 52 - Reserve in waiting status cant be canceled
ok 53 - Reserving an ageRestricted Biblio without a borrower dateofbirth succeeds
DBD::mysql::db begin_work failed: Already in a transaction at
/usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1489.
DBD::mysql::db begin_work failed: Already in a transaction at
/usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1489.
at /usr/share/perl5/DBIx/Class/Schema.pm line 1080.
...
Koha::Object::store('Koha::Patron=HASH(0x9e43b20)') called at
/home/ubuntu/kohaclone/C4/Members.pm line 542
C4::Members::ModMember('borrowernumber', 658535, 'dateofbirth', '2001-11-02')
called at t/db_dependent/Reserves.t line 516

This patch fixes this issue by replacing dbh autocommit=0 with
txn_begin.
Comment 3 Lari Taskula 2016-11-02 13:32:26 UTC
Created attachment 57096 [details] [review]
Bug 17539: Fix failing Reserves.t tests if category enrolmentfee > 0

To test:
1. Set patron category "S" with enrolmentfee 1
2. perl t/db_dependent/Reserves.t
3. Observe following errors:

not ok 59 - Bug 14464 - No fines at beginning
ok 60 - Bug 14464 - 1st reserve correctly created
ok 61 - Bug 14968 - Keep found column from reserve
not ok 62 - Bug 14464 - No fines after cancelling reserve with no charge configured
ok 63 - Bug 14464 - 2nd reserve correctly created
not ok 64 - Bug 14464 - No fines after cancelling reserve with no charge desired
ok 65 - Bug 14464 - 1st reserve correctly created
not ok 66 - Bug 14464 - Fine applied after cancelling reserve with charge desired and configured

4. Apply patch
5. perl t/db_dependent/Reserves.t
6. Observe test pass
Comment 4 Jonathan Druart 2016-11-02 13:52:38 UTC
Lari, good catch.
But still fail for me:
Tag "" is not a valid tag. at t/db_dependent/Reserves.t line 510.
Comment 5 Marcel de Rooy 2016-11-02 15:52:47 UTC
There is already an use_ok in the Reserves test.
If you add another use, maybe change use_ok to require_ok ?
Did the same somewhere else..
Comment 6 Jonathan Druart 2016-11-02 15:59:46 UTC
(In reply to Marcel de Rooy from comment #5)
> There is already an use_ok in the Reserves test.
> If you add another use, maybe change use_ok to require_ok ?
> Did the same somewhere else..

Since we have tests for these modules, I am not sure the use_ok are really useful.
Comment 7 Lari Taskula 2016-11-03 12:26:00 UTC
Created attachment 57127 [details] [review]
Bug 17539: Fix Undefined subroutine &C4::Circulation::GetItem error

perl t/db_dependent/Reserves.t

1..72
ok 1 - use C4::Reserves;
ok 2 - CheckReserves Test 1
ok 3 - CheckReserves() include reserve_id in its response
ok 4 - CheckReserves Test 2
ok 5 - CheckReserves Test 3
ok 6 - GetReservesControlBranch returns item home branch when set to ItemHomeLibrary
ok 7 - GetReservesControlBranch returns patron home branch when set to PatronLibrary
ok 8 - Item is correctly waiting
ok 9 - Item is correctly priority 1
ok 10 - Item is correctly priority 2
ok 11 - GetWaiting got only the waiting reserve
ok 12 - GetWaiting got the reserve for the correct borrower
Undefined subroutine &C4::Circulation::GetItem called at /home/ubuntu/kohaclone/C4/Circulation.pm line 1853.

This patch fixes this error. The solution was to use C4::Reserves before C4::Items.
Also due to this edit, change use_ok to require_ok
Comment 8 Lari Taskula 2016-11-03 12:26:21 UTC
Created attachment 57128 [details] [review]
Bug 17539: Fix Already in a transaction error

perl t/db_dependent/Reserves.t

1..72
ok 1 - use C4::Reserves;
ok 2 - CheckReserves Test 1
...
ok 52 - Reserve in waiting status cant be canceled
ok 53 - Reserving an ageRestricted Biblio without a borrower dateofbirth succeeds
DBD::mysql::db begin_work failed: Already in a transaction at
/usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1489.
DBD::mysql::db begin_work failed: Already in a transaction at
/usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1489.
at /usr/share/perl5/DBIx/Class/Schema.pm line 1080.
...
Koha::Object::store('Koha::Patron=HASH(0x9e43b20)') called at
/home/ubuntu/kohaclone/C4/Members.pm line 542
C4::Members::ModMember('borrowernumber', 658535, 'dateofbirth', '2001-11-02')
called at t/db_dependent/Reserves.t line 516

This patch fixes this issue by replacing dbh autocommit=0 with
txn_begin.
Comment 9 Lari Taskula 2016-11-03 12:26:43 UTC
Created attachment 57129 [details] [review]
Bug 17539: Fix failing Reserves.t tests if category enrolmentfee > 0

To test:
1. Set patron category "S" with enrolmentfee 1
2. perl t/db_dependent/Reserves.t
3. Observe following errors:

not ok 59 - Bug 14464 - No fines at beginning
ok 60 - Bug 14464 - 1st reserve correctly created
ok 61 - Bug 14968 - Keep found column from reserve
not ok 62 - Bug 14464 - No fines after cancelling reserve with no charge configured
ok 63 - Bug 14464 - 2nd reserve correctly created
not ok 64 - Bug 14464 - No fines after cancelling reserve with no charge desired
ok 65 - Bug 14464 - 1st reserve correctly created
not ok 66 - Bug 14464 - Fine applied after cancelling reserve with charge desired and configured

4. Apply patch
5. perl t/db_dependent/Reserves.t
6. Observe test pass
Comment 10 Lari Taskula 2016-11-03 12:34:41 UTC
(In reply to Jonathan Druart from comment #4)
> Lari, good catch.
> But still fail for me:
> Tag "" is not a valid tag. at t/db_dependent/Reserves.t line 510.
Strange! For me it seems to be working. Just a guess, could it be issue with caching as warned at lines 57-58, that set the kohafield for line 509:
57: # Somewhat arbitrary field chosen for age restriction unit tests. Must be added to db before the framework is cached
58: $dbh->do("update marc_subfield_structure set kohafield='biblioitems.agerestriction' where tagfield='521' and tagsubfield='a'");
...
509: my ( $ageres_tagid, $ageres_subfieldid ) = GetMarcFromKohaField( "biblioitems.agerestriction" );
510: $record->append_fields(  MARC::Field->new($ageres_tagid, '', '', $ageres_subfieldid => 'PEGI 16')  );

(In reply to Marcel de Rooy from comment #5)
> There is already an use_ok in the Reserves test.
> If you add another use, maybe change use_ok to require_ok ?
> Did the same somewhere else..
I squashed this into the first patch. Thanks!
Comment 11 Marcel de Rooy 2016-11-03 12:48:05 UTC
(In reply to Lari Taskula from comment #10)
> Strange! For me it seems to be working. 
Did not see that one too yesterday..
Comment 12 Tomás Cohen Arazi 2016-11-03 15:05:40 UTC
Created attachment 57134 [details] [review]
Bug 17539: Fix Undefined subroutine &C4::Circulation::GetItem error

perl t/db_dependent/Reserves.t

1..72
ok 1 - use C4::Reserves;
ok 2 - CheckReserves Test 1
ok 3 - CheckReserves() include reserve_id in its response
ok 4 - CheckReserves Test 2
ok 5 - CheckReserves Test 3
ok 6 - GetReservesControlBranch returns item home branch when set to ItemHomeLibrary
ok 7 - GetReservesControlBranch returns patron home branch when set to PatronLibrary
ok 8 - Item is correctly waiting
ok 9 - Item is correctly priority 1
ok 10 - Item is correctly priority 2
ok 11 - GetWaiting got only the waiting reserve
ok 12 - GetWaiting got the reserve for the correct borrower
Undefined subroutine &C4::Circulation::GetItem called at /home/ubuntu/kohaclone/C4/Circulation.pm line 1853.

This patch fixes this error. The solution was to use C4::Reserves before C4::Items.
Also due to this edit, change use_ok to require_ok

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Tomás Cohen Arazi 2016-11-03 15:05:49 UTC
Created attachment 57135 [details] [review]
Bug 17539: Fix Already in a transaction error

perl t/db_dependent/Reserves.t

1..72
ok 1 - use C4::Reserves;
ok 2 - CheckReserves Test 1
...
ok 52 - Reserve in waiting status cant be canceled
ok 53 - Reserving an ageRestricted Biblio without a borrower dateofbirth succeeds
DBD::mysql::db begin_work failed: Already in a transaction at
/usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1489.
DBD::mysql::db begin_work failed: Already in a transaction at
/usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1489.
at /usr/share/perl5/DBIx/Class/Schema.pm line 1080.
...
Koha::Object::store('Koha::Patron=HASH(0x9e43b20)') called at
/home/ubuntu/kohaclone/C4/Members.pm line 542
C4::Members::ModMember('borrowernumber', 658535, 'dateofbirth', '2001-11-02')
called at t/db_dependent/Reserves.t line 516

This patch fixes this issue by replacing dbh autocommit=0 with
txn_begin.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi 2016-11-03 15:05:55 UTC
Created attachment 57136 [details] [review]
Bug 17539: Fix failing Reserves.t tests if category enrolmentfee > 0

To test:
1. Set patron category "S" with enrolmentfee 1
2. perl t/db_dependent/Reserves.t
3. Observe following errors:

not ok 59 - Bug 14464 - No fines at beginning
ok 60 - Bug 14464 - 1st reserve correctly created
ok 61 - Bug 14968 - Keep found column from reserve
not ok 62 - Bug 14464 - No fines after cancelling reserve with no charge configured
ok 63 - Bug 14464 - 2nd reserve correctly created
not ok 64 - Bug 14464 - No fines after cancelling reserve with no charge desired
ok 65 - Bug 14464 - 1st reserve correctly created
not ok 66 - Bug 14464 - Fine applied after cancelling reserve with charge desired and configured

4. Apply patch
5. perl t/db_dependent/Reserves.t
6. Observe test pass

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2016-11-03 15:06:04 UTC
Created attachment 57137 [details] [review]
Bug 17539: (QA followup) Silence warnings

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Marcel de Rooy 2016-11-04 08:03:57 UTC
Created attachment 57151 [details] [review]
Bug 17539: Fix Undefined subroutine &C4::Circulation::GetItem error

perl t/db_dependent/Reserves.t

1..72
ok 1 - use C4::Reserves;
ok 2 - CheckReserves Test 1
ok 3 - CheckReserves() include reserve_id in its response
ok 4 - CheckReserves Test 2
ok 5 - CheckReserves Test 3
ok 6 - GetReservesControlBranch returns item home branch when set to ItemHomeLibrary
ok 7 - GetReservesControlBranch returns patron home branch when set to PatronLibrary
ok 8 - Item is correctly waiting
ok 9 - Item is correctly priority 1
ok 10 - Item is correctly priority 2
ok 11 - GetWaiting got only the waiting reserve
ok 12 - GetWaiting got the reserve for the correct borrower
Undefined subroutine &C4::Circulation::GetItem called at /home/ubuntu/kohaclone/C4/Circulation.pm line 1853.

This patch fixes this error. The solution was to use C4::Reserves before C4::Items.
Also due to this edit, change use_ok to require_ok

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2016-11-04 08:04:01 UTC
Created attachment 57152 [details] [review]
Bug 17539: Fix Already in a transaction error

perl t/db_dependent/Reserves.t

1..72
ok 1 - use C4::Reserves;
ok 2 - CheckReserves Test 1
...
ok 52 - Reserve in waiting status cant be canceled
ok 53 - Reserving an ageRestricted Biblio without a borrower dateofbirth succeeds
DBD::mysql::db begin_work failed: Already in a transaction at
/usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1489.
DBD::mysql::db begin_work failed: Already in a transaction at
/usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1489.
at /usr/share/perl5/DBIx/Class/Schema.pm line 1080.
...
Koha::Object::store('Koha::Patron=HASH(0x9e43b20)') called at
/home/ubuntu/kohaclone/C4/Members.pm line 542
C4::Members::ModMember('borrowernumber', 658535, 'dateofbirth', '2001-11-02')
called at t/db_dependent/Reserves.t line 516

This patch fixes this issue by replacing dbh autocommit=0 with
txn_begin.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2016-11-04 08:04:05 UTC
Created attachment 57153 [details] [review]
Bug 17539: Fix failing Reserves.t tests if category enrolmentfee > 0

To test:
1. Set patron category "S" with enrolmentfee 1
2. perl t/db_dependent/Reserves.t
3. Observe following errors:

not ok 59 - Bug 14464 - No fines at beginning
ok 60 - Bug 14464 - 1st reserve correctly created
ok 61 - Bug 14968 - Keep found column from reserve
not ok 62 - Bug 14464 - No fines after cancelling reserve with no charge configured
ok 63 - Bug 14464 - 2nd reserve correctly created
not ok 64 - Bug 14464 - No fines after cancelling reserve with no charge desired
ok 65 - Bug 14464 - 1st reserve correctly created
not ok 66 - Bug 14464 - Fine applied after cancelling reserve with charge desired and configured

4. Apply patch
5. perl t/db_dependent/Reserves.t
6. Observe test pass

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2016-11-04 08:04:09 UTC
Created attachment 57154 [details] [review]
Bug 17539: (QA followup) Silence warnings

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2016-11-04 08:04:13 UTC
Created attachment 57155 [details] [review]
Bug 17539: [QA Follow-up] Move handle after schema

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Jonathan Druart 2016-11-04 08:39:52 UTC
Ok so I tried it again:
It pass with an usual install (using the interface), but it did not with the new populate_db.pl script.
The bug is somewhere else, sorry!
Comment 22 Jonathan Druart 2016-11-04 08:43:33 UTC
(In reply to Jonathan Druart from comment #21)
> Ok so I tried it again:
> It pass with an usual install (using the interface), but it did not with the
> new populate_db.pl script.
> The bug is somewhere else, sorry!

Argh, actually I cannot recreate with populate_db.pl neither!
Comment 23 Kyle M Hall 2016-11-04 11:13:40 UTC
Pushed to master for 16.11, thanks Lari, Tomas, Marcel!
Comment 24 Jonathan Druart 2016-11-09 10:01:21 UTC
I did a full prove t/db_dependent, and got:

t/db_dependent/Reserves.t ................................ 28/72 C4::Context->userenv not defined! at /home/vagrant/kohaclone/C4/Items.pm line 2267.
Use of uninitialized value $tagno in concatenation (.) or string at /usr/share/perl5/MARC/Field.pm line 83.
Tag "" is not a valid tag. at t/db_dependent/Reserves.t line 526.
# Looks like your test exited with 255 just after 52.
t/db_dependent/Reserves.t ................................ Dubious, test returned 255 (wstat 65280, 0xff00)

Just after, trying to recreate:
prove t/db_dependent/Reserves.t
t/db_dependent/Reserves.t .. 1/72 C4::Context->userenv not defined! at /home/vagrant/kohaclone/C4/Items.pm line 2267.
t/db_dependent/Reserves.t .. ok     
All tests successful.
Files=1, Tests=72,  3 wallclock secs ( 0.02 usr  0.01 sys +  1.42 cusr  0.78 csys =  2.23 CPU)
Result: PASS
Comment 25 Marcel de Rooy 2016-11-09 12:04:32 UTC
(In reply to Jonathan Druart from comment #24)
> I did a full prove t/db_dependent, and got:
> 
> t/db_dependent/Reserves.t ................................ 28/72
> C4::Context->userenv not defined! at /home/vagrant/kohaclone/C4/Items.pm
> line 2267.
> Use of uninitialized value $tagno in concatenation (.) or string at
> /usr/share/perl5/MARC/Field.pm line 83.
> Tag "" is not a valid tag. at t/db_dependent/Reserves.t line 526.
> # Looks like your test exited with 255 just after 52.
> t/db_dependent/Reserves.t ................................ Dubious, test
> returned 255 (wstat 65280, 0xff00)
> 
> Just after, trying to recreate:
> prove t/db_dependent/Reserves.t
> t/db_dependent/Reserves.t .. 1/72 C4::Context->userenv not defined! at
> /home/vagrant/kohaclone/C4/Items.pm line 2267.
> t/db_dependent/Reserves.t .. ok     
> All tests successful.
> Files=1, Tests=72,  3 wallclock secs ( 0.02 usr  0.01 sys +  1.42 cusr  0.78
> csys =  2.23 CPU)
> Result: PASS

What is the connection between these issues and the use of kohadevbox ?
Comment 26 Tomás Cohen Arazi 2016-11-09 12:17:57 UTC
Jenkins doesn't use kohadevbox and shows the same behavior.