Tests fail in a weird way: kohadev-koha@7369d4443675:/kohadevbox/koha$ prove t/db_dependent/api/v1/items.t t/db_dependent/api/v1/items.t .. # Inactivity timeout # Failed test 'GET //RTrSH14TLmPlTAsO1RAR9x8MhJtMO7XJS5IKOwBKfwcFnpzHSGSEPM7k:thePassword123@/api/v1/items' # at t/db_dependent/api/v1/items.t line 75. # Failed test 'SWAGGER3.2.2' # at t/db_dependent/api/v1/items.t line 75. # got: undef # expected: '200' # Looks like you planned 12 tests but ran 5. # Looks like you failed 2 tests of 5 run. t/db_dependent/api/v1/items.t .. 1/2 # Failed test 'list() tests' # at t/db_dependent/api/v1/items.t line 95. Can't use an undefined value as an ARRAY reference at t/db_dependent/api/v1/items.t line 79. # Looks like your test exited with 255 just after 1. t/db_dependent/api/v1/items.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 2/2 subtests It happens almost randomly (I reproduced the first time I started koha-testing-docker with the master-bionic image, but then it wouldn't reproduce).
Still, but without any info: koha_1 | [17:33:12] t/db_dependent/api/v1/items.t ........................................... koha_1 | Failed 2/2 subtests
75 $t->get_ok( "//$userid:$password@/api/v1/items" ) 76 ->status_is( 200, 'SWAGGER3.2.2' ); This test is the slower when I run it locally, and You got "Inactivity timeout" when it failed. It there any kinds of timeout value we could extend? That would explain why it fails randomly.
I do recreate the issue with running several time the tests: for i in {1..6}; do timeout 60 prove -v t/db_dependent/api/v1/items.t& done Make sure you have the timeout, or the whole thing will freeze!
I have tried to extend Mojo timeout https://mojolicious.org/perldoc/Mojolicious/Guides/FAQ#What-does-Inactivity-timeout-mean export MOJO_INACTIVITY_TIMEOUT=120 for i in {1..6}; do timeout 120 prove -v t/db_dependent/api/v1/items.t& done I do no longer get the tests failing, but they all get stuck at this test.
Not directly related, but I noticed that we return ALL the items from the DB, and it is what the tests expect: 75 $t->get_ok( "//$userid:$password@/api/v1/items" ) 76 ->status_is( 200, 'SWAGGER3.2.2' ); 77 78 my $items_count = Koha::Items->search->count; 79 my $response_count = scalar @{ $t->tx->res->json }; 80 81 is( $items_count, $response_count, 'The API returns all the items' ); Wondering if this is correct. If not please open a separate bug report.
Tomas, I tried to remove the changes made by bug 24700 but it did not work. However, this worked (it is not a fix, but I wanted to confirm it came from there): diff --git a/Koha/REST/Plugin/Objects.pm b/Koha/REST/Plugin/Objects.pm index 81cc97633c..430518251c 100644 --- a/Koha/REST/Plugin/Objects.pm +++ b/Koha/REST/Plugin/Objects.pm @@ -116,6 +116,7 @@ sub register { $filtered_params = $c->merge_q_params( $filtered_params, $query_params, $result_set ); } # Perform search + $attributes->{page} = 1; my $objects = $result_set->search( $filtered_params, $attributes ); if ($objects->is_paged) { diff --git a/t/db_dependent/api/v1/items.t b/t/db_dependent/api/v1/items.t index 9eb623b96c..8b141ed781 100644 --- a/t/db_dependent/api/v1/items.t +++ b/t/db_dependent/api/v1/items.t @@ -78,7 +78,7 @@ subtest 'list() tests' => sub { my $items_count = Koha::Items->search->count; my $response_count = scalar @{ $t->tx->res->json }; - is( $items_count, $response_count, 'The API returns all the items' ); + is( 10, $response_count, 'The API returns all the items' ); $t->get_ok( "//$userid:$password@/api/v1/items?external_id=" . $item->barcode ) ->status_is(200)
Created attachment 105163 [details] [review] Bug 24229: Avoid fetching ALL items in test I'm not sure what we need to test here, besides we get results. I'm pretty sure we shouldn't allow a plain GET return all objects because it could cause a DOS very easily. But it certainly belongs to a separate bug/discussion. In this case, I put a constraint on the items domain (by using _per_page=10 in the query params) so it won't happen that under certain conditions the user agent used by Test::Mojo time outs. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Jonathan Druart from comment #4) > I have tried to extend Mojo timeout > https://mojolicious.org/perldoc/Mojolicious/Guides/FAQ#What-does-Inactivity- > timeout-mean > > export MOJO_INACTIVITY_TIMEOUT=120 > for i in {1..6}; do timeout 120 prove -v t/db_dependent/api/v1/items.t& done > > I do no longer get the tests failing, but they all get stuck at this test. I reproduced the problem easily with your loop. My patch fixes that.
(In reply to Jonathan Druart from comment #5) > Not directly related, but I noticed that we return ALL the items from the > DB, and it is what the tests expect: > > 75 $t->get_ok( "//$userid:$password@/api/v1/items" ) > 76 ->status_is( 200, 'SWAGGER3.2.2' ); > 77 > 78 my $items_count = Koha::Items->search->count; > 79 my $response_count = scalar @{ $t->tx->res->json }; > 80 > 81 is( $items_count, $response_count, 'The API returns all the items' ); > > Wondering if this is correct. If not please open a separate bug report. Tomas, can you answer this question please? As I said yesterday during the dev meeting, it would mean that one could call 6 times the GET /items route to kick the server down?
(In reply to Jonathan Druart from comment #9) > (In reply to Jonathan Druart from comment #5) > > Not directly related, but I noticed that we return ALL the items from the > > DB, and it is what the tests expect: > > > > 75 $t->get_ok( "//$userid:$password@/api/v1/items" ) > > 76 ->status_is( 200, 'SWAGGER3.2.2' ); > > 77 > > 78 my $items_count = Koha::Items->search->count; > > 79 my $response_count = scalar @{ $t->tx->res->json }; > > 80 > > 81 is( $items_count, $response_count, 'The API returns all the items' ); > > > > Wondering if this is correct. If not please open a separate bug report. > > Tomas, can you answer this question please? > > As I said yesterday during the dev meeting, it would mean that one could > call 6 times the GET /items route to kick the server down? I think I did on my commit message, Jonathan. I'll rephrase. Test::Mojo creates a user agent that is used internally. This user agent has a low timeout. Loading the V1.pm app in Test::Mojo is not the same as loading it with Plack and I'm not sure about the internals. This is not different than running a badly written report that takes too much. It might keep a starman worker busy for too long, etc. I think we need to force pagination and at some point I was sure we did.
(In reply to Tomás Cohen Arazi from comment #10) > I think we need to force pagination and at some point I was sure we did. Can you open a bug report for that please?
Created attachment 105182 [details] [review] Bug 24229: Avoid fetching ALL items in test I'm not sure what we need to test here, besides we get results. I'm pretty sure we shouldn't allow a plain GET return all objects because it could cause a DOS very easily. But it certainly belongs to a separate bug/discussion. In this case, I put a constraint on the items domain (by using _per_page=10 in the query params) so it won't happen that under certain conditions the user agent used by Test::Mojo time outs. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 105193 [details] [review] Bug 24229: Avoid fetching ALL items in test I'm not sure what we need to test here, besides we get results. I'm pretty sure we shouldn't allow a plain GET return all objects because it could cause a DOS very easily. But it certainly belongs to a separate bug/discussion. In this case, I put a constraint on the items domain (by using _per_page=10 in the query params) so it won't happen that under certain conditions the user agent used by Test::Mojo time outs. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
(In reply to Jonathan Druart from comment #11) > (In reply to Tomás Cohen Arazi from comment #10) > > I think we need to force pagination and at some point I was sure we did. > > Can you open a bug report for that please? Tomas, Joubu said, I should tell you to file that bug ;)
(In reply to Katrin Fischer from comment #14) > (In reply to Jonathan Druart from comment #11) > > (In reply to Tomás Cohen Arazi from comment #10) > > > I think we need to force pagination and at some point I was sure we did. > > > > Can you open a bug report for that please? > > Tomas, Joubu said, I should tell you to file that bug ;) Done, bug 25570.
Tomas, do we need this if we have bug 25570?
Nice work everyone! Pushed to master for 20.05
backported to 19.11.x for 19.11.07
Won't backport to 19.05.x, listing items didn't existing back then. https://gitlab.com/koha-community/Koha/-/blob/19.05.x/api/v1/swagger/paths/items.json https://gitlab.com/koha-community/Koha/-/blob/19.11.x/api/v1/swagger/paths/items.json Hence the tests failing with 404 errors ^^"