Bug 15152 - t/db_dependent/Reports_Guided.t should not depend on existing data
Summary: t/db_dependent/Reports_Guided.t should not depend on existing data
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-11-06 15:12 UTC by Jonathan Druart
Modified: 2016-06-21 21:40 UTC (History)
4 users (show)

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


Attachments
Bug 15152: Try and make Guided Reports related tests pass (21.77 KB, patch)
2015-11-06 15:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED OFF] Bug 15152: Try and make Guided Reports related tests pass (21.84 KB, patch)
2015-11-06 16:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15152: t/db_dependent/Reports_Guided.t should not depend on existing data (2.46 KB, patch)
2015-11-16 18:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15152: t/db_dependent/Reports_Guided.t should not depend on existing data (2.52 KB, patch)
2015-11-16 18:54 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2015-11-06 15:12:11 UTC
Jenkins is currently failing on

t_db_dependent_Reports_Guided_t.filter takes report id (393)
Failing for the past 1 build (Since Unstable#190 )
Took 2 ms.
Error Message

not ok 8 - filter takes report id

Standard Output

1..19
ok 1 - use C4::Reports::Guided;
ok 2 - Not breaking subqueries
ok 3 - Returns correct default offset
ok 4 - Returns correct default LIMIT
ok 5 - Correctly removes only final LIMIT
ok 6 - Returns correct default offset
ok 7 - Returns correct extracted LIMIT
ok 8 - Correctly removes only final LIMIT (with offset)
ok 9 - Returns correct extracted offset
ok 10 - Returns correct extracted LIMIT
ok 11 - Not breaking simple queries
ok 12 - Returns correct default offset
ok 13 - Returns correct default LIMIT
ok 14 - Correctly removes LIMIT in simple case
ok 15 - Returns correct default offset
ok 16 - Returns correct extracted LIMIT
ok 17 - Correctly removes LIMIT in simple case (with offset)
ok 18 - Returns correct extracted offset
ok 19 - Returns correct extracted LIMIT

Which does not make sense. I suspect that there is a mismatch with the different filenames.
Comment 1 Jonathan Druart 2015-11-06 15:17:02 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2015-11-06 16:15:43 UTC Comment hidden (obsolete)
Comment 3 Nick Clemens 2015-11-06 16:17:30 UTC
Test plan used:
1 - prove t/db_dependent/Reports_Guided.t - Fail test 8
2 - apply patches
3 - prove -v t/db_dependent/Reports/Guided.t 
4 - all tests pass
Comment 4 Jonathan Druart 2015-11-06 16:32:29 UTC
(In reply to Nick Clemens from comment #3)
> Test plan used:
> 1 - prove t/db_dependent/Reports_Guided.t - Fail test 8

Oh, really? What is the error?
Comment 5 Nick Clemens 2015-11-06 17:38:51 UTC
Well...I got it before, but can no longer reproduce
Comment 6 Katrin Fischer 2015-11-08 21:41:45 UTC
Interestingly, the test did fail the first time for me as well, and always after that:

prove t/db_dependent/Reports_Guided.t
t/db_dependent/Reports_Guided.t .. 3/18 
#   Failed test 'filter takes report id'
#   at t/db_dependent/Reports_Guided.t line 70.
#          got: '2'
#     expected: '1'
# Looks like you failed 1 test of 18.

Test Summary Report
-------------------
t/db_dependent/Reports_Guided.t (Wstat: 256 Tests: 18 Failed: 1)
  Failed test:  8
  Non-zero exit status: 1
Files=1, Tests=18,  2 wallclock secs ( 0.03 usr  0.00 sys +  1.09 cusr  0.05 csys =  1.17 CPU)
Result: FAIL
Comment 7 Katrin Fischer 2015-11-08 21:45:48 UTC
That should have read: ... and passed always after that.
Comment 8 Tomás Cohen Arazi 2015-11-08 22:55:10 UTC
(In reply to Katrin Fischer from comment #7)
> That should have read: ... and passed always after that.

Yeah, jenkins loads a fresh 3.20.0 db each time, that's why it always fails.
Comment 9 Tomás Cohen Arazi 2015-11-09 20:26:12 UTC
Can we have this tests split so non-db_dependent tests are in t/?
Comment 10 Jonathan Druart 2015-11-10 08:14:43 UTC
I need someone to send me a DB (or a way to create it) where I can reproduce the failure.
Comment 11 Tomás Cohen Arazi 2015-11-16 17:42:42 UTC
(In reply to Jonathan Druart from comment #10)
> I need someone to send me a DB (or a way to create it) where I can reproduce
> the failure.

Here you go:
[To set the failure scenario]
$ vagrant up jessie
$ vagrant ssh jessie
$ sudo -s
$ mysql
> DROP DATABASE koha_kohadev; CREATE DATABASE koha_kohadev; \q
$ zcat /vagrant/koha_3_20_00.sql.gz | mysql koha_kohadev
$ exit
$ sudo koha-shell kohadev
$ cd kohaclone
$ cat /vagrant/reports_guided_sequence.txt | xargs prove

now run the tests:
$ prove t/db_dependent/Reports_Guided.t
=> FAIL: The tests fail like this:
kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Reports_Guided.t 
t/db_dependent/Reports_Guided.t .. 3/18 
#   Failed test 'filter takes report id'
#   at t/db_dependent/Reports_Guided.t line 70.
#          got: '2'
#     expected: '1'
# Looks like you failed 1 test of 18.
t/db_dependent/Reports_Guided.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/18 subtests 

Test Summary Report
-------------------
t/db_dependent/Reports_Guided.t (Wstat: 256 Tests: 18 Failed: 1)
  Failed test:  8
  Non-zero exit status: 1
Files=1, Tests=18,  1 wallclock secs ( 0.03 usr  0.01 sys +  0.86 cusr  0.39 csys =  1.29 CPU)
Result: FAIL

run them again
=> SUCCESS: They now pass (i.e. side effects).
Comment 12 Tomás Cohen Arazi 2015-11-16 17:48:33 UTC
(In reply to Tomás Cohen Arazi from comment #11)
> (In reply to Jonathan Druart from comment #10)
> > I need someone to send me a DB (or a way to create it) where I can reproduce
> > the failure.

I forgot the updatedatabase.pl step:

$ sudo koha-shell kohadev
$ cd kohaclone
$ perl installer/data/mysql/updatedatabase.pl

Also, the needed file can be grabbed like this:

(on your kohadevbox dir)
(kohadevbox)$ wget https://theke.io/static/reports_guided_sequence.txt
(kohadevbox)$ wget https://theke.io/static/koha_3_20_00.sql.gz
Comment 13 Tomás Cohen Arazi 2015-11-16 18:33:54 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2015-11-16 18:54:36 UTC
Created attachment 44884 [details] [review]
Bug 15152: t/db_dependent/Reports_Guided.t should not depend on existing data

The current implementation relies on existing borrowers on the DB.
Even when the DB itself doesn't have a FK constraint, the function
that retrieves the reports from the DB relies on get_saved_reports_base_query
which creates a query that JOINs the full query with the 'borrowers' table, on
the borrowernumber. So it is doomed to fail on some scenarios.
Specially because the current tests expect specific borrowernumber values
to be present (1, 2 and 3).

To test:
[To set the failure scenario]
 $ vagrant up jessie
 $ vagrant ssh jessie
 $ wget https://theke.io/static/reports_guided_sequence.txt -O /vagrant/reports_guided_sequence.txt
 $ wget https://theke.io/static/koha_3_20_00.sql.gz -O /vagrant/koha_3_20_00.sql.gz
 $ sudo -s
 $ mysql
 > DROP DATABASE koha_kohadev; CREATE DATABASE koha_kohadev; \q
 $ zcat /vagrant/koha_3_20_00.sql.gz | mysql koha_kohadev
 $ exit
 $ sudo koha-shell kohadev
 $ cd kohaclone
 $ cat /vagrant/reports_guided_sequence.txt | xargs prove

- now run the tests:
 $ prove t/db_dependent/Reports_Guided.t
=> FAIL: The tests fail

- Apply the patch
- [re create the scenario]
- Run the test:
  $ prove t/db_dependent/Reports_Guided.t
=> SUCCESS: Tests now pass
- Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Tomás Cohen Arazi 2015-11-16 19:00:38 UTC
Patch pushed to master.