Bug 24657

Summary: Fix tests of bug 22284 - Groups of pickup locations for holds
Product: Koha Reporter: Agustín Moyano <agustinmoyano>
Component: Test SuiteAssignee: Agustín Moyano <agustinmoyano>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: gmcharlt, jonathan.druart+koha, joy, martin.renvoize, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26043
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00
Bug Depends on: 22284    
Bug Blocks:    
Attachments: Bug 24657: Fix t/db_dependent/Koha/Item.t and t/db_dependent/Holds.t
Bug 24657: Fix t/db_dependent/Koha/Item.t and t/db_dependent/Holds.t
Bug 24657: Fix t/db_dependent/Koha/Item.t and t/db_dependent/Holds.t
Bug 24657: Do not explicitely pass a branchcode

Description Agustín Moyano 2020-02-13 17:12:40 UTC
Tests in bug 22284 had conditions where they randomly fail
Comment 1 Agustín Moyano 2020-02-13 17:42:08 UTC
Created attachment 98883 [details] [review]
Bug 24657: Fix t/db_dependent/Koha/Item.t and t/db_dependent/Holds.t

on t/db_dependent/Koha/Item.t on line 172 I created 2 Koha::Library::Groups like this

my $root1 = $builder->build_object( { class => 'Koha::Library::Groups', value => { ft_local_hold_group => 1 } } );
my $root2 = $builder->build_object( { class => 'Koha::Library::Groups', value => { ft_local_hold_group => 1 } } );

I didn't realize this was creating 2 new libraries that sometimes messed up with tests, so I changed it to this

my $root1 = $builder->build_object( { class => 'Koha::Library::Groups', value => { ft_local_hold_group => 1, branchcode => undef } } );
my $root2 = $builder->build_object( { class => 'Koha::Library::Groups', value => { ft_local_hold_group => 1, branchcode => undef } } );

on t/db_dependent/Holds.t on line 1058 I created 3 libraries like this

my $library1  = $builder->build_object( { class => 'Koha::Libraries' } );
my $library2  = $builder->build_object( { class => 'Koha::Libraries' } );
my $library3  = $builder->build_object( { class => 'Koha::Libraries' } );

but they needed to be pickup_locations, and sometimes they wheren't set as such, so I changed it to this

my $library1  = $builder->build_object( { class => 'Koha::Libraries', value => {pickup_location => 1} } );
my $library2  = $builder->build_object( { class => 'Koha::Libraries', value => {pickup_location => 1} } );
my $library3  = $builder->build_object( { class => 'Koha::Libraries', value => {pickup_location => 1} } );

To test:
1. do not apply this patch
2. in bash:
   for i in {1..300}; do echo "loop $i"; prove t/db_dependent/Koha/Item.t t/db_dependent/Holds.t; if [ "$?" = "1" ]; then break; fi; done
3. Grab a cup of coffee (or tea if you are healthy) and wait for a while
4. Whithin 300 iterations there should be an error in any of both scripts and for loop should exit
5. Apply this patch
6. repeat step 2 and 3 (decaff this time!)
7. All 300 loops should pass
8. Sign off
Comment 2 Agustín Moyano 2020-02-13 17:46:37 UTC
Created attachment 98884 [details] [review]
Bug 24657: Fix t/db_dependent/Koha/Item.t and t/db_dependent/Holds.t

on t/db_dependent/Koha/Item.t on line 172 I created 2 Koha::Library::Groups like this

my $root1 = $builder->build_object( { class => 'Koha::Library::Groups', value => { ft_local_hold_group => 1 } } );
my $root2 = $builder->build_object( { class => 'Koha::Library::Groups', value => { ft_local_hold_group => 1 } } );

I didn't realize this was creating 2 new libraries that sometimes messed up with tests, so I changed it to this

my $root1 = $builder->build_object( { class => 'Koha::Library::Groups', value => { ft_local_hold_group => 1, branchcode => undef } } );
my $root2 = $builder->build_object( { class => 'Koha::Library::Groups', value => { ft_local_hold_group => 1, branchcode => undef } } );

on t/db_dependent/Holds.t on line 1058 I created 3 libraries like this

my $library1  = $builder->build_object( { class => 'Koha::Libraries' } );
my $library2  = $builder->build_object( { class => 'Koha::Libraries' } );
my $library3  = $builder->build_object( { class => 'Koha::Libraries' } );

but they needed to be pickup_locations, and sometimes they wheren't set as such, so I changed it to this

my $library1  = $builder->build_object( { class => 'Koha::Libraries', value => {pickup_location => 1} } );
my $library2  = $builder->build_object( { class => 'Koha::Libraries', value => {pickup_location => 1} } );
my $library3  = $builder->build_object( { class => 'Koha::Libraries', value => {pickup_location => 1} } );

To test:
1. do not apply this patch
2. in bash:
   for i in {1..300}; do echo "loop $i"; prove t/db_dependent/Koha/Item.t t/db_dependent/Holds.t; if [ "$?" = "1" ]; then break; fi; done
3. Grab a cup of coffee (or tea if you are healthy) and wait for a while
4. Whithin 300 iterations there should be an error in any of both scripts and for loop should exit
5. Apply this patch
6. repeat step 2 and 3 (decaff this time!)
7. All 300 loops should pass
8. Sign off
Comment 3 Jonathan Druart 2020-02-14 09:14:04 UTC
Comment on attachment 98884 [details] [review]
Bug 24657: Fix t/db_dependent/Koha/Item.t and t/db_dependent/Holds.t

Review of attachment 98884 [details] [review]:
-----------------------------------------------------------------

::: t/db_dependent/Koha/Item.t
@@ +173,5 @@
> +    my $root2 = $builder->build_object( { class => 'Koha::Library::Groups', value => { ft_local_hold_group => 1, branchcode => undef } } );
> +    my $library1 = $builder->build_object( { class => 'Koha::Libraries', value => { pickup_location => 1, branchcode => 'TEST1' } } );
> +    my $library2 = $builder->build_object( { class => 'Koha::Libraries', value => { pickup_location => 1, branchcode => 'TEST2' } } );
> +    my $library3 = $builder->build_object( { class => 'Koha::Libraries', value => { pickup_location => 0, branchcode => 'TEST3' } } );
> +    my $library4 = $builder->build_object( { class => 'Koha::Libraries', value => { pickup_location => 1, branchcode => 'TEST4' } } );

Do you really need to specify a branchcode here?
Comment 4 Jonathan Druart 2020-02-14 09:19:40 UTC
I got another random failure, but not related to bug 22284. First time I see it I think.
For the record:
t/db_dependent/Holds.t ...... 1/61
#   Failed test 'Test ModReserveMinusPriority()'
#   at t/db_dependent/Holds.t line 203.
#          got: undef
#     expected: '2304'
t/db_dependent/Holds.t ...... 61/61 # Looks like you failed 1 test of 61.
Comment 5 Jonathan Druart 2020-02-14 09:24:08 UTC
Created attachment 98917 [details] [review]
Bug 24657: Fix t/db_dependent/Koha/Item.t and t/db_dependent/Holds.t

on t/db_dependent/Koha/Item.t on line 172 I created 2 Koha::Library::Groups like this

my $root1 = $builder->build_object( { class => 'Koha::Library::Groups', value => { ft_local_hold_group => 1 } } );
my $root2 = $builder->build_object( { class => 'Koha::Library::Groups', value => { ft_local_hold_group => 1 } } );

I didn't realize this was creating 2 new libraries that sometimes messed up with tests, so I changed it to this

my $root1 = $builder->build_object( { class => 'Koha::Library::Groups', value => { ft_local_hold_group => 1, branchcode => undef } } );
my $root2 = $builder->build_object( { class => 'Koha::Library::Groups', value => { ft_local_hold_group => 1, branchcode => undef } } );

on t/db_dependent/Holds.t on line 1058 I created 3 libraries like this

my $library1  = $builder->build_object( { class => 'Koha::Libraries' } );
my $library2  = $builder->build_object( { class => 'Koha::Libraries' } );
my $library3  = $builder->build_object( { class => 'Koha::Libraries' } );

but they needed to be pickup_locations, and sometimes they wheren't set as such, so I changed it to this

my $library1  = $builder->build_object( { class => 'Koha::Libraries', value => {pickup_location => 1} } );
my $library2  = $builder->build_object( { class => 'Koha::Libraries', value => {pickup_location => 1} } );
my $library3  = $builder->build_object( { class => 'Koha::Libraries', value => {pickup_location => 1} } );

To test:
1. do not apply this patch
2. in bash:
   for i in {1..300}; do echo "loop $i"; prove t/db_dependent/Koha/Item.t t/db_dependent/Holds.t; if [ "$?" = "1" ]; then break; fi; done
3. Grab a cup of coffee (or tea if you are healthy) and wait for a while
4. Whithin 300 iterations there should be an error in any of both scripts and for loop should exit
5. Apply this patch
6. repeat step 2 and 3 (decaff this time!)
7. All 300 loops should pass
8. Sign off

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 6 Jonathan Druart 2020-02-14 09:24:11 UTC
Created attachment 98918 [details] [review]
Bug 24657: Do not explicitely pass a branchcode

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 7 Jonathan Druart 2020-02-14 09:24:26 UTC
Thanks Agustin, good catch!
Comment 8 Agustín Moyano 2020-02-14 13:44:25 UTC
(In reply to Jonathan Druart from comment #3)
> Comment on attachment 98884 [details] [review] [review]
> Bug 24657: Fix t/db_dependent/Koha/Item.t and t/db_dependent/Holds.t
> 
> Review of attachment 98884 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: t/db_dependent/Koha/Item.t
> @@ +173,5 @@
> > +    my $root2 = $builder->build_object( { class => 'Koha::Library::Groups', value => { ft_local_hold_group => 1, branchcode => undef } } );
> > +    my $library1 = $builder->build_object( { class => 'Koha::Libraries', value => { pickup_location => 1, branchcode => 'TEST1' } } );
> > +    my $library2 = $builder->build_object( { class => 'Koha::Libraries', value => { pickup_location => 1, branchcode => 'TEST2' } } );
> > +    my $library3 = $builder->build_object( { class => 'Koha::Libraries', value => { pickup_location => 0, branchcode => 'TEST3' } } );
> > +    my $library4 = $builder->build_object( { class => 'Koha::Libraries', value => { pickup_location => 1, branchcode => 'TEST4' } } );
> 
> Do you really need to specify a branchcode here?

Ups, that slipped away.. I used it to verify which libraries where tested, and when a library without one of those branchcodes appeared I realized that somewhere they where created.

Thanks for the patch
Comment 9 Martin Renvoize 2020-02-20 08:56:37 UTC
Tests only and all look like sane and sensible changes to me.

Passing QA
Comment 10 Martin Renvoize 2020-02-20 09:11:14 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 11 Joy Nelson 2020-03-13 00:50:48 UTC
enhancement not backported to 19.11.x
Comment 12 Jonathan Druart 2020-07-22 10:53:56 UTC
(In reply to Jonathan Druart from comment #4)
> I got another random failure, but not related to bug 22284. First time I see
> it I think.
> For the record:
> t/db_dependent/Holds.t ...... 1/61
> #   Failed test 'Test ModReserveMinusPriority()'
> #   at t/db_dependent/Holds.t line 203.
> #          got: undef
> #     expected: '2304'
> t/db_dependent/Holds.t ...... 61/61 # Looks like you failed 1 test of 61.

See bug 26043