Bug 10289

Summary: UT: Reserves.t needs to create its own data
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Test SuiteAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact:
Severity: normal    
Priority: P5 - low CC: bgkriegel, gmcharlt, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 10487, 10273    
Attachments: Bug 10289: UT: Reserves.t needs to create its own data
Bug 10289: UT: Reserves.t needs to create its own data
Bug 10289: UT: Reserves.t needs to create its own data
Bug 10289: UT: Reserves.t needs to create its own data
Bug 10289: UT: Reserves.t needs to create its own data

Description Jonathan Druart 2013-05-20 14:34:42 UTC
t/db_dependent/Reserves.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-20 14:38:58 UTC Comment hidden (obsolete)
Comment 2 Jared Camins-Esakov 2013-06-04 18:03:09 UTC
This one does not work for me. I get the following output (with DEBUG on):

jcamins@kohadev:~/kohaclone$ perl t/db_dependent/Reserves.t 
1..4
ok 1 - use C4::Reserves;
# 
# Creating biblio instance for testing.
# Creating item instance for testing.
 INSERT INTO borrowers SET   borrowers.firstname = ? ,  borrowers.dateenrolled = ? ,  borrowers.userid = ? ,  borrowers.categorycode = ? ,  borrowers.surname = ? ,  borrowers.dateexpiry = ? ,  borrowers.password = ? ,  borrowers.cardnumber = ? ,  borrowers.branchcode = ? my firstname,2013-06-04,myfirstname.mysurname,S,my surname,2021-09-04,!,CARDNUMBER42,CPL at /home/jcamins/kohaclone/C4/SQLHelper.pm line 182.
SELECT borrowers.*, categories.category_type, categories.description
    FROM borrowers 
    LEFT JOIN categories on borrowers.categorycode=categories.categorycode WHERE borrowernumber = ? 75 at /home/jcamins/kohaclone/C4/Members.pm line 556.
SELECT borrowers.*, categories.category_type, categories.description
    FROM borrowers 
    LEFT JOIN categories on borrowers.categorycode=categories.categorycode WHERE borrowernumber = ? 75 at /home/jcamins/kohaclone/C4/Members.pm line 556.
not ok 2 - CheckReserves Test 1
#   Failed test 'CheckReserves Test 1'
#   at t/db_dependent/Reserves.t line 66.
SELECT borrowers.*, categories.category_type, categories.description
    FROM borrowers 
    LEFT JOIN categories on borrowers.categorycode=categories.categorycode WHERE borrowernumber = ? 75 at /home/jcamins/kohaclone/C4/Members.pm line 556.
not ok 3 - CheckReserves Test 2
#   Failed test 'CheckReserves Test 2'
#   at t/db_dependent/Reserves.t line 69.
SELECT borrowers.*, categories.category_type, categories.description
    FROM borrowers 
    LEFT JOIN categories on borrowers.categorycode=categories.categorycode WHERE borrowernumber = ? 75 at /home/jcamins/kohaclone/C4/Members.pm line 556.
not ok 4 - CheckReserves Test 3
#   Failed test 'CheckReserves Test 3'
#   at t/db_dependent/Reserves.t line 72.
# Deleting item testing instance.
# Deleting biblio testing instance.
# Deleting borrower.
# Looks like you failed 3 tests of 4.
Comment 3 Jonathan Druart 2013-06-05 08:11:02 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2013-06-05 08:12:31 UTC
Jared, thank you for testing.

I confirm the tests pass here :-/
Could you retry with the last patch please? It replaces ok with is. We should have a more readable failure.
Comment 5 Jonathan Druart 2013-06-05 08:13:23 UTC Comment hidden (obsolete)
Comment 6 Jared Camins-Esakov 2013-06-07 13:00:49 UTC
(In reply to comment #4)
> Jared, thank you for testing.
> 
> I confirm the tests pass here :-/
> Could you retry with the last patch please? It replaces ok with is. We
> should have a more readable failure.

I'm still getting failures:
jcamins@kohadev:~/kohaclone$ prove t/db_dependent/Reserves.t 
t/db_dependent/Reserves.t .. 1/4 # 
# Creating biblio instance for testing.
# Creating item instance for testing.
 INSERT INTO borrowers SET   borrowers.firstname = ? ,  borrowers.dateenrolled = ? ,  borrowers.userid = ? ,  borrowers.categorycode = ? ,  borrowers.surname = ? ,  borrowers.dateexpiry = ? ,  borrowers.password = ? ,  borrowers.cardnumber = ? ,  borrowers.branchcode = ? my firstname,2013-06-07,myfirstname.mysurname,S,my surname,2021-09-07,!,CARDNUMBER42,CPL at /home/jcamins/kohaclone/C4/SQLHelper.pm line 182.
SELECT borrowers.*, categories.category_type, categories.description
    FROM borrowers 
    LEFT JOIN categories on borrowers.categorycode=categories.categorycode WHERE borrowernumber = ? 78 at /home/jcamins/kohaclone/C4/Members.pm line 556.
SELECT borrowers.*, categories.category_type, categories.description
    FROM borrowers 
    LEFT JOIN categories on borrowers.categorycode=categories.categorycode WHERE borrowernumber = ? 78 at /home/jcamins/kohaclone/C4/Members.pm line 556.

#   Failed test 'CheckReserves Test 1'
#   at t/db_dependent/Reserves.t line 67.
#          got: ''
#     expected: 'Reserved'
SELECT borrowers.*, categories.category_type, categories.description
    FROM borrowers 
    LEFT JOIN categories on borrowers.categorycode=categories.categorycode WHERE borrowernumber = ? 78 at /home/jcamins/kohaclone/C4/Members.pm line 556.
t/db_dependent/Reserves.t .. 3/4 
#   Failed test 'CheckReserves Test 2'
#   at t/db_dependent/Reserves.t line 70.
#          got: ''
#     expected: 'Reserved'
SELECT borrowers.*, categories.category_type, categories.description
    FROM borrowers 
    LEFT JOIN categories on borrowers.categorycode=categories.categorycode WHERE borrowernumber = ? 78 at /home/jcamins/kohaclone/C4/Members.pm line 556.

#   Failed test 'CheckReserves Test 3'
#   at t/db_dependent/Reserves.t line 73.
#          got: ''
#     expected: 'Reserved'
# Deleting item testing instance.
# Deleting biblio testing instance.
# Deleting borrower.
# Looks like you failed 3 tests of 4.
t/db_dependent/Reserves.t .. Dubious, test returned 3 (wstat 768, 0x300)
Failed 3/4 subtests 

Test Summary Report
-------------------
t/db_dependent/Reserves.t (Wstat: 768 Tests: 4 Failed: 3)
  Failed tests:  2-4
  Non-zero exit status: 3
Files=1, Tests=4,  1 wallclock secs ( 0.03 usr  0.01 sys +  0.60 cusr  0.06 csys =  0.70 CPU)
Result: FAIL
Comment 7 Jonathan Druart 2013-06-07 15:26:06 UTC
(In reply to comment #6)
> (In reply to comment #4)
> > Jared, thank you for testing.
> > 
> > I confirm the tests pass here :-/
> > Could you retry with the last patch please? It replaces ok with is. We
> > should have a more readable failure.
> 
> I'm still getting failures:

Yes, it is normal, I did not change the way to retrieve data.

I am really sorry but the output does not give information for debugging this.
I think it is caused by your data.
I use the database generated with the script provided on bug 10337 (depends on bug 10298). Like that the database just contains sample data.
Comment 8 Jonathan Druart 2013-06-14 13:45:39 UTC
(In reply to comment #6)
> I'm still getting failures:

Are these tests pass with the master's ut file?
Comment 9 Jared Camins-Esakov 2013-06-14 13:50:44 UTC
(In reply to comment #8)
> (In reply to comment #6)
> > I'm still getting failures:
> 
> Are these tests pass with the master's ut file?

No. Isn't that the point of the patch, that the unit tests should pass even if there isn't any data for them?
Comment 10 Jonathan Druart 2013-06-14 14:01:04 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #6)
> > > I'm still getting failures:
> > 
> > Are these tests pass with the master's ut file?
> 
> No. Isn't that the point of the patch, that the unit tests should pass even
> if there isn't any data for them?

Yes of course, but in fact the only data I create is the borrower. So if the DB contains at least 1 borrower, this patch is useless.
Comment 11 Kyle M Hall 2013-06-21 13:09:33 UTC Comment hidden (obsolete)
Comment 12 Chris Cormack 2013-07-07 07:46:39 UTC
Created attachment 19448 [details] [review]
Bug 10289: UT: Reserves.t needs to create its own data

Try before the patch:
prove t/db_dependent/Reserves.t

And after, it should produce:
  t/db_dependent/Reserves.t .. 1/4 #
  # Creating biblio instance for testing.
  # Creating item instance for testing.
  # Deleting item testing instance.
  # Deleting biblio testing instance.
  # Deleting borrower.
  t/db_dependent/Reserves.t .. ok
  All tests successful.
  Files=1, Tests=4,  1 wallclock secs ( 0.02 usr  0.01 sys +  0.39 cusr  0.02 csys =  0.44 CPU)
  Result: PASS

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 13 Galen Charlton 2013-07-08 14:20:32 UTC
(In reply to Jonathan Druart from comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (In reply to comment #6)
> > > > I'm still getting failures:
> > > 
> > > Are these tests pass with the master's ut file?
> > 
> > No. Isn't that the point of the patch, that the unit tests should pass even
> > if there isn't any data for them?
> 
> Yes of course, but in fact the only data I create is the borrower. So if the
> DB contains at least 1 borrower, this patch is useless.

That's a little too strong of a requirement, IMO, as I, for one, and I suspect most developers, would like to be able to run the DB-dependent tests in our normal testing databases.

I rather suspect what Jared is running into is that his test database already has a patron whose cardnumber is CARDNUMBER42.

I think that the requirement that tests should "create their own data" has a more general expression: "tests should make as few assumptions about the state of the database as possible".  What this means in this specific case is that the cardnumber for the patron created for this test should be set in such a way as to be guaranteed unique for that test run.
Comment 14 Galen Charlton 2013-07-08 14:32:11 UTC
Pushed to master.  Thanks, Jonathan!

I've also pushed two follow-ups, one to wrap the tests in a transaction and another to remove the hard-coded patron cardnumber, which doesn't need to be set to any specific value for the tests to work.
Comment 15 Jonathan Druart 2013-07-08 14:51:56 UTC
(In reply to Galen Charlton from comment #13)
> > Yes of course, but in fact the only data I create is the borrower. So if the
> > DB contains at least 1 borrower, this patch is useless.
> That's a little too strong of a requirement, IMO, as I, for one, and I
> suspect most developers, would like to be able to run the DB-dependent tests
> in our normal testing databases.

Currently nobody is able to launch them, and the goal is to launch tests with sample data.
They don't insert patron, so I think we have to have some prerequisites.

> I rather suspect what Jared is running into is that his test database
> already has a patron whose cardnumber is CARDNUMBER42.

Yes, it was maybe to "generic" :)

(In reply to Galen Charlton from comment #14)
> Pushed to master.  Thanks, Jonathan!
> 
> I've also pushed two follow-ups, one to wrap the tests in a transaction and
> another to remove the hard-coded patron cardnumber, which doesn't need to be
> set to any specific value for the tests to work.

Thanks for the followups!
But now tests fail if a borrower exists with a cardnumber == NULL (and I don't have any idea how it's happen, but I got 1 in my DB :))
Comment 16 Jonathan Druart 2013-07-08 14:55:06 UTC
(In reply to Jonathan Druart from comment #15)
> But now tests fail if a borrower exists with a cardnumber == NULL (and I
> don't have any idea how it's happen, but I got 1 in my DB :))

Forget that, they pass!
Comment 17 Tomás Cohen Arazi 2013-07-16 17:30:18 UTC
This patch has been pushed to 3.12.x, will be in 3.12.2.

Thanks Jonathan!
Comment 18 Bernardo Gonzalez Kriegel 2013-08-04 13:41:37 UTC
Pushed to 3.10.x, will be in 3.10.10