Bug 14256 - Tests for TestBuilder fail randomly
Summary: Tests for TestBuilder fail randomly
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Kyle M Hall
URL:
Whiteboard:
Keywords:
Depends on: 12603
Blocks:
  Show dependency treegraph
 
Reported: 2015-05-22 16:37 UTC by Jonathan Druart
Modified: 2016-12-05 21:23 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14256: Make TestBuilder regenerates a string if to short (1.31 KB, patch)
2015-05-22 16:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14256: Make TestBuilder regenerates a string if to short (1.64 KB, patch)
2015-05-24 15:16 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14256: Make TestBuilder regenerates a string if to short (1.69 KB, patch)
2015-06-02 19:13 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14256: (followup) do not repeat existing values on primary columns (2.66 KB, patch)
2015-06-02 19:14 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14256: (followup) check for unique constrainta to regenerate random data (3.31 KB, patch)
2015-06-03 18:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14256: (followup) check for unique constrainta to regenerate random data (3.27 KB, patch)
2015-06-03 19:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14256: Check for unique constraint to regenerate random data (3.23 KB, patch)
2015-06-04 09:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14256: (follow-up) Check for unique constraint to regenerate random data (4.75 KB, patch)
2015-06-04 09:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED OFF] Bug 14256: Check for unique constraint to regenerate random data (3.29 KB, patch)
2015-06-04 12:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED OFF] Bug 14256: (follow-up) Check for unique constraint to regenerate random data (4.82 KB, patch)
2015-06-04 12:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[PASSED QA] Bug 14256: Check for unique constraint to regenerate random data (3.35 KB, patch)
2015-06-09 18:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 14256: (follow-up) Check for unique constraint to regenerate random data (4.88 KB, patch)
2015-06-09 18:50 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2015-05-22 16:37:05 UTC

    
Comment 1 Jonathan Druart 2015-05-22 16:38:45 UTC
To reproduce

diff --git a/t/db_dependent/TestBuilder.t b/t/db_dependent/TestBuilder.t
index bec4834..93e3fb8 100644
--- a/t/db_dependent/TestBuilder.t
+++ b/t/db_dependent/TestBuilder.t
@@ -31,8 +31,9 @@ is( $builder->build(), undef, 'build without arguments returns undef' );
 
 my @sources    = $builder->schema->sources;
 my $nb_failure = 0;
+@sources = ('Koha::Schema::Result::Borrower') x 1000;
 for my $source (@sources) {
-    eval { $builder->build( { source => $source } ); };
+    $builder->build( { source => $source } );
     $nb_failure++ if ($@);
 }
 is( $nb_failure, 0, 'TestBuilder can create a entry for every sources' );
Comment 2 Jonathan Druart 2015-05-22 16:45:25 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2015-05-22 18:29:06 UTC
(In reply to Jonathan Druart from comment #2)
> QA: Actually I don't know if the better could be to use the existing
> value, but maybe not...

We shouldn't reuse the data. To me, the point of TestBuilder is to avoid side-effects while testing: i.e. avoid situations in which we think our code works, but is actually working only because of side-effects. TestBuilder provides a simple approach to writing integration tests.
Comment 4 M. Tompsett 2015-05-24 06:12:14 UTC
What did you have in the categories table by default?
I'm getting explosions "all" the time.
Comment 5 Jonathan Druart 2015-05-24 09:32:19 UTC
(In reply to M. Tompsett from comment #4)
> What did you have in the categories table by default?
> I'm getting explosions "all" the time.

"All the time": with or without the changes on comment 1?
I always use the sample data to launch tests.
Comment 6 M. Tompsett 2015-05-24 14:40:21 UTC
mtompset@debian:~/kohaclone$ prove t/db_dependent/TestBuilder.t
t/db_dependent/TestBuilder.t .. 1/41 DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_library`.`borrower_attribute_types`, CONSTRAINT `category_code_fk` FOREIGN KEY (`category_code`) REFERENCES `categories` (`categorycode`)) at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832.
DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_library`.`borrower_attribute_types`, CONSTRAINT `category_code_fk` FOREIGN KEY (`category_code`) REFERENCES `categories` (`categorycode`)) at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832.
DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_library`.`borrower_attribute_types`, CONSTRAINT `category_code_fk` FOREIGN KEY (`category_code`) REFERENCES `categories` (`categorycode`)) at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832.
t/db_dependent/TestBuilder.t .. 3/41

With and without the patch.
Comment 7 M. Tompsett 2015-05-24 14:41:29 UTC
(In reply to M. Tompsett from comment #6)
> With and without the patch.

bug fix patch... now to try with comment #1 tweak, but I'm sure this is unrelated.
Comment 8 Tomás Cohen Arazi 2015-05-24 14:45:35 UTC
(In reply to M. Tompsett from comment #7)
> (In reply to M. Tompsett from comment #6)
> > With and without the patch.
> 
> bug fix patch... now to try with comment #1 tweak, but I'm sure this is
> unrelated.

Please try not to make the comment list too long as it will become useless.
Comment 9 M. Tompsett 2015-05-24 15:16:09 UTC Comment hidden (obsolete)
Comment 10 Tomás Cohen Arazi 2015-06-02 19:13:49 UTC Comment hidden (obsolete)
Comment 11 Tomás Cohen Arazi 2015-06-02 19:14:35 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2015-06-03 07:58:16 UTC
Comment on attachment 39789 [details] [review]
Bug 14256: (followup) do not repeat existing values on primary columns

Review of attachment 39789 [details] [review]:
-----------------------------------------------------------------

::: t/lib/TestBuilder.pm
@@ +244,5 @@
> +        my $data_type = $col_info->{data_type};
> +        $data_type =~ s| |_|;
> +
> +        if ( any { $data_type eq $_ } (keys $gen_type) ) {
> +            my $value_ok = 'no';

A boolean would be enough.

@@ +249,5 @@
> +            while ( $value_ok eq 'no' ) {
> +                # generate value
> +                $col_value = $gen_type->{$data_type}->( $self, { info => $col_info } );
> +                # should value be unique?
> +                my $primary_names = $self->schema->source($source)->primary_columns();

What about unique keys?

@@ +250,5 @@
> +                # generate value
> +                $col_value = $gen_type->{$data_type}->( $self, { info => $col_info } );
> +                # should value be unique?
> +                my $primary_names = $self->schema->source($source)->primary_columns();
> +                if ( not ( any { $col_name eq $_ } $primary_names and

any takes a list in parameter, not a ref.
Comment 13 Tomás Cohen Arazi 2015-06-03 18:59:49 UTC Comment hidden (obsolete)
Comment 14 Tomás Cohen Arazi 2015-06-03 19:02:00 UTC Comment hidden (obsolete)
Comment 15 M. Tompsett 2015-06-03 20:26:46 UTC
Comment on attachment 39825 [details] [review]
Bug 14256: (followup) check for unique constrainta to regenerate random data

Review of attachment 39825 [details] [review]:
-----------------------------------------------------------------

::: t/lib/TestBuilder.pm
@@ +192,5 @@
> +                }
> +
> +                my $count = $self->schema
> +                                 ->resultset( $source )
> +                                 ->search( $condition )

Making the changes suggested in comment #1 sometimes triggers a KABOOM here, because condition is not right somehow. I think something goes wrong with the building loop above. { ARRAY => undef }?!
Comment 16 Jonathan Druart 2015-06-04 09:41:29 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2015-06-04 09:41:35 UTC Comment hidden (obsolete)
Comment 18 Tomás Cohen Arazi 2015-06-04 12:30:20 UTC Comment hidden (obsolete)
Comment 19 Tomás Cohen Arazi 2015-06-04 12:30:31 UTC Comment hidden (obsolete)
Comment 20 Tomás Cohen Arazi 2015-06-04 12:53:20 UTC
Marking as signed-off. The first patch was a followup for Jonathan's... Im not sure about the workflow here, but it would be enough if some dbix experienced dev like Kyle does QA on it.
Comment 21 Kyle M Hall 2015-06-09 18:50:26 UTC
Created attachment 40041 [details] [review]
[PASSED QA] Bug 14256: Check for unique constraint to regenerate random data

Unique constraints should be checked when creating random data. Otherwise
we get failures when the generated data already exists on the DB.

This patch takes advantage of ->unique_constraints() to do the job,
looping through all the unique constraints defined for the source.

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Kyle M Hall 2015-06-09 18:50:45 UTC
Created attachment 40042 [details] [review]
[PASSED QA] Bug 14256: (follow-up) Check for unique constraint to regenerate random data

There were some issues in the previous patch. This patch fixes the
following:
- rename $value with $original_value
- remove $at_least_one_constraint_failed and $values_ok which make the
  code unnecessarily complicated
- the constraints have to be checked only if no original value is passed
- _buildColumnValue created a key to the default value hashref, it broke
  the test:
    last BUILD_VALUE if exists( $default_value->{$source} );

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Kyle M Hall 2015-06-09 18:52:18 UTC
Looks good! It definitely fixes the issue and I see no particular problems with it.
Comment 24 Tomás Cohen Arazi 2015-06-10 19:04:29 UTC
Patches pushed to master.

Thanks Jonathan!
Comment 25 Chris Cormack 2015-06-11 20:07:13 UTC
Pushed to 3.20.x will be in 3.20.1 (note for other RMaints, not needed on other branches)