Attachment 92175 Details for Bug 23177 – Bug 23177: [18.11.x] (QA follow-up) Move three subs from the middle to the top in Circulation.t

[patch] Bug 23177: [18.11.x] (QA follow-up) Move three subs from the middle to the top in Circulation.t

Bug-23177-1811x-QA-follow-up-Move-three-subs-from-.patch (text/plain), 15.75 KB, created by Nick Clemens (kidclamp) on 2019-08-13 11:58:16 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Nick Clemens (kidclamp)

Created: 2019-08-13 11:58:16 UTC

Size: 15.75 KB

patch

obsolete

>From 9e08e6e86fdbbf665a44f5dea57dc87c693a2116 Mon Sep 17 00:00:00 2001
>From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Date: Fri, 21 Jun 2019 09:00:27 +0000
>Subject: [PATCH] Bug 23177: [18.11.x] (QA follow-up) Move three subs from the
> middle to the top in Circulation.t
>
>Removed trailing comma for last sub too.
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>
>Bug 23177: (QA follow-up) Move rollback to the end
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>
>Bug 23177: (QA follow-up) Remove subtest txn_begin and rollback
>
>This resolves:
>DBIx::Class::Storage::txn_rollback(): A txn_rollback in nested transaction is ineffective! (depth 1)
>Removing unneeded subtest txn's. We should probably add them everywhere or
>just at global level.
>
>Test plan:
>With all three patches applied, run Circulation.t a few times.
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>
>Bug 23177: (RM follow-up) Further test clarifications
>
>This patch makes a few minor improvements to Circulation.t
>1) Adds a name to some of the scoped blocks by converting them to
>   subtests.
>2) Adds output messages to some tests where they were missing.
>
>Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>
>Bug 23177: Rebase fixes
>---
> t/db_dependent/Circulation.t | 158 +++++++++++++++++++++----------------------
> 1 file changed, 78 insertions(+), 80 deletions(-)
>
>diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t
>index 156be69c0b..eca86fd357 100755
>--- a/t/db_dependent/Circulation.t
>+++ b/t/db_dependent/Circulation.t
>@@ -18,7 +18,8 @@
> use Modern::Perl;
> use utf8;
> 
>-use Test::More tests => 123;
>+use Test::More tests => 40;
>+use Test::MockModule;
> 
> use Data::Dumper;
> use DateTime;
>@@ -43,6 +44,53 @@ use Koha::Subscriptions;
> use Koha::Account::Lines;
> use Koha::Account::Offsets;
> 
>+sub set_userenv {
>+    my ( $library ) = @_;
>+    t::lib::Mocks::mock_userenv({ branchcode => $library->{branchcode} });
>+}
>+
>+sub str {
>+    my ( $error, $question, $alert ) = @_;
>+    my $s;
>+    $s  = %$error    ? ' (error: '    . join( ' ', keys %$error    ) . ')' : '';
>+    $s .= %$question ? ' (question: ' . join( ' ', keys %$question ) . ')' : '';
>+    $s .= %$alert    ? ' (alert: '    . join( ' ', keys %$alert    ) . ')' : '';
>+    return $s;
>+}
>+
>+sub test_debarment_on_checkout {
>+    my ($params) = @_;
>+    my $item     = $params->{item};
>+    my $library  = $params->{library};
>+    my $patron   = $params->{patron};
>+    my $due_date = $params->{due_date} || dt_from_string;
>+    my $return_date = $params->{return_date} || dt_from_string;
>+    my $expected_expiration_date = $params->{expiration_date};
>+
>+    $expected_expiration_date = output_pref(
>+        {
>+            dt         => $expected_expiration_date,
>+            dateformat => 'sql',
>+            dateonly   => 1,
>+        }
>+    );
>+    my @caller      = caller;
>+    my $line_number = $caller[2];
>+    AddIssue( $patron, $item->{barcode}, $due_date );
>+
>+    my ( undef, $message ) = AddReturn( $item->{barcode}, $library->{branchcode}, undef, undef, $return_date );
>+    is( $message->{WasReturned} && exists $message->{Debarred}, 1, 'AddReturn must have debarred the patron' )
>+        or diag('AddReturn returned message ' . Dumper $message );
>+    my $debarments = Koha::Patron::Debarments::GetDebarments(
>+        { borrowernumber => $patron->{borrowernumber}, type => 'SUSPENSION' } );
>+    is( scalar(@$debarments), 1, 'Test at line ' . $line_number );
>+
>+    is( $debarments->[0]->{expiration},
>+        $expected_expiration_date, 'Test at line ' . $line_number );
>+    Koha::Patron::Debarments::DelUniqueDebarment(
>+        { borrowernumber => $patron->{borrowernumber}, type => 'SUSPENSION' } );
>+};
>+
> my $schema = Koha::Database->schema;
> $schema->storage->txn_begin;
> my $builder = t::lib::TestBuilder->new;
>@@ -214,8 +262,9 @@ $dbh->do(
> );
> 
> my ( $reused_itemnumber_1, $reused_itemnumber_2 );
>-{
>-# CanBookBeRenewed tests
>+subtest "CanBookBeRenewed tests" => sub {
>+    plan tests => 68;
>+
>     C4::Context->set_preference('ItemsDeniedRenewal','');
>     # Generate test biblio
>     my $biblio = $builder->build_sample_biblio();
>@@ -485,7 +534,7 @@ my ( $reused_itemnumber_1, $reused_itemnumber_2 );
>     is ($new_log_size, $old_log_size + 1, 'renew log successfully added');
> 
>     my $fines = Koha::Account::Lines->search( { borrowernumber => $renewing_borrower->{borrowernumber}, itemnumber => $item_7->itemnumber } );
>-    is( $fines->count, 2 );
>+    is( $fines->count, 2, 'AddRenewal left both fines' );
>     is( $fines->next->accounttype, 'F', 'Fine on renewed item is closed out properly' );
>     is( $fines->next->accounttype, 'F', 'Fine on renewed item is closed out properly' );
>     $fines->delete();
>@@ -883,10 +932,11 @@ my ( $reused_itemnumber_1, $reused_itemnumber_2 );
>     LostItem( $item_3->itemnumber, 'test', 0 );
>     my $accountline = Koha::Account::Lines->find( { itemnumber => $item_3->itemnumber } );
>     is( $accountline->issue_id, $checkout->id, "Issue id added for lost replacement fee charge" );
>-  }
>+};
>+
>+subtest "GetUpcomingDueIssues" => sub {
>+    plan tests => 12;
> 
>-{
>-    # GetUpcomingDueIssues tests
>     my $branch   = $library2->{branchcode};
> 
>     #Create another record
>@@ -964,9 +1014,9 @@ my ( $reused_itemnumber_1, $reused_itemnumber_2 );
>     $upcoming_dues = C4::Circulation::GetUpcomingDueIssues();
>     is ( scalar ( @$upcoming_dues), 2, "days_in_advance is 7 in GetUpcomingDueIssues if not provided" );
> 
>-}
>+};
> 
>-{
>+subtest "Bug 13841 - Do not create new 0 amount fines" => sub {
>     my $branch   = $library2->{branchcode};
> 
>     my $biblio = $builder->build_sample_biblio();
>@@ -1006,9 +1056,9 @@ my ( $reused_itemnumber_1, $reused_itemnumber_2 );
>     my $count = $hr->{count};
> 
>     is ( $count, 0, "Calling UpdateFine on non-existant fine with an amount of 0 does not result in an empty fine" );
>-}
>+};
> 
>-{
>+subtest "AllowRenewalIfOtherItemsAvailable tests" => sub {
>     $dbh->do('DELETE FROM issues');
>     $dbh->do('DELETE FROM items');
>     $dbh->do('DELETE FROM issuingrules');
>@@ -1094,7 +1144,7 @@ my ( $reused_itemnumber_1, $reused_itemnumber_2 );
> 
>     ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber );
>     is( $renewokay, 0, 'Bug 14337 - Verify the borrower can not renew with a hold on the record if AllowRenewalIfOtherItemsAvailable is enabled but the only available item is notforloan' );
>-}
>+};
> 
> {
>     # Don't allow renewing onsite checkout
>@@ -1218,7 +1268,7 @@ subtest 'CanBookBeIssued & AllowReturnToBranch' => sub {
>     ( $error, $question, $alerts ) = CanBookBeIssued( $patron_2, $item->{barcode} );
>     is( keys(%$question) + keys(%$alerts), 0, 'There should not be any errors or alerts (impossible)' . str($error, $question, $alerts) );
>     is( exists $error->{RETURN_IMPOSSIBLE}, 1, 'RETURN_IMPOSSIBLE must be set' );
>-    is( $error->{branch_to_return},         $holdingbranch->{branchcode} );
>+    is( $error->{branch_to_return},         $holdingbranch->{branchcode}, 'branch_to_return matched holdingbranch' );
>     ## Can be issued from holdinbranch
>     set_userenv($holdingbranch);
>     ( $error, $question, $alerts ) = CanBookBeIssued( $patron_2, $item->{barcode} );
>@@ -1229,7 +1279,7 @@ subtest 'CanBookBeIssued & AllowReturnToBranch' => sub {
>     ( $error, $question, $alerts ) = CanBookBeIssued( $patron_2, $item->{barcode} );
>     is( keys(%$question) + keys(%$alerts), 0, 'There should not be any errors or alerts (impossible)' . str($error, $question, $alerts) );
>     is( exists $error->{RETURN_IMPOSSIBLE}, 1, 'RETURN_IMPOSSIBLE must be set' );
>-    is( $error->{branch_to_return},         $holdingbranch->{branchcode} );
>+    is( $error->{branch_to_return},         $holdingbranch->{branchcode}, 'branch_to_return matches holdingbranch' );
> 
>     # AllowReturnToBranch == homebranch
>     t::lib::Mocks::mock_preference( 'AllowReturnToBranch', 'homebranch' );
>@@ -1243,13 +1293,13 @@ subtest 'CanBookBeIssued & AllowReturnToBranch' => sub {
>     ( $error, $question, $alerts ) = CanBookBeIssued( $patron_2, $item->{barcode} );
>     is( keys(%$question) + keys(%$alerts), 0, 'There should not be any errors or alerts (impossible)' . str($error, $question, $alerts) );
>     is( exists $error->{RETURN_IMPOSSIBLE}, 1, 'RETURN_IMPOSSIBLE must be set' );
>-    is( $error->{branch_to_return},         $homebranch->{branchcode} );
>+    is( $error->{branch_to_return},         $homebranch->{branchcode}, 'branch_to_return matches homebranch' );
>     ## Cannot be issued from holdinbranch
>     set_userenv($otherbranch);
>     ( $error, $question, $alerts ) = CanBookBeIssued( $patron_2, $item->{barcode} );
>     is( keys(%$question) + keys(%$alerts), 0, 'There should not be any errors or alerts (impossible)' . str($error, $question, $alerts) );
>     is( exists $error->{RETURN_IMPOSSIBLE}, 1, 'RETURN_IMPOSSIBLE must be set' );
>-    is( $error->{branch_to_return},         $homebranch->{branchcode} );
>+    is( $error->{branch_to_return},         $homebranch->{branchcode}, 'branch_to_return matches homebranch' );
> 
>     # TODO t::lib::Mocks::mock_preference('AllowReturnToBranch', 'homeorholdingbranch');
> };
>@@ -1288,42 +1338,42 @@ subtest 'AddIssue & AllowReturnToBranch' => sub {
>     t::lib::Mocks::mock_preference( 'AllowReturnToBranch', 'anywhere' );
>     ## Can be issued from homebranch
>     set_userenv($homebranch);
>-    is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), $ref_issue );
>+    is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), $ref_issue, 'AllowReturnToBranch - anywhere | Can be issued from homebranch');
>     set_userenv($holdingbranch); AddIssue( $patron_1, $item->{barcode} ); # Reinsert the original issue
>     ## Can be issued from holdinbranch
>     set_userenv($holdingbranch);
>-    is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), $ref_issue );
>+    is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), $ref_issue, 'AllowReturnToBranch - anywhere | Can be issued from holdingbranch');
>     set_userenv($holdingbranch); AddIssue( $patron_1, $item->{barcode} ); # Reinsert the original issue
>     ## Can be issued from another branch
>     set_userenv($otherbranch);
>-    is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), $ref_issue );
>+    is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), $ref_issue, 'AllowReturnToBranch - anywhere | Can be issued from otherbranch');
>     set_userenv($holdingbranch); AddIssue( $patron_1, $item->{barcode} ); # Reinsert the original issue
> 
>     # AllowReturnToBranch == holdinbranch
>     t::lib::Mocks::mock_preference( 'AllowReturnToBranch', 'holdingbranch' );
>     ## Cannot be issued from homebranch
>     set_userenv($homebranch);
>-    is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), '' );
>+    is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), '', 'AllowReturnToBranch - holdingbranch | Cannot be issued from homebranch');
>     ## Can be issued from holdingbranch
>     set_userenv($holdingbranch);
>-    is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), $ref_issue );
>+    is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), $ref_issue, 'AllowReturnToBranch - holdingbranch | Can be issued from holdingbranch');
>     set_userenv($holdingbranch); AddIssue( $patron_1, $item->{barcode} ); # Reinsert the original issue
>     ## Cannot be issued from another branch
>     set_userenv($otherbranch);
>-    is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), '' );
>+    is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), '', 'AllowReturnToBranch - holdingbranch | Cannot be issued from otherbranch');
> 
>     # AllowReturnToBranch == homebranch
>     t::lib::Mocks::mock_preference( 'AllowReturnToBranch', 'homebranch' );
>     ## Can be issued from homebranch
>     set_userenv($homebranch);
>-    is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), $ref_issue );
>+    is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), $ref_issue, 'AllowReturnToBranch - homebranch | Can be issued from homebranch' );
>     set_userenv($holdingbranch); AddIssue( $patron_1, $item->{barcode} ); # Reinsert the original issue
>     ## Cannot be issued from holdinbranch
>     set_userenv($holdingbranch);
>-    is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), '' );
>+    is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), '', 'AllowReturnToBranch - homebranch | Cannot be issued from holdingbranch' );
>     ## Cannot be issued from another branch
>     set_userenv($otherbranch);
>-    is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), '' );
>+    is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), '', 'AllowReturnToBranch - homebranch | Cannot be issued from otherbranch' );
>     # TODO t::lib::Mocks::mock_preference('AllowReturnToBranch', 'homeorholdingbranch');
> };
> 
>@@ -2772,15 +2822,11 @@ subtest 'AddReturn should clear items.onloan for unissued items' => sub {
>     is( $item->onloan, undef, 'AddReturn did clear items.onloan' );
> };
> 
>-$schema->storage->txn_rollback;
>-C4::Context->clear_syspref_cache();
>-$cache->clear_from_cache('single_holidays');
> 
> subtest 'AddRenewal and AddIssuingCharge tests' => sub {
> 
>     plan tests => 10;
> 
>-    $schema->storage->txn_begin;
> 
>     my $issuing_charges = 15;
>     my $title   = 'A title';
>@@ -2860,14 +2906,12 @@ subtest 'AddRenewal and AddIssuingCharge tests' => sub {
>     is( $line->accounttype, 'Rent', 'Fine on renewed item is closed out properly' );
>     is( $line->description, "Renewal of Rental Item $title $barcode", 'AddRenewal set a hardcoded description for the accountline' );
> 
>-    $schema->storage->txn_rollback;
> };
> 
> subtest 'ProcessOfflinePayment() tests' => sub {
> 
>     plan tests => 3;
> 
>-    $schema->storage->txn_begin;
> 
>     my $amount = 123;
> 
>@@ -2883,55 +2927,9 @@ subtest 'ProcessOfflinePayment() tests' => sub {
>     my $line = $lines->next;
>     is( $line->amount+0, $amount * -1, 'amount picked from params' );
> 
>-    $schema->storage->txn_rollback;
> };
> 
> 
>-
>-sub set_userenv {
>-    my ( $library ) = @_;
>-    t::lib::Mocks::mock_userenv({ branchcode => $library->{branchcode} });
>-}
>-
>-sub str {
>-    my ( $error, $question, $alert ) = @_;
>-    my $s;
>-    $s  = %$error    ? ' (error: '    . join( ' ', keys %$error    ) . ')' : '';
>-    $s .= %$question ? ' (question: ' . join( ' ', keys %$question ) . ')' : '';
>-    $s .= %$alert    ? ' (alert: '    . join( ' ', keys %$alert    ) . ')' : '';
>-    return $s;
>-}
>-
>-sub test_debarment_on_checkout {
>-    my ($params) = @_;
>-    my $item     = $params->{item};
>-    my $library  = $params->{library};
>-    my $patron   = $params->{patron};
>-    my $due_date = $params->{due_date} || dt_from_string;
>-    my $return_date = $params->{return_date} || dt_from_string;
>-    my $expected_expiration_date = $params->{expiration_date};
>-
>-    $expected_expiration_date = output_pref(
>-        {
>-            dt         => $expected_expiration_date,
>-            dateformat => 'sql',
>-            dateonly   => 1,
>-        }
>-    );
>-    my @caller      = caller;
>-    my $line_number = $caller[2];
>-    AddIssue( $patron, $item->{barcode}, $due_date );
>-
>-    my ( undef, $message ) = AddReturn( $item->{barcode}, $library->{branchcode},
>-        undef, undef, $return_date );
>-    is( $message->{WasReturned} && exists $message->{Debarred}, 1, 'AddReturn must have debarred the patron' )
>-        or diag('AddReturn returned message ' . Dumper $message );
>-    my $debarments = Koha::Patron::Debarments::GetDebarments(
>-        { borrowernumber => $patron->{borrowernumber}, type => 'SUSPENSION' } );
>-    is( scalar(@$debarments), 1, 'Test at line ' . $line_number );
>-
>-    is( $debarments->[0]->{expiration},
>-        $expected_expiration_date, 'Test at line ' . $line_number );
>-    Koha::Patron::Debarments::DelUniqueDebarment(
>-        { borrowernumber => $patron->{borrowernumber}, type => 'SUSPENSION' } );
>-}
>+$schema->storage->txn_rollback;
>+C4::Context->clear_syspref_cache();
>+$cache->clear_from_cache('single_holidays');
>-- 
>2.11.0

Actions: View | Diff | Splinter Review

Attachments on bug 23177: 90880 | 90881 | 90882 | 90899 | 90900 | 90901 | 90902 | 92175