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.
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
Created attachment 39871 [details] [review] Bug 14334: t/db_dependent/Bookseller.t AutoCommit fix TEST PLAN ---------- 1) git reset --hard origin/master 2) prove t/db_dependent/Bookseller.t -- horrible failure about transaction 3) apply this patch 4) prove t/db_dependent/Bookseller.t -- no issues 5) koha qa test tools.
Created attachment 39872 [details] [review] Bug 14334: t/db_dependent/Budgets.t AutoCommit fix TEST PLAN ---------- 1) git reset --hard origin/master 2) prove t/db_dependent/Budgets.t -- horrible failure about transaction 3) apply this patch 4) prove t/db_dependent/Budgets.t -- no issues 5) koha qa test tools.
Created attachment 39873 [details] [review] Bug 14334: t/db_dependent/Letters.t AutoCommit fix TEST PLAN ---------- 1) git reset --hard origin/master 2) prove t/db_dependent/Letters.t -- horrible failure about transaction 3) apply this patch 4) prove t/db_dependent/Letters.t -- no issues 5) koha qa test tools.
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
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?
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?
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
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
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
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?
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
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
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
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.
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
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
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
Borrowed some of this patches so we can rely on jenkins. Will be back to this once finished. Thanks Mark!
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.
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
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
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?
(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.
(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.
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
(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.
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>
git grep -l RaiseError|wc -l 29 What about these DBI remainders ?
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>
(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.
(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 :)
Indeed :) I do not have a strong opinion about them, I can re-add them if you think we must keep them.
Awesome work all! Pushed to master for 19.05
(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..
(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 :)
Pushed to 18.11.x for 18.11.01
backported to 18.05 for 18.05.08
Pushed to 17.11.x for 17.11.14
Follow-ups on bug 22001 are waiting for signoff for more that a year now.