Bug 16502 - Table koha_plugin_com_bywatersolutions_kitchensink_mytable not always dropped after running Plugin.t
Summary: Table koha_plugin_com_bywatersolutions_kitchensink_mytable not always dropped...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 16586
  Show dependency treegraph
 
Reported: 2016-05-11 14:44 UTC by Marcel de Rooy
Modified: 2017-12-07 22:17 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 16502: Table koha_plugin_com_bywatersolutions_kitchensink_mytable not always dropped after running Plugin.t (3.83 KB, patch)
2016-05-16 17:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16502: Add additional test to Plugins.t (2.27 KB, patch)
2016-05-16 17:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16502: Adjust test for GetPlugins (1.35 KB, patch)
2016-05-23 07:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16502: Replace a few other ok-calls by is-calls (1.96 KB, patch)
2016-05-23 07:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16502: Add additional test to Plugins.t (2.28 KB, patch)
2016-05-26 00:02 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16502: Adjust test for GetPlugins (1.35 KB, patch)
2016-05-26 00:02 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16502: Replace a few other ok-calls by is-calls (1.96 KB, patch)
2016-05-26 00:03 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 16502: Table koha_plugin_com_bywatersolutions_kitchensink_mytable not always dropped after running Plugin.t (3.94 KB, patch)
2016-05-27 06:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16502: Add additional test to Plugins.t (2.32 KB, patch)
2016-05-27 06:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16502: Adjust test for GetPlugins (1.40 KB, patch)
2016-05-27 06:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16502: Replace a few other ok-calls by is-calls (2.00 KB, patch)
2016-05-27 06:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16502: Table koha_plugin_com_bywatersolutions_kitchensink_mytable not always dropped after running Plugin.t (3.96 KB, patch)
2016-06-11 15:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16502: Add additional test to Plugins.t (2.35 KB, patch)
2016-06-11 15:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16502: Adjust test for GetPlugins (1.42 KB, patch)
2016-06-11 15:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16502: Replace a few other ok-calls by is-calls (2.03 KB, patch)
2016-06-11 15:13 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 Marcel de Rooy 2016-05-11 14:44:38 UTC
Happened to notice that running this test (t/db_dependent/Plugin.t) twice may not have the expected result. Sometimes(?) I need to manually drop the table. Note that quickly adding a transaction does not resolve it.
Comment 1 Marcel de Rooy 2016-05-16 17:12:28 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2016-05-16 17:13:09 UTC Comment hidden (obsolete)
Comment 3 Srdjan Jankovic 2016-05-23 02:30:34 UTC
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'
Comment 4 Marcel de Rooy 2016-05-23 06:53:34 UTC
(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.
Comment 5 Marcel de Rooy 2016-05-23 07:09:48 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2016-05-23 07:09:51 UTC Comment hidden (obsolete)
Comment 7 Srdjan Jankovic 2016-05-26 00:02:08 UTC Comment hidden (obsolete)
Comment 8 Srdjan Jankovic 2016-05-26 00:02:25 UTC Comment hidden (obsolete)
Comment 9 Srdjan Jankovic 2016-05-26 00:03:55 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2016-05-26 06:40:58 UTC
Thanks Srdjan for signing off.
I notice that the first patch did not get your signoff line.
Was that intentional or just forgotten?
Comment 11 Srdjan Jankovic 2016-05-26 23:26:38 UTC
Apologies, my mistake, did not adjust for the additional patch.
Comment 12 Marcel de Rooy 2016-05-27 06:19:35 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2016-05-27 06:19:39 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2016-05-27 06:19:42 UTC Comment hidden (obsolete)
Comment 15 Marcel de Rooy 2016-05-27 06:19:46 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2016-05-27 06:21:25 UTC
(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
Comment 17 Srdjan Jankovic 2016-05-27 06:31:41 UTC
I meant to do it sometimes today, after settling current affairs. But this is ok too. Thanks.
Comment 18 Jonathan Druart 2016-06-11 15:13:44 UTC
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>
Comment 19 Jonathan Druart 2016-06-11 15:13:47 UTC
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>
Comment 20 Jonathan Druart 2016-06-11 15:13:51 UTC
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>
Comment 21 Jonathan Druart 2016-06-11 15:13:56 UTC
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>
Comment 22 Kyle M Hall 2016-06-17 14:34:45 UTC
Pushed to master for 16.11, thanks Marcel!
Comment 23 Frédéric Demians 2016-06-24 05:44:26 UTC
Pushed in 16.05. Will be in 16.05.01.
Comment 24 Julian Maurice 2016-07-01 06:41:19 UTC
Patches pushed to 3.22.x, will be in 3.22.9
Comment 25 Chris Cormack 2016-07-07 23:09:33 UTC
Pushed to 3.20.x will be in 3.20.13