Bug 26405 - Circulation.t fails on 'AddRenewal left both fines'
Summary: Circulation.t fails on 'AddRenewal left both fines'
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low minor (vote)
Assignee: Emmi Takkinen
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-09-08 12:58 UTC by Emmi Takkinen
Modified: 2021-05-24 18:57 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.06,20.05.12


Attachments
Bug 26405: Prevent Circulation.t failing on 'AddRenewal left both fines' (1.34 KB, patch)
2020-12-03 08:46 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 26405: Prevent Circulation.t failing on 'AddRenewal left both fines' (1.39 KB, patch)
2021-03-16 17:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26405: Prevent Circulation.t failing on 'AddRenewal left both fines' (1.46 KB, patch)
2021-04-23 16:18 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Emmi Takkinen 2020-09-08 12:58:33 UTC
Running prove t/db_dependent/Circulation.t, I receive following error:

t/db_dependent/Circulation.t .. 1/48 
    #   Failed test 'AddRenewal left both fines'
    #   at t/db_dependent/Circulation.t line 591.
    #          got: '1'
    #     expected: '2'
    # Looks like you planned 83 tests but ran 28.
    # Looks like you failed 1 test of 28 run.
t/db_dependent/Circulation.t .. 15/48 
#   Failed test 'CanBookBeRenewed tests'
#   at t/db_dependent/Circulation.t line 1255.
Can't call method "status" on an undefined value at t/db_dependent/Circulation.t line 593.

Adding t::lib::Mocks::mock_preference('CalculateFinesOnReturn', 1) solved problem since bug 23051 added following to Circulation.pm: 

if ( !$skipfinecalc && C4::Context->preference('CalculateFinesOnReturn') ) {
            _CalculateAndUpdateFine( { issue => $issue, item => $item_unblessed, borrower => $patron_unblessed } );
        }

However I'm unsure if this is appropriate solution or are those tests invalid at this point.
Comment 1 Jonathan Druart 2020-12-02 13:43:42 UTC
Yes, we could mock the pref. Can you provide a patch please?
Comment 2 Emmi Takkinen 2020-12-03 08:46:06 UTC
Created attachment 114135 [details] [review]
Bug 26405: Prevent Circulation.t failing on 'AddRenewal left both fines'

Bug 23051 added option to skip calculating fines even when
"CalculateFinesOnReturn" is enabled. Due this Circulation.t
fails on  test  "AddRenewal left both fines" if
"CalculateFinesOnReturn"  is disabled on ones system.
Mocking syspref while testing fixes failig tests.

To test:
1. Disable "CalculateFinesOnReturn".
2. Run prove t/db_dependent/Circulation.t
=> Tests fail
3. Apply patch.
4. Run prove t/db_dependent/Circulation.t again
=> Tests are succesful
Comment 3 Tomás Cohen Arazi 2021-03-16 17:32:32 UTC
Created attachment 118342 [details] [review]
Bug 26405: Prevent Circulation.t failing on 'AddRenewal left both fines'

Bug 23051 added option to skip calculating fines even when
"CalculateFinesOnReturn" is enabled. Due this Circulation.t
fails on  test  "AddRenewal left both fines" if
"CalculateFinesOnReturn"  is disabled on ones system.
Mocking syspref while testing fixes failig tests.

To test:
1. Disable "CalculateFinesOnReturn".
2. Run prove t/db_dependent/Circulation.t
=> Tests fail
3. Apply patch.
4. Run prove t/db_dependent/Circulation.t again
=> Tests are succesful

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Tomás Cohen Arazi 2021-03-16 17:35:25 UTC
Good catch. Those tests do not cover the case in which the syspref is set. But I believe this is tested in t/db_dependent/Circulation/Returns.t (somehow).
Comment 5 Kyle M Hall 2021-04-23 16:18:17 UTC
Created attachment 120122 [details] [review]
Bug 26405: Prevent Circulation.t failing on 'AddRenewal left both fines'

Bug 23051 added option to skip calculating fines even when
"CalculateFinesOnReturn" is enabled. Due this Circulation.t
fails on  test  "AddRenewal left both fines" if
"CalculateFinesOnReturn"  is disabled on ones system.
Mocking syspref while testing fixes failig tests.

To test:
1. Disable "CalculateFinesOnReturn".
2. Run prove t/db_dependent/Circulation.t
=> Tests fail
3. Apply patch.
4. Run prove t/db_dependent/Circulation.t again
=> Tests are succesful

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 6 Jonathan Druart 2021-04-27 14:25:27 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 7 Fridolin Somers 2021-04-30 08:54:24 UTC
Pushed to 20.11.x for 20.11.06
Comment 8 Andrew Fuerste-Henry 2021-05-24 18:51:00 UTC
Pushed to 20.05.x for 20.05.12
Comment 9 Victor Grousset/tuxayo 2021-05-24 18:57:46 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.