Bug 14334 - DBI fighting DBIx over Autocommit in tests
Summary: DBI fighting DBIx over Autocommit in tests
Status: ASSIGNED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: M. Tompsett
QA Contact: Galen Charlton
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-06-04 19:35 UTC by M. Tompsett
Modified: 2016-07-06 15:49 UTC (History)
3 users (show)

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


Attachments
Bug 14334: t/db_dependent/Bookseller.t AutoCommit fix (1.41 KB, patch)
2015-06-04 20:03 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/Budgets.t AutoCommit fix (1.39 KB, patch)
2015-06-04 20:10 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/Letters.t AutoCommit fix (1.45 KB, patch)
2015-06-04 20:14 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/Accounts.t AutoCommit fix (1.63 KB, patch)
2015-06-04 20:28 UTC, M. 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, M. 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, M. Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/AuthoritiesMarc.t AutoCommit fix (1.33 KB, patch)
2015-06-04 21:05 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/Biblio.t AutoCommit fix (1.67 KB, patch)
2015-06-05 04:20 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/Borrower.t AutoCommit fix (1.36 KB, patch)
2015-06-05 04:36 UTC, M. 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, M. 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, M. 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, M. Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/Borrowers.t AutoCommit fix (1.18 KB, patch)
2015-06-05 05:11 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/Branch.t AutoCommit fix (1.27 KB, patch)
2015-06-05 05:16 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/Charset.t AutoCommit fix (1.37 KB, patch)
2015-06-10 16:35 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14334: t/db_dependent/Circulation.t AutoCommit fix (2.44 KB, patch)
2015-06-10 23:08 UTC, M. 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

Note You need to log in before you can comment on or make changes to this bug.
Description M. 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 M. 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 M. Tompsett 2015-06-04 20:03:47 UTC Comment hidden (obsolete)
Comment 3 M. Tompsett 2015-06-04 20:10:18 UTC Comment hidden (obsolete)
Comment 4 M. Tompsett 2015-06-04 20:14:25 UTC Comment hidden (obsolete)
Comment 5 M. 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 M. 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 M. 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 M. 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 M. 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 M. 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 M. 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 M. 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 M. 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 M. 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 M. 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 M. 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 M. 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 M. 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 M. 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 M. 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 M. 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 M. 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 M. 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.