Bug 18802

Summary: Circulation.t fails if finesMode != "Do not calculate"
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Test SuiteAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: alexbuckley, fridolin.somers, katrin.fischer, m.de.rooy, nick
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17680
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 14390    
Bug Blocks: 19009    
Attachments: Bug 18802: Fix Circulation.t if finesMode ne 'production'
Bug 18802: Fix Circulation.t if finesMode ne 'production'
Bug 18802: Fix Circulation.t if finesMode ne 'production'
Bug 18802: Fix Circulation.t if finesMode ne 'production'
Bug 18802: Fix Circulation.t if finesMode ne 'production'

Description Jonathan Druart 2017-06-14 14:23:49 UTC

    
Comment 1 Jonathan Druart 2017-06-14 14:32:42 UTC
Caused by bug 14390.

prove t/db_dependent/Circulation.t
t/db_dependent/Circulation.t .. 16/95 DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.  SQL that returns multiple rows is DEPRECATED for ->find and ->single at t/db_dependent/Circulation.t line 491
t/db_dependent/Circulation.t .. 56/95 
    #   Failed test 'Can auto renew, OPACFineNoRenewals=10, patron has 10'
    #   at t/db_dependent/Circulation.t line 670.
    #          got: 'auto_too_much_oweing'
    #     expected: 'auto_renew'
    # Looks like you failed 1 test of 6.

#   Failed test 'auto_too_much_oweing | OPACFineNoRenewalsBlockAutoRenew'
#   at t/db_dependent/Circulation.t line 679.
t/db_dependent/Circulation.t .. 94/95 # Looks like you failed 1 test of 95.
Comment 2 Jonathan Druart 2017-06-14 14:36:12 UTC
Created attachment 64286 [details] [review]
Bug 18802: Fix Circulation.t if finesMode ne 'production'

If finesMode is not set to production, only 1 fine will be created (the renewal
one will not). This is what assumes the tests.
If set to 'production', the tests will fail because the fines will not
be deleted (because of the DBIx::Class) warning.

prove t/db_dependent/Circulation.t
t/db_dependent/Circulation.t .. 16/95 DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.  SQL that returns multiple rows is DEPRECATED for ->find and ->single at t/db_dependent/Circulation.t line 491
t/db_dependent/Circulation.t .. 56/95
    #   Failed test 'Can auto renew, OPACFineNoRenewals=10, patron has 10'
    #   at t/db_dependent/Circulation.t line 670.
    #          got: 'auto_too_much_oweing'
    #     expected: 'auto_renew'
    # Looks like you failed 1 test of 6.

Test plan:
  prove t/db_dependent/Circulation.t
should return green whatever the value of finesMode
Comment 3 Alex Buckley 2017-06-25 17:49:37 UTC
Hi Jonathan

When running Circulation.t on a clean branch all 95 tests pass successfully.

When running Circulation.t after applying this patch the tests are not successful because 97 tests are run when 96 are expected so can you please change the number of tests expected.

>Dubious, test returned 255 (wstat 65280, 0xff00)
>Failed 1/97 subtests 

>Test Summary Report
>-------------------
>Circulation.t (Wstat: 65280 Tests: 96 Failed: 0)
>  Non-zero exit status: 255
>  Parse errors: Bad plan.  You planned 97 tests but ran 96.
Comment 4 Jonathan Druart 2017-07-05 19:57:38 UTC
Created attachment 64829 [details] [review]
Bug 18802: Fix Circulation.t if finesMode ne 'production'

If finesMode is not set to production, only 1 fine will be created (the renewal
one will not). This is what assumes the tests.
If set to 'production', the tests will fail because the fines will not
be deleted (because of the DBIx::Class) warning.

prove t/db_dependent/Circulation.t
t/db_dependent/Circulation.t .. 16/95 DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.  SQL that returns multiple rows is DEPRECATED for ->find and ->single at t/db_dependent/Circulation.t line 491
t/db_dependent/Circulation.t .. 56/95
    #   Failed test 'Can auto renew, OPACFineNoRenewals=10, patron has 10'
    #   at t/db_dependent/Circulation.t line 670.
    #          got: 'auto_too_much_oweing'
    #     expected: 'auto_renew'
    # Looks like you failed 1 test of 6.

Test plan:
  prove t/db_dependent/Circulation.t
should return green whatever the value of finesMode
Comment 5 Alex Buckley 2017-07-30 20:24:18 UTC
Created attachment 65327 [details] [review]
Bug 18802: Fix Circulation.t if finesMode ne 'production'

If finesMode is not set to production, only 1 fine will be created (the renewal
one will not). This is what assumes the tests.
If set to 'production', the tests will fail because the fines will not
be deleted (because of the DBIx::Class) warning.

prove t/db_dependent/Circulation.t
t/db_dependent/Circulation.t .. 16/95 DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.  SQL that returns multiple rows is DEPRECATED for ->find and ->single at t/db_dependent/Circulation.t line 491
t/db_dependent/Circulation.t .. 56/95
    #   Failed test 'Can auto renew, OPACFineNoRenewals=10, patron has 10'
    #   at t/db_dependent/Circulation.t line 670.
    #          got: 'auto_too_much_oweing'
    #     expected: 'auto_renew'
    # Looks like you failed 1 test of 6.

Test plan:
  prove t/db_dependent/Circulation.t
should return green whatever the value of finesMode

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 6 Nick Clemens 2017-07-31 11:08:39 UTC
In one case you run 3 tests, in another you run 2 - this causes count errors depending on the values set.

Wouldn't it be better to mock the pref and test both secnarios?
Comment 7 Jonathan Druart 2017-07-31 19:34:56 UTC
Created attachment 65375 [details] [review]
Bug 18802: Fix Circulation.t if finesMode ne 'production'

If finesMode is not set to production, only 1 fine will be created (the renewal
one will not). This is what assumes the tests.
If set to 'production', the tests will fail because the fines will not
be deleted (because of the DBIx::Class) warning.
Now we mock the value before charging.

prove t/db_dependent/Circulation.t
t/db_dependent/Circulation.t .. 16/95 DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.  SQL that returns multiple rows is DEPRECATED for ->find and ->single at t/db_dependent/Circulation.t line 491
t/db_dependent/Circulation.t .. 56/95
    #   Failed test 'Can auto renew, OPACFineNoRenewals=10, patron has 10'
    #   at t/db_dependent/Circulation.t line 670.
    #          got: 'auto_too_much_oweing'
    #     expected: 'auto_renew'
    # Looks like you failed 1 test of 6.

Test plan:
  prove t/db_dependent/Circulation.t
should return green whatever the value of finesMode
Comment 8 Marcel de Rooy 2017-08-04 06:28:04 UTC
Created attachment 65488 [details] [review]
Bug 18802: Fix Circulation.t if finesMode ne 'production'

If finesMode is not set to production, only 1 fine will be created (the renewal
one will not). This is what assumes the tests.
If set to 'production', the tests will fail because the fines will not
be deleted (because of the DBIx::Class) warning.
Now we mock the value before charging.

prove t/db_dependent/Circulation.t
t/db_dependent/Circulation.t .. 16/95 DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.  SQL that returns multiple rows is DEPRECATED for ->find and ->single at t/db_dependent/Circulation.t line 491
t/db_dependent/Circulation.t .. 56/95
    #   Failed test 'Can auto renew, OPACFineNoRenewals=10, patron has 10'
    #   at t/db_dependent/Circulation.t line 670.
    #          got: 'auto_too_much_oweing'
    #     expected: 'auto_renew'
    # Looks like you failed 1 test of 6.

Test plan:
  prove t/db_dependent/Circulation.t
should return green whatever the value of finesMode

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2017-08-04 06:29:20 UTC
(In reply to Nick Clemens from comment #6)
> Wouldn't it be better to mock the pref and test both secnarios?

Test now only uses production. But as I understand, the focus of this test is on the renewal side, not the fines.
Comment 10 Jonathan Druart 2017-08-08 12:45:08 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 11 Fridolin Somers 2017-09-29 08:54:05 UTC
Pushed to 17.05.x, will be in 17.05.05.
Comment 12 Katrin Fischer 2017-10-01 21:20:04 UTC
Doesn't apply to 16.11.x, please rebase if you want it included.