|
Description
Marcel de Rooy
2016-05-11 14:44:38 UTC
Created attachment 51544 [details] [review] Bug 16502: Table koha_plugin_com_bywatersolutions_kitchensink_mytable not always dropped after running Plugin.t If you run Plugin.t, the above table will still be present (when you did not enable UseKohaPlugins). This would trigger a warning when running the test a second time. Why? The uninstall call does its work not completely due to a small inconsistency in Koha::Plugins::Handler::delete when calling run without the enable_plugins parameter. This patch resolves that inconsistency and also removes an unneeded skip in Plugin.t in case the KitchenSink module already exists. Note: This is a small fix. But I wonder if the Handler routines run and delete should not have been implemented in Koha::Plugins::Base. Also note that plugins/plugins-uninstall.pl will not be affacted by this change, since it checks whether the pref is enabled before calling the delete method. Test plan: [1] Do not yet install this patch. [2] Verify that plugins are enabled in koha-conf.xml. [3] Disable UseKohaPlugins in System Preferences! [4] Run t/db_dependent/Plugins.t. [5] Verify that table koha_plugin_com_bywatersolutions_kitchensink_mytable still exists. (It should have been deleted.) Remove it manually. [6] Apply this patch. [7] Run the test again. [8] Verify that the table does not exist. [9] Run the test again (without warnings). Created attachment 51545 [details] [review] Bug 16502: Add additional test to Plugins.t In order to verify if the delete now really works, we add one test in Plugins.t. Test plan: [1] Run the test. [2] Bonus: Comment line 63 in Plugins.t where delete is called. Run the test again. It should fail now. There's something odd with that t/db_dependent/Plugins.t. Without the patch, I've changed it:
diff --git a/t/db_dependent/Plugins.t b/t/db_dependent/Plugins.t
index 3271b1a..c5faf87 100755
--- a/t/db_dependent/Plugins.t
+++ b/t/db_dependent/Plugins.t
@@ -43,7 +43,7 @@ ok( $plugin->get_qualified_table_name('mytable') eq 'koha_plugin_test_mytable',
ok( $plugin->get_plugin_http_path() eq '/plugin/Koha/Plugin/Test', 'Test $plugin->get_plugin_http_path()' );
my @plugins = Koha::Plugins->new({ enable_plugins => 1 })->GetPlugins( 'report' );
-ok( $plugins[0]->get_metadata()->{'name'} eq 'Test Plugin', "Koha::Plugins::GetPlugins functions correctly" );
+is( $plugins[0]->get_metadata()->{'name'}, 'Test Plugin', "Koha::Plugins::GetPlugins functions correctly" );
SKIP: {
my $plugins_dir = C4::Context->config("pluginsdir");
and I'm getting
not ok 18 - Koha::Plugins::GetPlugins functions correctly
# Failed test 'Koha::Plugins::GetPlugins functions correctly'
# at t/db_dependent/Plugins.t line 46.
# got: 'Example Kitchen-Sink Plugin'
# expected: 'Test Plugin'
(In reply to Srdjan Jankovic from comment #3) > There's something odd with that t/db_dependent/Plugins.t. ... > and I'm getting > not ok 18 - Koha::Plugins::GetPlugins functions correctly > # Failed test 'Koha::Plugins::GetPlugins functions correctly' > # at t/db_dependent/Plugins.t line 46. > # got: 'Example Kitchen-Sink Plugin' > # expected: 'Test Plugin' Srdjan, I made the same change and it seems to work with me. But actually this test is based on the assumption that GetPlugins always finds the Test Plugin first. Which could very well not be the case. Since there can be a lot of plugins present, the test should only verify if the Test Plugin is in the array. Also note that the array is not sorted by the routine, so this should have been taken in account too. Adding a follow-up. Created attachment 51713 [details] [review] Bug 16502: Adjust test for GetPlugins The current test assumes that GetPlugins will return the test plugin as the first one in the array. This is not correct. This patch adjusts the test to a grep. Test plan: Run the test. Bonus: Add additional plugins. Run the test again. Created attachment 51714 [details] [review] Bug 16502: Replace a few other ok-calls by is-calls Trivial changes that speak for themselves.. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 51804 [details] [review] [SIGNED-OFF] Bug 16502: Add additional test to Plugins.t In order to verify if the delete now really works, we add one test in Plugins.t. Test plan: [1] Run the test. [2] Bonus: Comment line 63 in Plugins.t where delete is called. Run the test again. It should fail now. Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Created attachment 51805 [details] [review] [SIGNED-OFF] Bug 16502: Adjust test for GetPlugins The current test assumes that GetPlugins will return the test plugin as the first one in the array. This is not correct. This patch adjusts the test to a grep. Test plan: Run the test. Bonus: Add additional plugins. Run the test again. Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Created attachment 51806 [details] [review] [SIGNED-OFF] Bug 16502: Replace a few other ok-calls by is-calls Trivial changes that speak for themselves.. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Thanks Srdjan for signing off. I notice that the first patch did not get your signoff line. Was that intentional or just forgotten? Apologies, my mistake, did not adjust for the additional patch. Created attachment 51853 [details] [review] Bug 16502: Table koha_plugin_com_bywatersolutions_kitchensink_mytable not always dropped after running Plugin.t If you run Plugin.t, the above table will still be present (when you did not enable UseKohaPlugins). This would trigger a warning when running the test a second time. Why? The uninstall call does its work not completely due to a small inconsistency in Koha::Plugins::Handler::delete when calling run without the enable_plugins parameter. This patch resolves that inconsistency and also removes an unneeded skip in Plugin.t in case the KitchenSink module already exists. Note: This is a small fix. But I wonder if the Handler routines run and delete should not have been implemented in Koha::Plugins::Base. Also note that plugins/plugins-uninstall.pl will not be affacted by this change, since it checks whether the pref is enabled before calling the delete method. Test plan: [1] Do not yet install this patch. [2] Verify that plugins are enabled in koha-conf.xml. [3] Disable UseKohaPlugins in System Preferences! [4] Run t/db_dependent/Plugins.t. [5] Verify that table koha_plugin_com_bywatersolutions_kitchensink_mytable still exists. (It should have been deleted.) Remove it manually. [6] Apply this patch. [7] Run the test again. [8] Verify that the table does not exist. [9] Run the test again (without warnings). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Created attachment 51854 [details] [review] Bug 16502: Add additional test to Plugins.t In order to verify if the delete now really works, we add one test in Plugins.t. Test plan: [1] Run the test. [2] Bonus: Comment line 63 in Plugins.t where delete is called. Run the test again. It should fail now. Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Created attachment 51855 [details] [review] Bug 16502: Adjust test for GetPlugins The current test assumes that GetPlugins will return the test plugin as the first one in the array. This is not correct. This patch adjusts the test to a grep. Test plan: Run the test. Bonus: Add additional plugins. Run the test again. Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Created attachment 51856 [details] [review] Bug 16502: Replace a few other ok-calls by is-calls Trivial changes that speak for themselves.. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Srdjan <srdjan@catalyst.net.nz> (In reply to Srdjan Jankovic from comment #11) > Apologies, my mistake, did not adjust for the additional patch. OK I added your signoff line. Thx I meant to do it sometimes today, after settling current affairs. But this is ok too. Thanks. Created attachment 52260 [details] [review] Bug 16502: Table koha_plugin_com_bywatersolutions_kitchensink_mytable not always dropped after running Plugin.t If you run Plugin.t, the above table will still be present (when you did not enable UseKohaPlugins). This would trigger a warning when running the test a second time. Why? The uninstall call does its work not completely due to a small inconsistency in Koha::Plugins::Handler::delete when calling run without the enable_plugins parameter. This patch resolves that inconsistency and also removes an unneeded skip in Plugin.t in case the KitchenSink module already exists. Note: This is a small fix. But I wonder if the Handler routines run and delete should not have been implemented in Koha::Plugins::Base. Also note that plugins/plugins-uninstall.pl will not be affacted by this change, since it checks whether the pref is enabled before calling the delete method. Test plan: [1] Do not yet install this patch. [2] Verify that plugins are enabled in koha-conf.xml. [3] Disable UseKohaPlugins in System Preferences! [4] Run t/db_dependent/Plugins.t. [5] Verify that table koha_plugin_com_bywatersolutions_kitchensink_mytable still exists. (It should have been deleted.) Remove it manually. [6] Apply this patch. [7] Run the test again. [8] Verify that the table does not exist. [9] Run the test again (without warnings). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 52261 [details] [review] Bug 16502: Add additional test to Plugins.t In order to verify if the delete now really works, we add one test in Plugins.t. Test plan: [1] Run the test. [2] Bonus: Comment line 63 in Plugins.t where delete is called. Run the test again. It should fail now. Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 52262 [details] [review] Bug 16502: Adjust test for GetPlugins The current test assumes that GetPlugins will return the test plugin as the first one in the array. This is not correct. This patch adjusts the test to a grep. Test plan: Run the test. Bonus: Add additional plugins. Run the test again. Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 52263 [details] [review] Bug 16502: Replace a few other ok-calls by is-calls Trivial changes that speak for themselves.. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Pushed to master for 16.11, thanks Marcel! Pushed in 16.05. Will be in 16.05.01. Patches pushed to 3.22.x, will be in 3.22.9 Pushed to 3.20.x will be in 3.20.13 |