Bug 18712 - make test is failing with an empty DB - t/Matcher.t
Summary: make test is failing with an empty DB - t/Matcher.t
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (command-line installer) (show other bugs)
Version: master
Hardware: All All
: P3 normal (vote)
Assignee: Jonathan Druart
QA Contact: Nick Clemens
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-01 03:29 UTC by Bob Ewart
Modified: 2017-11-26 16:26 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Full log of 'make test' including STDERR and STDOUT (45.99 KB, text/x-log)
2017-06-01 03:29 UTC, Bob Ewart
Details
Bug 18712: Fix t/Matcher.t if DB is empty (1.19 KB, patch)
2017-11-02 13:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18712: Remove unecessary fixtures (1.76 KB, patch)
2017-11-02 13:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18712: Fix t/Matcher.t if DB is empty (1.29 KB, patch)
2017-11-06 17:27 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18712: Remove unecessary fixtures (1.80 KB, patch)
2017-11-06 17:27 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18712: Added missing done_testing (605 bytes, patch)
2017-11-06 17:27 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18712: Fix t/Matcher.t if DB is empty (1.35 KB, patch)
2017-11-13 13:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18712: Remove unecessary fixtures (1.86 KB, patch)
2017-11-13 13:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18712: QA Followup - Add count of tests (642 bytes, patch)
2017-11-13 13:44 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Bob Ewart 2017-06-01 03:29:40 UTC
Created attachment 63893 [details]
Full log of 'make test' including STDERR and STDOUT

Build Koha from the tarball.  Answer no to 'run the database-dependent test suite (no, yes) no.

Running 'make test' generates many errors trying to access the database. Most of the tests which do generate DBD::mysql::st errors still show up as 'ok'.  See the attached log.

t/Matcher.t and t/Prices.t do fail

Test Summary Report
-------------------
t/Matcher.t                       (Wstat: 768 Tests: 12 Failed: 3)
  Failed tests:  3-5
  Non-zero exit status: 3
t/Prices.t                        (Wstat: 65280 Tests: 5 Failed: 1)
  Failed test:  5
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 16 tests but ran 5.
Files=100, Tests=11528, 95 wallclock secs ( 0.89 usr  0.16 sys + 81.23 cusr  3.06 csys = 85.34 CPU)
Result: FAIL
Failed 2/100 test programs. 4/11528 subtests failed.
Makefile:9572: recipe for target 'test_dynamic' failed
make: *** [test_dynamic] Error 255
Comment 1 Mason James 2017-06-01 06:09:10 UTC
> t/Matcher.t and t/Prices.t do fail
> 
> Test Summary Report
> -------------------
> t/Matcher.t                       (Wstat: 768 Tests: 12 Failed: 3)
>   Failed tests:  3-5
>   Non-zero exit status: 3
> t/Prices.t                        (Wstat: 65280 Tests: 5 Failed: 1)
>   Failed test:  5
>   Non-zero exit status: 255
>   Parse errors: Bad plan.  You planned 16 tests but ran 5.
> Files=100, Tests=11528, 95 wallclock secs ( 0.89 usr  0.16 sys + 81.23 cusr 
> 3.06 csys = 85.34 CPU)
> Result: FAIL
> Failed 2/100 test programs. 4/11528 subtests failed.
> Makefile:9572: recipe for target 'test_dynamic' failed
> make: *** [test_dynamic] Error 255



the Prices.t test does seem to be currently failing on jenkins..

https://jenkins.koha-community.org/job/Koha_Master_U16/lastBuild/consoleText
Build #171 (31-May-2017 02:11:06)

#   Failed test 'No tests run for subtest "Configuration 1: 0 0"'
#   at t/Prices.t line 155.
Can't call method "listincgst" on an undefined value at /srv/jenkins/workspace/Koha_Master_U16/C4/Acquisition.pm line 2883.
# Looks like your test exited with 255 just after 5.
[23:35:52] t/Prices.t ................................................ 
Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 12/16 subtests
Comment 2 int 2017-10-25 21:26:02 UTC
'make test' fails differently depending on the database (with 'run the database-dependent test suite' is set to 'no').
If he can't connect to a database (e.g. 'Access denied'), Koha_ExternalContent_OverDrive.t and XSLT.t fail.
If he can connect to an empty database or a database without data (but with the tables), Matcher.t and Prices.t fail. This seems to be your case.

With a "real" database with data, Matcher.t seems to work. I didn't investigate this further, but if you import kohastructure.sql, e.g.
> mysql -u root -p kohatest < installer/data/mysql/kohastructure.sql
and everything from installer/data/mysql/mandatory and installer/data/mysql/en, it should work. (If I didn't miss something.)

Prices.t seems to rely on some testdata, see the fixture data in t/Prices.t (fixtures_ok).
If there is a database-connection available, he seem to want this data from the database (and not the mock anymore). So if you set it up in the database, the test also works:
> INSERT INTO aqbooksellers (id, name, listincgst, invoiceincgst)
> VALUES (1, '0 0', 0, 0), (2, '0 1', 0, 1), (3, '1 0', 1, 0), (4, '1 1', 1, 1);
I would assume that if 'run the database-dependent test suite' is set to 'no', 'make test' ignores even working database credentials in koha-conf.xml. This doesn't seem to be the case right now. I don't know if this is a general problem or if this bug should be split into several for each failing test (Koha_ExternalContent_OverDrive.t, XSLT.t, Matcher.t and Prices.t).
Comment 3 Jonathan Druart 2017-11-02 13:22:06 UTC
We have 2 problems here:
1/ t/Matcher.t is failing with an empty DB, the data are not mocked correctly (Prices.t has been fixed earlier, maybe with bug 18289)
2/ We access DB during compilation time, which is quite bad.
From C4::Auth, we retrieve system preference values in the BEGIN block:
  62     $cas       = C4::Context->preference('casAuthentication');
  64     $caslogout = C4::Context->preference('casLogout');
This should be rewritten and fixed, but it does not prevent the tests to pass, but raises the "DBI connect failed" warning.
Comment 4 Jonathan Druart 2017-11-02 13:24:16 UTC
Created attachment 68890 [details] [review]
Bug 18712: Fix t/Matcher.t if DB is empty

t/Matcher.t is failing with an empty DB, the data are not mocked
correctly

Test plan:
drop your DB and recreate it without any data and prove t/Matcher.t
Comment 5 Jonathan Druart 2017-11-02 13:28:50 UTC
Created attachment 68891 [details] [review]
Bug 18712: Remove unecessary fixtures

2 fixtures are in a subroutine that is never called, it can be removed
Comment 6 M. Tompsett 2017-11-06 17:27:00 UTC
Created attachment 68985 [details] [review]
Bug 18712: Fix t/Matcher.t if DB is empty

t/Matcher.t is failing with an empty DB, the data are not mocked
correctly

Test plan:
drop your DB and recreate it without any data and prove t/Matcher.t

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 7 M. Tompsett 2017-11-06 17:27:03 UTC
Created attachment 68986 [details] [review]
Bug 18712: Remove unecessary fixtures

2 fixtures are in a subroutine that is never called, it can be removed

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 8 M. Tompsett 2017-11-06 17:27:06 UTC
Created attachment 68987 [details] [review]
Bug 18712: Added missing done_testing

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 9 M. Tompsett 2017-11-06 17:27:56 UTC
Though, I think the last two patches probable belong on their own bug.
Comment 10 Nick Clemens 2017-11-13 13:37:51 UTC
Created attachment 69119 [details] [review]
Bug 18712: Fix t/Matcher.t if DB is empty

t/Matcher.t is failing with an empty DB, the data are not mocked
correctly

Test plan:
drop your DB and recreate it without any data and prove t/Matcher.t

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 11 Nick Clemens 2017-11-13 13:37:54 UTC
Created attachment 69120 [details] [review]
Bug 18712: Remove unecessary fixtures

2 fixtures are in a subroutine that is never called, it can be removed

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 12 Nick Clemens 2017-11-13 13:44:15 UTC
Created attachment 69121 [details] [review]
Bug 18712: QA Followup - Add count of tests

Before patches we did 47 tests or none, we should plan the same number
of tests now
Comment 13 Nick Clemens 2017-11-13 13:44:56 UTC
(In reply to M. Tompsett from comment #8)
> Created attachment 68987 [details] [review] [review]
> Bug 18712: Added missing done_testing
> 
> Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Added followup, we can count these tests so should have a planned number
Comment 14 M. Tompsett 2017-11-13 15:26:25 UTC
(In reply to Nick Clemens from comment #13)
> (In reply to M. Tompsett from comment #8)
> > Created attachment 68987 [details] [review] [review] [review]
> > Bug 18712: Added missing done_testing
> > 
> > Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
> 
> Added followup, we can count these tests so should have a planned number

Not that I'll challenge this, but using done_testing is better, in my opinion, in that it prevents test conflicts when multiple bugs add tests to the same test file. I've encountered this a couple times when working on bugs related to tests.
Comment 15 Tomás Cohen Arazi 2017-11-13 15:54:26 UTC
(In reply to M. Tompsett from comment #14)
> (In reply to Nick Clemens from comment #13)
> > (In reply to M. Tompsett from comment #8)
> > > Created attachment 68987 [details] [review] [review] [review] [review]
> > > Bug 18712: Added missing done_testing
> > > 
> > > Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
> > 
> > Added followup, we can count these tests so should have a planned number
> 
> Not that I'll challenge this, but using done_testing is better, in my
> opinion, in that it prevents test conflicts when multiple bugs add tests to
> the same test file. I've encountered this a couple times when working on
> bugs related to tests.

Maybe koha-devel is a good place to discuss such a change.
Comment 16 Jonathan Druart 2017-11-26 16:26:02 UTC
Pushed to master for 17.11, thanks to everybody involved!