|
Description
Kyle M Hall (khall)
2021-04-23 15:53:30 UTC
Created attachment 120120 [details] [review] Bug 28211: Replace use of call_recursive() with call() This is based on Julian's idea on bug 28026 where we could get rid of call_recursive by passing refs as arguments to call. Test Plan: 1) Apply this patch 2) prove t/db_dependent/Koha/Plugins/Plugins.t 3) prove t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t I get this test failure:
root@kohadevbox:koha(master)$ prove t/db_dependent/Koha/Plugins/Plugins.t
t/db_dependent/Koha/Plugins/Plugins.t .. 5/55 Plugin error (Test Plugin): item_barcode_transform called with parameter: 1 at /kohadevbox/koha/Koha/Plugins.pm line 74.
Plugin error (Test Plugin): item_barcode_transform called with parameter: abcd at /kohadevbox/koha/Koha/Plugins.pm line 74.
# Failed test 'Only four plugins found via a metadata tag'
# at t/db_dependent/Koha/Plugins/Plugins.t line 129.
# got: '2'
# expected: '4'
# Looks like you failed 1 test of 2.
# Failed test 'GetPlugins() tests'
# at t/db_dependent/Koha/Plugins/Plugins.t line 132.
Use of uninitialized value $interface in string eq at /kohadevbox/koha/C4/Templates.pm line 279.
Use of uninitialized value $interface in string eq at /kohadevbox/koha/C4/Templates.pm line 280.
Use of uninitialized value $interface in string eq at /kohadevbox/koha/C4/Templates.pm line 282.
Use of uninitialized value $lang in concatenation (.) or string at /kohadevbox/koha/C4/Templates.pm line 296.
Use of uninitialized value $lang in concatenation (.) or string at /kohadevbox/koha/C4/Templates.pm line 296.
Use of uninitialized value $lang in concatenation (.) or string at /kohadevbox/koha/C4/Templates.pm line 296.
t/db_dependent/Koha/Plugins/Plugins.t .. 50/55 # Looks like you failed 1 test of 55.
t/db_dependent/Koha/Plugins/Plugins.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/55 subtests
Test Summary Report
-------------------
t/db_dependent/Koha/Plugins/Plugins.t (Wstat: 256 Tests: 55 Failed: 1)
Failed test: 7
Non-zero exit status: 1
Files=1, Tests=55, 5 wallclock secs ( 0.02 usr 0.01 sys + 1.80 cusr 0.39 csys = 2.22 CPU)
Result: FAIL
Created attachment 120875 [details] [review] Bug 28211 Fix count of plugins that should be installed for unit test Created attachment 120882 [details] [review] Bug 28211: Replace use of call_recursive() with call() This is based on Julian's idea on bug 28026 where we could get rid of call_recursive by passing refs as arguments to call. Test Plan: 1) Apply this patch 2) prove t/db_dependent/Koha/Plugins/Plugins.t 3) prove t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 120883 [details] [review] Bug 28211 Fix count of plugins that should be installed for unit test Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> I would recommend to merge this report with the call_recursive report. Both are signed off. We could eliminate the code we actually do not want to push. The call_recursive name was a bit misleading too btw. (In reply to Marcel de Rooy from comment #6) > I would recommend to merge this report with the call_recursive report. Both > are signed off. We could eliminate the code we actually do not want to push. > > The call_recursive name was a bit misleading too btw. The two bugs are on the other sides of 26351 and 26352. Merging them would require modifying the patches for those bugs, both of which have passed qa. I think we should simply push the four bugs as a unit rather than risk introducing regressions. (In reply to Kyle M Hall from comment #7) > (In reply to Marcel de Rooy from comment #6) > > I would recommend to merge this report with the call_recursive report. Both > > are signed off. We could eliminate the code we actually do not want to push. > > > > The call_recursive name was a bit misleading too btw. > > The two bugs are on the other sides of 26351 and 26352. Merging them would > require modifying the patches for those bugs, both of which have passed qa. > I think we should simply push the four bugs as a unit rather than risk > introducing regressions. Okay. I missed the other two ;) Applying: Bug 26351: Clean up includes in Circulation.pm error: sha1 information is lacking or useless (C4/Circulation.pm). error: could not build fake ancestor Patch failed at 0001 Bug 26351: Clean up includes in Circulation.pm hint: Use 'git am --show-current-patch' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-26351-Clean-up-includes-in-Circulationpm-DpTlZT.patch Cannot apply cleanly patches from bug 26351. Everything will be left dirty. git bz apply --continue will not continue the process if patches from other bug reports need to be applied. Patches applied fine for me. Did you apply on top of 26352? Created attachment 122425 [details] [review] Bug 28211: Replace use of call_recursive() with call() This is based on Julian's idea on bug 28026 where we could get rid of call_recursive by passing refs as arguments to call. Test Plan: 1) Apply this patch 2) prove t/db_dependent/Koha/Plugins/Plugins.t 3) prove t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 122426 [details] [review] Bug 28211: Replace use of call_recursive() with call() This is based on Julian's idea on bug 28026 where we could get rid of call_recursive by passing refs as arguments to call. Test Plan: 1) Apply this patch 2) prove t/db_dependent/Koha/Plugins/Plugins.t 3) prove t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Created attachment 122427 [details] [review] Bug 28211: Fix count of plugins that should be installed for unit test Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Asked a question about one of the base reports still not pushed. Created attachment 125892 [details] [review] Bug 28211: Replace use of call_recursive() with call() This is based on Julian's idea on bug 28026 where we could get rid of call_recursive by passing refs as arguments to call. Test Plan: 1) Apply this patch 2) prove t/db_dependent/Koha/Plugins/Plugins.t 3) prove t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t QAing Created attachment 125926 [details] [review] Bug 28211: Replace use of call_recursive() with call() This is based on Julian's idea on bug 28026 where we could get rid of call_recursive by passing refs as arguments to call. Test Plan: 1) Apply this patch 2) prove t/db_dependent/Koha/Plugins/Plugins.t 3) prove t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 125927 [details] [review] Bug 28211: (QA follow-up) Remove test_call_recursive Obsoleted now. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 125928 [details] [review] Bug 28211: (QA follow-up) POD changes [1] Resolving qa warn on POD coverage. [2] Adding a line about the passing refs-tric. [3] Removing an outdated line on available methods. [4] Removing POD section for after_hold_create. Strayed. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> No blocker. But this behavior seems a little bit hacky to me. I would personally rather get back a value from a plugin and decide what to do with it. I think we are missing a test (passing refs from one plugin to another) for ->call, can you double check please? Created attachment 126274 [details] [review] Bug 28211: (QA follow-up) Add unit tests Pushed to master for 21.11, thanks to everybody involved! We should highlight this behaviour in the release notes and on the wiki page. Tests are failing, can you fix ASAP please?
t/db_dependent/Koha/REST/Plugin/PluginRoutes.t .. Calling 'install' died for plugin Koha::Plugin::BrokenInstall at /kohadevbox/koha/Koha/Plugins.pm line 199.Calling 'upgrade' died for plugin Koha::Plugin::BrokenUpgrade at /kohadevbox/koha/Koha/Plugins.pm line 199.
t/db_dependent/Koha/REST/Plugin/PluginRoutes.t .. 1/4 Calling 'install' died for plugin Koha::Plugin::BrokenInstall at /kohadevbox/koha/Koha/Plugins.pm line 199.
Calling 'upgrade' died for plugin Koha::Plugin::BrokenUpgrade at /kohadevbox/koha/Koha/Plugins.pm line 199.
# Failed test 'Plugin enabled, route defined'
# at t/db_dependent/Koha/REST/Plugin/PluginRoutes.t line 125.
# Looks like you failed 1 test of 2.t/db_dependent/Koha/REST/Plugin/PluginRoutes.t .. 2/4
# Failed test 'Disabled plugins tests'# at t/db_dependent/Koha/REST/Plugin/PluginRoutes.t line 129.
Calling 'install' died for plugin Koha::Plugin::BrokenInstall at /kohadevbox/koha/Koha/Plugins.pm line 199.
Calling 'upgrade' died for plugin Koha::Plugin::BrokenUpgrade at /kohadevbox/koha/Koha/Plugins.pm line 199.
Plugin error (Test Plugin): patron_barcode_transform called with parameter: swJd8T4G9NdzlYsedEoNpzbbda70 at /kohadevbox/koha/Koha/Plugins.pm line 84.
t/db_dependent/Koha/REST/Plugin/PluginRoutes.t .. 3/4 Calling 'install' died for plugin Koha::Plugin::BrokenInstall at /kohadevbox/koha/Koha/Plugins.pm line 199.
Calling 'upgrade' died for plugin Koha::Plugin::BrokenUpgrade at /kohadevbox/koha/Koha/Plugins.pm line 199.
Calling 'install' died for plugin Koha::Plugin::BrokenInstall at /kohadevbox/koha/Koha/Plugins.pm line 199.
Calling 'upgrade' died for plugin Koha::Plugin::BrokenUpgrade at /kohadevbox/koha/Koha/Plugins.pm line 199.
t/db_dependent/Koha/REST/Plugin/PluginRoutes.t .. 4/4 # Looks like you failed 1 test of 4.
t/db_dependent/Koha/REST/Plugin/PluginRoutes.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/4 subtests
Test Summary Report
-------------------
t/db_dependent/Koha/REST/Plugin/PluginRoutes.t (Wstat: 256 Tests: 4 Failed: 1)
Failed test: 2
Non-zero exit status: 1
Files=1, Tests=4, 31 wallclock secs ( 0.03 usr 0.00 sys + 30.24 cusr 0.41 csys = 30.68 CPU)
Result: FAIL
(In reply to Jonathan Druart from comment #25) > Tests are failing, can you fix ASAP please? I'm not seeing any reason this test module is causing the failures. If you rm that test module, the unit tests pass! Tomas is going to take a look to see if he can spot something I missed. Created attachment 126471 [details] [review] Bug 28211: Fix test # Failed test 'Plugin enabled, route defined' # at t/db_dependent/Koha/REST/Plugin/PluginRoutes.t line 125. # Looks like you failed 1 test of 2. 105 $good_plugin = $plugin 106 if $plugin->{metadata}->{description} eq 'Test plugin'; The wrong plugin was considered the good one. Patch pushed to master. Created attachment 126497 [details] [review] Bug 28211: (QA follow-up) Test for hook calls properly Some warnings, that mean a hook has been called were not tested. This patch adds those tests, and does some tidy also. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> (In reply to Tomás Cohen Arazi from comment #29) > Created attachment 126497 [details] [review] [review] > Bug 28211: (QA follow-up) Test for hook calls properly > > Some warnings, that mean a hook has been called were not tested. > This patch adds those tests, and does some tidy also. > > Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Pushed to master! |