Bug 32353 - reserves.item_group_id should be undefined in tests by default
Summary: reserves.item_group_id should be undefined in tests by default
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 24857 31447
Blocks:
  Show dependency treegraph
 
Reported: 2022-11-25 12:05 UTC by Jonathan Druart
Modified: 2023-12-28 20:42 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.04


Attachments
Bug 32353: Don't generate item group for items in tests (1.33 KB, patch)
2022-11-25 12:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32353: Pick the default value for FK (1.40 KB, patch)
2022-11-25 12:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32353: Pick the default value for FK (1.44 KB, patch)
2022-11-25 12:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32353: Don't generate item group for holds in tests (1.34 KB, patch)
2022-11-28 09:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32353: Pick the default value for FK (1.49 KB, patch)
2023-01-03 19:04 UTC, David Nind
Details | Diff | Splinter Review
Bug 32353: Don't generate item group for holds in tests (1.38 KB, patch)
2023-01-03 19:04 UTC, David Nind
Details | Diff | Splinter Review
Bug 32353: Pick the default value for FK (1.54 KB, patch)
2023-02-10 19:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32353: Don't generate item group for holds in tests (1.44 KB, patch)
2023-02-10 19:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32353: Fix erm_agreements.t (3.37 KB, patch)
2023-02-23 08:42 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 Jonathan Druart 2022-11-25 12:05:48 UTC
TestBuilder should set it to NULL and don't generate the item group.
Comment 1 Jonathan Druart 2022-11-25 12:08:03 UTC
Created attachment 144253 [details] [review]
Bug 32353: Don't generate item group for items in tests

See bug 31447, we don't want the tests to deal with potential failures
because of some item groups.
Comment 2 Jonathan Druart 2022-11-25 12:19:10 UTC
Created attachment 144254 [details] [review]
Bug 32353: Pick the default value for FK
Comment 3 Jonathan Druart 2022-11-25 12:20:16 UTC
Entered a rabbit hole here, and found a TestBuilder bug I think.
Comment 4 Jonathan Druart 2022-11-25 12:24:07 UTC
Created attachment 144255 [details] [review]
Bug 32353: Pick the default value for FK
Comment 5 Jonathan Druart 2022-11-25 12:24:28 UTC
Still failing actually...
Comment 6 Marcel de Rooy 2022-11-25 13:08:29 UTC
+    } elsif( !exists $value->{$col_name}
+           && exists $self->{default_values}{$source}{$col_name} ) {
+        my $v = $self->{default_values}{$source}{$col_name};
+        $v = &$v() if ref($v) eq 'CODE';
+        push @$retvalue, $v;
     } elsif( $col_info->{is_foreign_key} || _should_be_fk($source,$col_name) ) {
         if( exists $value->{$col_name} ) {
             if( !defined $value->{$col_name} && !$col_info->{is_nullable} ) {
@@ -427,10 +432,6 @@ sub _buildColumnValue {
             return;
         }
         push @$retvalue, $value->{$col_name};
-    } elsif( exists $self->{default_values}{$source}{$col_name} ) {
-        my $v = $self->{default_values}{$source}{$col_name};
-        $v = &$v() if ref($v) eq 'CODE';
-        push @$retvalue, $v;

Sure about this change? This has been working for years?
Comment 7 Jonathan Druart 2022-11-25 13:10:46 UTC
(In reply to Marcel de Rooy from comment #6)
> Sure about this change? This has been working for years?

We have no default on FK
Comment 8 Marcel de Rooy 2022-11-28 08:04:43 UTC
(In reply to Jonathan Druart from comment #7)
> (In reply to Marcel de Rooy from comment #6)
> > Sure about this change? This has been working for years?
> 
> We have no default on FK

A FK might have a default value. We should not make assumptions based on the current state of our tables. If someone adds one, we forget to update something else, etc. etc.
Comment 9 Marcel de Rooy 2022-11-28 08:18:29 UTC
(In reply to Jonathan Druart from comment #3)
> Entered a rabbit hole here, and found a TestBuilder bug I think.

Creating a TestBuilder bug, I tend to think..
Comment 10 Marcel de Rooy 2022-11-28 08:20:18 UTC
Shouldnt we do something like this:

diff --git a/t/lib/TestBuilder.pm b/t/lib/TestBuilder.pm
index 5a08b3a018..92068f83c4 100644
--- a/t/lib/TestBuilder.pm
+++ b/t/lib/TestBuilder.pm
@@ -583,6 +583,8 @@ sub _gen_default_values {
             damaged            => 0,
             materials          => undef,
             more_subfields_xml => undef,
+        },
+        ItemGroupItem => {
             item_group_id      => undef,
         },
         Category => {


There is no such field in items:
CREATE TABLE `item_group_items` (
  `item_group_items_id` int(11) NOT NULL AUTO_INCREMENT COMMENT 'id for the group/item link',
  `item_group_id` int(11) NOT NULL DEFAULT 0 COMMENT 'foreign key making this table a 1 to 1 join from items to item groups',
  `item_id` int(11) NOT NULL DEFAULT 0 COMMENT 'foreign key linking this table to the items table',
Comment 11 Marcel de Rooy 2022-11-28 08:21:07 UTC
We need a good test here before we change TestBuilder imo.
Comment 12 Jonathan Druart 2022-11-28 09:15:53 UTC
it's reserves.item_group_id, not items.item_group_id. I've noticed my mistake on Friday, and correct the patch but... hadn't attached it here it seems :-/
Comment 13 Jonathan Druart 2022-11-28 09:16:55 UTC
Created attachment 144279 [details] [review]
Bug 32353: Don't generate item group for holds in tests

See bug 31447, we don't want the tests to deal with potential failures
because of some item groups.
Comment 14 Marcel de Rooy 2022-11-28 09:21:58 UTC
# Subtest: transferbook tests
    1..9
DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.  SQL that returns multiple rows is DEPRECATED for ->find and ->single at /usr/share/koha/Koha/Objects.pm line 96
    ok 1 - Koha::Patron->store raises an exception on missing params
DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.  SQL that returns multiple rows is DEPRECATED for ->find and ->single at /usr/share/koha/Koha/Objects.pm line 96
    ok 2 - Koha::Patron->store raises an exception on missing params
DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.  SQL that returns multiple rows is DEPRECATED for ->find and ->single at /usr/share/koha/Koha/Objects.pm line 96
    ok 3 - Koha::Patron->store raises an exception on missing params
DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.  SQL that returns multiple rows is DEPRECATED for ->find and ->single at /usr/share/koha/Koha/Objects.pm line 96
    # Looks like you planned 9 tests but ran 3.
not ok 57 - transferbook tests
#   Failed test 'transferbook tests'
#   at t/db_dependent/Circulation.t line 5394.
Exception 'Koha::Exceptions::MissingParameter' thrown 'The to parameter is mandatory'
# Looks like your test exited with 11 just after 57.

Might be something else ?
Comment 15 Jonathan Druart 2023-01-02 15:35:06 UTC
(In reply to Marcel de Rooy from comment #14)
> Might be something else ?

I think so, it's passing for me.
Comment 16 Jonathan Druart 2023-01-02 15:38:29 UTC
Original problem found with the following warning:
  Odd number of elements in anonymous hash at t/db_dependent/Circulation.t line 5719.
Comment 17 David Nind 2023-01-03 19:04:20 UTC
Created attachment 144957 [details] [review]
Bug 32353: Pick the default value for FK

Signed-off-by: David Nind <david@davidnind.com>
Comment 18 David Nind 2023-01-03 19:04:25 UTC
Created attachment 144958 [details] [review]
Bug 32353: Don't generate item group for holds in tests

See bug 31447, we don't want the tests to deal with potential failures
because of some item groups.

Signed-off-by: David Nind <david@davidnind.com>
Comment 19 David Nind 2023-01-03 19:10:55 UTC
Testing notes (using KTD):

1. Before the patches are applied, ran prove t/db_dependent/Circulation.t

2. Note that tests pass, but there are warnings:

root@kohadevbox:koha(bz32353)$ prove t/db_dependent/Circulation.t 
t/db_dependent/Circulation.t .. 60/64 Odd number of elements in anonymous hash at t/db_dependent/Circulation.t line 5712.
Use of uninitialized value $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 1119.
Use of uninitialized value $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 1119.
Use of uninitialized value $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 1119.
Use of uninitialized value $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 1119.
t/db_dependent/Circulation.t .. ok     
All tests successful.
Files=1, Tests=64, 26 wallclock secs ( 0.15 usr  0.02 sys + 18.55 cusr  2.50 csys = 21.22 CPU)
Result: PASS

3. Applied patches and restarted everything.

4. Run test again. Note that "....60/64 Odd number of elements in anonymous hash at t/db_dependent/Circulation.t line 5712." warning/output after running the tests is gone:

root@kohadevbox:koha(bz32353)$ prove t/db_dependent/Circulation.t 
t/db_dependent/Circulation.t .. 60/64 Use of uninitialized value $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 1119.
Use of uninitialized value $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 1119.
Use of uninitialized value $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 1119.
Use of uninitialized value $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 1119.
t/db_dependent/Circulation.t .. ok     
All tests successful.
Files=1, Tests=64, 26 wallclock secs ( 0.13 usr  0.01 sys + 18.61 cusr  2.52 csys = 21.27 CPU)
Result: PASS

I hope this is sufficient for testing - I don't understand all the other stuff 8-).
Comment 20 Kyle M Hall 2023-02-10 19:38:44 UTC
Created attachment 146516 [details] [review]
Bug 32353: Pick the default value for FK

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Kyle M Hall 2023-02-10 19:38:50 UTC
Created attachment 146517 [details] [review]
Bug 32353: Don't generate item group for holds in tests

See bug 31447, we don't want the tests to deal with potential failures
because of some item groups.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Tomás Cohen Arazi 2023-02-22 13:11:19 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 23 Jonathan Druart 2023-02-23 08:42:57 UTC
Created attachment 147191 [details] [review]
Bug 32353: Fix erm_agreements.t

Agreement.vendor_id has a default value defined in TestBuilder, but here
we were assuming there was one.
We should then generate it and pass the value when building the
agreement objects.

The failure was:
koha_1       |     #   Failed test '200 OK'
koha_1       |     #   at t/db_dependent/api/v1/erm_agreements.t line 95.
koha_1       |     #          got: '400'
koha_1       |     #     expected: '200'
koha_1       |
koha_1       |     #   Failed test 'exact match for JSON Pointer ""'
koha_1       |     #   at t/db_dependent/api/v1/erm_agreements.t line 95.
koha_1       |     #     Structures begin differing at:
koha_1       |     #          $got = HASH(0x55cde781a480)
koha_1       |     #     $expected = ARRAY(0x55cde75af9c8)
koha_1       |     # Looks like you failed 2 tests of 32.

400 because we were calling
http://localhost:8081/api/v1/erm/agreements?vendor_id='
which returns {"errors":[{"message":"Expected integer - got string.","path":"\/vendor_id"}],"status":400}%
Comment 24 Tomás Cohen Arazi 2023-02-23 10:45:42 UTC
(In reply to Jonathan Druart from comment #23)
> Created attachment 147191 [details] [review] [review]
> Bug 32353: Fix erm_agreements.t
> 
> Agreement.vendor_id has a default value defined in TestBuilder, but here
> we were assuming there was one.
> We should then generate it and pass the value when building the
> agreement objects.
> 
> The failure was:
> koha_1       |     #   Failed test '200 OK'
> koha_1       |     #   at t/db_dependent/api/v1/erm_agreements.t line 95.
> koha_1       |     #          got: '400'
> koha_1       |     #     expected: '200'
> koha_1       |
> koha_1       |     #   Failed test 'exact match for JSON Pointer ""'
> koha_1       |     #   at t/db_dependent/api/v1/erm_agreements.t line 95.
> koha_1       |     #     Structures begin differing at:
> koha_1       |     #          $got = HASH(0x55cde781a480)
> koha_1       |     #     $expected = ARRAY(0x55cde75af9c8)
> koha_1       |     # Looks like you failed 2 tests of 32.
> 
> 400 because we were calling
> http://localhost:8081/api/v1/erm/agreements?vendor_id='
> which returns {"errors":[{"message":"Expected integer - got
> string.","path":"\/vendor_id"}],"status":400}%

Thanks!
Comment 25 Martin Renvoize 2023-02-27 13:38:34 UTC
Many hands makes light work, thankyou everyone!

Pushed to 22.11.x for the next release
Comment 26 Lucas Gass 2023-03-08 13:12:01 UTC
Missing dependencies for 22.05.x, no backport.