Bug 32350 - We should die if TestBuilder is passed a column we're not expecting
Summary: We should die if TestBuilder is passed a column we're not expecting
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low minor (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 32351
  Show dependency treegraph
 
Reported: 2022-11-25 08:18 UTC by Martin Renvoize
Modified: 2023-06-08 22:32 UTC (History)
3 users (show)

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


Attachments
Bug 32350: Die on unrecognised column passed to testbuilder (1.13 KB, patch)
2022-11-25 08:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32350: Die on unrecognised column passed to testbuilder (1.38 KB, patch)
2022-11-25 08:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32350: Die on unrecognised column passed to testbuilder (1.48 KB, patch)
2022-11-25 10:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32350: Use array_minus and ignore nesting (2.29 KB, patch)
2022-11-25 10:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32350: Add subtest for bad columns (2.13 KB, patch)
2022-11-25 10:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32350: Add subtest for bad columns (2.13 KB, patch)
2022-11-25 11:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32350: Die on unrecognised column passed to testbuilder (1.50 KB, patch)
2022-11-25 11:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32350: Use array_minus and ignore nesting (2.32 KB, patch)
2022-11-25 11:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32350: Add subtest for bad columns (2.16 KB, patch)
2022-11-25 11:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32350: (follow-up) Remove List::Util import (685 bytes, patch)
2022-11-25 11:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32350: Use array_minus and ignore nesting (2.24 KB, patch)
2022-11-28 07:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32350: Add subtest for bad columns (2.20 KB, patch)
2022-11-28 07:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32350: Use array_minus and ignore nesting (2.26 KB, patch)
2022-11-30 15:48 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32350: Add subtest for bad columns (2.21 KB, patch)
2022-11-30 15:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2022-11-25 08:18:22 UTC

    
Comment 1 Martin Renvoize 2022-11-25 08:20:53 UTC
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.
Comment 2 Martin Renvoize 2022-11-25 08:22:14 UTC
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.
Comment 3 Martin Renvoize 2022-11-25 08:24:37 UTC
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.
Comment 4 Marcel de Rooy 2022-11-25 08:36:26 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2022-11-25 08:36:57 UTC
I would have used Array::List::intersect
Comment 6 Jonathan Druart 2022-11-25 08:37:18 UTC
(In reply to Jonathan Druart from comment #5)
> I would have used Array::List::intersect

In _buildColumnValues
Comment 7 Marcel de Rooy 2022-11-25 08:37:29 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2022-11-25 08:40:37 UTC
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 ) {
Comment 9 Jonathan Druart 2022-11-25 08:41:26 UTC
Something in the lines, with a good die message :)
Comment 10 Marcel de Rooy 2022-11-25 09:10:24 UTC
Needs a bit more logic for the "FK hashes" in value. On it now.
Comment 11 Marcel de Rooy 2022-11-25 09:11:24 UTC
(In reply to Jonathan Druart from comment #8)
>  
> +    my @intersect = intersect( keys %$original_value, @columns );
> +    die Dumper @intersect if @intersect;
> +

No intersect. Wrong logic.
Comment 12 Marcel de Rooy 2022-11-25 10:22:39 UTC
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>
Comment 13 Marcel de Rooy 2022-11-25 10:22:44 UTC
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>
Comment 14 Marcel de Rooy 2022-11-25 10:22:49 UTC
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>
Comment 15 Marcel de Rooy 2022-11-25 10:25:58 UTC
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
Comment 16 Marcel de Rooy 2022-11-25 10:28:11 UTC
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.
Comment 17 Marcel de Rooy 2022-11-25 10:40:51 UTC
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.
Comment 18 Marcel de Rooy 2022-11-25 10:41:36 UTC
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
Comment 19 Marcel de Rooy 2022-11-25 10:44:41 UTC
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.
Comment 20 Marcel de Rooy 2022-11-25 11:02:43 UTC
(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
Comment 21 Marcel de Rooy 2022-11-25 11:05:58 UTC
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>
Comment 22 Marcel de Rooy 2022-11-25 11:06:31 UTC
Couldnt live with it:

-             value => { surname => 'Pass', categorycode => { description => 'bla', wrong_nested => 1 }},
+             value => { surname => 'WontPass', categorycode => { description => 'bla', wrong_nested => 1 }},
Comment 23 Martin Renvoize 2022-11-25 11:44:45 UTC
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>
Comment 24 Martin Renvoize 2022-11-25 11:44:50 UTC
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>
Comment 25 Martin Renvoize 2022-11-25 11:44:55 UTC
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>
Comment 26 Martin Renvoize 2022-11-25 11:45:00 UTC
Created attachment 144248 [details] [review]
Bug 32350: (follow-up) Remove List::Util import

We replaced it with Array::Utils
Comment 27 Martin Renvoize 2022-11-25 11:45:44 UTC
Great teamwork here :)
Comment 28 Jonathan Druart 2022-11-25 11:52:30 UTC
First and last patches are useless, can we remove them?
Comment 29 Marcel de Rooy 2022-11-25 13:03:24 UTC
(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 :)
Comment 30 Marcel de Rooy 2022-11-28 07:57:13 UTC
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>
Comment 31 Marcel de Rooy 2022-11-28 07:57:17 UTC
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>
Comment 32 Marcel de Rooy 2022-11-28 07:58:00 UTC
(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..
Comment 33 Tomás Cohen Arazi 2022-11-30 15:48:47 UTC
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>
Comment 34 Tomás Cohen Arazi 2022-11-30 15:49:06 UTC
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>
Comment 35 Tomás Cohen Arazi 2022-11-30 15:58:31 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 36 Tomás Cohen Arazi 2022-11-30 18:24:35 UTC
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.
Comment 37 Martin Renvoize 2022-12-01 12:56:11 UTC
Thanks everyone, pushing to 22.11.x for 22.11.01.