$ 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.
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.
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>
The tests still don't pass, Tomas could you have a look?
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
Does this patch fix it?
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>
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>
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>
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
(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.
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
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?
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>
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>
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>
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>
(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?
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?
Created attachment 57584 [details] [review] Bug 17431: Remove use of C4::Items and C4::Biblio
This last patch removes the 2 uses of C4::Items and C4::Biblio, please move forward!
Created attachment 57585 [details] [review] Bug 17431: Remove use of C4::Items and C4::Biblio
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>
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>
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>
Created attachment 57603 [details] [review] Bug 17431: Remove use of C4::Items and C4::Biblio Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed to master for 16.11, thanks Lari, Jonathan!