Bug 31676 - Make db_dependent/Circulation.t tests more robust
Summary: Make db_dependent/Circulation.t tests more robust
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Gustafsson
QA Contact: Testopia
URL:
Keywords:
: 31493 (view as bug list)
Depends on:
Blocks: 31735
  Show dependency treegraph
 
Reported: 2022-10-04 17:22 UTC by David Gustafsson
Modified: 2023-12-28 20:43 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00


Attachments
Bug 31676: Fix db_dependent/Circulation.t tests (18.80 KB, patch)
2022-10-04 17:29 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 31676: Fix db_dependent/Circulation.t tests (19.36 KB, patch)
2022-10-06 14:38 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 31676: Fix db_dependent/Circulation.t tests (19.31 KB, patch)
2022-10-06 14:47 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 31676: Fix db_dependent/Circulation.t tests (18.81 KB, patch)
2022-10-06 14:49 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 31676: Fix db_dependent/Circulation.t tests (18.86 KB, patch)
2022-10-11 22:34 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 31676: Fix db_dependent/Circulation.t tests (18.92 KB, patch)
2022-10-22 10:24 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Gustafsson 2022-10-04 17:22:29 UTC
A lot of tests in db_dependent/Circulation.t are currently failing, many seems to have done so for quite a while so it's a bit puzzling how this has gone under radar for such a long time.

If any of the changes are unclear I'm happy to expand more on the different addressed issues.

A short summary of the changes:

- The patron attribute autorenew_checkouts was added a while ago, patrons for some tests needed this set to preserve previous behavior

- OverduesBlockRenewing-test relied on a state that was mutated by new tests, these have been moved to a place where the state is as was before this regression.

- The test "AddRenewal left both fines" broke when account line statuses where refactored (probably), there should be one one fine

- Fixed a test which expects 'auto_to_soon' error. CanBookBeRenewed will not return this status unless the $cron parameter is set to true

- "Incremented fee tests" builds an item with missing "library" parameter since treating the library object as a hashref. This cases a circulation-rule not match and serveral tests fail as a result.

- The "AddIssue records staff who checked out item if appropriate" subtest treats some objects as hashrefs when retrieving properties, and also checks the wrong issue objects in the last test.
Comment 1 David Gustafsson 2022-10-04 17:29:27 UTC
Created attachment 141330 [details] [review]
Bug 31676: Fix db_dependent/Circulation.t tests

To test:
1) Run tests in t/db_dependent/Circulation.t and verify
   that fails (the script should actually crash before all
   can be run).
2) Apply the patch
3) Verifiy that all tests now pass

Sponsored-by: Gothenburg University Library
Comment 2 Katrin Fischer 2022-10-04 20:48:07 UTC
Hm, I ran these tests just yesterday without fail, I still see it in my bash history:

prove t/db_dependent/Circulation.t t/db_dependent/Circulation/*

Could it depend on having an untouched sample database to start with? But it is a bit odd.
Comment 3 David Gustafsson 2022-10-05 14:34:29 UTC
Ok, that's strange. I think most of the test failing for me could pass with certain sysprefs set, but at least the one where two account lines are expected I just can't figure out how it would ever do so.

In this test an item is issued with due date five weeks ago, UpdateFine is then run which creates an account line with type "OVERDUE" and status "UNRETURNED" (line 722 in current master). AddRenewal is then called twice for this item in turn calling _CalculateAndUpdateFine which checks for a fine calling CalcFine and then possibly calling UpdateFine if got a fine amount.

In the first call to AddRenewal the due date is five weeks in the past, so CalcFine will return a fine. UpdateFine is then called which will fetch the account line and adjust the amount (to the same value as before), then calling _FixOverduesOnReturn which sets the account line status to "RENEWED". Due date for the issue is then set to a date in the future.

In the second call CalcFine will not return a fine since due date is in the future thus UpdateFine is not called (which I have also verified in my instance).

I just can't figure out how UpdateFine could ever add more than one account line in this test case. I also don't see any sysprefs or other state that is not already mocked which could effect AddRenewal behavior. Obviously since the tests work for you there should be such a case, but personally I can't figure out how to reproduce it. I also tried resetting all sysprefs to default values but this test still fails. From what I can see circulation rules are also cleared before running the tests so existing ones should not effect the results. The docker setup currently running koha on makes it very cumbersome to completely reset the koha instance to a completely clean installation, but if I had access to an instance where the tests do indeed pass it would probably be trial to find out what is causing this issue.
Comment 4 David Gustafsson 2022-10-05 14:35:53 UTC
*trivial
Comment 5 David Gustafsson 2022-10-05 15:18:53 UTC
Now I got the test to work, the syspref cache wasn't probably cleared when ran it the first time. Tests fail for me since the "RenewalPeriodBase" syspref was set to "now". The default value is "date_due", so when renewing the item the new due date is still in the past and will incur fees also on the next renewal (even if the previously generated fee was calculated using the whole period). This is pretty weird behavior and perhaps should be looked over, but at least found the culprit and the tests should set this syspref to one of the options (personally would prefer "now" sisnce the "date_due"-behavior is pretty confusing.
Comment 6 David Gustafsson 2022-10-05 15:27:57 UTC
Setting RenewalPeriodBase to "now" also explains why the 'OverduesBlockRenewing'-tests failed for me, since when the period is "date_due" the item is still overdue even though it was renewed twice. So if using "now" theses tests has to be moved as in the patch.
Comment 7 David Gustafsson 2022-10-05 17:23:45 UTC
To make the 'Incremented fee tests' fail set CircControl to "ItemHomeLibrary" (which is the default) and HomeOrHoldingBranch to "homebranch" (default is "holdingbranch"). The test will now fail since the item does not have the correct homebranch set (since is set using $library->{branchcode} where library is an object).

The reason why the tests pass anyway despite missing both homebranch and holdingbranch is that holdingbranch is set when one of the tests calls AddIssue which will in turn set the expected holdingbranch from userenv ($item_object->holdingbranch(C4::Context->userenv->{'branch'}));

The $item will then have the expected homebranch (but not holdingbranch) for the rest of the tests. If the item is passed the correct branch value to begin with all combinations of the above settings will work.
Comment 8 David Gustafsson 2022-10-05 17:26:41 UTC
Will update the testplan (probably tomorrow) so includes how to reproduce the test-bugs.
Comment 9 David Gustafsson 2022-10-06 14:38:09 UTC
Created attachment 141439 [details] [review]
Bug 31676: Fix db_dependent/Circulation.t tests

To test:
1) Set "RenewalPeriodBase" syspref to "the current date",
   "CircControl" to "the library the item is from." and
   "HomeOrHoldingBranch" to "the item's home library (homebranch)."
2) Run tests in t/db_dependent/Circulation.t and verify
   that fails (the script should actually crash before all
   can be run).
3) Apply the patch
4) Verifiy that all tests now pass

Sponsored-by: Gothenburg University Library
Comment 10 David Gustafsson 2022-10-06 14:39:24 UTC
Now updated the test plan so the errors are reproducible.
Comment 11 David Gustafsson 2022-10-06 14:47:39 UTC
Created attachment 141463 [details] [review]
Bug 31676: Fix db_dependent/Circulation.t tests

To test:
1) Set "RenewalPeriodBase" syspref to "the current date",
   "CircControl" to "the library the item is from." and
   "HomeOrHoldingBranch" to "the item's home library (homebranch)."
2) Run tests in t/db_dependent/Circulation.t and verify
   that fails (the script should actually crash before all
   can be run).
3) Apply the patch
4) Verifiy that all tests now pass

Sponsored-by: Gothenburg University Library
Comment 12 David Gustafsson 2022-10-06 14:49:07 UTC
Created attachment 141464 [details] [review]
Bug 31676: Fix db_dependent/Circulation.t tests

To test:
1) Set "RenewalPeriodBase" syspref to "the current date",
   "CircControl" to "the library the item is from." and
   "HomeOrHoldingBranch" to "the item's home library (homebranch)."
2) Run tests in t/db_dependent/Circulation.t and verify
   that fails (the script should actually crash before all
   can be run).
3) Apply the patch
4) Verifiy that all tests now pass

Sponsored-by: Gothenburg University Library
Comment 13 David Gustafsson 2022-10-06 14:52:05 UTC
Reverted some of the changes in the patch which found wasn't actually necessary for tests to succeed.
Comment 14 Lucas Gass 2022-10-11 22:34:12 UTC
Created attachment 141625 [details] [review]
Bug 31676: Fix db_dependent/Circulation.t tests

To test:
1) Set "RenewalPeriodBase" syspref to "the current date",
   "CircControl" to "the library the item is from." and
   "HomeOrHoldingBranch" to "the item's home library (homebranch)."
2) Run tests in t/db_dependent/Circulation.t and verify
   that fails (the script should actually crash before all
   can be run).
3) Apply the patch
4) Verifiy that all tests now pass

Sponsored-by: Gothenburg University Library

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 15 Katrin Fischer 2022-10-22 10:23:36 UTC
This makes the tests a little more robust/less dependent on the default setup. Recently I noticed that most of the time they would only pass on a new db/reset_all database for me, so hopeful that will help with that :)
Comment 16 Katrin Fischer 2022-10-22 10:24:12 UTC
Created attachment 142457 [details] [review]
Bug 31676: Fix db_dependent/Circulation.t tests

To test:
1) Set "RenewalPeriodBase" syspref to "the current date",
   "CircControl" to "the library the item is from." and
   "HomeOrHoldingBranch" to "the item's home library (homebranch)."
2) Run tests in t/db_dependent/Circulation.t and verify
   that fails (the script should actually crash before all
   can be run).
3) Apply the patch
4) Verifiy that all tests now pass

Sponsored-by: Gothenburg University Library

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 17 Tomás Cohen Arazi 2022-10-24 14:11:33 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 18 Lucas Gass 2022-12-05 21:37:40 UTC
Does not apply cleanly to 22.05.x, no backport
Comment 19 Jonathan Druart 2023-05-04 11:51:41 UTC
*** Bug 31493 has been marked as a duplicate of this bug. ***