Bug 18802 - Circulation.t fails if finesMode != "Do not calculate"
Summary: Circulation.t fails if finesMode != "Do not calculate"
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 14390
Blocks: 19009
  Show dependency treegraph
 
Reported: 2017-06-14 14:23 UTC by Jonathan Druart
Modified: 2017-11-15 20:30 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18802: Fix Circulation.t if finesMode ne 'production' (2.74 KB, patch)
2017-06-14 14:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18802: Fix Circulation.t if finesMode ne 'production' (2.75 KB, patch)
2017-07-05 19:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18802: Fix Circulation.t if finesMode ne 'production' (2.68 KB, patch)
2017-07-30 20:24 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18802: Fix Circulation.t if finesMode ne 'production' (3.02 KB, patch)
2017-07-31 19:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18802: Fix Circulation.t if finesMode ne 'production' (3.11 KB, patch)
2017-08-04 06:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.