Summary: | ArticleRequests.t fails on existing requests | ||
---|---|---|---|
Product: | Koha | Reporter: | Marcel de Rooy <m.de.rooy> |
Component: | Test Suite | Assignee: | Jonathan Druart <jonathan.druart> |
Status: | CLOSED FIXED | QA Contact: | Marcel de Rooy <m.de.rooy> |
Severity: | normal | ||
Priority: | P5 - low | CC: | fridolin.somers, jonathan.druart, martin.renvoize, nick |
Version: | master | ||
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: | 18403 | ||
Bug Blocks: | |||
Attachments: |
Bug 20866: Fix ArticleRequests.t when table contains records
[ALTERNATIVE PATH] Bug 20866: Make ArticleRequests.t pass even if table is not empty Bug 20866: Make ArticleRequests.t pass even if table is not empty Bug 20866: Make ArticleRequests.t pass even if table is not empty Bug 20866: (QA follow-up) Use build_object and remove two tests |
Description
Marcel de Rooy
2018-06-04 08:25:25 UTC
Created attachment 75750 [details] [review] Bug 20866: Fix ArticleRequests.t when table contains records Subtest search_limited fails. Assumptions in that code may not be true. Simple fix for now: clear the table at the start. Test plan: [1] Create an article request. [2] Run t/db_dependent/ArticleRequests.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signing off too: trivial fix There are some problems hidden your patch actually. Created attachment 75784 [details] [review] [ALTERNATIVE PATH] Bug 20866: Make ArticleRequests.t pass even if table is not empty Previous patch from Marcel removed the data from article_requests, I do not think it is a good idea to remove existing data, it could hide bugs. This patch moves the count inside the subtest block in order to have the correct value when these tests are executed. Previous code was broken: Koha::Patron->store does not generate a userid, and ->search_limited did not work correctly (Koha::Patron->has_permission returned early because of the non-existing userid, 'return unless $self->userid;'); Test plan: [1] Create an article request. [2] Run t/db_dependent/ArticleRequests.t Created attachment 75791 [details] [review] Bug 20866: Make ArticleRequests.t pass even if table is not empty Previous patch from Marcel removed the data from article_requests, I do not think it is a good idea to remove existing data, it could hide bugs. This patch moves the count inside the subtest block in order to have the correct value when these tests are executed. Previous code was broken: Koha::Patron->store does not generate a userid, and ->search_limited did not work correctly (Koha::Patron->has_permission returned early because of the non-existing userid, 'return unless $self->userid;'); Test plan: [1] Create an article request. [2] Run t/db_dependent/ArticleRequests.t Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 75813 [details] [review] Bug 20866: Make ArticleRequests.t pass even if table is not empty Previous patch from Marcel removed the data from article_requests, I do not think it is a good idea to remove existing data, it could hide bugs. This patch moves the count inside the subtest block in order to have the correct value when these tests are executed. Previous code was broken: Koha::Patron->store does not generate a userid, and ->search_limited did not work correctly (Koha::Patron->has_permission returned early because of the non-existing userid, 'return unless $self->userid;'); Test plan: [1] Create an article request. [2] Run t/db_dependent/ArticleRequests.t Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 75814 [details] [review] Bug 20866: (QA follow-up) Use build_object and remove two tests The unique constraint on userid is handled in TestBuilder. So let's use it. The two tests if count==$count do not make much sense anymore when we call ->count a few lines before. The check with search_limited is enough. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Pushed to master for 18.11, awesome work all! This patch has been pushed to 18.05.x and will be in 18.05.01 Depends on Bug 18403 not in 17.11.x |