Description
Jonathan Druart
2023-12-06 15:52:44 UTC
Failing again with today's run: https://jenkins.koha-community.org/job/Koha_Master/2762/ t_db_dependent_TestBuilder_t.build_sample_biblio() tests (2071) Looking here now Created attachment 159670 [details] [review] Bug 35507: Clear cache key for enabled plugins at deletion KitchenSink is still in cache key and causes other tests to fail. Test plan: Install some after_biblio_action plugin. Delete the plugin without this patch. Run t/db_dependent/TestBuilder.t [FAIL] Delete plugin with this patch Run t/db_dependent/TestBuilder.t [PASS] Will still improve the test plan a bit. But it should work: not ok 1 - No encoding warnings! # Failed test 'No encoding warnings!' # at t/db_dependent/TestBuilder.t line 519. # found warning: Failed to load Koha::Plugin::TestItemBarcodeTransform: Could not find or check module 'Koha::Plugin::TestItemBarcodeTransform' at /usr/share/koha/Koha/Plugins.pm line 124. # didn't expect to find a warning # Looks like you failed 1 test of 1. With this patch, Plugins.t will remove KitchenSink from the enabled cache key too, resolving issues in other tests. Should we clear on all plugin-related test files? subtests? Should we generalize this cleanup step? I know we rollback, and then call ->delete() in some places, we should just add this cache cleaning too. $ ls t/db_dependent/Koha/Plugins/ Account_hooks.t* Broken.t* Patron.t* BackgroundJob.t* Circulation_hooks.t* Plugins.t* Barcode_transform_hooks.t* Holds_hooks.t* Recall_hooks.t* Biblio_and_Items_plugin_hooks.t* KitchenSinkPlugin.kpz authority_hooks.t* (In reply to Tomás Cohen Arazi from comment #7) > Should we clear on all plugin-related test files? subtests? Should we > generalize this cleanup step? I know we rollback, and then call ->delete() > in some places, we should just add this cache cleaning too. > > $ ls t/db_dependent/Koha/Plugins/ > Account_hooks.t* Broken.t* Patron.t* > BackgroundJob.t* Circulation_hooks.t* Plugins.t* > Barcode_transform_hooks.t* Holds_hooks.t* > Recall_hooks.t* > Biblio_and_Items_plugin_hooks.t* KitchenSinkPlugin.kpz > authority_hooks.t* BackgroundJob.t does: $schema->storage->txn_rollback; Koha::Plugins::Methods->delete; Thats definitely in the wrong order. Same for Barcode_transform_hooks.t Copy/paste? Recall_hooks.t Biblio_and_Items_plugin_hooks.t Etc Yes, we should do some cleanup here ! A valid question is however why did we not see this failure much more? It surely depends on the excecution order of test scripts. This still needs a bit more digging. Will try now. Most of the time it is wrong. If you would have plugins on your system, they are removed too. git grep -C1 -P "Plugins::Methods?->delete" t/db_dependent/Koha/Plugins/Account_hooks.t- t/db_dependent/Koha/Plugins/Account_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Account_hooks.t- $schema->storage->txn_rollback; -- t/db_dependent/Koha/Plugins/BackgroundJob.t- $schema->storage->txn_rollback; t/db_dependent/Koha/Plugins/BackgroundJob.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/BackgroundJob.t-}; -- t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t- $schema->storage->txn_rollback; t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t-}; -- t/db_dependent/Koha/Plugins/Biblio_and_Items_plugin_hooks.t- $schema->storage->txn_rollback; t/db_dependent/Koha/Plugins/Biblio_and_Items_plugin_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Biblio_and_Items_plugin_hooks.t-}; -- t/db_dependent/Koha/Plugins/Circulation_hooks.t- $schema->storage->txn_rollback; t/db_dependent/Koha/Plugins/Circulation_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Circulation_hooks.t-}; -- t/db_dependent/Koha/Plugins/Holds_hooks.t- t/db_dependent/Koha/Plugins/Holds_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Account_hooks.t- t/db_dependent/Koha/Plugins/Account_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Account_hooks.t- $schema->storage->txn_rollback; -- t/db_dependent/Koha/Plugins/BackgroundJob.t- $schema->storage->txn_rollback; t/db_dependent/Koha/Plugins/BackgroundJob.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/BackgroundJob.t-}; -- t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t- $schema->storage->txn_rollback; t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t-}; -- t/db_dependent/Koha/Plugins/Biblio_and_Items_plugin_hooks.t- $schema->storage->txn_rollback; t/db_dependent/Koha/Plugins/Biblio_and_Items_plugin_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Biblio_and_Items_plugin_hooks.t-}; -- t/db_dependent/Koha/Plugins/Circulation_hooks.t- $schema->storage->txn_rollback; t/db_dependent/Koha/Plugins/Circulation_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Circulation_hooks.t-}; -- t/db_dependent/Koha/Plugins/Holds_hooks.t- t/db_dependent/Koha/Plugins/Holds_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Holds_hooks.t- $schema->storage->txn_rollback; -- t/db_dependent/Koha/Plugins/Holds_hooks.t- t/db_dependent/Koha/Plugins/Holds_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Holds_hooks.t- $schema->storage->txn_rollback; -- t/db_dependent/Koha/Plugins/Holds_hooks.t- t/db_dependent/Koha/Plugins/Holds_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Holds_hooks.t- $schema->storage->txn_rollback; -- t/db_dependent/Koha/Plugins/Patron.t- $schema->storage->txn_rollback; t/db_dependent/Koha/Plugins/Patron.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Patron.t-}; -- t/db_dependent/Koha/Plugins/Plugins.t- # Temporarily remove any installed plugins data t/db_dependent/Koha/Plugins/Plugins.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Plugins.t- $schema->resultset('PluginData')->delete(); -- t/db_dependent/Koha/Plugins/Plugins.t- # Temporarily remove any installed plugins data t/db_dependent/Koha/Plugins/Plugins.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Plugins.t- $schema->resultset('PluginData')->delete(); -- t/db_dependent/Koha/Plugins/Plugins.t- # Temporarily remove any installed plugins data t/db_dependent/Koha/Plugins/Plugins.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Plugins.t- $schema->resultset('PluginData')->delete(); -- root@master:/usr/share/koha# git grep -C1 -P "Plugins::Methods?->delete" t/db_dependent/Koha/Plugins/Account_hooks.t- t/db_dependent/Koha/Plugins/Account_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Account_hooks.t- $schema->storage->txn_rollback; -- t/db_dependent/Koha/Plugins/BackgroundJob.t- $schema->storage->txn_rollback; t/db_dependent/Koha/Plugins/BackgroundJob.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/BackgroundJob.t-}; -- t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t- $schema->storage->txn_rollback; t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t-}; -- t/db_dependent/Koha/Plugins/Biblio_and_Items_plugin_hooks.t- $schema->storage->txn_rollback; t/db_dependent/Koha/Plugins/Biblio_and_Items_plugin_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Biblio_and_Items_plugin_hooks.t-}; -- t/db_dependent/Koha/Plugins/Circulation_hooks.t- $schema->storage->txn_rollback; t/db_dependent/Koha/Plugins/Circulation_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Circulation_hooks.t-}; -- t/db_dependent/Koha/Plugins/Holds_hooks.t- t/db_dependent/Koha/Plugins/Holds_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Holds_hooks.t- $schema->storage->txn_rollback; -- t/db_dependent/Koha/Plugins/Holds_hooks.t- t/db_dependent/Koha/Plugins/Holds_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Holds_hooks.t- $schema->storage->txn_rollback; -- t/db_dependent/Koha/Plugins/Holds_hooks.t- t/db_dependent/Koha/Plugins/Holds_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Holds_hooks.t- $schema->storage->txn_rollback; -- t/db_dependent/Koha/Plugins/Patron.t- $schema->storage->txn_rollback; t/db_dependent/Koha/Plugins/Patron.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Patron.t-}; -- t/db_dependent/Koha/Plugins/Plugins.t- # Temporarily remove any installed plugins data t/db_dependent/Koha/Plugins/Plugins.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Plugins.t- $schema->resultset('PluginData')->delete(); -- t/db_dependent/Koha/Plugins/Plugins.t- # Temporarily remove any installed plugins data t/db_dependent/Koha/Plugins/Plugins.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Plugins.t- $schema->resultset('PluginData')->delete(); -- t/db_dependent/Koha/Plugins/Plugins.t- # Temporarily remove any installed plugins data t/db_dependent/Koha/Plugins/Plugins.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Plugins.t- $schema->resultset('PluginData')->delete(); -- t/db_dependent/Koha/Plugins/Plugins.t- # Temporarily remove any installed plugins data t/db_dependent/Koha/Plugins/Plugins.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Plugins.t- -- t/db_dependent/Koha/Plugins/Plugins.t- # Make sure there's no previous installs or leftovers on DB t/db_dependent/Koha/Plugins/Plugins.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Plugins.t- $schema->resultset('PluginData')->delete; -- t/db_dependent/Koha/Plugins/Plugins.t-$schema->storage->txn_begin; t/db_dependent/Koha/Plugins/Plugins.t:Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Plugins.t-$schema->resultset('PluginData')->delete; -- t/db_dependent/Koha/Plugins/Plugins.t- t/db_dependent/Koha/Plugins/Plugins.t:Koha::Plugins::Methods->delete; -- t/db_dependent/Koha/Plugins/Recall_hooks.t- t/db_dependent/Koha/Plugins/Recall_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Recall_hooks.t- $schema->storage->txn_rollback; -- t/db_dependent/Koha/Plugins/authority_hooks.t- $schema->storage->txn_rollback; t/db_dependent/Koha/Plugins/authority_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/authority_hooks.t-}; root@master:/usr/share/koha# git grep -C1 -P "Plugins::Methods?->delete" > zzz root@master:/usr/share/koha# cat zzz t/db_dependent/Koha/Plugins/Account_hooks.t- t/db_dependent/Koha/Plugins/Account_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Account_hooks.t- $schema->storage->txn_rollback; -- t/db_dependent/Koha/Plugins/BackgroundJob.t- $schema->storage->txn_rollback; t/db_dependent/Koha/Plugins/BackgroundJob.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/BackgroundJob.t-}; -- t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t- $schema->storage->txn_rollback; t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t-}; -- t/db_dependent/Koha/Plugins/Biblio_and_Items_plugin_hooks.t- $schema->storage->txn_rollback; t/db_dependent/Koha/Plugins/Biblio_and_Items_plugin_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Biblio_and_Items_plugin_hooks.t-}; -- t/db_dependent/Koha/Plugins/Circulation_hooks.t- $schema->storage->txn_rollback; t/db_dependent/Koha/Plugins/Circulation_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Circulation_hooks.t-}; -- t/db_dependent/Koha/Plugins/Holds_hooks.t- t/db_dependent/Koha/Plugins/Holds_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Holds_hooks.t- $schema->storage->txn_rollback; -- t/db_dependent/Koha/Plugins/Holds_hooks.t- t/db_dependent/Koha/Plugins/Holds_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Holds_hooks.t- $schema->storage->txn_rollback; -- t/db_dependent/Koha/Plugins/Holds_hooks.t- t/db_dependent/Koha/Plugins/Holds_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Holds_hooks.t- $schema->storage->txn_rollback; -- t/db_dependent/Koha/Plugins/Patron.t- $schema->storage->txn_rollback; t/db_dependent/Koha/Plugins/Patron.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Patron.t-}; -- t/db_dependent/Koha/Plugins/Plugins.t- # Temporarily remove any installed plugins data t/db_dependent/Koha/Plugins/Plugins.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Plugins.t- $schema->resultset('PluginData')->delete(); -- t/db_dependent/Koha/Plugins/Plugins.t- # Temporarily remove any installed plugins data t/db_dependent/Koha/Plugins/Plugins.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Plugins.t- $schema->resultset('PluginData')->delete(); -- t/db_dependent/Koha/Plugins/Plugins.t- # Temporarily remove any installed plugins data t/db_dependent/Koha/Plugins/Plugins.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Plugins.t- $schema->resultset('PluginData')->delete(); -- t/db_dependent/Koha/Plugins/Plugins.t- # Temporarily remove any installed plugins data t/db_dependent/Koha/Plugins/Plugins.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Plugins.t- -- t/db_dependent/Koha/Plugins/Plugins.t- # Make sure there's no previous installs or leftovers on DB t/db_dependent/Koha/Plugins/Plugins.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Plugins.t- $schema->resultset('PluginData')->delete; -- t/db_dependent/Koha/Plugins/Plugins.t-$schema->storage->txn_begin; t/db_dependent/Koha/Plugins/Plugins.t:Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Plugins.t-$schema->resultset('PluginData')->delete; -- t/db_dependent/Koha/Plugins/Plugins.t- t/db_dependent/Koha/Plugins/Plugins.t:Koha::Plugins::Methods->delete; -- t/db_dependent/Koha/Plugins/Recall_hooks.t- t/db_dependent/Koha/Plugins/Recall_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/Recall_hooks.t- $schema->storage->txn_rollback; -- t/db_dependent/Koha/Plugins/authority_hooks.t- $schema->storage->txn_rollback; t/db_dependent/Koha/Plugins/authority_hooks.t: Koha::Plugins::Methods->delete; t/db_dependent/Koha/Plugins/authority_hooks.t-}; === Preliminary conclusion When you call InstallPlugins, or plugin->enable or ->disable, the cache will already be cleared. This covers quite a few cases. But not all, explaining for a bit the reduced number of failures. Things are definitely related to the KitchenSink stuff in Plugins.t and the order of execution. What about temporarily disabling plugins in TestBuilder.t ? Or in build_sample_biblio ? Does not sound very nice. Not the way to go imo. Adding a clear cache in Methods->delete probably works? Yes, we even catch the Handler->delete case too. Since Handler->delete calls uninstall on the plugin. Deletes from plugin_data. And does Methods->search->delete! But it does not feel like the best solution imo. The actions for deleting records in plugin_data and plugin_methods should be done on a general level and not be repeated in all those test scripts etc. (Which requires more code changes definitely. But reordering all deletes after rollback does require a lot of those changes too..) We need a simple fix that can be backported to versions having bug 29672 (i.e. 22.11) And an enhancement taking care of the cleanup and wrong order stuff. Adding 35536 on top of this one. Comment on attachment 159670 [details] [review] Bug 35507: Clear cache key for enabled plugins at deletion Obsoleting in favor of another approach. Is this ready for SO? (In reply to Marcel de Rooy from comment #8) > BackgroundJob.t does: > $schema->storage->txn_rollback; > Koha::Plugins::Methods->delete; > Thats definitely in the wrong order. I recall we did it that way because InstallPlugins triggered a commit on the transaction. So we decided 'plugin tests will be considered destructive' (in terms of the DB data) and did it this way so there's no test plugin information on the DB when other tests are run. Maybe that's no longer needed. But I don't recall any recent changes in the area. (In reply to Katrin Fischer from comment #13) > Is this ready for SO? No, I removed the patch :) (In reply to Tomás Cohen Arazi from comment #14) > I recall we did it that way because InstallPlugins triggered a commit on the > transaction. So we decided 'plugin tests will be considered destructive' (in > terms of the DB data) and did it this way so there's no test plugin > information on the DB when other tests are run. Interesting! I am not sure where that implicit commit hides? Do you see it? Lots of fun here in the meantime. Discovered that we miss a rollback in Plugins.t too. And running prove Plugins.t ends up with additional data in a clean master install: +----------------------------------------+-----------------------+--------------+ | plugin_class | plugin_key | plugin_value | +----------------------------------------+-----------------------+--------------+ | Koha::Plugin::BadAPIRoute | __INSTALLED_VERSION__ | 0.01 | | Koha::Plugin::MarcFieldValues | __INSTALLED_VERSION__ | 1 | | Koha::Plugin::Test | __ENABLED__ | 0 | | Koha::Plugin::Test | __INSTALLED__ | 1 | | Koha::Plugin::Test | __INSTALLED_VERSION__ | v1.01 | | Koha::Plugin::TestItemBarcodeTransform | __INSTALLED_VERSION__ | 1.01 | +----------------------------------------+-----------------------+--------------+ Still WIP Weird stuff. Debugging Plugins.t gives me the following: Between those two lines: [1] my $classname = ref($plugin); AND [2] like( $plugin->test_template($fn), qr/^I am $fn/, 'Template works' ); There must be an implicit commit somewhere. If you end on the classname line, you will have no committed data, after the test_template line you have. Note that test_template calls get_template_and_user (and checkauth etc) in C4/Auth. Somewhere we commit the changes in plugin_data and plugin_methods. This is costing me too much time now to continue. Sorry ;) Assistance appreciated.. Created attachment 159739 [details] [review] Bug 35507: Add Plugins::Method->delete Delete record in plugin_data too and clear cache. Note: Adding the singular delete, makes the plural loop and call the singular in Koha::Objects. Clearing the cache is done in the store_data call of Koha::Plugins. Test plan: Run t/db_dependent/Koha/Plugins/Method.t Run other Koha plugin tests: Attached a patch still WIP. Before entering the rabbit hole of Plugins.t (In reply to Marcel de Rooy from comment #19) > Created attachment 159739 [details] [review] [review] > Bug 35507: Add Plugins::Method->delete > > Delete record in plugin_data too and clear cache. > > Note: Adding the singular delete, makes the plural loop and > call the singular in Koha::Objects. Clearing the cache is > done in the store_data call of Koha::Plugins. > > Test plan: > Run t/db_dependent/Koha/Plugins/Method.t > Run other Koha plugin tests: (In reply to Marcel de Rooy from comment #18) > Weird stuff. Debugging Plugins.t gives me the following: > > Between those two lines: > > [1] my $classname = ref($plugin); > > AND > > [2] like( $plugin->test_template($fn), qr/^I am $fn/, 'Template works' ); > > There must be an implicit commit somewhere. If you end on the classname > line, you will have no committed data, after the test_template line you have. > > Note that test_template calls get_template_and_user (and checkauth etc) in > C4/Auth. Somewhere we commit the changes in plugin_data and plugin_methods. > > This is costing me too much time now to continue. Sorry ;) > Assistance appreciated.. CGI::Session \o/ (In reply to Marcel de Rooy from comment #19) > Created attachment 159739 [details] [review] [review] > Bug 35507: Add Plugins::Method->delete > > Delete record in plugin_data too and clear cache. > > Note: Adding the singular delete, makes the plural loop and > call the singular in Koha::Objects. Clearing the cache is > done in the store_data call of Koha::Plugins. > > Test plan: > Run t/db_dependent/Koha/Plugins/Method.t > Run other Koha plugin tests: Have you tried enabling DBIC_TRACE? I don't if it will show the implicit commit: DBIC_TRACE=1 prove t/db_dependent/TestBuilder.t (In reply to Tomás Cohen Arazi from comment #21) > > CGI::Session > > \o/ Yes, I ended with some suspicion about the session flushes (incl Search history). (In reply to Kyle M Hall from comment #22) > (In reply to Marcel de Rooy from comment #19) > > Created attachment 159739 [details] [review] [review] [review] > > Bug 35507: Add Plugins::Method->delete > > > > Delete record in plugin_data too and clear cache. > > > > Note: Adding the singular delete, makes the plural loop and > > call the singular in Koha::Objects. Clearing the cache is > > done in the store_data call of Koha::Plugins. > > > > Test plan: > > Run t/db_dependent/Koha/Plugins/Method.t > > Run other Koha plugin tests: > > Have you tried enabling DBIC_TRACE? I don't if it will show the implicit > commit: > DBIC_TRACE=1 prove t/db_dependent/TestBuilder.t Will do. Thx (In reply to Marcel de Rooy from comment #24) > > DBIC_TRACE=1 prove t/db_dependent/TestBuilder.t DBIC seems ok. Running with DBI_TRACE might show more? Seeing in that trace rollback ineffective with AutoCommit enabled at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1651. <- prepare('SELECT flag, user_permissions.code FROM user_permissions JOIN permissions USING (module_bit, code) JOIN userflags ON (module_bit = bit) JOIN borrowers USING (borrowernumber) WHERE userid = ?')= ( DBI::st=HASH(0x55b072677978) ) [1 items] at Auth.pm line 2165 <- execute(undef)= ( '0E0' ) [1 items] at Auth.pm line 2171 !! The ERROR '2000' was CLEARED by call to fetch method <- fetchrow_hashref= ( undef ) [1 items] at Auth.pm line 2174 <- DESTROY(DBI::st=HASH(0x55b072677bb8))= ( undef ) [1 items] at Auth.pm line 2177 <- DESTROY(DBI::st=HASH(0x55b072677558))= ( undef ) [1 items] at Auth.pm line 2133 <- DESTROY(DBI::st=HASH(0x55b0726772b8))= ( undef ) [1 items] at Auth.pm line 2307 <- FETCH('Active')= ( 1 ) [1 items] at DBI.pm line 939 rollback ineffective with AutoCommit enabled at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1651. <- rollback= ( '' ) [1 items] at DBI.pm line 1651 <- disconnect_all= ( ) [0 items] (not implemented) at DBI.pm line 758 # Looks like you planned 62 tests but ran 40. get_template_and_user => haspermission => .. => get_user_subpermissions Tomas, maybe this works from auth_authenticate_api_request.t : # FIXME: CGI::Session::Driver::DBI explicitly sets AutoCommit=1 [1] which breaks the rollback in out tests. # Until we change into some other library, set SessionStorage to 'tmp' # [1] https://metacpan.org/source/CGI::Session::Driver::DBI#L28 t::lib::Mocks::mock_preference( 'SessionStorage', 'tmp' ); Not whole story yet, but now seeing this in KitchenSink: return C4::Context->dbh->do( " CREATE TABLE IF NOT EXISTS $table ( `borrowernumber` INT( 11 ) NOT NULL ) ENGINE = INNODB; " ); Thats a commit ! What do you folks think: Shall we remove the KitchenSink stuff from Plugins.t since it implicitly commits cruft in the database ? (In reply to Marcel de Rooy from comment #30) > What do you folks think: Shall we remove the KitchenSink stuff from > Plugins.t since it implicitly commits cruft in the database ? I would opt for moving it in a separate test and creating the database before installing the plugin (it includes an exist test). Controlling what we 'commit' when creating. Created attachment 159744 [details] [review] Bug 35507: Add Plugins::Method->delete Delete record in plugin_data too and clear cache. Note: Adding the singular delete, makes the plural loop and call the singular in Koha::Objects. Clearing the cache is done in the store_data call of Koha::Plugins. Test plan: Run prove t/db_dependent/Koha/Plugins Run prove $(git grep -l Koha::Plugin | grep -P "^t\/db" | grep -v "Koha/Plugins") Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 159745 [details] [review] Bug 35507: Plugins.t - Mock SessionStorage, remove KitchenSink Removing the CGI::Session behavior to auto-commit by mocking SessionStorage makes that the $plugin->test_template does no longer commit. Since KitchenSink includes a CREATE TABLE, removing that code removes the second implicit commit. We might move that code to its own script? Adds a missing rollback too at the end. Test plan: Run prove t/db_dependent/Koha/Plugins/Plugins.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tomas: We get now one failure here: Koha/REST/Plugin/PluginRoutes.t ..... 3/4 # Failed test 'Plugin enabled, route defined as C4::Context->needs_install is false' # at Koha/REST/Plugin/PluginRoutes.t line 247. # Looks like you failed 1 test of 2. Koha/REST/Plugin/PluginRoutes.t ..... 4/4 # Failed test 'needs_install use case tests' # at Koha/REST/Plugin/PluginRoutes.t line 252. # Looks like you failed 1 test of 4. Koha/REST/Plugin/PluginRoutes.t ..... Dubious, test returned 1 (wstat 256, 0x100) Failed 1/4 subtests It seems that this test assumes that the plugin is there or so, and now no longer is? Can you confirm? Still thinking too about the REPLACE with 0 in Method->delete. We could also remove but then we need to explicitly clear cache. Created attachment 159801 [details] [review] Bug 35507: Plugins.t - Mock SessionStorage, remove KitchenSink Removing the CGI::Session behavior to auto-commit by mocking SessionStorage makes that the $plugin->test_template does no longer commit. Since KitchenSink includes a CREATE TABLE, removing that code removes the second implicit commit. We might move that code to its own script? Adds a missing rollback too at the end. Test plan: Run prove t/db_dependent/Koha/Plugins/Plugins.t Verify that no records got added to plugin_data/methods table. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 159802 [details] [review] Bug 35507: Add Plugins::Method->delete Delete record in plugin_data too and clear cache. Note: Adding the singular delete, makes the plural loop and call the singular in Koha::Objects. Clearing the cache is done in the store_data call of Koha::Plugins. Test plan: Run prove prove $(git grep -l Koha::Plugin | grep -P "^t\/db") Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to Marcel de Rooy from comment #34) > Tomas: > > We get now one failure here: > > Koha/REST/Plugin/PluginRoutes.t ..... 3/4 > # Failed test 'Plugin enabled, route defined as > C4::Context->needs_install is false' > # at Koha/REST/Plugin/PluginRoutes.t line 247. > # Looks like you failed 1 test of 2. > Koha/REST/Plugin/PluginRoutes.t ..... 4/4 > # Failed test 'needs_install use case tests' > # at Koha/REST/Plugin/PluginRoutes.t line 252. > # Looks like you failed 1 test of 4. > Koha/REST/Plugin/PluginRoutes.t ..... Dubious, test returned 1 (wstat 256, > 0x100) > Failed 1/4 subtests > > It seems that this test assumes that the plugin is there or so, and now no > longer is? Can you confirm? This seems no longer relevant. Will add a tiny fix on the enh report for this unit test btw. (In reply to Marcel de Rooy from comment #35) > Still thinking too about the REPLACE with 0 in Method->delete. We could also > remove but then we need to explicitly clear cache. I did now. Remove instead of disable. NOTE FOR QA: This is a small fix that could be backported. Enhancing it and preventing data loss on the enh report. And separate report for restoring KitchenSink stuff. (In reply to Marcel de Rooy from comment #37) > Note: Adding the singular delete, makes the plural loop and > call the singular in Koha::Objects. Clearing the cache is > done in the store_data call of Koha::Plugins. Hmm. I updated that. Created attachment 159803 [details] [review] Bug 35507: Add Plugins::Method->delete Delete record in plugin_data too and clear cache. Note: Adding the singular delete, makes the plural loop and call the singular in Koha::Objects. Test plan: Run prove prove $(git grep -l Koha::Plugin | grep -P "^t\/db") Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 159817 [details] [review] Bug 35507: Plugins.t - Mock SessionStorage, remove KitchenSink Removing the CGI::Session behavior to auto-commit by mocking SessionStorage makes that the $plugin->test_template does no longer commit. Since KitchenSink includes a CREATE TABLE, removing that code removes the second implicit commit. We might move that code to its own script? Adds a missing rollback too at the end. Test plan: Run prove t/db_dependent/Koha/Plugins/Plugins.t Verify that no records got added to plugin_data/methods table. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 159818 [details] [review] Bug 35507: Add Plugins::Method->delete Delete record in plugin_data too and clear cache. Note: Adding the singular delete, makes the plural loop and call the singular in Koha::Objects. Test plan: Run prove prove $(git grep -l Koha::Plugin | grep -P "^t\/db") Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Trusting Marcel to continue the work he has started here. (In reply to Jonathan Druart from comment #45) > Trusting Marcel to continue the work he has started here. Bug 35536 has been submitted now. Comment on attachment 159818 [details] [review] Bug 35507: Add Plugins::Method->delete Review of attachment 159818 [details] [review]: ----------------------------------------------------------------- ::: Koha/Plugins/Method.pm @@ +43,5 @@ > + my ($self) = @_; > + > + # Remove corresponding records from plugin_data. Clear cache. > + my $cond = { plugin_class => $self->plugin_class }; > + Koha::Database->new->schema->resultset('PluginData')->search($cond)->delete; This scares me.. delete is called from inside InstallPlugins.. InstallPlugins is called from upload-plugin.pl and misc/devel/install_plugins.pl So.. on first look, this appears to me that for any new plugin installed will nuke all data for all other plugins in the system at the time. Plugin upgrades too. InstallPlugins contains Koha::Plugins::Methods->search({ plugin_class => $plugin_class })->delete(); So this is restricted to $plugin_class. What is scary exactly? (In reply to Marcel de Rooy from comment #48) > InstallPlugins contains > Koha::Plugins::Methods->search({ plugin_class => $plugin_class })->delete(); > > So this is restricted to $plugin_class. What is scary exactly? Scary: removing from plugin_data too. (In reply to Marcel de Rooy from comment #29) > Not whole story yet, but now seeing this in KitchenSink: > > return C4::Context->dbh->do( " > CREATE TABLE IF NOT EXISTS $table ( > `borrowernumber` INT( 11 ) NOT NULL > ) ENGINE = INNODB; > " ); > > Thats a commit ! And the DROP TABLE as well :) (In reply to Marcel de Rooy from comment #35) > Still thinking too about the REPLACE with 0 in Method->delete. We could also > remove but then we need to explicitly clear cache. Martin: This was a mistake in hindsight. Before this change, I just disabled. Comment on attachment 159818 [details] [review] Bug 35507: Add Plugins::Method->delete Going back to older version and extending test a little bit Hmm reintroducing this guy now: t/db_dependent/Koha/REST/Plugin/PluginRoutes.t ............... 3/4 # Failed test 'Plugin enabled, route defined as C4::Context->needs_install is false' # at t/db_dependent/Koha/REST/Plugin/PluginRoutes.t line 247. # Looks like you failed 1 test of 2. t/db_dependent/Koha/REST/Plugin/PluginRoutes.t ............... 4/4 # Failed test 'needs_install use case tests' # at t/db_dependent/Koha/REST/Plugin/PluginRoutes.t line 252. # Looks like you failed 1 test of 4. t/db_dependent/Koha/REST/Plugin/PluginRoutes.t ............... Dubious, test returned 1 (wstat 256, 0x100) Created attachment 159846 [details] [review] Bug 35507: Add Plugins::Method->delete Delete record in plugin_data too and clear cache. Note: Adding the singular delete, makes the plural loop and call the singular in Koha::Objects. Clearing the cache is done in the store_data call of Koha::Plugins. Test plan: Run prove $(git grep -l Koha::Plugin | grep -P "^t\/db") Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 159847 [details] [review] Bug 35507: (follow-up) Fix PluginRoutes.t Why this works, is a mystery to me :) Test plan: Run t/db_dependent/Koha/REST/Plugin/PluginRoutes.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 159848 [details] [review] Bug 35507: Plugins.t - Mock SessionStorage, remove KitchenSink Removing the CGI::Session behavior to auto-commit by mocking SessionStorage makes that the $plugin->test_template does no longer commit. Since KitchenSink includes a CREATE TABLE, removing that code removes the second implicit commit. We might move that code to its own script? Adds a missing rollback too at the end. Test plan: Run prove t/db_dependent/Koha/Plugins/Plugins.t Verify that no records got added to plugin_data/methods table. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 159849 [details] [review] Bug 35507: Add Plugins::Method->delete Delete record in plugin_data too and clear cache. Note: Adding the singular delete, makes the plural loop and call the singular in Koha::Objects. Clearing the cache is done in the store_data call of Koha::Plugins. Test plan: Run prove $(git grep -l Koha::Plugin | grep -P "^t\/db") Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 159850 [details] [review] Bug 35507: (follow-up) Fix PluginRoutes.t Why this works, is a mystery to me :) Test plan: Run t/db_dependent/Koha/REST/Plugin/PluginRoutes.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> (In reply to Kyle M Hall from comment #58) > Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Thanks! Still thinking about a potential issue Created attachment 159860 [details] [review] Bug 35507: Add verbose parameters in Koha::Plugins Yet another approach ;) Driemaal is scheepsrecht / Third time lucky.. Test plan: Run prove $(git grep -l Koha::Plugin | grep -P "^t\/db") Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> I finally got the least harmful approach now. Removing the Method->delete here and postponing that approach to 35536 and adding a verbose parameter in Koha::Plugins at the can_load calls. Testing the verbose parameter and further refinements, all go to 35536 now. We spent way too much time here already. Moving the mysterious PluginRoutes.t issue to 35536 too. Final round for Martin hopefully now? prove $(git grep -l Koha::Plugin | grep -P "^t\/db") t/db_dependent/ImportBatch.t ................................. ok t/db_dependent/Koha/Patron/Consents.t ........................ ok t/db_dependent/Koha/Patron_generate_userid.t ................. ok t/db_dependent/Koha/Plugins/Account_hooks.t .................. ok t/db_dependent/Koha/Plugins/BackgroundJob.t .................. ok t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t ........ ok t/db_dependent/Koha/Plugins/Biblio_and_Items_plugin_hooks.t .. ok t/db_dependent/Koha/Plugins/Broken.t ......................... ok t/db_dependent/Koha/Plugins/Circulation_hooks.t .............. ok t/db_dependent/Koha/Plugins/Holds_hooks.t .................... ok t/db_dependent/Koha/Plugins/Patron.t ......................... 1/5 Plugin error (Test Plugin): Exception 'Koha::Exception' thrown 'patron_barcode_transform called with parameter: test_cn_1' Plugin error (Test Plugin): Exception 'Koha::Exception' thrown 'patron_barcode_transform called with parameter: test_cn_1' Plugin error (Test Plugin): Exception 'Koha::Exception' thrown 'patron_barcode_transform called with parameter: test_cn_1' t/db_dependent/Koha/Plugins/Patron.t ......................... ok t/db_dependent/Koha/Plugins/Plugins.t ........................ ok t/db_dependent/Koha/Plugins/Recall_hooks.t ................... ok t/db_dependent/Koha/Plugins/authority_hooks.t ................ ok t/db_dependent/Koha/REST/Plugin/PluginRoutes.t ............... ok t/db_dependent/Koha/Template/Plugin/KohaPlugins.t ............ ok All tests successful. Files=16, Tests=135, 131 wallclock secs ( 0.21 usr 0.06 sys + 122.23 cusr 5.97 csys = 128.47 CPU) Result: PASS Created attachment 159861 [details] [review] Bug 35507: Plugins.t - Mock SessionStorage, remove KitchenSink Removing the CGI::Session behavior to auto-commit by mocking SessionStorage makes that the $plugin->test_template does no longer commit. Since KitchenSink includes a CREATE TABLE, removing that code removes the second implicit commit. We might move that code to its own script? Adds a missing rollback too at the end. Test plan: Run prove t/db_dependent/Koha/Plugins/Plugins.t Verify that no records got added to plugin_data/methods table. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 159862 [details] [review] Bug 35507: Add verbose parameters in Koha::Plugins Yet another approach ;) Driemaal is scheepsrecht / Third time lucky.. Test plan: Run prove $(git grep -l Koha::Plugin | grep -P "^t\/db") Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Great work, thanks for all the effort. No regressions found now, QA scripts happy and tests are now consistently passing :) Passing QA Pushed for 24.05! Well done everyone, thank you! Pushed to 23.11.x for 23.11.02 Doesn't apply cleanly to 23.05.x, no backport. If needed please rebase. Backported to 23.05.x for upcoming 23.05.09. |