Bug 15323 - ./t/Prices.t fails without a valid database
Summary: ./t/Prices.t fails without a valid database
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 15084
Blocks: 20466
  Show dependency treegraph
 
Reported: 2015-12-06 14:35 UTC by Mason James
Modified: 2018-03-23 16:08 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 15323: Use fixtures for the active currency (1.88 KB, patch)
2016-03-30 10:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15323: Use fixtures for the active currency (1.99 KB, patch)
2016-03-30 15:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[PASSED QA] Bug 15323: Use fixtures for the active currency (2.06 KB, patch)
2016-03-30 21:24 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mason James 2015-12-06 14:35:48 UTC
more info to come...
Comment 1 Mason James 2015-12-08 01:59:48 UTC
hmm, a bit odd.. but git-bisect says this commit broke the test?!

commit bc668dccd3af3fba14e04a764499b0d762c63455
Author: Tomas Cohen Arazi <tomascohen@theke.io>
Date:   Fri Nov 6 11:21:56 2015 -0300
    Bug 15151: Avoid DB access to load C4::Members

any ideas, good people?
Comment 2 Jonathan Druart 2016-01-05 13:59:09 UTC
t/Prices.t is not data-related:

% sudo service mysql stop
[ ok ] Stopping MariaDB database server: mysqld.

% prove t/Prices.t
t/Prices.t .. ok     
All tests successful.
Files=1, Tests=16,  2 wallclock secs ( 0.02 usr  0.00 sys +  1.40 cusr  0.04 csys =  1.46 CPU)
Result: PASS
Comment 3 Liz Rea 2016-01-05 20:30:02 UTC
I've seen this too, but I didn't have a chance to look into it. 

Maybe it is ok in isolation, but not ok when run with other tests?
Comment 4 Jonathan Druart 2016-01-06 09:05:03 UTC
Let me know if you manage to recreate.
Comment 5 Larry Baerveldt 2016-01-15 17:36:07 UTC
I've seen this error too when attempting to build a package. Version tested was 3.20.06. Here's the test failure message:

DBI connect('dbname=koha;host=localhost;port=3306','kohaadmin',...) failed: Can't connect to local MySQL server through socket '/var/run/mysqld/mysqld.sock' (2) at /usr/share/perl5/DBIx/Connector.pm line 32.
Can't call method "do" on unblessed reference at /tmp/buildd/koha-3.20.06+20160114144139.2d762685/blib/PERL_MODULE_DIR/C4/Context.pm line 793.
    # Child (Configuration 1: 0 0) exited without calling finalize()

#   Failed test 'Configuration 1: 0 0'
#   at /usr/share/perl/5.20/Test/Builder.pm line 276.
# Looks like you planned 16 tests but ran 5.
# Looks like you failed 1 test of 5 run.
# Looks like your test exited with 255 just after 5.
t/Prices.t .......................... 
Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 12/16 subtests 
t/Print.t ........................... ok

...
Test Summary Report
-------------------
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.
Comment 6 Colin Campbell 2016-03-29 14:47:35 UTC
On current master running prove t/Prices.t with the database up or down I get:

~/kohaclone$ prove t/Prices.t
t/Prices.t .. 1/16 DBIx::Class::Storage::DBI::_warn_undetermined_driver(): This version of DBIC does not yet seem to supply a driver for your particular RDBMS and/or connection method ('Mock'). While we will attempt to continue anyway, the results are likely to be underwhelming. Please upgrade DBIC, and if this message does not go away, file a bugreport including the following info:
{
  DBD => "DBD::Mock",
  DBD_VER => "1.45",
  DBIC_DRIVER => "DBIx::Class::Storage::DBI",
  DBIC_DSN => "DBI:Mock:",
  DBIC_VER => "0.082820",
  DBI_VER => "1.633"
}
DBIx::Class::Storage::DBI::sql_maker(): Your storage class (DBIx::Class::Storage::DBI) does not set sql_limit_dialect and you have not supplied an explicit limit_dialect in your connection_info. DBIC will attempt to use the GenericSubQ dialect, which works on most databases but can be (and often is) painfully slow. Please file an RT ticket against 'DBIx::Class::Storage::DBI' at /home/cc/kohaclone/Koha/Objects.pm line 150
DBI bind_columns: invalid number of arguments: got handle + 0, expected handle + between 1 and -1
Usage: $h->bind_columns(\$var1 [, \$var2, ...]) at /usr/share/perl5/vendor_perl/DBIx/Class/Storage/DBI/Cursor.pm line 129.
    # Child (Configuration 1: 0 0) exited without calling finalize()

#   Failed test 'Configuration 1: 0 0'
#   at /usr/share/perl5/vendor_perl/Test/Builder.pm line 279.
# Looks like you planned 16 tests but ran 5.
# Looks like you failed 1 test of 5 run.
# Looks like your test exited with 255 just after 5.
t/Prices.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 12/16 subtests 

Test Summary Report
-------------------
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=1, Tests=5,  1 wallclock secs ( 0.02 usr  0.00 sys +  1.15 cusr  0.06 csys =  1.23 CPU)

(Perl version 5.22.1)

However if I remove the line:

    my $context_module = t::lib::Mocks::mock_dbh;

all tests pass (if DB up)

looks like the mocking is broken in some versions of the libs
Comment 7 Jonathan Druart 2016-03-30 10:29:53 UTC
Colin,
What you describe has been introduced by bug 15084, fix coming.
Comment 8 Jonathan Druart 2016-03-30 10:32:49 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2016-03-30 11:15:05 UTC
Why not move this test to db_dependent?
Comment 10 Jonathan Druart 2016-03-30 11:37:54 UTC
(In reply to Marcel de Rooy from comment #9)
> Why not move this test to db_dependent?

Because we can easily avoid this dependency? :)
Comment 11 Marcel de Rooy 2016-03-30 11:57:23 UTC
(In reply to Jonathan Druart from comment #10)
> (In reply to Marcel de Rooy from comment #9)
> > Why not move this test to db_dependent?
> 
> Because we can easily avoid this dependency? :)

Seems we past that station here.
Comment 12 Jonathan Druart 2016-03-30 12:32:53 UTC
(In reply to Marcel de Rooy from comment #11)
> (In reply to Jonathan Druart from comment #10)
> > (In reply to Marcel de Rooy from comment #9)
> > > Why not move this test to db_dependent?
> > 
> > Because we can easily avoid this dependency? :)
> 
> Seems we past that station here.

What do you mean?
The price formatting should not be dependent on the data, so if we can keep this independence, let's keep it :)
Comment 13 Tomás Cohen Arazi 2016-03-30 13:14:30 UTC
(In reply to Marcel de Rooy from comment #9)
> Why not move this test to db_dependent?

We should really have mocked unit tests for everything. Most of the problems we have with (db_dependent) tests are side-effects from previous tests. So, everything is much more complicated to debug than it should.
Comment 14 Marcel de Rooy 2016-03-30 13:56:29 UTC
(In reply to Tomás Cohen Arazi from comment #13)
> We should really have mocked unit tests for everything. Most of the problems
> we have with (db_dependent) tests are side-effects from previous tests. So,
> everything is much more complicated to debug than it should.

Side-effects from previous tests: might depend on how you arrange your tests. You could just have the same effects when using SQLite inmemory. Some large tests may need more than one rollback.

I am not saying that we should not try to make a test db independent. But imo we need to balance the work of doing so (mocking all Context stuff etc., running Test::DBIx etc.) against the advantages we hope to achieve. And although mocking may help focus on the specific test goal, not mocking might help revealing other issues..
Comment 15 Tomás Cohen Arazi 2016-03-30 15:31:07 UTC Comment hidden (obsolete)
Comment 16 Katrin Fischer 2016-03-30 21:24:46 UTC
Created attachment 49695 [details] [review]
[PASSED QA] Bug 15323: Use fixtures for the active currency

prove t/Prices.t
fails after bug 15084 has been pushed
It's caused by
  commit 1538e9ecf47642c4974693ff499c3e95e4d71977
    Bug 15084: Replace C4::Budgets::GetCurrencies with
    Koha::Acquisition::Currencies->search

Koha::Number::Price->_format_params calls
Koha::Acquisition::Currencies->get_active, which requests the DB.
The currency data should be mocked.

Test plan:
sudo service mysql stop
prove t/Prices.t
 should return green

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Patch works as expected and passes the qa-tools tests.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 17 Brendan Gallagher 2016-03-31 18:18:24 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks!
Comment 18 Julian Maurice 2016-04-06 13:59:23 UTC
Patch pushed to 3.22.x, will be in 3.22.6
Comment 19 Frédéric Demians 2016-04-27 15:40:40 UTC
Pushed to 3.20.x, will be in 3.20.11.