Bug 26405

Summary: Circulation.t fails on 'AddRenewal left both fines'
Product: Koha Reporter: Emmi Takkinen <emmi.takkinen>
Component: Test SuiteAssignee: Emmi Takkinen <emmi.takkinen>
Status: CLOSED FIXED QA Contact: Kyle M Hall <kyle>
Severity: minor    
Priority: P5 - low CC: andrewfh, fridolin.somers, jonathan.druart, kyle, tomascohen, victor
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
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'
Bug 26405: Prevent Circulation.t failing on 'AddRenewal left both fines'
Bug 26405: Prevent Circulation.t failing on 'AddRenewal left both fines'

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.