Bug 24657 - Fix tests of bug 22284 - Groups of pickup locations for holds
Summary: Fix tests of bug 22284 - Groups of pickup locations for holds
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Agustín Moyano
QA Contact: Testopia
URL:
Keywords:
Depends on: 22284
Blocks:
  Show dependency treegraph
 
Reported: 2020-02-13 17:12 UTC by Agustín Moyano
Modified: 2020-05-20 09:56 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 24657: Fix t/db_dependent/Koha/Item.t and t/db_dependent/Holds.t (5.62 KB, patch)
2020-02-13 17:42 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24657: Fix t/db_dependent/Koha/Item.t and t/db_dependent/Holds.t (5.62 KB, patch)
2020-02-13 17:46 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24657: Fix t/db_dependent/Koha/Item.t and t/db_dependent/Holds.t (5.69 KB, patch)
2020-02-14 09:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24657: Do not explicitely pass a branchcode (2.20 KB, patch)
2020-02-14 09:24 UTC, Jonathan Druart
Details | Diff | Splinter Review

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