Bug 14334 - DBI fighting DBIx over Autocommit in tests
Summary: DBI fighting DBIx over Autocommit in tests
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 22001
  Show dependency treegraph
 
Reported: 2015-06-04 19:35 UTC by Mark Tompsett
Modified: 2020-02-14 11:01 UTC (History)
11 users (show)

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


Attachments
Bug 14334: t/db_dependent/Bookseller.t AutoCommit fix (1.41 KB, patch)
2015-06-04 20:03 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/Budgets.t AutoCommit fix (1.39 KB, patch)
2015-06-04 20:10 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/Letters.t AutoCommit fix (1.45 KB, patch)
2015-06-04 20:14 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/Accounts.t AutoCommit fix (1.63 KB, patch)
2015-06-04 20:28 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/Auth_with_cas.t AutoCommit fix (1.30 KB, patch)
2015-06-04 20:47 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/Auth_with_ldap.t AutoCommit fix (1.22 KB, patch)
2015-06-04 21:02 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/AuthoritiesMarc.t AutoCommit fix (1.33 KB, patch)
2015-06-04 21:05 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/Biblio.t AutoCommit fix (1.67 KB, patch)
2015-06-05 04:20 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/Borrower.t AutoCommit fix (1.36 KB, patch)
2015-06-05 04:36 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/Borrower_Debarments.t AutoCommit fix (1.52 KB, patch)
2015-06-05 04:40 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/Borrower_Discharge.t AutoCommit fix (1.99 KB, patch)
2015-06-05 05:04 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/Borrower_Files.t AutoCommit fix (1.23 KB, patch)
2015-06-05 05:08 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/Borrowers.t AutoCommit fix (1.18 KB, patch)
2015-06-05 05:11 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/Branch.t AutoCommit fix (1.27 KB, patch)
2015-06-05 05:16 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/Charset.t AutoCommit fix (1.37 KB, patch)
2015-06-10 16:35 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/Circulation.t AutoCommit fix (2.44 KB, patch)
2015-06-10 23:08 UTC, Mark Tompsett
Details | Diff | Splinter Review
[ FOR DISCUSSION ] Bug 14334: Add a Database module for db dependent tests (1.89 KB, patch)
2015-06-16 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14334: Remove AutoCommit from tests (30.63 KB, patch)
2018-12-07 17:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14334: Remove AutoCommit from tests (30.90 KB, patch)
2018-12-12 15:20 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14334: Remove AutoCommit from tests (30.78 KB, patch)
2018-12-14 08:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Tompsett 2015-06-04 19:35:06 UTC
As per an email on the KohaDev list, tcohen pointed out the problem.
gmcharlt pointed out a solution here: http://paste.lisp.org/display/149194
--- BEGIN PASTE ---
diff --git a/t/db_dependent/Letters.t b/t/db_dependent/Letters.t
index f53e75b..8599c3f 100644
--- a/t/db_dependent/Letters.t
+++ b/t/db_dependent/Letters.t
@@ -45,11 +45,15 @@ use t::lib::Mocks;
 use Koha::DateUtils qw( dt_from_string output_pref );
 use Koha::Acquisition::Order;
 use Koha::Acquisition::Bookseller;
+use Koha::Database;
 
 my $dbh = C4::Context->dbh;
 
+my $database = Koha::Database->new();
+my $schema = $database->schema();
+$schema->storage->txn_begin();
+
 # Start transaction
-$dbh->{AutoCommit} = 0;
 $dbh->{RaiseError} = 1;
 
 $dbh->do(q|DELETE FROM letter|);
@@ -341,4 +345,4 @@ is($err, 1, "Successfully sent claim");
 is($mail{'To'}, 'testemail@mydomain.com', "mailto correct in sent claim");
 is($mail{'Message'}, 'my vendor|John Smith|Ordernumber ' . $ordernumber . ' (Silence in the library) (1 ordered)', 'Claim notice text constructed successfully');
 
-$dbh->rollback;
+$schema->storage->txn_rollback();
--- END PASTE ---

And I have volunteered to do it.
Comment 1 Mark Tompsett 2015-06-04 19:55:10 UTC
Shoot! Those things which are NOT tests scare me, particularly rebuild_zebra.pl -- going to investigate further.

$ git grep -i -l autocommit
installer/data/mysql/updatedatabase.pl
misc/batchRebuildItemsTables.pl
misc/commit_file.pl
misc/cronjobs/create_koc_db.pl
misc/cronjobs/delete_patrons.pl
misc/cronjobs/update_totalissues.pl
misc/link_bibs_to_authorities.pl
misc/maintenance/borrowers-force-messaging-defaults
misc/maintenance/fix_accountlines_date.pl
misc/migration_tools/bulkmarcimport.pl
misc/migration_tools/create_analytical_rel.pl
misc/migration_tools/rebuild_zebra.pl
misc/stage_file.pl
test/progressbar.pl
tools/batch_delete_records.pl
tools/manage-marc-import.pl
tools/stage-marc-import.pl
t/db_dependent/Accounts.t
t/db_dependent/Acquisition/FillWithDefaultValues.t
t/db_dependent/Auth.t
t/db_dependent/Auth_with_cas.t
t/db_dependent/Auth_with_ldap.t
t/db_dependent/AuthoritiesMarc.t
t/db_dependent/BackgroundJob.t
t/db_dependent/Biblio.t
t/db_dependent/Bookseller.t
t/db_dependent/Borrower.t
t/db_dependent/Borrower_Debarments.t
t/db_dependent/Borrower_Discharge.t
t/db_dependent/Borrower_Files.t
t/db_dependent/Borrowers.t
t/db_dependent/Branch.t
t/db_dependent/Budgets.t
t/db_dependent/Charset.t
t/db_dependent/Circulation.t
t/db_dependent/Circulation/CheckIfIssuedToPatron.t
t/db_dependent/Circulation/CheckValidBarcode.t
t/db_dependent/Circulation/GetIssues.t
t/db_dependent/Circulation/IsItemIssued.t
t/db_dependent/Circulation/IssuingRules/maxsuspensiondays.t
t/db_dependent/Circulation/Returns.t
t/db_dependent/Circulation_Branch.t
t/db_dependent/Circulation_Issuingrule.t
t/db_dependent/Circulation_OfflineOperation.t
t/db_dependent/Circulation_issue.t
t/db_dependent/Circulation_transfers.t
t/db_dependent/ClassSource.t
t/db_dependent/ColumnsSettings.t
t/db_dependent/Contract.t
t/db_dependent/CourseReserves.t
t/db_dependent/Creators/Lib.t
t/db_dependent/Csv.t
t/db_dependent/FrameworkPlugin.t
t/db_dependent/Holds.t
t/db_dependent/Holds/LocalHoldsPriority.t
t/db_dependent/Holds/RevertWaitingStatus.t
t/db_dependent/HoldsQueue.t
t/db_dependent/Holidays.t
t/db_dependent/ILSDI_Services.t
t/db_dependent/ImportBatch.t
t/db_dependent/Items.t
t/db_dependent/Items/GetItemsForInventory.t
t/db_dependent/Items_DelItem.t
t/db_dependent/Koha.t
t/db_dependent/Koha/GetDailyQuote.t
t/db_dependent/Koha/IsKohaFieldLinked.t
t/db_dependent/Koha/Item/Search/Field.t
t/db_dependent/Koha_Misc_Files.t
t/db_dependent/Koha_borrower_modifications.t
t/db_dependent/Labels/t_Batch.t
t/db_dependent/Languages.t
t/db_dependent/Letters.t
t/db_dependent/Letters/GetLetterTemplates.t
t/db_dependent/Letters/GetLettersAvailableForALibrary.t
t/db_dependent/Log.t
t/db_dependent/MarcModificationTemplates.t
t/db_dependent/Members.t
t/db_dependent/Members/AddEnrolmentFeeIfNeeded.t
t/db_dependent/Members/GetAllIssues.t
t/db_dependent/Members/GetPendingIssues.t
t/db_dependent/Members/IssueSlip.t
t/db_dependent/Members_Attributes.t
t/db_dependent/MungeMarcPrice.t
t/db_dependent/NewsChannels.t
t/db_dependent/OAI/Sets.t
t/db_dependent/Overdues.t
t/db_dependent/Ratings.t
t/db_dependent/Record/marcrecord2csv.t
t/db_dependent/Reports_Guided.t
t/db_dependent/Reserves.t
t/db_dependent/Review.t
t/db_dependent/RotatingCollections.t
t/db_dependent/Search/History.t
t/db_dependent/Search_SearchHistory.t
t/db_dependent/Serials.t
t/db_dependent/Serials/Claims.t
t/db_dependent/Serials/Frequency.t
t/db_dependent/Serials/GetNextDate.t
t/db_dependent/Serials/GetNextSeq.t
t/db_dependent/Serials/Numberpattern.t
t/db_dependent/Serials_2.t
t/db_dependent/ShelfBrowser.t
t/db_dependent/Stats.t
t/db_dependent/Suggestions.t
t/db_dependent/Template/Plugin/Branches.t
t/db_dependent/Template/Plugin/Categories.t
t/db_dependent/Utils/Datatables_Members.t
t/db_dependent/Utils/Datatables_Virtualshelves.t
t/db_dependent/VirtualShelves.t
t/db_dependent/XISBN.t
t/db_dependent/sysprefs.t
Comment 2 Mark Tompsett 2015-06-04 20:03:47 UTC Comment hidden (obsolete)
Comment 3 Mark Tompsett 2015-06-04 20:10:18 UTC Comment hidden (obsolete)
Comment 4 Mark Tompsett 2015-06-04 20:14:25 UTC Comment hidden (obsolete)
Comment 5 Mark Tompsett 2015-06-04 20:28:09 UTC
Created attachment 39874 [details] [review]
Bug 14334: t/db_dependent/Accounts.t AutoCommit fix

There is no easy way to trigger an obvious problem, as this test
uses Koha::Borrowers and other DBIx things, which may or may not
be causing it to not blow up.

TEST PLAN
---------
1) apply patch
2) git diff origin/master
   -- Are the additions/subtractions similar to
      http://paste.lisp.org/display/149194?
3) run koha qa test tools
Comment 6 Mark Tompsett 2015-06-04 20:37:36 UTC
t/db_dependent/Acquisition/FillWithDefaultValues.t seems to only read from the DB. I think the removal of the dbh code in the test file is the correct solution, and add a comment about how to do AutoCommit = 0 stuff if a test needs to write. Thoughts?
Comment 7 Mark Tompsett 2015-06-04 20:40:38 UTC
t/db_dependent/Auth.t seems to only read from the DB. I think the removal of the dbh code in the test file is the correct solution, and add a comment about how to do AutoCommit = 0 stuff if a test needs to write. Thoughts?
Comment 8 Mark Tompsett 2015-06-04 20:47:00 UTC
Created attachment 39875 [details] [review]
Bug 14334: t/db_dependent/Auth_with_cas.t AutoCommit fix

There is no easy way to trigger an obvious problem.

TEST PLAN
---------
1) apply patch
2) git diff origin/master
   -- Are the additions/subtractions similar to
      http://paste.lisp.org/display/149194?
3) run koha qa test tools
Comment 9 Mark Tompsett 2015-06-04 21:02:04 UTC
Created attachment 39876 [details] [review]
Bug 14334: t/db_dependent/Auth_with_ldap.t AutoCommit fix

There is no easy way to trigger an obvious problem.

TEST PLAN
---------
1) apply patch
2) git diff origin/master
   -- Are the additions/subtractions similar to
      http://paste.lisp.org/display/149194?
3) run koha qa test tools
Comment 10 Mark Tompsett 2015-06-04 21:05:43 UTC
Created attachment 39877 [details] [review]
Bug 14334: t/db_dependent/AuthoritiesMarc.t AutoCommit fix

There is no easy way to trigger an obvious problem.

TEST PLAN
---------
1) apply patch
2) git diff origin/master
   -- Are the additions/subtractions similar to
      http://paste.lisp.org/display/149194?
3) run koha qa test tools
Comment 11 Mark Tompsett 2015-06-05 04:04:55 UTC
I was not expecting to see: $dbh->{AutoCommit} = 1;
in t/db_dependent/BackgroundJob.t at all.
Is this correct? Why even include it then?
Comment 12 Mark Tompsett 2015-06-05 04:20:24 UTC
Created attachment 39879 [details] [review]
Bug 14334: t/db_dependent/Biblio.t AutoCommit fix

There is no easy way to trigger an obvious problem.

TEST PLAN
---------
1) apply patch
2) git diff origin/master
   -- Are the additions/subtractions similar to
      http://paste.lisp.org/display/149194?
   -- this is a special case, in that the transaction
      rollbacks seem to terminate the transaction in DBIx.
      So there needs to be a transaction for each subtest.
3) run koha qa test tools
Comment 13 Mark Tompsett 2015-06-05 04:36:28 UTC
Created attachment 39880 [details] [review]
Bug 14334: t/db_dependent/Borrower.t AutoCommit fix

There is no easy way to trigger an obvious problem.

TEST PLAN
---------
1) apply patch
2) git diff origin/master
   -- Are the additions/subtractions similar to
      http://paste.lisp.org/display/149194?
   -- I added explicit rollback, since I do not know the
      default behaviour of DBIx.
3) run koha qa test tools
Comment 14 Mark Tompsett 2015-06-05 04:40:35 UTC
Created attachment 39881 [details] [review]
Bug 14334: t/db_dependent/Borrower_Debarments.t AutoCommit fix

There is no easy way to trigger an obvious problem.

TEST PLAN
---------
1) apply patch
2) git diff origin/master
   -- Are the additions/subtractions similar to
      http://paste.lisp.org/display/149194?
3) run koha qa test tools
Comment 15 Mark Tompsett 2015-06-05 05:04:55 UTC
Created attachment 39882 [details] [review]
Bug 14334: t/db_dependent/Borrower_Discharge.t AutoCommit fix

There is no easy way to trigger an obvious problem.

TEST PLAN
---------
1) apply patch
2) git diff origin/master
   -- Are the additions/subtractions similar to
      http://paste.lisp.org/display/149194?
3) run koha qa test tools

NOTE: I am uncertain of what yohann.dufour@biblibre.com
      means in his FIXME comments. No problems experienced
      under Debian Jessie.
Comment 16 Mark Tompsett 2015-06-05 05:08:54 UTC
Created attachment 39883 [details] [review]
Bug 14334: t/db_dependent/Borrower_Files.t AutoCommit fix

There is no easy way to trigger an obvious problem.

TEST PLAN
---------
1) apply patch
2) git diff origin/master
   -- Are the additions/subtractions similar to
      http://paste.lisp.org/display/149194?
3) run koha qa test tools
Comment 17 Mark Tompsett 2015-06-05 05:11:44 UTC
Created attachment 39884 [details] [review]
Bug 14334: t/db_dependent/Borrowers.t AutoCommit fix

There is no easy way to trigger an obvious problem.

TEST PLAN
---------
1) apply patch
2) git diff origin/master
   -- Are the additions/subtractions similar to
      http://paste.lisp.org/display/149194?
3) run koha qa test tools
Comment 18 Mark Tompsett 2015-06-05 05:16:15 UTC
Created attachment 39885 [details] [review]
Bug 14334: t/db_dependent/Branch.t AutoCommit fix

There is no easy way to trigger an obvious problem.

TEST PLAN
---------
1) apply patch
2) git diff origin/master
   -- Are the additions/subtractions similar to
      http://paste.lisp.org/display/149194?
3) run koha qa test tools
Comment 19 Tomás Cohen Arazi 2015-06-07 22:07:25 UTC
Borrowed some of this patches so we can rely on jenkins.
Will be back to this once finished.

Thanks Mark!
Comment 20 Mark Tompsett 2015-06-07 22:58:28 UTC
Okay. Sorry it may take longer. Just was in a car accident. If anyone wishes to continue. What you see is what you get. I haven't made it further yet.
Comment 21 Mark Tompsett 2015-06-10 16:35:35 UTC
Created attachment 40080 [details] [review]
Bug 14334: t/db_dependent/Charset.t AutoCommit fix

There is no easy way to trigger an obvious problem.

TEST PLAN
---------
1) apply patch
2) git diff origin/master
   -- Are the additions/subtractions similar to
      http://paste.lisp.org/display/149194?
3) run koha qa test tools
Comment 22 Mark Tompsett 2015-06-10 23:08:03 UTC
Created attachment 40086 [details] [review]
Bug 14334: t/db_dependent/Circulation.t AutoCommit fix

There is no easy way to trigger an obvious problem.
Confirmed with khall that tweaking those two other lines to
ensure the autocommit fix will be correct.
(http://irc.koha-community.org/koha/2015-06-10#i_1686796)

TEST PLAN
---------
1) apply patch
2) git diff origin/master
   -- Are the additions/subtractions similar to
      http://paste.lisp.org/display/149194?
3) run koha qa test tools
Comment 23 Jonathan Druart 2015-06-16 14:31:03 UTC
Created attachment 40184 [details] [review]
[ FOR DISCUSSION ] Bug 14334: Add a Database module for db dependent tests

What about something like that to avoid c/p?
Comment 24 Mark Tompsett 2015-06-16 15:02:36 UTC
(In reply to Jonathan Druart from comment #23)
> Created attachment 40184 [details] [review] [review]
> [ FOR DISCUSSION ] Bug 14334: Add a Database module for db dependent tests
> 
> What about something like that to avoid c/p?

So, what are you thinking exactly, because I still see all the files needing modification if we make some db_dependent test module(s)?

Plus, this isn't completely C&P. There was a correction or two as well.
Comment 25 Mark Tompsett 2016-07-06 15:49:59 UTC
(In reply to Jonathan Druart from comment #23)
> Created attachment 40184 [details] [review] [review]
> [ FOR DISCUSSION ] Bug 14334: Add a Database module for db dependent tests
> 
> What about something like that to avoid c/p?

+1 I like this. If I get back to this bug, I'll use that.
Comment 26 Jonathan Druart 2018-12-07 17:23:11 UTC
Created attachment 82989 [details] [review]
Bug 14334: Remove AutoCommit from tests

And use a DBIx transaction instead.

Test plan:
prove that the test files modified by this patch are passing
Comment 27 Jonathan Druart 2018-12-07 17:23:51 UTC
(In reply to Jonathan Druart from comment #23)
> Created attachment 40184 [details] [review] [review]
> [ FOR DISCUSSION ] Bug 14334: Add a Database module for db dependent tests
> 
> What about something like that to avoid c/p?

That was wrong, sorry. Sometimes we need to rollback before END.
Comment 28 Josef Moravec 2018-12-12 15:20:59 UTC
Created attachment 83129 [details] [review]
Bug 14334: Remove AutoCommit from tests

And use a DBIx transaction instead.

Test plan:
prove that the test files modified by this patch are passing

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 29 Marcel de Rooy 2018-12-14 08:41:38 UTC
git grep -l RaiseError|wc -l
29
What about these DBI remainders ?
Comment 30 Marcel de Rooy 2018-12-14 08:42:23 UTC
Created attachment 83201 [details] [review]
Bug 14334: Remove AutoCommit from tests

And use a DBIx transaction instead.

Test plan:
prove that the test files modified by this patch are passing

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Jonathan Druart 2018-12-14 14:06:18 UTC
(In reply to Marcel de Rooy from comment #29)
> git grep -l RaiseError|wc -l
> 29
> What about these DBI remainders ?

That should not hurt, I'd even say it should be set for all tests.
Comment 32 Marcel de Rooy 2018-12-14 14:23:12 UTC
(In reply to Jonathan Druart from comment #31)
> (In reply to Marcel de Rooy from comment #29)
> > git grep -l RaiseError|wc -l
> > 29
> > What about these DBI remainders ?
> 
> That should not hurt, I'd even say it should be set for all tests.

You removed 34 :)
Comment 33 Jonathan Druart 2018-12-14 14:39:32 UTC
Indeed :)
I do not have a strong opinion about them, I can re-add them if you think we must keep them.
Comment 34 Nick Clemens 2018-12-14 15:32:41 UTC
Awesome work all!

Pushed to master for 19.05
Comment 35 Marcel de Rooy 2018-12-14 15:56:31 UTC
(In reply to Jonathan Druart from comment #33)
> Indeed :)
> I do not have a strong opinion about them, I can re-add them if you think we
> must keep them.

Not so sure if we need them at all.
From DBIx::Class::Storage::DBI:
This Storage driver normally installs its own HandleError, sets RaiseError and ShowErrorStatement on, and sets PrintError off on all database handles..
Comment 36 Jonathan Druart 2018-12-14 16:00:20 UTC
(In reply to Marcel de Rooy from comment #35)
> (In reply to Jonathan Druart from comment #33)
> > Indeed :)
> > I do not have a strong opinion about them, I can re-add them if you think we
> > must keep them.
> 
> Not so sure if we need them at all.
> From DBIx::Class::Storage::DBI:
> This Storage driver normally installs its own HandleError, sets RaiseError
> and ShowErrorStatement on, and sets PrintError off on all database handles..

Koha::Database:
 95             RaiseError => $ENV{DEBUG} ? 1 : 0,

We disable it :)
Comment 37 Martin Renvoize 2018-12-17 12:09:15 UTC
Pushed to 18.11.x for 18.11.01
Comment 38 Lucas Gass 2018-12-26 20:22:05 UTC
backported to 18.05 for 18.05.08
Comment 39 Fridolin Somers 2019-01-02 10:22:42 UTC
Pushed to 17.11.x for 17.11.14
Comment 40 Jonathan Druart 2020-02-14 09:00:57 UTC
Follow-ups on bug 22001 are waiting for signoff for more that a year now.