Bug 10332 - UT: CourseReserves.t needs to create its own data
Summary: UT: CourseReserves.t needs to create its own data
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 10273
  Show dependency treegraph
 
Reported: 2013-05-24 12:23 UTC by Jonathan Druart
Modified: 2014-12-07 20:02 UTC (History)
6 users (show)

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


Attachments
Bug 10332: CourseReserves.t needs to create its own data (3.71 KB, patch)
2013-05-24 12:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10332: CourseReserves.t needs to create its own data (3.97 KB, patch)
2013-06-04 12:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10332: CourseReserves.t needs to create its own data (4.13 KB, patch)
2013-06-04 17:59 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 10332: Data could exist in the courses table. (1.88 KB, patch)
2013-06-17 09:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10332: Data could exist in the courses table. (1.34 KB, patch)
2013-06-17 09:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10332: CourseReserves.t needs to create its own data (4.18 KB, patch)
2013-06-17 15:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10332: Data could exist in the courses table. (1.40 KB, patch)
2013-06-17 15:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10332: CourseReserves.t needs to create its own data (4.23 KB, patch)
2013-06-18 15:43 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 10332: Data could exist in the courses table. (1.45 KB, patch)
2013-06-18 15:43 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 10332: further improvements to course reserves tests (2.15 KB, patch)
2013-06-18 15:43 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 10332: further improvements to course reserves tests (2.21 KB, patch)
2013-06-18 19:54 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2013-05-24 12:23:03 UTC
t/db_dependent/Acquisition/CourseReserves.t depends on the Jenkins database.
We should create the data we want and delete them at the end.
Comment 1 Jonathan Druart 2013-05-24 12:25:42 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2013-05-24 21:48:36 UTC
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
Comment 3 Chris Cormack 2013-05-24 23:16:26 UTC
(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.
Comment 4 Jonathan Druart 2013-05-27 09:07:09 UTC
(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.
Comment 5 Srdjan Jankovic 2013-06-02 23:57:49 UTC
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?
Comment 6 Jonathan Druart 2013-06-04 12:35:02 UTC
(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.
Comment 7 Jonathan Druart 2013-06-04 12:35:19 UTC Comment hidden (obsolete)
Comment 8 Jared Camins-Esakov 2013-06-04 17:59:37 UTC Comment hidden (obsolete)
Comment 9 Srdjan Jankovic 2013-06-05 02:37:34 UTC
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?
Comment 10 Jared Camins-Esakov 2013-06-05 03:20:59 UTC
(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.
Comment 11 Jonathan Druart 2013-06-05 07:26:08 UTC
(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.
Comment 12 Srdjan Jankovic 2013-06-06 00:23:41 UTC
> Yes we should look deeper into this way. The t/db_dependent/HoldsQueue.t
> already use a roolback.

No wonder, I wrote it :)
Comment 13 Kyle M Hall 2013-06-14 16:46:18 UTC
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.
Comment 14 Katrin Fischer 2013-06-16 06:33:39 UTC
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 :)
Comment 15 Jonathan Druart 2013-06-17 09:16:07 UTC
(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).
Comment 16 Jonathan Druart 2013-06-17 09:17:12 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2013-06-17 09:18:25 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2013-06-17 11:08:18 UTC
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).
Comment 19 Jonathan Druart 2013-06-17 11:25:21 UTC
(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.
Comment 20 Kyle M Hall 2013-06-17 12:53:30 UTC
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.
Comment 21 Marcel de Rooy 2013-06-17 12:56:48 UTC
(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 :)
Comment 22 Kyle M Hall 2013-06-17 15:37:34 UTC Comment hidden (obsolete)
Comment 23 Kyle M Hall 2013-06-17 15:37:46 UTC Comment hidden (obsolete)
Comment 24 Galen Charlton 2013-06-17 16:21:12 UTC
(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.
Comment 25 Jonathan Druart 2013-06-18 07:23:00 UTC
(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.
Comment 26 Galen Charlton 2013-06-18 15:43:32 UTC
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>
Comment 27 Galen Charlton 2013-06-18 15:43:43 UTC
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>
Comment 28 Galen Charlton 2013-06-18 15:43:47 UTC Comment hidden (obsolete)
Comment 29 Galen Charlton 2013-06-18 15:47:21 UTC
(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.
Comment 30 Jonathan Druart 2013-06-18 19:54:36 UTC
Galen, thanks it is a great and easy improvement!
Comment 31 Jonathan Druart 2013-06-18 19:54:59 UTC
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>
Comment 32 Galen Charlton 2013-06-18 23:41:25 UTC
Pushed to master.  Thanks, Jonathan and Kyle!