Bug 14256

Summary: Tests for TestBuilder fail randomly
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Test SuiteAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Kyle M Hall <kyle>
Severity: normal    
Priority: P5 - low CC: chris, mtompset, tomascohen, veron
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14195
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 12603    
Bug Blocks:    
Attachments: Bug 14256: Make TestBuilder regenerates a string if to short
Bug 14256: Make TestBuilder regenerates a string if to short
Bug 14256: Make TestBuilder regenerates a string if to short
Bug 14256: (followup) do not repeat existing values on primary columns
Bug 14256: (followup) check for unique constrainta to regenerate random data
Bug 14256: (followup) check for unique constrainta to regenerate random data
Bug 14256: Check for unique constraint to regenerate random data
Bug 14256: (follow-up) Check for unique constraint to regenerate random data
[SIGNED OFF] Bug 14256: Check for unique constraint to regenerate random data
[SIGNED OFF] Bug 14256: (follow-up) Check for unique constraint to regenerate random data
[PASSED QA] Bug 14256: Check for unique constraint to regenerate random data
[PASSED QA] Bug 14256: (follow-up) Check for unique constraint to regenerate random data

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 Mark 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 Mark 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 Mark 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 Mark 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 Mark 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)