Bug 10332

Hi Jonathan 

I get this

DBD::mysql::db do failed: Cannot truncate a table referenced in a foreign key constraint (`koha_persona`.`course_instructors`, CONSTRAINT `course_instructors_ibfk_2` FOREIGN KEY (`course_id`) REFERENCES `koha_persona`.`courses` (`course_id`)) at t/db_dependent/CourseReserves.t line 22.
DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_persona`.`items`, CONSTRAINT `items_ibfk_2` FOREIGN KEY (`homebranch`) REFERENCES `branches` (`branchcode`) ON UPDATE CASCADE) at C4/Items.pm line 2105.


When trying on my machine

(In reply to comment #2)
> Hi Jonathan 
> 
> I get this
> 
> DBD::mysql::db do failed: Cannot truncate a table referenced in a foreign
> key constraint (`koha_persona`.`course_instructors`, CONSTRAINT
> `course_instructors_ibfk_2` FOREIGN KEY (`course_id`) REFERENCES
> `koha_persona`.`courses` (`course_id`)) at t/db_dependent/CourseReserves.t
> line 22.
> DBD::mysql::st execute failed: Cannot add or update a child row: a foreign
> key constraint fails (`koha_persona`.`items`, CONSTRAINT `items_ibfk_2`
> FOREIGN KEY (`homebranch`) REFERENCES `branches` (`branchcode`) ON UPDATE
> CASCADE) at C4/Items.pm line 2105.
> 
> 
> When trying on my machine

The second error to do with branches was a data one, fixed that, but I am still getting the can't truncate error.

(In reply to comment #2)
> Hi Jonathan 
> 
> I get this
> 
> DBD::mysql::db do failed: Cannot truncate a table referenced in a foreign
> key constraint (`koha_persona`.`course_instructors`, CONSTRAINT
> `course_instructors_ibfk_2` FOREIGN KEY (`course_id`) REFERENCES
> `koha_persona`.`courses` (`course_id`)) at t/db_dependent/CourseReserves.t
> line 22.

I cannot reproduce :-/
The error is weird, the script truncate the course_instructors table before the courses table. So normally no course_instructors.course_id should refers to a courses.course_id.

Jonathan,

I'm getting
DBD::mysql::db do failed: Cannot truncate a table referenced in a foreign key constraint (`koha`.`course_instructors`, CONSTRAINT `course_instructors_ibfk_2` FOREIGN KEY (`course_id`) REFERENCES `koha`.`courses` (`course_id`)) at t/db_dependent/CourseReserves.t line 22.
DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha`.`items`, CONSTRAINT `items_ibfk_2` FOREIGN KEY (`homebranch`) REFERENCES `branches` (`branchcode`) ON UPDATE CASCADE) at C4/Items.pm line 2105.

Do I need to do some prep first, or some special config?

(In reply to comment #5)
> Do I need to do some prep first, or some special config?
No, it is the goal of this patch :)

Since the course course are removed at the end of the test file, I think the truncate queries are useless. I will provide a new patch.

I know this is not the right forum to raise the topic, but I've done worse things in my life:

When I do this kind of testing, I do it within a transaction, and rollback at the end. That way I do not need to think about a proper cleanup. Has there been any thoughts on that subject?

(In reply to comment #9)
> I know this is not the right forum to raise the topic, but I've done worse
> things in my life:
> 
> When I do this kind of testing, I do it within a transaction, and rollback
> at the end. That way I do not need to think about a proper cleanup. Has
> there been any thoughts on that subject?

I love that idea! But I have no idea if it's possible to do that within our existing code.

(In reply to comment #9)
> When I do this kind of testing, I do it within a transaction, and rollback
> at the end. That way I do not need to think about a proper cleanup. Has
> there been any thoughts on that subject?

Yes we should look deeper into this way. The t/db_dependent/HoldsQueue.t already use a roolback.

> Yes we should look deeper into this way. The t/db_dependent/HoldsQueue.t
> already use a roolback.

No wonder, I wrote it :)

Test 10 at the least may fail if there is any data already in the courses table. This patch removes the truncations that ensure the tables are empty. I think there are two possibilities:

1) Do we require that the tables be empty before running the unit test? If so, perhaps there should be a test for that. No other changes needed.

or

2) Implement the transaction discussed in comments 9 - 12.

If we required the tables to be empty, would that mean that you could never run the test on a installation where you have played around with course reserves? I think that makes the second option sound a lot better :)

(In reply to comment #13)
> Test 10 at the least may fail if there is any data already in the courses
> table. This patch removes the truncations that ensure the tables are empty.

Yes, good catch. I will provide a followup. We could require an empty table but it is a useless restriction.

(In reply to comment #14)
> If we required the tables to be empty, would that mean that you could never
> run the test on a installation where you have played around with course
> reserves?

It agrees with bug 10273, bug 10337 and my mail on koha-devel (subject: "Where are we on unit tests?", date: June, 4th 2013 16:53).

Thanks for the followup Jonathan! Everything looks, except koha-qa.pl fail because there is no use strict ( or use Modern::Perl ). Could you amend your followup to include one of those. After that I think it will be good to pass QA!

FAIL   t/db_dependent/CourseReserves.t
   OK     pod
   OK     forbidden patterns
   OK     valid
   FAIL   critic # TestingAndDebugging::RequireUseStrict: Got 1 violation(s).

(In reply to comment #18)
> Thanks for the followup Jonathan! Everything looks, except koha-qa.pl fail
> because there is no use strict ( or use Modern::Perl ). Could you amend your
> followup to include one of those. After that I think it will be good to pass
> QA!
> 
> FAIL   t/db_dependent/CourseReserves.t
>    OK     pod
>    OK     forbidden patterns
>    OK     valid
>    FAIL   critic # TestingAndDebugging::RequireUseStrict: Got 1 violation(s).

Kyle,
Looks good here:
$ qa -c 2 -v 2
testing 2 commit(s) (applied to 77a5e3e 'Bug 10439: fix bug preventing the lib')

 OK	t/db_dependent/CourseReserves.t
   OK	  pod
   OK	  forbidden patterns
   OK	  valid
   OK	  critic

the file contains "use Modern::Perl;".

Your t/perlcriticrc (Koha src) file should contain
[TestingAndDebugging::RequireUseStrict]
equivalent_modules = Modern::Perl

[TestingAndDebugging::RequireUseWarnings]
equivalent_modules = Modern::Perl

Maybe your qa tools repository is not up-to-date.

I bet you're right. Thanks!

Kyle

(In reply to comment #19)
> (In reply to comment #18)
> > Thanks for the followup Jonathan! Everything looks, except koha-qa.pl fail
> > because there is no use strict ( or use Modern::Perl ). Could you amend your
> > followup to include one of those. After that I think it will be good to pass
> > QA!
> > 
> > FAIL   t/db_dependent/CourseReserves.t
> >    OK     pod
> >    OK     forbidden patterns
> >    OK     valid
> >    FAIL   critic # TestingAndDebugging::RequireUseStrict: Got 1 violation(s).
> 
> Kyle,
> Looks good here:
> $ qa -c 2 -v 2
> testing 2 commit(s) (applied to 77a5e3e 'Bug 10439: fix bug preventing the
> lib')
> 
>  OK	t/db_dependent/CourseReserves.t
>    OK	  pod
>    OK	  forbidden patterns
>    OK	  valid
>    OK	  critic
> 
> the file contains "use Modern::Perl;".
> 
> Your t/perlcriticrc (Koha src) file should contain
> [TestingAndDebugging::RequireUseStrict]
> equivalent_modules = Modern::Perl
> 
> [TestingAndDebugging::RequireUseWarnings]
> equivalent_modules = Modern::Perl
> 
> Maybe your qa tools repository is not up-to-date.

(In reply to comment #20)
> > Maybe your qa tools repository is not up-to-date.

Could be the perlcritic version too..
Version 1.116 warns about Modern Perl :)

(In reply to comment #9)
> I know this is not the right forum to raise the topic, but I've done worse
> things in my life:
> 
> When I do this kind of testing, I do it within a transaction, and rollback
> at the end. That way I do not need to think about a proper cleanup. Has
> there been any thoughts on that subject?

I think this is the way to go.  Perhaps we could use MyTAP (http://theory.github.io/mytap/) as a framework for this.

(In reply to comment #24)
> I think this is the way to go.  Perhaps we could use MyTAP
> (http://theory.github.io/mytap/) as a framework for this.

Hi Galen,
Yes maybe. But as I said in my mail (see comment 15), the first step is that all unit tests pass on any instance.
They could be improved later.

Created attachment 19127 [details] [review]
Bug 10332: CourseReserves.t needs to create its own data

prove t/db_dependent/CourseReserves.t
t/db_dependent/CourseReserves.t .. ok
All tests successful.
Files=1, Tests=20,  2 wallclock secs ( 0.02 usr  0.00 sys +  0.43 cusr
0.03 csys =  0.48 CPU)
Result: PASS

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Without correct data, test fails prior to patch, passes no matter what
after patch is applied.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Galen Charlton <gmc@esilibrary.com>

Created attachment 19128 [details] [review]
Bug 10332: Data could exist in the courses table.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Galen Charlton <gmc@esilibrary.com>

(In reply to comment #25)
> Yes maybe. But as I said in my mail (see comment 15), the first step is that
> all unit tests pass on any instance.
> They could be improved later.

No, they can be improved now! :)

I've attached a follow-up that wraps the test cases in a transaction and removes the cleanup code that is now unnecessary as a consequence.  The follow-up also uses AddItem rather than AddItemFromMarc, on the theory that it would be nice if most/all of the test cases can pass regardless of MARC flavor.

Setting back to signed-off pending QA review of my follow-up.

Galen, thanks it is a great and easy improvement!

Created attachment 19135 [details] [review]
Bug 10332: further improvements to course reserves tests

- Wrap tests in a database transaction; that
  way, no special cleanup code is required
- Use AddItem rather than AddItemFromMarc to create
  test items; there's no need to build in a
  dependence on any particular MARC flavor or
  framework in our test cases (except, of course,
  for test cases that are explicitly exercising
  code that involves the frameworks).

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Pushed to master.  Thanks, Jonathan and Kyle!