Bug 14778

Summary: DBIC should create/own the DB handler
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: abl, chris, david.roberts, dcook, fridolin.somers, gmcharlt, gwilliams, josef.moravec, jweaver, kyle.m.hall, martin.renvoize, nick, olli-antti.kivilahti, robin, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14375
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14374
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15247
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 15151, 18224, 18226    
Attachments: Bug 14778: Get rid of DBIx::Connector
Bug 14778: Make sure the dbh returned is checked by DBIC
Bug 14778: Make 3 tests pass
Bug 14778: Make the installer test pass - use DBI missing
Bug 14778: Set PrintError as it was before
Bug 14778: [DO NOT WORK] Try to make DBI:Mock pass but fail
Bug 14778: Example - Replace DBI::Mock with Test::DBIx::Class - Sitemapper.t
Bug 14778: Example - Replace DBI::Mock with Test::DBIx::Class - Sitemapper.t
Bug 14778: Mocks Koha::Database->_new_schema instead of C4::Context->_new_dbh
Bug 14778: Use mock_dbh where it should be used
Bug 14778: Install fixtures for t/Calendar.t
Bug 14778: Install fixtures for t/Biblio.t
Bug 14778: Install fixtures for t/Images.t
Bug 14778: Install fixtures for t/ItemType.t
Bug 14778: Install fixtures for t/Koha.t
Bug 14778: Install fixtures for t/Letters.t
Bug 14778: Install fixtures for t/Matcher.t
Bug 14778: Install fixtures for t/Members_AttributeTypes.t
Bug 14778: Install fixtures for t/SocialData.t
Bug 14778: Make Barcodes_ValueBuilder.t db dependent
Bug 14778: Mock the dbh for t/db_dependent/Search.t
Bug 14778: Remove t/Acquisition/Invoices.t
Bug 14778: Get rid of DBIx::Connector
Bug 14778: Make sure the dbh returned is checked by DBIC
Bug 14778: Make 3 tests pass
Bug 14778: Make the installer test pass - use DBI missing
Bug 14778: Set PrintError as it was before
Bug 14778: Example - Replace DBI::Mock with Test::DBIx::Class - Sitemapper.t
Bug 14778: Mocks Koha::Database->_new_schema instead of C4::Context->_new_dbh
Bug 14778: Use mock_dbh where it should be used
Bug 14778: Install fixtures for t/Calendar.t
Bug 14778: Install fixtures for t/Biblio.t
Bug 14778: Install fixtures for t/Images.t
Bug 14778: Install fixtures for t/ItemType.t
Bug 14778: Install fixtures for t/Koha.t
Bug 14778: Install fixtures for t/Letters.t
Bug 14778: Install fixtures for t/Matcher.t
Bug 14778: Install fixtures for t/Members_AttributeTypes.t
Bug 14778: Install fixtures for t/SocialData.t
Bug 14778: Make Barcodes_ValueBuilder.t db dependent
Bug 14778: Mock the dbh for t/db_dependent/Search.t
Bug 14778: Remove t/Acquisition/Invoices.t
[PASSED QA] Bug 14778: Get rid of DBIx::Connector
[PASSED QA] Bug 14778: Make sure the dbh returned is checked by DBIC
[PASSED QA] Bug 14778: Make 3 tests pass
[PASSED QA] Bug 14778: Make the installer test pass - use DBI missing
[PASSED QA] Bug 14778: Set PrintError as it was before
[PASSED QA] Bug 14778: Example - Replace DBI::Mock with Test::DBIx::Class - Sitemapper.t
[PASSED QA] Bug 14778: Mocks Koha::Database->_new_schema instead of C4::Context->_new_dbh
[PASSED QA] Bug 14778: Use mock_dbh where it should be used
[PASSED QA] Bug 14778: Install fixtures for t/Calendar.t
[PASSED QA] Bug 14778: Install fixtures for t/Biblio.t
[PASSED QA] Bug 14778: Install fixtures for t/Images.t
[PASSED QA] Bug 14778: Install fixtures for t/ItemType.t
[PASSED QA] Bug 14778: Install fixtures for t/Koha.t
[PASSED QA] Bug 14778: Install fixtures for t/Letters.t
[PASSED QA] Bug 14778: Install fixtures for t/Matcher.t
[PASSED QA] Bug 14778: Install fixtures for t/Members_AttributeTypes.t
[PASSED QA] Bug 14778: Install fixtures for t/SocialData.t
[PASSED QA] Bug 14778: Make Barcodes_ValueBuilder.t db dependent
[PASSED QA] Bug 14778: Mock the dbh for t/db_dependent/Search.t
[PASSED QA] Bug 14778: Remove t/Acquisition/Invoices.t
Bug 14778: Make Search.t pass - replace SHOW COLUMNS

Description Jonathan Druart 2015-09-03 14:11:04 UTC
We should get rid of DBI::Connector and use the DB connection created by DBIx::Class.
Comment 1 Jonathan Druart 2015-09-03 14:11:49 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2015-09-03 14:14:02 UTC
What about this patch guys?
Comment 3 Jonathan Druart 2015-09-04 10:19:45 UTC
I confirm that this patch fixes bug 14375.

% sudo echo 'wait_timeout = 10' >> /etc/mysql/conf.d/koha.cnf
% more test.pl
use Modern::Perl;
use C4::Context;
my $dbh = C4::Context->dbh;
$dbh->{RaiseError} = 1;
$dbh->{PrintError} = 1;

say $dbh->do('SELECT * FROM systempreferences');

sleep 11;

$dbh = C4::Context->dbh;
say $dbh->do('SELECT * FROM systempreferences');

% git checkout master
% perl test.pl
518
DBD::mysql::db do failed: MySQL server has gone away [for Statement "SELECT * FROM systempreferences"] at test.pl line 12.
DBD::mysql::db do failed: MySQL server has gone away [for Statement "SELECT * FROM systempreferences"] at test.pl line 12.

% git checkout -b bug_14726; git bz apply 14726
perl test.pl
518
518

(518 prefs, really???)
Comment 4 Jonathan Druart 2015-09-04 10:32:53 UTC
% git checkout -b bug_14726; git bz apply 14726

read bug 14778
Comment 5 Jonathan Druart 2015-09-04 10:42:22 UTC
(In reply to Jonathan Druart from comment #3)
> I confirm that this patch fixes bug 14375.

Well actually I am not sure.
I have tried again and now it passes on master and not on bug_14778.
Comment 6 Jonathan Druart 2015-09-04 10:54:23 UTC
MariaDB [koha]> delete from aqbooksellers;

% more benchmark_bookseller.pl
#!/usr/bin/perl
use Modern::Perl;
use C4::Bookseller;
use Koha::Acquisition::Bookseller;


for my $i ( 1 .. 1000 ) {
    C4::Bookseller::AddBookseller({
        name => "name$i",
    });
}

my $booksellers = Koha::Acquisition::Bookseller->search;
Koha::Acquisition::Bookseller->search;

% git checkout master
% time perl benchmark_bookseller.pl
perl benchmark_bookseller.pl  4.89s user 0.14s system 8% cpu 1:00.42 total

MariaDB [koha]> delete from aqbooksellers;
% git checkout bug_14778
% time perl benchmark_bookseller.plt
perl benchmark_bookseller.pl  5.02s user 0.17s system 8% cpu 1:00.64 total
Comment 7 Jonathan Druart 2015-09-04 11:23:56 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2015-09-04 11:24:11 UTC
% git checkout bug_14788 # With the 2 patches
% time perl benchmark_bookseller.pl
perl benchmark_bookseller.pl  5.68s user 0.29s system 9% cpu 1:00.04 total
Comment 9 Jonathan Druart 2015-09-04 11:27:25 UTC
I have also tried

diff --git a/Koha/Database.pm b/Koha/Database.pm
index 014554f..619ba6e 100644
--- a/Koha/Database.pm
+++ b/Koha/Database.pm
@@ -118,7 +118,10 @@ sub schema {
     my $params = shift;
 
     unless ( $params->{new} ) {
-        return $database->{schema} if defined $database->{schema};
+        if ( defined $database->{schema} ) {
+            $database->{schema}->storage->ensure_connected
+                and return $database->{schema};
+        }
     }
 
     $database->{schema} = &_new_schema();

But
% time perl benchmark_bookseller.pl
perl benchmark_bookseller.pl  7.14s user 0.18s system 11% cpu 1:03.67 total

I am not sure it is useful, DBIC certainly checks it on its side.
Comment 10 Jonathan Druart 2015-09-04 11:30:10 UTC
Note: Encoding looks good (read/write).
Comment 11 Tomás Cohen Arazi 2015-09-07 17:27:04 UTC
Jonathan. Everything looks good to me. Can we mark it "Needs signoff"? I added Galen and Kyle to the CC
Comment 12 Jonathan Druart 2015-09-14 07:54:41 UTC
Yep, this needs to be tested widely.
Comment 13 Fridolin Somers 2015-09-15 09:33:51 UTC
Great patch, but in this case C4::Context depends on C4::Database and C4::Database depends on C4::Context :(
Should we move database handler method to C4::Database ?
I know it will change a lot of code but isn't it more logical ?
Comment 14 Jonathan Druart 2015-09-15 09:40:03 UTC
(In reply to Fridolin SOMERS from comment #13)
> Great patch, but in this case C4::Context depends on C4::Database and
> C4::Database depends on C4::Context :(
> Should we move database handler method to C4::Database ?
> I know it will change a lot of code but isn't it more logical ?

Yes of course it is.
But I would like to get this one into master and then provide a huge change to correct all occurrences.
Comment 15 Fridolin Somers 2015-09-16 07:03:47 UTC
(In reply to Jonathan Druart from comment #14)
> (In reply to Fridolin SOMERS from comment #13)
> > Great patch, but in this case C4::Context depends on C4::Database and
> > C4::Database depends on C4::Context :(
> > Should we move database handler method to C4::Database ?
> > I know it will change a lot of code but isn't it more logical ?
> 
> Yes of course it is.
> But I would like to get this one into master and then provide a huge change
> to correct all occurrences.

Oki, we are on the same page then.
I'll try it.
Comment 16 Tomás Cohen Arazi 2015-10-07 12:50:13 UTC
This patchset removes DBIx::Connector usage, but is also moving the database handler creation responsability into the Koha::Database class.

It looks great, convenient and makes a lot of sense.

But right now it breaks some tests. Probably due to problems on the tests themselves:

Test Summary Report
-------------------
t/db_dependent/Borrower.t                            (Wstat: 65280 Tests: 8 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 13 tests but ran 8.
t/db_dependent/Circulation_issuingrules.t            (Wstat: 768 Tests: 7 Failed: 3)
  Failed tests:  2, 6-7
  Non-zero exit status: 3
t/db_dependent/Installer.t                           (Wstat: 65280 Tests: 1 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 9 tests but ran 1.
t/db_dependent/Items.t                               (Wstat: 65280 Tests: 6 Failed: 1)
  Failed test:  6
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 9 tests but ran 6.
t/db_dependent/Reports_Guided.t                      (Wstat: 256 Tests: 18 Failed: 1)
  Failed test:  16
  Non-zero exit status: 1
t/db_dependent/Search.t                              (Wstat: 65280 Tests: 1 Failed: 1)
  Failed test:  1
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 4 tests but ran 1.
t/db_dependent/Sitemapper.t                          (Wstat: 1536 Tests: 14 Failed: 6)
  Failed tests:  6, 8, 10, 12-14
  Non-zero exit status: 6
Comment 17 Tomás Cohen Arazi 2015-10-07 13:41:20 UTC
Ok, a brief about the errors and causes.

Trivial:
t/db_dependent/Borrower.t: transaction race, probably TestBuilder's fault.
t/db_dependent/Installer.t: Uses DBI directly, should be easy to tweak
t/db_dependent/Items.t: transaction race, probably TestBuilder's fault.
t/db_dependent/Reports_Guided.t: DBIx doesn't raise the warning the test expect.
t/db_dependent/Search.t: It seems to be failing to connect. The code actually mocks the connection, the _new_dbh function. There shouldn't be a behaviour change, but it fails.
t/db_dependent/Sitemapper.t: It seems to be failing to connect. The code actually mocks the connection, the _new_dbh function. There shouldn't be a behaviour change, but it fails.


Might be an important issue:
t/db_dependent/Circulation_issuingrules.t: results differ from expected
Comment 18 Jonathan Druart 2015-10-08 10:42:23 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2015-10-08 10:42:29 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2015-10-08 10:42:32 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2015-10-08 10:42:36 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2015-10-15 08:42:16 UTC
Could anyone interested in this bug report have a look at the last patch and try to understand what's wrong?
Comment 23 Jonathan Druart 2015-10-15 14:18:49 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2015-10-15 14:19:34 UTC
This is the way to go, it will take me ages to update all occurrences...
Comment 25 Jonathan Druart 2015-10-15 14:56:09 UTC
Second try
- t/Acquisition/Invoice.t: I don't understand what it tested here.
- t/Barcodes_ValueBuilder.t: It does not seem to work because of the complex select in C4::Barcodes::ValueBuilder (substring_index, etc.)
DBI Exception: DBD::SQLite::db prepare failed: no such function: SUBSTRING [for Statement "SELECT MAX(CAST(SUBSTRING(barcode,-4) AS signed)) AS number FROM items WHERE barcode REGEXP ?"] at t/Barcodes_ValueBuilder.t line 56
Comment 26 Tomás Cohen Arazi 2015-10-20 02:43:02 UTC
FTR: http://lists.scsys.co.uk/pipermail/dbix-class/2006-June/001690.html

Also, this patches fixed the "MySQL server has gone away" problem in the test server.
Comment 27 Tomás Cohen Arazi 2015-10-20 02:47:39 UTC
*** Bug 15034 has been marked as a duplicate of this bug. ***
Comment 28 Jonathan Druart 2015-10-20 15:01:44 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2015-10-20 15:01:47 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2015-10-20 15:01:50 UTC Comment hidden (obsolete)
Comment 31 Jonathan Druart 2015-10-20 15:01:54 UTC Comment hidden (obsolete)
Comment 32 Jonathan Druart 2015-10-20 15:01:59 UTC Comment hidden (obsolete)
Comment 33 Jonathan Druart 2015-10-20 15:02:03 UTC Comment hidden (obsolete)
Comment 34 Jonathan Druart 2015-10-20 15:02:09 UTC Comment hidden (obsolete)
Comment 35 Jonathan Druart 2015-10-20 15:02:14 UTC Comment hidden (obsolete)
Comment 36 Jonathan Druart 2015-10-20 15:02:19 UTC Comment hidden (obsolete)
Comment 37 Jonathan Druart 2015-10-20 15:02:23 UTC Comment hidden (obsolete)
Comment 38 Jonathan Druart 2015-10-20 15:02:28 UTC Comment hidden (obsolete)
Comment 39 Jonathan Druart 2015-10-20 15:02:32 UTC Comment hidden (obsolete)
Comment 40 Jonathan Druart 2015-10-20 15:02:36 UTC Comment hidden (obsolete)
Comment 41 Jonathan Druart 2015-10-20 15:02:41 UTC Comment hidden (obsolete)
Comment 42 Jonathan Druart 2015-10-20 15:02:47 UTC Comment hidden (obsolete)
Comment 43 Jonathan Druart 2015-10-20 15:03:36 UTC
Enjoy
Comment 44 Jonathan Druart 2015-10-20 15:12:36 UTC
Note about DBIx::Class warnings:

These changes will raise warnings:

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.631"
}

They can be hidden creating a package DBIx::Class::Storage::DBI::Mock (in /usr/share/perl5/DBIx/Class/Storage/DBI/Mock.pm for instance):
===
package DBIx::Class::Storage::DBI::Mock;
use base DBIx::Class::Storage::DBI;
1;
===

We will still get:
DBIx::Class::Storage::DBI::sql_maker(): Your storage class (DBIx::Class::Storage::DBI::Mock) 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::Mock'

This could be fixed later with deeper examinations.
Comment 45 Tomás Cohen Arazi 2015-10-20 18:46:07 UTC
The only (and not minor) issue I see is: tests relying on Test::DBIx::Class only make sense in t/* (i.e. the point of db_dependent tests is that they are integration tests, using the DB for real). So the problem is that if we don't have Test::DBIx::Class packaged, then the .deb packages cannot build (they need prove t/ pass clean).

So we need to either:
(a) have Test::DBIx::Class and friends packaged (Robin?)
(b) have those tests conditional to its availability:

use Module::Load::Conditional qw/check_install/;

BEGIN {
    if ( check_install( module => 'Test::DBIx::Class' ) ) {
        plan tests => 11;
    } else {
        plan skip_all => "Need Test::DBIx::Class"
    }
}


(b) is sub optimal. We want to run the tests. But (a) needs Robin's intervention I think, so I have no clue. Help!
Comment 46 Robin Sheat 2015-10-20 22:23:29 UTC
(In reply to Tomás Cohen Arazi from comment #45)
> So we need to either:
> (a) have Test::DBIx::Class and friends packaged (Robin?)

I'll have a look to see how feasible this is. It's probably not terrible.
Comment 47 Tomás Cohen Arazi 2015-10-20 22:54:08 UTC
*** Bug 15031 has been marked as a duplicate of this bug. ***
Comment 48 Tomás Cohen Arazi 2015-10-20 23:18:09 UTC
*** Bug 13699 has been marked as a duplicate of this bug. ***
Comment 49 Martin Renvoize 2015-10-21 08:40:33 UTC
Agree with Tomas, all looks good here to me.. but he's right about either making the tests conditions or packaging up Test::DBIx::Class and friends.

Jonathan, many thanks for seeing this one through.. it's a real step in the right direction and I know was a bit of an *** to work through.
Comment 50 Jonathan Druart 2015-10-21 09:49:31 UTC
On Jessie:
  dh-make-perl --pkg-perl --build --cpan Test::DBIx::Class
build a wonderfull libtest-dbix-class-perl_0.45-1_all.deb package without any error.
Comment 51 Martin Renvoize 2015-10-23 10:35:46 UTC
Poke, just awaiting yeah or neigh from Robin on this one.. I'll happily sign everything off again once we have a definitive In/Out on packaging the test dependencies :)
Comment 52 Robin Sheat 2015-10-23 10:59:37 UTC
(In reply to Martin Renvoize from comment #51)
> Poke, just awaiting yeah or neigh from Robin on this one.. I'll happily sign
> everything off again once we have a definitive In/Out on packaging the test
> dependencies :)

Oh bother*. I'm speaking at a conference next week and spent most of my time in the past few days preparing for that. I might get on to this in the weekend, but it's hard to promise. And next week I'll be in Hobart. So sorry for not being on to this quickly.

* said Poo as he carved Eeyore's name into a black candle.
Comment 53 Martin Renvoize 2015-10-23 13:39:01 UTC
Thanks for the reply Robin.. I'll go ahead and get the ball rolling with the signoff and we can worry about the packaging stuff after that.
Comment 54 Martin Renvoize 2015-10-23 13:39:44 UTC
I'm afraid I get a QA script failure Jonathan:  

FAIL   t/db_dependent/Barcodes_ValueBuilder.t
   OK     critic
   OK     forbidden patterns
   OK     pod
   OK     spelling
   FAIL   valid
                "my" variable $DEBUG masks earlier declaration in same scope 

Any thoughts?
Comment 55 Martin Renvoize 2015-10-23 14:07:32 UTC Comment hidden (obsolete)
Comment 56 Martin Renvoize 2015-10-23 14:07:36 UTC Comment hidden (obsolete)
Comment 57 Martin Renvoize 2015-10-23 14:07:40 UTC Comment hidden (obsolete)
Comment 58 Martin Renvoize 2015-10-23 14:07:44 UTC Comment hidden (obsolete)
Comment 59 Martin Renvoize 2015-10-23 14:07:47 UTC Comment hidden (obsolete)
Comment 60 Martin Renvoize 2015-10-23 14:07:51 UTC Comment hidden (obsolete)
Comment 61 Martin Renvoize 2015-10-23 14:07:55 UTC Comment hidden (obsolete)
Comment 62 Martin Renvoize 2015-10-23 14:07:58 UTC Comment hidden (obsolete)
Comment 63 Martin Renvoize 2015-10-23 14:08:02 UTC Comment hidden (obsolete)
Comment 64 Martin Renvoize 2015-10-23 14:08:06 UTC Comment hidden (obsolete)
Comment 65 Martin Renvoize 2015-10-23 14:08:09 UTC Comment hidden (obsolete)
Comment 66 Martin Renvoize 2015-10-23 14:08:13 UTC Comment hidden (obsolete)
Comment 67 Martin Renvoize 2015-10-23 14:08:17 UTC Comment hidden (obsolete)
Comment 68 Martin Renvoize 2015-10-23 14:08:21 UTC Comment hidden (obsolete)
Comment 69 Martin Renvoize 2015-10-23 14:08:24 UTC Comment hidden (obsolete)
Comment 70 Martin Renvoize 2015-10-23 14:08:28 UTC Comment hidden (obsolete)
Comment 71 Martin Renvoize 2015-10-23 14:08:32 UTC Comment hidden (obsolete)
Comment 72 Martin Renvoize 2015-10-23 14:08:36 UTC Comment hidden (obsolete)
Comment 73 Martin Renvoize 2015-10-23 14:08:39 UTC Comment hidden (obsolete)
Comment 74 Martin Renvoize 2015-10-23 14:08:43 UTC Comment hidden (obsolete)
Comment 75 Martin Renvoize 2015-10-23 14:11:47 UTC
Signed everything off, we decided the failure in the qa script was in fact an error in the modules itself which only presents in the qa script run (the tests do all pass if run with prove)

Well tidy up the offending module later.. it's not a killer.. more of a style thing.
Comment 76 Kyle M Hall 2015-10-23 14:55:57 UTC
Created attachment 43857 [details] [review]
[PASSED QA] Bug 14778: Get rid of DBIx::Connector

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 77 Kyle M Hall 2015-10-23 14:56:06 UTC
Created attachment 43858 [details] [review]
[PASSED QA] Bug 14778: Make sure the dbh returned is checked by DBIC

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 78 Kyle M Hall 2015-10-23 14:56:11 UTC
Created attachment 43859 [details] [review]
[PASSED QA] Bug 14778: Make 3 tests pass

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 79 Kyle M Hall 2015-10-23 14:56:15 UTC
Created attachment 43860 [details] [review]
[PASSED QA] Bug 14778: Make the installer test pass - use DBI missing

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 80 Kyle M Hall 2015-10-23 14:56:19 UTC
Created attachment 43861 [details] [review]
[PASSED QA] Bug 14778: Set PrintError as it was before

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 81 Kyle M Hall 2015-10-23 14:56:23 UTC
Created attachment 43862 [details] [review]
[PASSED QA] Bug 14778: Example - Replace DBI::Mock with Test::DBIx::Class - Sitemapper.t

We can use Test::DBIx::Class to install fixtures before our tests.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 82 Kyle M Hall 2015-10-23 14:56:28 UTC
Created attachment 43863 [details] [review]
[PASSED QA] Bug 14778: Mocks Koha::Database->_new_schema instead of C4::Context->_new_dbh

C4::Context::_new_dbh does not exist anymore.
Koha::Database::_new_schema should be mocked instead.

Will fix:
- t/00-load.t
- t/Breeding.t
- t/ImportBatch.t
- t/Message.t
- t/Overdues.t
- t/Prices.t
- t/RotatingCollections.t
- t/Search.t
- t/SuggestionEngine_AuthorityFile.t
- t/XSLT.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 83 Kyle M Hall 2015-10-23 14:56:32 UTC
Created attachment 43864 [details] [review]
[PASSED QA] Bug 14778: Use mock_dbh where it should be used

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 84 Kyle M Hall 2015-10-23 14:56:36 UTC
Created attachment 43865 [details] [review]
[PASSED QA] Bug 14778: Install fixtures for t/Calendar.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 85 Kyle M Hall 2015-10-23 14:56:39 UTC
Created attachment 43866 [details] [review]
[PASSED QA] Bug 14778: Install fixtures for t/Biblio.t

Note that it already passed before

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 86 Kyle M Hall 2015-10-23 14:56:43 UTC
Created attachment 43867 [details] [review]
[PASSED QA] Bug 14778: Install fixtures for t/Images.t

Note that this tests file were completely buggy before.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 87 Kyle M Hall 2015-10-23 14:56:46 UTC
Created attachment 43868 [details] [review]
[PASSED QA] Bug 14778: Install fixtures for t/ItemType.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 88 Kyle M Hall 2015-10-23 14:56:51 UTC
Created attachment 43869 [details] [review]
[PASSED QA] Bug 14778: Install fixtures for t/Koha.t

Warning: This patch modifies a module!
What's the need of the binary function here?
The data are case insensitive, so no need to use this mysql function.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 89 Kyle M Hall 2015-10-23 14:56:55 UTC
Created attachment 43870 [details] [review]
[PASSED QA] Bug 14778: Install fixtures for t/Letters.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 90 Kyle M Hall 2015-10-23 14:57:00 UTC
Created attachment 43871 [details] [review]
[PASSED QA] Bug 14778: Install fixtures for t/Matcher.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 91 Kyle M Hall 2015-10-23 14:57:05 UTC
Created attachment 43872 [details] [review]
[PASSED QA] Bug 14778: Install fixtures for t/Members_AttributeTypes.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 92 Kyle M Hall 2015-10-23 14:57:09 UTC
Created attachment 43873 [details] [review]
[PASSED QA] Bug 14778: Install fixtures for t/SocialData.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 93 Kyle M Hall 2015-10-23 14:57:13 UTC
Created attachment 43874 [details] [review]
[PASSED QA] Bug 14778: Make Barcodes_ValueBuilder.t db dependent

The get_barcode subroutines call very mysql specific functions and it's
not possible to easily use fixtures here.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 94 Kyle M Hall 2015-10-23 14:57:17 UTC
Created attachment 43875 [details] [review]
[PASSED QA] Bug 14778: Mock the dbh for t/db_dependent/Search.t

No need to mock the items' columns (?)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 95 Kyle M Hall 2015-10-23 14:57:21 UTC
Created attachment 43876 [details] [review]
[PASSED QA] Bug 14778: Remove t/Acquisition/Invoices.t

Using Test::DBIx::Class now, we cannot do what we did in this file with DBD::Mock.
Since the Invoice[s] subroutines are already tested in
t/db_dependent/Acquisition/Invoices.t there is no special needs to have
these ones.
Instead of loosing 2 hours on this file, I would prefer to remove it.
Feel free to provide a counter patch.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 96 Tomás Cohen Arazi 2015-10-23 15:03:13 UTC
Patches pushed to master.

Thanks Jonathan!
Comment 97 Jonathan Druart 2015-10-26 14:00:59 UTC
Created attachment 44003 [details] [review]
Bug 14778: Make Search.t pass - replace SHOW COLUMNS

For an unkown reason, when executed from a test file, the 'SHOW COLUMNS'
statement does not return anything.
We need to retrieve the column list from the DBIx::Class resultset.
Comment 98 Robin Sheat 2015-11-02 03:12:44 UTC
(In reply to Jonathan Druart from comment #50)
> On Jessie:
>   dh-make-perl --pkg-perl --build --cpan Test::DBIx::Class
> build a wonderfull libtest-dbix-class-perl_0.45-1_all.deb package without
> any error.

I suspect you had an unclean environment.

The dh-make-perl says:

Needs the following modules for which there are no debian packages available:
 - Test::mysqld
 - MooseX::Attribute::ENV
 - Test::PostgreSQL
 - DateTime::Format::Pg
 - DBIx::Class::Schema::PopulateMore
 - DBIx::Class::UUIDColumns
 - DBIx::Class::TimeStamp
 - Data::Visitor

The build (wheezy in a pbuild env) refuses to go anywhere:

The following packages have unmet dependencies:
 pbuilder-satisfydepends-dummy : Depends: libdbix-class-schema-populatemore-perl (>= 0.16) which is a virtual package.
                                 Depends: libdbix-class-timestamp-perl (>= 0.13) but it is not going to be installed.
                                 Depends: libdbix-class-uuidcolumns-perl (>= 0.02005) which is a virtual package.
                                 Depends: libmoosex-attribute-env-perl (>= 0.01) which is a virtual package.
                                 Depends: libmoosex-types-perl (>= 0.23) but it is not going to be installed.
                                 Depends: libossp-uuid-perl (>= 1.215) but it is not going to be installed.
                                 Depends: libsql-translator-perl (>= 0.11006) but it is not going to be installed.
                                 Depends: libtest-deep-perl (>= 0.106) but it is not going to be installed.
                                 Depends: libtest-mysqld-perl (>= 0.14) which is a virtual package.
                                 Depends: libtest-postgresql-perl (>= 0.09) which is a virtual package.
                                 Depends: perl (>= 5.15.2) but 5.14.2-21+deb7u2 is installed.

I don't think it's possible to make this work in a supported environment. There's no way I'm backporting a new perl version, for one :)
Comment 99 Tomás Cohen Arazi 2015-11-06 17:24:51 UTC
*** Bug 14375 has been marked as a duplicate of this bug. ***
Comment 100 Jonathan Druart 2015-11-11 13:08:44 UTC
*** Bug 14600 has been marked as a duplicate of this bug. ***
Comment 101 Jonathan Druart 2016-02-29 11:36:35 UTC
Quick note about
  Bug 14778: Install fixtures for t/Koha.t

time prove t/Koha.t now returns ~15s instead of ~2sec before
Comment 102 David Cook 2017-05-22 06:54:09 UTC
I'm using the following:

use t::lib::Mocks;

BEGIN {
    t::lib::Mocks::mock_dbh;
}

However, I'm still getting the warning:

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:

I'm using the newest DBIx::Class, so I don't think it's a case of upgrading DBIC.
Comment 103 Katrin Fischer 2020-01-07 23:28:47 UTC
*** Bug 14742 has been marked as a duplicate of this bug. ***