Bug 18146 - C4::Circulation CanBookBeRenewed lacks full test coverage
Summary: C4::Circulation CanBookBeRenewed lacks full test coverage
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-20 16:02 UTC by Mark Tompsett
Modified: 2021-06-14 21:35 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
This enhancement improves the test coverage for OverduesBlockRenewing and removes some of the warning messages.
Version(s) released in:
21.05.00


Attachments
Bug 18146: Fix tests for OverduesBlockRenewing (6.01 KB, patch)
2021-01-29 15:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18146: (follow-up) Remove warn for uninitialized value (1.26 KB, patch)
2021-01-29 15:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18146: Fix tests for OverduesBlockRenewing (6.05 KB, patch)
2021-01-30 05:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 18146: (follow-up) Remove warn for uninitialized value (1.30 KB, patch)
2021-01-30 05:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 18146: Fix tests for OverduesBlockRenewing (6.11 KB, patch)
2021-02-14 15:16 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 18146: (follow-up) Remove warn for uninitialized value (1.36 KB, patch)
2021-02-14 15:16 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 Mark Tompsett 2017-02-20 16:02:50 UTC
While testing bug 17941, I discovered two cases where test coverage was lacking from t/db_dependent/Circulation.t

Around line 2725:
    if ( $restricted and $restrictionblockrenewing ) {
        return ( 0, 'restriction');
    } elsif ( ($hasoverdues and $overduesblockrenewing eq 'block') || ($itemissue->{overdue} and $overduesblockrenewing eq 'blockitem') ) {
        return ( 0, 'overdue');
    }

The elseif is never triggered.


Around line 2759:
        if ( $soonestrenewal > DateTime->now( time_zone => C4::Context->tz() ) )
        {
            return ( 0, "auto_too_soon" ) if $itemissue->{auto_renew};
            return ( 0, "too_soon" );
        }
        elsif ( $itemissue->{auto_renew} ) {
            return ( 0, "auto_renew" );
        }

There is no else, but the code never attempts something that would trigger one.

To reproduce add:
print STDERR "CHECK!\n";
just before the return ( 0, 'overdue') in the first case, and after the elsif's curly brace in the second case.

running t/db_dependent/Circulation should not generate the CHECK points.
Comment 1 Nick Clemens 2021-01-29 15:27:54 UTC
Created attachment 116040 [details] [review]
Bug 18146: Fix tests for OverduesBlockRenewing

This pref was supposedly covered by tests, but the conditions were wrong
and we didn't test the reasons we were failing so the code was being missed

To test:
1 - Add a warn around in the conditional at line 2748:
} elsif ( ($hasoverdues and $overduesblockrenewing eq 'block') || ($itemissue->{overdue} and $overduesblockrenewing eq 'blockitem') ) {
     warn "SUCCESS!"
     return ( 0, 'overdue');
}
2 - prove -v t/db_dependent/Circulation.t | grep SUCCESS
3 - No output
4 - Apply patch
5 - Repeat
6 - SUCCESS!

https://bugs.koha-community.org/show_bug.cgi?id=18146
Comment 2 Nick Clemens 2021-01-29 15:27:56 UTC
Created attachment 116041 [details] [review]
Bug 18146: (follow-up) Remove warn for uninitialized value

In the previous patch you may have noticed many warns when running the tests

We add guarantor charges to a variable to determine if over the limit, but we don't initialize that value

We should

To test:
1 - Apply first patch and follow test plan
2 - Note warns when proving test
3 - Apply this patch
4 - prove
5 - No more warns
Comment 3 David Nind 2021-01-30 05:51:05 UTC
Created attachment 116089 [details] [review]
Bug 18146: Fix tests for OverduesBlockRenewing

This pref was supposedly covered by tests, but the conditions were wrong
and we didn't test the reasons we were failing so the code was being missed

To test:
1 - Add a warn around in the conditional at line 2748:
} elsif ( ($hasoverdues and $overduesblockrenewing eq 'block') || ($itemissue->{overdue} and $overduesblockrenewing eq 'blockitem') ) {
     warn "SUCCESS!"
     return ( 0, 'overdue');
}
2 - prove -v t/db_dependent/Circulation.t | grep SUCCESS
3 - No output
4 - Apply patch
5 - Repeat
6 - SUCCESS!

https://bugs.koha-community.org/show_bug.cgi?id=18416

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 David Nind 2021-01-30 05:51:08 UTC
Created attachment 116090 [details] [review]
Bug 18146: (follow-up) Remove warn for uninitialized value

In the previous patch you may have noticed many warns when running the tests

We add guarantor charges to a variable to determine if over the limit, but we don't initialize that value

We should

To test:
1 - Apply first patch and follow test plan
2 - Note warns when proving test
3 - Apply this patch
4 - prove
5 - No more warns

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2021-01-30 05:53:46 UTC
Testing notes - for step 1 of the first test plan:
- the file to edit is C4/Circulation.pm
- there should be a semicolon after warn "SUCCESS!" (that is: warn "SUCCESS!";)
Comment 6 Katrin Fischer 2021-02-14 15:16:41 UTC
Created attachment 116865 [details] [review]
Bug 18146: Fix tests for OverduesBlockRenewing

This pref was supposedly covered by tests, but the conditions were wrong
and we didn't test the reasons we were failing so the code was being missed

To test:
1 - Add a warn around in the conditional at line 2748:
} elsif ( ($hasoverdues and $overduesblockrenewing eq 'block') || ($itemissue->{overdue} and $overduesblockrenewing eq 'blockitem') ) {
     warn "SUCCESS!"
     return ( 0, 'overdue');
}
2 - prove -v t/db_dependent/Circulation.t | grep SUCCESS
3 - No output
4 - Apply patch
5 - Repeat
6 - SUCCESS!

https://bugs.koha-community.org/show_bug.cgi?id=18416

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 7 Katrin Fischer 2021-02-14 15:16:45 UTC
Created attachment 116866 [details] [review]
Bug 18146: (follow-up) Remove warn for uninitialized value

In the previous patch you may have noticed many warns when running the tests

We add guarantor charges to a variable to determine if over the limit, but we don't initialize that value

We should

To test:
1 - Apply first patch and follow test plan
2 - Note warns when proving test
3 - Apply this patch
4 - prove
5 - No more warns

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 8 Katrin Fischer 2021-02-14 15:17:38 UTC
More tests are better, but there are still a lot of warns running theses thests before and after the patch:

Use of uninitialized value in concatenation (.) or string at /home/vagrant/kohaclone/C4/Biblio.pm line 657.
t/db_dependent/Circulation.t .. 32/52 Use of uninitialized value in concatenation (.) or string at /home/vagrant/kohaclone/C4/Biblio.pm line 657.
Comment 9 Jonathan Druart 2021-02-15 11:08:29 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 10 Fridolin Somers 2021-02-19 15:33:22 UTC
Enhancement not pushed to 20.11.x