|
Description
Jonathan Druart
2022-11-25 12:05:48 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. Created attachment 144254 [details] [review] Bug 32353: Pick the default value for FK Entered a rabbit hole here, and found a TestBuilder bug I think. Created attachment 144255 [details] [review] Bug 32353: Pick the default value for FK Still failing actually... + } 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?
(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 (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. (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.. 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',
We need a good test here before we change TestBuilder imo. 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 :-/ 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. # 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 ?
(In reply to Marcel de Rooy from comment #14) > Might be something else ? I think so, it's passing for me. Original problem found with the following warning: Odd number of elements in anonymous hash at t/db_dependent/Circulation.t line 5719. Created attachment 144957 [details] [review] Bug 32353: Pick the default value for FK Signed-off-by: David Nind <david@davidnind.com> 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> 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-). 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> 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> Pushed to master for 23.05. Nice work everyone, thanks! 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}% (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! Many hands makes light work, thankyou everyone! Pushed to 22.11.x for the next release Missing dependencies for 22.05.x, no backport. |