Bug 35507 - Fix handling plugins in unit tests causing random failures on Jenkins
Summary: Fix handling plugins in unit tests causing random failures on Jenkins
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 35536 35548
  Show dependency treegraph
 
Reported: 2023-12-06 15:52 UTC by Jonathan Druart
Modified: 2024-04-26 20:28 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.02,23.05.09


Attachments
Bug 35507: Clear cache key for enabled plugins at deletion (1.19 KB, patch)
2023-12-08 11:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35507: Add Plugins::Method->delete (4.82 KB, patch)
2023-12-12 13:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35507: Add Plugins::Method->delete (4.93 KB, patch)
2023-12-12 15:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35507: Plugins.t - Mock SessionStorage, remove KitchenSink (4.16 KB, patch)
2023-12-12 15:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35507: Plugins.t - Mock SessionStorage, remove KitchenSink (4.23 KB, patch)
2023-12-13 14:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35507: Add Plugins::Method->delete (5.21 KB, patch)
2023-12-13 14:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35507: Add Plugins::Method->delete (5.14 KB, patch)
2023-12-13 14:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35507: Plugins.t - Mock SessionStorage, remove KitchenSink (4.26 KB, patch)
2023-12-14 07:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35507: Add Plugins::Method->delete (5.18 KB, patch)
2023-12-14 07:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35507: Add Plugins::Method->delete (5.50 KB, patch)
2023-12-14 14:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35507: (follow-up) Fix PluginRoutes.t (1014 bytes, patch)
2023-12-14 15:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35507: Plugins.t - Mock SessionStorage, remove KitchenSink (4.32 KB, patch)
2023-12-14 16:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35507: Add Plugins::Method->delete (5.52 KB, patch)
2023-12-14 16:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35507: (follow-up) Fix PluginRoutes.t (1.01 KB, patch)
2023-12-14 16:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35507: Add verbose parameters in Koha::Plugins (4.36 KB, patch)
2023-12-15 08:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35507: Plugins.t - Mock SessionStorage, remove KitchenSink (4.38 KB, patch)
2023-12-15 10:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35507: Add verbose parameters in Koha::Plugins (4.38 KB, patch)
2023-12-15 10:36 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2023-12-06 15:52:44 UTC
koha_1       |     #   Failed test 'No encoding warnings!'
koha_1       |     #   at t/db_dependent/TestBuilder.t line 519.
koha_1       |     # found warning: Failed to load Koha::Plugin::Com::ByWaterSolutions::KitchenSink: Could not find or check module 'Koha::Plugin::Com::ByWaterSolutions::KitchenSink' at /kohadevbox/koha/Koha/Plugins.pm line 124.
koha_1       |     # didn't expect to find a warning
koha_1       |     # Looks like you failed 1 test of 1.
koha_1       | 
koha_1       | #   Failed test 'build_sample_biblio() tests'
koha_1       | #   at t/db_dependent/TestBuilder.t line 523.

Has been failing on Koha_Master/2759 but fixed in 2760.

The output is full of 
Failed to load Koha::Plugin::Com::ByWaterSolutions::KitchenSink: Could not find or check module 'Koha::Plugin::Com::ByWaterSolutions::KitchenSink' at /kohadevbox/koha/Koha/Plugins.pm line 124.
Comment 1 Katrin Fischer 2023-12-07 13:37:04 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)
Comment 2 Marcel de Rooy 2023-12-08 09:41:09 UTC
Looking here now
Comment 3 Marcel de Rooy 2023-12-08 11:01:20 UTC
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]
Comment 4 Marcel de Rooy 2023-12-08 11:02:01 UTC
Will still improve the test plan a bit.
Comment 5 Marcel de Rooy 2023-12-08 11:06:18 UTC
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.
Comment 6 Marcel de Rooy 2023-12-08 11:07:22 UTC
With this patch, Plugins.t will remove KitchenSink from the enabled cache key too, resolving issues in other tests.
Comment 7 Tomás Cohen Arazi 2023-12-08 21:31:19 UTC
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*
Comment 8 Marcel de Rooy 2023-12-11 09:43:03 UTC
(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.
Comment 9 Marcel de Rooy 2023-12-11 09:58:46 UTC
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-};
Comment 10 Marcel de Rooy 2023-12-11 10:23:50 UTC
=== 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.
Comment 11 Marcel de Rooy 2023-12-11 10:30:52 UTC
Adding 35536 on top of this one.
Comment 12 Marcel de Rooy 2023-12-11 10:31:42 UTC
Comment on attachment 159670 [details] [review]
Bug 35507: Clear cache key for enabled plugins at deletion

Obsoleting in favor of another approach.
Comment 13 Katrin Fischer 2023-12-11 12:16:25 UTC
Is this ready for SO?
Comment 14 Tomás Cohen Arazi 2023-12-11 12:18:34 UTC
(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.
Comment 15 Marcel de Rooy 2023-12-11 12:23:45 UTC
(In reply to Katrin Fischer from comment #13)
> Is this ready for SO?

No, I removed the patch :)
Comment 16 Marcel de Rooy 2023-12-11 13:57:49 UTC
(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?
Comment 17 Marcel de Rooy 2023-12-11 15:18:33 UTC
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
Comment 18 Marcel de Rooy 2023-12-12 13:16:18 UTC
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..
Comment 19 Marcel de Rooy 2023-12-12 13:43:38 UTC
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:
Comment 20 Marcel de Rooy 2023-12-12 13:44:29 UTC
Attached a patch still WIP. Before entering the rabbit hole of Plugins.t
Comment 21 Tomás Cohen Arazi 2023-12-12 13:45:04 UTC
(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/
Comment 22 Kyle M Hall 2023-12-12 13:45:49 UTC
(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
Comment 23 Marcel de Rooy 2023-12-12 13:47:35 UTC
(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).
Comment 24 Marcel de Rooy 2023-12-12 13:52:10 UTC
(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
Comment 25 Marcel de Rooy 2023-12-12 13:55:50 UTC
(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.
Comment 26 Marcel de Rooy 2023-12-12 13:57:41 UTC
    <- 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.
Comment 27 Marcel de Rooy 2023-12-12 13:59:22 UTC
get_template_and_user => haspermission => .. => get_user_subpermissions
Comment 28 Marcel de Rooy 2023-12-12 14:05:18 UTC
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' );
Comment 29 Marcel de Rooy 2023-12-12 14:55:18 UTC
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 !
Comment 30 Marcel de Rooy 2023-12-12 14:59:42 UTC
What do you folks think: Shall we remove the KitchenSink stuff from Plugins.t since it implicitly commits cruft in the database ?
Comment 31 Marcel de Rooy 2023-12-12 15:23:47 UTC
(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.
Comment 32 Marcel de Rooy 2023-12-12 15:35:53 UTC
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>
Comment 33 Marcel de Rooy 2023-12-12 15:35:56 UTC
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>
Comment 34 Marcel de Rooy 2023-12-12 15:37:04 UTC
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?
Comment 35 Marcel de Rooy 2023-12-12 15:43:15 UTC
Still thinking too about the REPLACE with 0 in Method->delete. We could also remove but then we need to explicitly clear cache.
Comment 36 Marcel de Rooy 2023-12-13 14:44:55 UTC
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>
Comment 37 Marcel de Rooy 2023-12-13 14:44:58 UTC
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>
Comment 38 Marcel de Rooy 2023-12-13 14:45:45 UTC
(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.
Comment 39 Marcel de Rooy 2023-12-13 14:46:16 UTC
(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.
Comment 40 Marcel de Rooy 2023-12-13 14:47:02 UTC
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.
Comment 41 Marcel de Rooy 2023-12-13 14:51:27 UTC
(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.
Comment 42 Marcel de Rooy 2023-12-13 14:52:11 UTC
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>
Comment 43 Jonathan Druart 2023-12-14 07:43:26 UTC
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>
Comment 44 Jonathan Druart 2023-12-14 07:43:29 UTC
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>
Comment 45 Jonathan Druart 2023-12-14 07:44:53 UTC
Trusting Marcel to continue the work he has started here.
Comment 46 Marcel de Rooy 2023-12-14 10:26:04 UTC
(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 47 Martin Renvoize 2023-12-14 13:18:50 UTC
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.
Comment 48 Marcel de Rooy 2023-12-14 14:03:49 UTC
InstallPlugins contains
Koha::Plugins::Methods->search({ plugin_class => $plugin_class })->delete();

So this is restricted to $plugin_class. What is scary exactly?
Comment 49 Marcel de Rooy 2023-12-14 14:24:51 UTC
(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.
Comment 50 Marcel de Rooy 2023-12-14 14:29:24 UTC
(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 :)
Comment 51 Marcel de Rooy 2023-12-14 14:30:57 UTC
(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 52 Marcel de Rooy 2023-12-14 14:35:10 UTC
Comment on attachment 159818 [details] [review]
Bug 35507: Add Plugins::Method->delete

Going back to older version and extending test a little bit
Comment 53 Marcel de Rooy 2023-12-14 14:45:24 UTC
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)
Comment 54 Marcel de Rooy 2023-12-14 14:50:24 UTC
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>
Comment 55 Marcel de Rooy 2023-12-14 15:38:04 UTC
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>
Comment 56 Kyle M Hall 2023-12-14 16:19:50 UTC
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>
Comment 57 Kyle M Hall 2023-12-14 16:19:56 UTC
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>
Comment 58 Kyle M Hall 2023-12-14 16:19:58 UTC
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>
Comment 59 Marcel de Rooy 2023-12-15 07:12:57 UTC
(In reply to Kyle M Hall from comment #58)
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Thanks!
Comment 60 Marcel de Rooy 2023-12-15 07:13:30 UTC
Still thinking about a potential issue
Comment 61 Marcel de Rooy 2023-12-15 08:58:17 UTC
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>
Comment 62 Marcel de Rooy 2023-12-15 08:58:50 UTC
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?
Comment 63 Marcel de Rooy 2023-12-15 09:03:12 UTC
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
Comment 64 Martin Renvoize 2023-12-15 10:36:46 UTC
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>
Comment 65 Martin Renvoize 2023-12-15 10:36:49 UTC
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>
Comment 66 Martin Renvoize 2023-12-15 10:37:30 UTC
Great work, thanks for all the effort.

No regressions found now, QA scripts happy and tests are now consistently passing :)

Passing QA
Comment 67 Katrin Fischer 2023-12-18 14:25:37 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 68 Fridolin Somers 2024-01-03 14:15:36 UTC
Pushed to 23.11.x for 23.11.02
Comment 69 Lucas Gass 2024-01-04 20:06:24 UTC
Doesn't apply cleanly to 23.05.x, no backport. If needed please rebase.
Comment 70 Lucas Gass 2024-02-14 16:30:05 UTC
Backported to 23.05.x for upcoming 23.05.09.