Bug 17431

Summary: Fix failing test t/db_dependent/api/v1/holds.t
Product: Koha Reporter: Lari Taskula <lari.taskula>
Component: Web servicesAssignee: Lari Taskula <lari.taskula>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, kyle, martin.renvoize, tomascohen, veron
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 14695    
Bug Blocks: 17565    
Attachments: Bug 17431 - Fix failing test t/db_dependent/api/v1/holds.t
Bug 17431 - Fix failing test t/db_dependent/api/v1/holds.t
Bug 17431: (follow-up) Fix issuingrules
Bug 17431 - Fix failing test t/db_dependent/api/v1/holds.t
Bug 17431: (follow-up) Fix issuingrules
Bug 17431: Create the object we need instead of assuming something exists
Bug 17431: Fix another fail in t/db_dependent/api/v1/holds.t
[SIGNED-OFF] Bug 17431 - Fix failing test t/db_dependent/api/v1/holds.t
[SIGNED-OFF] Bug 17431: (follow-up) Fix issuingrules
[SIGNED-OFF] Bug 17431: Create the object we need instead of assuming something exists
[SIGNED-OFF] Bug 17431: Fix another fail in t/db_dependent/api/v1/holds.t
Bug 17431: Remove use of C4::Items and C4::Biblio
Bug 17431: Remove use of C4::Items and C4::Biblio
Bug 17431: Fix failing test t/db_dependent/api/v1/holds.t
Bug 17431: (follow-up) Fix issuingrules
Bug 17431: Create the object we need instead of assuming something exists
Bug 17431: Remove use of C4::Items and C4::Biblio

Description Lari Taskula 2016-10-12 11:20:11 UTC
$ perl t/db_dependent/api/v1/holds.t
...
not ok 42 - similar match for JSON Pointer "/error"
    #   Failed test 'similar match for JSON Pointer "/error"'
    #   at t/db_dependent/api/v1/holds.t line 291.
    #                   'Reserve cannot be placed. Reason: itemAlreadyOnHold'
    #     doesn't match '(?^u:tooManyReserves)'
    # Looks like you failed 1 test of 42.
not ok 4 - Test endpoints with permission
#   Failed test 'Test endpoints with permission'
#   at t/db_dependent/api/v1/holds.t line 294.
# Looks like you failed 1 test of 4.
Comment 1 Lari Taskula 2016-10-12 11:32:41 UTC
Created attachment 56252 [details] [review]
Bug 17431 - Fix failing test t/db_dependent/api/v1/holds.t

This patch fixed failing test for t/db_dependent/api/v1/holds.t which was broken
after Bug 14695.

$ perl t/db_dependent/api/v1/holds.t
...
not ok 42 - similar match for JSON Pointer "/error"
    #   Failed test 'similar match for JSON Pointer "/error"'
    #   at t/db_dependent/api/v1/holds.t line 291.
    #                   'Reserve cannot be placed. Reason: itemAlreadyOnHold'
    #     doesn't match '(?^u:tooManyReserves)'
    # Looks like you failed 1 test of 42.
not ok 4 - Test endpoints with permission

To test:
1. Run t/db_dependent/api/v1/holds.t and observe it fail.
2. Apply patch.
3. Run the test again and observe it pass.
Comment 2 Marc Véron 2016-10-12 14:11:37 UTC
Created attachment 56288 [details] [review]
Bug 17431 - Fix failing test t/db_dependent/api/v1/holds.t

This patch fixed failing test for t/db_dependent/api/v1/holds.t which was broken
after Bug 14695.

$ perl t/db_dependent/api/v1/holds.t
...
not ok 42 - similar match for JSON Pointer "/error"
    #   Failed test 'similar match for JSON Pointer "/error"'
    #   at t/db_dependent/api/v1/holds.t line 291.
    #                   'Reserve cannot be placed. Reason: itemAlreadyOnHold'
    #     doesn't match '(?^u:tooManyReserves)'
    # Looks like you failed 1 test of 42.
not ok 4 - Test endpoints with permission

To test:
1. Run t/db_dependent/api/v1/holds.t and observe it fail.
2. Apply patch.
3. Run the test again and observe it pass.

Signed-off-by: Marc <veron@veron.ch>
Comment 3 Jonathan Druart 2016-10-19 12:10:14 UTC
The tests still don't pass, Tomas could you have a look?
Comment 4 Lari Taskula 2016-10-19 12:21:00 UTC
Created attachment 56655 [details] [review]
Bug 17431: (follow-up) Fix issuingrules

Issuingrules could had prevented holds.t from passing with error tooManyReserves.
This patch sets issuingrules at the beginning of the test to make sure reserves
are allowed.

To test:
1. Apply patch & Run t/db_dependent/api/v1/holds.t
2. Observe test pass
Comment 5 Lari Taskula 2016-10-19 12:21:22 UTC
Does this patch fix it?
Comment 6 Jonathan Druart 2016-10-19 13:15:26 UTC
Created attachment 56656 [details] [review]
Bug 17431 - Fix failing test t/db_dependent/api/v1/holds.t

This patch fixed failing test for t/db_dependent/api/v1/holds.t which was broken
after Bug 14695.

$ perl t/db_dependent/api/v1/holds.t
...
not ok 42 - similar match for JSON Pointer "/error"
    #   Failed test 'similar match for JSON Pointer "/error"'
    #   at t/db_dependent/api/v1/holds.t line 291.
    #                   'Reserve cannot be placed. Reason: itemAlreadyOnHold'
    #     doesn't match '(?^u:tooManyReserves)'
    # Looks like you failed 1 test of 42.
not ok 4 - Test endpoints with permission

To test:
1. Run t/db_dependent/api/v1/holds.t and observe it fail.
2. Apply patch.
3. Run the test again and observe it pass.

Signed-off-by: Marc <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 7 Jonathan Druart 2016-10-19 13:15:30 UTC
Created attachment 56657 [details] [review]
Bug 17431: (follow-up) Fix issuingrules

Issuingrules could had prevented holds.t from passing with error tooManyReserves.
This patch sets issuingrules at the beginning of the test to make sure reserves
are allowed.

To test:
1. Apply patch & Run t/db_dependent/api/v1/holds.t
2. Observe test pass

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 8 Jonathan Druart 2016-10-19 13:15:34 UTC
Created attachment 56658 [details] [review]
Bug 17431: Create the object we need instead of assuming something exists

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 9 Kyle M Hall 2016-10-21 17:07:11 UTC
t/db_dependent/api/v1/holds.t .. 1/4
    #   Failed test '403 Forbidden'
    #   at t/db_dependent/api/v1/holds.t line 163.
    #          got: '401'
    #     expected: '403'

    #   Failed test '403 Forbidden'
    #   at t/db_dependent/api/v1/holds.t line 167.
    #          got: '401'
    #     expected: '403'

    #   Failed test '403 Forbidden'
    #   at t/db_dependent/api/v1/holds.t line 171.
    #          got: '401'
    #     expected: '403'

    #   Failed test '403 Forbidden'
    #   at t/db_dependent/api/v1/holds.t line 175.
    #          got: '401'
    #     expected: '403'

    #   Failed test '403 Forbidden'
    #   at t/db_dependent/api/v1/holds.t line 179.
    #          got: '401'
    #     expected: '403'
    # Looks like you failed 5 tests of 10.

#   Failed test 'Test endpoints without permission'
#   at t/db_dependent/api/v1/holds.t line 181.

    #   Failed test '201 Created'
    #   at t/db_dependent/api/v1/holds.t line 189.
    #          got: '401'
    #     expected: '201'

    #   Failed test 'has value for JSON Pointer "/reserve_id"'
    #   at t/db_dependent/api/v1/holds.t line 189.

    #   Failed test '200 OK'
    #   at t/db_dependent/api/v1/holds.t line 199.
    #          got: '401'
    #     expected: '200'

    #   Failed test 'exact match for JSON Pointer "/0/borrowernumber"'
    #   at t/db_dependent/api/v1/holds.t line 199.
    #          got: undef
    #     expected: '109'

    #   Failed test 'exact match for JSON Pointer "/0/biblionumber"'
    #   at t/db_dependent/api/v1/holds.t line 199.
    #          got: undef
    #     expected: '570'

    #   Failed test 'exact match for JSON Pointer "/0/itemnumber"'
    #   at t/db_dependent/api/v1/holds.t line 199.
    #          got: undef
    #     expected: '606'

    #   Failed test 'exact match for JSON Pointer "/0/expirationdate"'
    #   at t/db_dependent/api/v1/holds.t line 199.
    #          got: undef
    #     expected: '2016-10-31'

    #   Failed test 'exact match for JSON Pointer "/0/branchcode"'
    #   at t/db_dependent/api/v1/holds.t line 199.
    #          got: undef
    #     expected: 'cZD9z_'
    # Looks like you planned 15 tests but ran 10.
    # Looks like you failed 8 tests of 10 run.

#   Failed test 'Test endpoints without permission, but accessing own object'
#   at t/db_dependent/api/v1/holds.t line 212.
Can't call method "reserve_id" on an undefined value at t/db_dependent/api/v1/holds.t line 204.
# Looks like your test exited with 255 just after 3.
t/db_dependent/api/v1/holds.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 3/4 subtests

Test Summary Report
-------------------
t/db_dependent/api/v1/holds.t (Wstat: 65280 Tests: 3 Failed: 2)
  Failed tests:  2-3
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 4 tests but ran 3.
Files=1, Tests=3,  8 wallclock secs ( 0.04 usr  0.01 sys +  7.27 cusr  0.28 csys =  7.60 CPU)
Result: FAIL
Comment 10 Lari Taskula 2016-10-24 08:20:18 UTC
(In reply to Kyle M Hall from comment #9)
Could there be a pending database update? I could replicate it when I hadn't run updatedatabase.pl, but after it the test passes for me. Koha::REST::V1::authenticate_api_request will give HTTP 401 if check_cookie_auth returns "maintenance". Perhaps 503 would be better.
Comment 11 Lari Taskula 2016-11-07 13:38:36 UTC
Created attachment 57258 [details] [review]
Bug 17431: Fix another fail in t/db_dependent/api/v1/holds.t

perl t/db_dependent/api/v1/holds.t fails with 500

ok 2 - Test endpoints without permission
    # Subtest: Test endpoints without permission, but accessing own object
    1..15
    ok 1 - POST /api/v1/holds
    not ok 2 - 201 Created
    #   Failed test '201 Created'
    #   at t/db_dependent/api/v1/holds.t line 189.
    #          got: '500'
    #     expected: '201'
    not ok 3 - has value for JSON Pointer "/reserve_id"
    #   Failed test 'has value for JSON Pointer "/reserve_id"'
    #   at t/db_dependent/api/v1/holds.t line 189.
    ok 4 - GET /api/v1/holds?borrowernumber=659207
    ok 5 - 200 OK
    not ok 6 - exact match for JSON Pointer "/0/borrowernumber"
    #   Failed test 'exact match for JSON Pointer "/0/borrowernumber"'
    #   at t/db_dependent/api/v1/holds.t line 199.
    #          got: undef
    #     expected: '659207'

To test:
1. Run t/db_dependent/api/v1/holds.t
2. Observe above failure
3. Apply patch
4. Observe test pass
Comment 12 Lari Taskula 2016-11-07 13:46:35 UTC
I'm not completely sure why changing the order of "use" in the test fixes some issues like being unable to call GetItem in C4::Reserves. That seemed to be the issue for returning 500 in POST /api/v1/holds. I came across with similar issue in Reserves.t. Does the fix make sense and can somebody explain what's going on?
Comment 13 Josef Moravec 2016-11-07 21:00:26 UTC
Created attachment 57282 [details] [review]
[SIGNED-OFF] Bug 17431 - Fix failing test t/db_dependent/api/v1/holds.t

This patch fixed failing test for t/db_dependent/api/v1/holds.t which was broken
after Bug 14695.

$ perl t/db_dependent/api/v1/holds.t
...
not ok 42 - similar match for JSON Pointer "/error"
    #   Failed test 'similar match for JSON Pointer "/error"'
    #   at t/db_dependent/api/v1/holds.t line 291.
    #                   'Reserve cannot be placed. Reason: itemAlreadyOnHold'
    #     doesn't match '(?^u:tooManyReserves)'
    # Looks like you failed 1 test of 42.
not ok 4 - Test endpoints with permission

To test:
1. Run t/db_dependent/api/v1/holds.t and observe it fail.
2. Apply patch.
3. Run the test again and observe it pass.

Signed-off-by: Marc <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 14 Josef Moravec 2016-11-07 21:00:32 UTC
Created attachment 57283 [details] [review]
[SIGNED-OFF] Bug 17431: (follow-up) Fix issuingrules

Issuingrules could had prevented holds.t from passing with error tooManyReserves.
This patch sets issuingrules at the beginning of the test to make sure reserves
are allowed.

To test:
1. Apply patch & Run t/db_dependent/api/v1/holds.t
2. Observe test pass

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 15 Josef Moravec 2016-11-07 21:00:38 UTC
Created attachment 57284 [details] [review]
[SIGNED-OFF] Bug 17431: Create the object we need instead of assuming something exists

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 16 Josef Moravec 2016-11-07 21:00:43 UTC
Created attachment 57285 [details] [review]
[SIGNED-OFF] Bug 17431: Fix another fail in t/db_dependent/api/v1/holds.t

perl t/db_dependent/api/v1/holds.t fails with 500

ok 2 - Test endpoints without permission
    # Subtest: Test endpoints without permission, but accessing own object
    1..15
    ok 1 - POST /api/v1/holds
    not ok 2 - 201 Created
    #   Failed test '201 Created'
    #   at t/db_dependent/api/v1/holds.t line 189.
    #          got: '500'
    #     expected: '201'
    not ok 3 - has value for JSON Pointer "/reserve_id"
    #   Failed test 'has value for JSON Pointer "/reserve_id"'
    #   at t/db_dependent/api/v1/holds.t line 189.
    ok 4 - GET /api/v1/holds?borrowernumber=659207
    ok 5 - 200 OK
    not ok 6 - exact match for JSON Pointer "/0/borrowernumber"
    #   Failed test 'exact match for JSON Pointer "/0/borrowernumber"'
    #   at t/db_dependent/api/v1/holds.t line 199.
    #          got: undef
    #     expected: '659207'

To test:
1. Run t/db_dependent/api/v1/holds.t
2. Observe above failure
3. Apply patch
4. Observe test pass

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 17 Jonathan Druart 2016-11-08 09:13:47 UTC
(In reply to Lari Taskula from comment #12)
> I'm not completely sure why changing the order of "use" in the test fixes
> some issues like being unable to call GetItem in C4::Reserves. That seemed
> to be the issue for returning 500 in POST /api/v1/holds. I came across with
> similar issue in Reserves.t. Does the fix make sense and can somebody
> explain what's going on?

I really would like to understand what's happening here, there is something hidden we need to catch.

Just a thought:

diff --git a/Koha/REST/V1/Hold.pm b/Koha/REST/V1/Hold.pm
index 7f0b1ff..ae1cd4d 100644
--- a/Koha/REST/V1/Hold.pm
+++ b/Koha/REST/V1/Hold.pm
@@ -77,6 +77,7 @@ sub add {
 
     my $biblio = C4::Biblio::GetBiblio($biblionumber);
 
+    warn $can_reserve;
     my $can_reserve =
       $itemnumber
       ? CanItemBeReserved( $borrowernumber, $itemnumber )

Make the tests fail again.
I also see the following warning (several):
  item-level_itypes set but no itemtype set for item (1064) at /home/vagrant/kohaclone/Koha/Schema/Result/Item.pm line 698.
when the tests pass, but not when they fail.

Could someone else have a look and try to understand what's happened here?
Comment 18 Martin Renvoize 2016-11-08 09:39:46 UTC
I'm generally not a big fan of bare 'use' statements for this exact reason.  It's generally good practice to state which routines your wanting to import.. an order change like this feels like a sub is getting redefined silently and so it's likely stuff will just start randomly failing again further down the line.

Could we work out where the actual issue lies and either specify the imports so we're clear about exactly which routine from where we're testing or rename the routine to be unique across the codebase to remove ambiguity?
Comment 19 Jonathan Druart 2016-11-17 07:55:24 UTC
Created attachment 57584 [details] [review]
Bug 17431: Remove use of C4::Items and C4::Biblio
Comment 20 Jonathan Druart 2016-11-17 07:56:38 UTC
This last patch removes the 2 uses of C4::Items and C4::Biblio, please move forward!
Comment 21 Jonathan Druart 2016-11-17 07:58:04 UTC
Created attachment 57585 [details] [review]
Bug 17431: Remove use of C4::Items and C4::Biblio
Comment 22 Tomás Cohen Arazi 2016-11-17 14:28:51 UTC
Created attachment 57600 [details] [review]
Bug 17431: Fix failing test t/db_dependent/api/v1/holds.t

This patch fixed failing test for t/db_dependent/api/v1/holds.t which was broken
after Bug 14695.

$ perl t/db_dependent/api/v1/holds.t
...
not ok 42 - similar match for JSON Pointer "/error"
    #   Failed test 'similar match for JSON Pointer "/error"'
    #   at t/db_dependent/api/v1/holds.t line 291.
    #                   'Reserve cannot be placed. Reason: itemAlreadyOnHold'
    #     doesn't match '(?^u:tooManyReserves)'
    # Looks like you failed 1 test of 42.
not ok 4 - Test endpoints with permission

To test:
1. Run t/db_dependent/api/v1/holds.t and observe it fail.
2. Apply patch.
3. Run the test again and observe it pass.

Signed-off-by: Marc <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi 2016-11-17 14:29:03 UTC
Created attachment 57601 [details] [review]
Bug 17431: (follow-up) Fix issuingrules

Issuingrules could had prevented holds.t from passing with error tooManyReserves.
This patch sets issuingrules at the beginning of the test to make sure reserves
are allowed.

To test:
1. Apply patch & Run t/db_dependent/api/v1/holds.t
2. Observe test pass

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Tomás Cohen Arazi 2016-11-17 14:29:12 UTC
Created attachment 57602 [details] [review]
Bug 17431: Create the object we need instead of assuming something exists

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 25 Tomás Cohen Arazi 2016-11-17 14:29:23 UTC
Created attachment 57603 [details] [review]
Bug 17431: Remove use of C4::Items and C4::Biblio

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 26 Kyle M Hall 2016-11-17 16:23:32 UTC
Pushed to master for 16.11, thanks Lari, Jonathan!