Description
Jonathan Druart
2015-05-22 16:37:05 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' ); Created attachment 39430 [details] [review] Bug 14256: Make TestBuilder regenerates a string if to short In some cases, especially branchcode and categorycode, the generated string for a primary key already existed in DB. Which make the tests fail, and the data are not gererated. This patch regenerate a string if the one generated is too short and can be longer. Test plan: Use the diff from the previous comment, and confirm that the tests only fail in the very few cases. QA: Actually I don't know if the better could be to use the existing value, but maybe not... (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. What did you have in the categories table by default? I'm getting explosions "all" the time. (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. 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. (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. (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. Created attachment 39463 [details] [review] Bug 14256: Make TestBuilder regenerates a string if to short In some cases, especially branchcode and categorycode, the generated string for a primary key already existed in DB. Which make the tests fail, and the data are not gererated. This patch regenerate a string if the one generated is too short and can be longer. Test plan: Use the diff from the previous comment, and confirm that the tests only fail in the very few cases. QA: Actually I don't know if the better could be to use the existing value, but maybe not... Signed-off-by: Mark Tompsett <mtompset@hotmail.com> NOTE: Perhaps a hardcoded size like >=2 could be replaced by the minimum of (+1 max string length in the field, size cap). After a couple glitches testing, my bad, it does reduce failures drastically. I had category code of length 4 in my database at the time. Created attachment 39788 [details] [review] Bug 14256: Make TestBuilder regenerates a string if to short In some cases, especially branchcode and categorycode, the generated string for a primary key already existed in DB. Which make the tests fail, and the data are not gererated. This patch regenerate a string if the one generated is too short and can be longer. Test plan: Use the diff from the previous comment, and confirm that the tests only fail in the very few cases. QA: Actually I don't know if the better could be to use the existing value, but maybe not... Signed-off-by: Mark Tompsett <mtompset@hotmail.com> NOTE: Perhaps a hardcoded size like >=2 could be replaced by the minimum of (+1 max string length in the field, size cap). After a couple glitches testing, my bad, it does reduce failures drastically. I had category code of length 4 in my database at the time. Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com> Created attachment 39789 [details] [review] Bug 14256: (followup) do not repeat existing values on primary columns This followup patch makes _buildColumnValue test the generated random value so TestBuilder doesn't try to insert repeated values on primary columns. It also removes an unnecesary use of 'eval' that made it difficult to spot errors. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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. Created attachment 39824 [details] [review] Bug 14256: (followup) check for unique constrainta 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. Created attachment 39825 [details] [review] Bug 14256: (followup) check for unique constrainta 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. Edit: removed leftover variable 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 }?! Created attachment 39835 [details] [review] 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. Created attachment 39836 [details] [review] 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} ); Created attachment 39854 [details] [review] [SIGNED OFF] 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> Created attachment 39855 [details] [review] [SIGNED OFF] 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> 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. 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> 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> Looks good! It definitely fixes the issue and I see no particular problems with it. Patches pushed to master. Thanks Jonathan! Pushed to 3.20.x will be in 3.20.1 (note for other RMaints, not needed on other branches) |