Created attachment 144229 [details] [review] Bug 32350: Die on unrecognised column passed to testbuilder This should prevent mis-matches between tests and database fields in the future and reduce our chances of hitting random test failures. However, it may have a performance impact on the test suite.
Is this a crazy idea of worthwhile.. I can't make up my mind.. It would have helped prevent the series of bugs Jonathan is now working through after the boolean fix.. I think.
Created attachment 144230 [details] [review] Bug 32350: Die on unrecognised column passed to testbuilder This should prevent mis-matches between tests and database fields in the future and reduce our chances of hitting random test failures. However, it may have a performance impact on the test suite.
[09:35] <marcelr> ashimema: you should do the check in build not in build_object [09:35] <marcelr> we are using build too still [09:35] <marcelr> and build_object calls bild [09:35] <marcelr> build
I would have used Array::List::intersect
(In reply to Jonathan Druart from comment #5) > I would have used Array::List::intersect In _buildColumnValues
(In reply to Marcel de Rooy from comment #4) > [09:35] <marcelr> ashimema: you should do the check in build not in > build_object > [09:35] <marcelr> we are using build too still > [09:35] <marcelr> and build_object calls bild > [09:35] <marcelr> build Nonsense, sorry
diff --git a/t/lib/TestBuilder.pm b/t/lib/TestBuilder.pm index d065835ded0..d9e48b45c54 100644 --- a/t/lib/TestBuilder.pm +++ b/t/lib/TestBuilder.pm @@ -8,6 +8,7 @@ use Koha::Biblios qw( _type ); use Koha::Items qw( _type ); use Koha::DateUtils qw( dt_from_string ); +use Array::Utils qw( intersect ); use Bytes::Random::Secure; use Carp qw( carp ); use Module::Load qw( load ); @@ -280,6 +281,9 @@ sub _buildColumnValues { my @columns = $self->schema->source($source)->columns; my %unique_constraints = $self->schema->source($source)->unique_constraints(); + my @intersect = intersect( keys %$original_value, @columns ); + die Dumper @intersect if @intersect; + my $build_value = 5; # we try max $build_value times if there are unique constraints BUILD_VALUE: while ( $build_value ) {
Something in the lines, with a good die message :)
Needs a bit more logic for the "FK hashes" in value. On it now.
(In reply to Jonathan Druart from comment #8) > > + my @intersect = intersect( keys %$original_value, @columns ); > + die Dumper @intersect if @intersect; > + No intersect. Wrong logic.
Created attachment 144233 [details] [review] Bug 32350: Die on unrecognised column passed to testbuilder This should prevent mis-matches between tests and database fields in the future and reduce our chances of hitting random test failures. However, it may have a performance impact on the test suite. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 144234 [details] [review] Bug 32350: Use array_minus and ignore nesting Note: Test will be extended in follow-up. This fixes the module_bit hash to follow the FK path from user_permissions to permissions to userflags. One step was missed in the existing test, although it did not fail. The change here revealed that now. Test plan: Run t/db_dependent/TestBuilder.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 144235 [details] [review] Bug 32350: Add subtest for bad columns Test plan: Run t/db_dependent/TestBuilder.t And now run the whole test suite :) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
t/db_dependent/Koha/Holds.t ................... 6/10 # No tests run! # Failed test 'No tests run for subtest "Test Koha::Hold::item_group"' # at t/db_dependent/Koha/Holds.t line 669. Error: value hash contains unrecognized columns: biblionumber at /usr/share/koha/t/lib/TestBuilder.pm line 287. # Looks like your test exited with 255 just after 8. t/db_dependent/Koha/Holds.t ................... Dubious, test returned 255 (wstat 65280, 0xff00) Failed 3/10 subtests First hit in t/db../Koha
t/db_dependent/Koha/Patron.t .................. 6/19 # No tests run! # Failed test 'No tests run for subtest "unique attributes tests"' # at t/db_dependent/Koha/Patron.t line 593. # Looks like you planned 16 tests but ran 13. # Looks like you failed 1 test of 13 run. t/db_dependent/Koha/Patron.t .................. 7/19 # Failed test 'extended_attributes' # at t/db_dependent/Koha/Patron.t line 727. Error: value hash contains unrecognized columns: unique at /usr/share/koha/t/lib/TestBuilder.pm line 287. # Looks like your test exited with 255 just after 7. t/db_dependent/Koha/Patron.t .................. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 13/19 subtests t/db_dependent/Koha/Patrons.t ................. 1/44 # Looks like you planned 9 tests but ran 4. t/db_dependent/Koha/Patrons.t ................. 5/44 # Failed test 'guarantees' # at t/db_dependent/Koha/Patrons.t line 235. Error: value hash contains unrecognized columns: guarantorid at /usr/share/koha/t/lib/TestBuilder.pm line 287.
Here are few more from t/db.. in root dir ./Acquisition.t ........................... 63/71 # Failed test 'No tests run for subtest "ModReceiveOrder replacementprice tests"' # at ./Acquisition.t line 748. Error: value hash contains unrecognized columns: booksellerid at /usr/share/koha/t/lib/TestBuilder.pm line 287. ./Budgets.t ............................... 105/154 # No tests run! # Failed test 'No tests run for subtest "GetBudgetSpent and GetBudgetOrdered and GetBudgetHierarchy shipping and adjustments"' # at ./Budgets.t line 1039. Error: value hash contains unrecognized columns: budget_total at /usr/share/koha/t/lib/TestBuilder.pm line 287. ./Circulation.t ........................... 33/64 # No tests run! # Failed test 'No tests run for subtest "CanBookBeIssued + AutoReturnCheckedOutItems"' # at ./Circulation.t line 2895. Error: value hash contains unrecognized columns: library at /usr/share/koha/t/lib/TestBuilder.pm line 287. ./Illrequests.t ........................... 9/15 # No tests run! # Failed test 'No tests run for subtest "Checking out"' # at ./Illrequests.t line 1312. Error: value hash contains unrecognized columns: category_type at /usr/share/koha/t/lib/TestBuilder.pm line 287. ./Members.t ............................... 43/54 # Looks like you planned 8 tests but ran 5. ./Members.t ............................... 53/54 # Failed test 'purgeSelfRegistration' # at ./Members.t line 470. Error: value hash contains unrecognized columns: acocunttype at /usr/share/koha/t/lib/TestBuilder.pm line 287. ./Reserves.t .............................. 72/77 # Looks like you planned 2 tests but ran 1. # Failed test 'IsAvailableForItemLevelRequest() tests' # at ./Reserves.t line 1475. Error: value hash contains unrecognized columns: notloan at /usr/share/koha/t/lib/TestBuilder.pm line 287.
Not sure if following are related. I missed Test::DBIx::Class. Skipped selenium and www. ./Holds.t ................................. 76/76 # Looks like you failed 2 tests of 76. ./Search.t (Wstat: 65280 Tests: 1 Failed: 1) Failed test: 1 Non-zero exit status: 255 Parse errors: Bad plan. You planned 3 tests but ran 1. ./Sitemapper.t (Wstat: 512 Tests: 2 Failed: 0) Non-zero exit status: 2 Parse errors: Bad plan. You planned 16 tests but ran 2. ./Suggestions.t (Wstat: 256 Tests: 91 Failed: 1) Failed test: 89 Non-zero exit status: 1 ./check_sysprefs.t (Wstat: 256 Tests: 795 Failed: 1) Failed test: 294 Non-zero exit status: 1 ./yaml.t (Wstat: 1024 Tests: 4 Failed: 4) Failed tests: 1-4 Non-zero exit status: 4
I think it makes sense to fix those tests on follow-up reports. And push when it looks like we are green ;) Will fix some now on a follow-up.
(In reply to Marcel de Rooy from comment #19) > I think it makes sense to fix those tests on follow-up reports. And push > when it looks like we are green ;) > Will fix some now on a follow-up. Probably better to fix them all on 32351
Created attachment 144239 [details] [review] Bug 32350: Add subtest for bad columns Test plan: Run t/db_dependent/TestBuilder.t And now run the whole test suite :) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Couldnt live with it: - value => { surname => 'Pass', categorycode => { description => 'bla', wrong_nested => 1 }}, + value => { surname => 'WontPass', categorycode => { description => 'bla', wrong_nested => 1 }},
Created attachment 144245 [details] [review] Bug 32350: Die on unrecognised column passed to testbuilder This should prevent mis-matches between tests and database fields in the future and reduce our chances of hitting random test failures. However, it may have a performance impact on the test suite. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 144246 [details] [review] Bug 32350: Use array_minus and ignore nesting Note: Test will be extended in follow-up. This fixes the module_bit hash to follow the FK path from user_permissions to permissions to userflags. One step was missed in the existing test, although it did not fail. The change here revealed that now. Test plan: Run t/db_dependent/TestBuilder.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 144247 [details] [review] Bug 32350: Add subtest for bad columns Test plan: Run t/db_dependent/TestBuilder.t And now run the whole test suite :) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 144248 [details] [review] Bug 32350: (follow-up) Remove List::Util import We replaced it with Array::Utils
Great teamwork here :)
First and last patches are useless, can we remove them?
(In reply to Jonathan Druart from comment #28) > First and last patches are useless, can we remove them? You're welcome to do it in the QA step :)
Created attachment 144276 [details] [review] Bug 32350: Use array_minus and ignore nesting Note: Test will be extended in follow-up. This fixes the module_bit hash to follow the FK path from user_permissions to permissions to userflags. One step was missed in the existing test, although it did not fail. The change here revealed that now. Test plan: Run t/db_dependent/TestBuilder.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 144277 [details] [review] Bug 32350: Add subtest for bad columns Test plan: Run t/db_dependent/TestBuilder.t And now run the whole test suite :) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
(In reply to Jonathan Druart from comment #28) > First and last patches are useless, can we remove them? So I did now, making Martin loose his attribution. One of the reasons why I didnt do it in the first place..
Created attachment 144349 [details] [review] Bug 32350: Use array_minus and ignore nesting Note: Test will be extended in follow-up. This fixes the module_bit hash to follow the FK path from user_permissions to permissions to userflags. One step was missed in the existing test, although it did not fail. The change here revealed that now. Test plan: Run t/db_dependent/TestBuilder.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 144350 [details] [review] Bug 32350: Add subtest for bad columns Test plan: Run t/db_dependent/TestBuilder.t And now run the whole test suite :) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed to master for 22.11. Nice work everyone, thanks!
Bad timing for BZ to be down. I pushed this to see what happens on jenkins, then I'll move to bug 32351 so we chase all the problems. Thanks everyone.
Thanks everyone, pushing to 22.11.x for 22.11.01.