Attachment 89856 Details for Bug 22917 – Bug 22917: Prevent Circulation.t if tests ran too slowly

[patch] Bug 22917: Prevent Circulation.t if tests ran too slowly

Bug-22917-Prevent-Circulationt-if-tests-ran-too-sl.patch (text/plain), 2.94 KB, created by Martin Renvoize (ashimema) on 2019-05-16 16:13:55 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Martin Renvoize (ashimema)

Created: 2019-05-16 16:13:55 UTC

Size: 2.94 KB

patch

obsolete

>From e02ee6b5eef958d04d558fadc61f4b8749231fe7 Mon Sep 17 00:00:00 2001
>From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
>Date: Wed, 15 May 2019 19:46:48 -0500
>Subject: [PATCH] Bug 22917: Prevent Circulation.t if tests ran too slowly
>
>After bug 21213 we wanted to know why Circulation.t was failing randomly on a given test.
>Since it is pushed, it failed (at least) twice, with the same error:
>
>    #   Failed test 'AddReturn must have debarred the patron'
>    #   at t/db_dependent/Circulation.t line 3112.
>    #          got: ''
>    #     expected: '1'
>    # AddReturn returned message $VAR1 = {
>    #           'WasReturned' => 1
>    #         };
>
>    #   Failed test 'Test at line 1918'
>    #   at t/db_dependent/Circulation.t line 3116.
>    #          got: '0'
>    #     expected: '1'
>
>    #   Failed test 'Test at line 1918'
>    #   at t/db_dependent/Circulation.t line 3119.
>    #          got: undef
>    #     expected: '2019-05-30'
>    # Looks like you failed 3 tests of 21.
>
>The test at line 3113 expects the flags 'WasReturned' and 'Debarred' to be set,
>but only WasReturned is.
>Which means the patron has not been debarred. It is not because the checkout has
>not been detected as overdue.
>
>If you apply only the first patch you will see that the tests are failing with
>the exact same failures.
>Indeed, if due_date is not passed to test_debarment_on_checkout, it is set to now (dt_from_string).
>However, if the call and the test of the parameters inside the subroutine takes
>more than 1 second,
>then due_date will be after what we really expect. To reproduce that, we add
>1 minute to due_date and we observe the tests failing.
>
>The trick here (and we should have in all our tests) is to mock
>DateTime->now to make sure dt_from_string will always return the same
>value, it is what we expect from our tests (in 99.9% of the cases at
>least).
>
>Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>---
> t/db_dependent/Circulation.t | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
>diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t
>index 79180a3a30..37ab7a57b7 100755
>--- a/t/db_dependent/Circulation.t
>+++ b/t/db_dependent/Circulation.t
>@@ -53,6 +53,11 @@ $schema->storage->txn_begin;
> my $builder = t::lib::TestBuilder->new;
> my $dbh = C4::Context->dbh;
> 
>+# Prevent random failures by mocking ->now
>+my $now_value       = DateTime->now();
>+my $mocked_datetime = Test::MockModule->new('DateTime');
>+$mocked_datetime->mock( 'now', sub { return $now_value->clone; } );
>+
> # Start transaction
> $dbh->{RaiseError} = 1;
> 
>@@ -1915,7 +1920,6 @@ subtest 'AddReturn + suspension_chargeperiod' => sub {
>             item            => $item_1,
>             library         => $library,
>             patron          => $patron,
>-            due_date        => dt_from_string->add(minutes=> 1),
>             return_date     => dt_from_string->add(days => 5),
>             expiration_date => dt_from_string->add(days => 5 + (5 * 2 - 1) ),
>         }
>-- 
>2.20.1

Actions: View | Diff | Splinter Review

Attachments on bug 22917: 89797 | 89798 | 89799 | 89853 | 89855 | 89856 | 89857 | 89876 | 89877 | 90018 | 90019 | 90020 | 90021