Bug 33718 - _new_Zconn crashes on a bug in t::lib::Mocks::mock_config
Summary: _new_Zconn crashes on a bug in t::lib::Mocks::mock_config
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Marcel de Rooy
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-05-11 07:12 UTC by Marcel de Rooy
Modified: 2023-12-28 20:47 UTC (History)
5 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:
23.05.00,22.11.07


Attachments
Bug 33718: Fix the mock of Koha::Config->get in mock_config (2.29 KB, patch)
2023-05-11 12:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33718: Fix the mock of Koha::Config->get in mock_config (2.36 KB, patch)
2023-05-11 13:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33718: Fix the mock of Koha::Config->get in mock_config (2.37 KB, patch)
2023-05-11 21:16 UTC, David Nind
Details | Diff | Splinter Review
Bug 33718: Fix the mock of Koha::Config->get in mock_config (2.42 KB, patch)
2023-05-12 06:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33718: Fix the mock of Koha::Config->get in mock_config (2.44 KB, patch)
2023-05-14 11:06 UTC, Katrin Fischer
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 2023-05-11 07:12:32 UTC
Related to enabling pref LinkerRelink and friends, and running build_sample_biblio in a test like I found with Koha/Pseudonymization.t.
See test plan for enabling prefs on report 33277

Debugging brought me to this line:
    my $host = _common_config($server, 'listen')->{content};

The crash:
{UNKNOWN}: Can't use string ("authorities") as a HASH ref while "strict refs" in use at /usr/share/koha/C4/Context.pm line 590. at /usr/share/koha/C4/Biblio.pm line 304
Comment 1 Jonathan Druart 2023-05-11 10:00:09 UTC
If it's from tests only you need to call mock_userenv.
Comment 2 Marcel de Rooy 2023-05-11 11:25:56 UTC
(In reply to Jonathan Druart from comment #1)
> If it's from tests only you need to call mock_userenv.

It is a weird error that cannot be reproduced consistently yet. Context passes a second 'listen' parameter to Koha::Config->get and in that routine I did not see that parameter. Needs a bit more digging still.
Comment 3 Marcel de Rooy 2023-05-11 12:41:39 UTC
Got it. Patch coming. It is a more fundamental error in t::lib::Mocks.
Comment 4 Marcel de Rooy 2023-05-11 12:59:37 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2023-05-11 13:01:16 UTC
Created attachment 151075 [details] [review]
Bug 33718: Fix the mock of Koha::Config->get in mock_config

It should not mock calls for other sections than 'config' in
koha-conf.xml.

Test plan:
Without this patch:
[1] Enable CatalogModuleRelink and LinkerRelink. This will trigger
a SearchAuthorities call when creating a sample biblio.
Note: SearchAuthorities calls Zconn and gets back information from
a wrong part of koha-conf.xml.
[2] Run t/db_dependent/Koha/Pseudonymization.t
You should see something like:
  {UNKNOWN}: Can't use string ("authorities") as a HASH ref while "strict refs" in use at /usr/share/koha/C4/Context.pm line 587. at /usr/share/koha/C4/Biblio.pm line 302

With this patch:
[3] Run t/db_dependent/Koha/Pseudonymization.t. Should pass now.
[4] git grep -l mock_config | xargs -i{} prove {}
    Exclude Mocks.pm.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2023-05-11 13:02:55 UTC
Note for QA:

The crash refers to authorities (this comes from config/authorityserver in koha-conf while we expected information from the listen section in this line:
my $host = _common_config($server, 'listen')->{content};
Comment 7 David Nind 2023-05-11 21:16:42 UTC
Created attachment 151101 [details] [review]
Bug 33718: Fix the mock of Koha::Config->get in mock_config

It should not mock calls for other sections than 'config' in
koha-conf.xml.

Test plan:
Without this patch:
[1] Enable CatalogModuleRelink and LinkerRelink. This will trigger
a SearchAuthorities call when creating a sample biblio.
Note: SearchAuthorities calls Zconn and gets back information from
a wrong part of koha-conf.xml.
[2] Run t/db_dependent/Koha/Pseudonymization.t
You should see something like:
  {UNKNOWN}: Can't use string ("authorities") as a HASH ref while "strict refs" in use at /usr/share/koha/C4/Context.pm line 587. at /usr/share/koha/C4/Biblio.pm line 302

With this patch:
[3] Run t/db_dependent/Koha/Pseudonymization.t. Should pass now.
[4] git grep -l mock_config | xargs -i{} prove {}
    Exclude Mocks.pm.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2023-05-11 21:19:21 UTC
Testing notes (using KTD):

1. I had to enable AutoLinkBiblios to get the error message in step 2 (see comment 59 in bug 33277).

2. Tests pass with no errors after applying the patch.

3. For step 4, I ran: git grep -l mock_config | xargs -i{} prove {}
   => lots of tests are run and these should all pass except for t/lib/Mocks.pm 

  t/lib/Mocks.pm .. No subtests run 
  
  Test Summary Report
  -------------------
  t/lib/Mocks.pm (Wstat: 0 Tests: 0 Failed: 0)
    Parse errors: No plan found in TAP output
  Files=1, Tests=0,  0 wallclock secs ( 0.01 usr  0.00 sys +  0.16 cusr  0.03 csys =  0.20 CPU)
  Result: FAIL
Comment 9 Marcel de Rooy 2023-05-12 06:12:49 UTC
(In reply to David Nind from comment #8)

Thx David.

> 1. I had to enable AutoLinkBiblios to get the error message in step 2 (see
> comment 59 in bug 33277).

I checked my setup. Yes, I had it enabled. Good catch.

>   t/lib/Mocks.pm .. No subtests run 

Yes, that was expected :)
Comment 10 Marcel de Rooy 2023-05-12 06:14:59 UTC
Created attachment 151110 [details] [review]
Bug 33718: Fix the mock of Koha::Config->get in mock_config

It should not mock calls for other sections than 'config' in
koha-conf.xml.

Test plan:
Without this patch:
[1] Enable AutoLinkBiblios, CatalogModuleRelink and LinkerRelink.
This will trigger a SearchAuthorities call when creating a sample biblio.
Note: SearchAuthorities calls Zconn and gets back information from
a wrong part of koha-conf.xml.
[2] Run t/db_dependent/Koha/Pseudonymization.t
You should see something like:
  {UNKNOWN}: Can't use string ("authorities") as a HASH ref while "strict refs" in use at /usr/share/koha/C4/Context.pm line 587. at /usr/share/koha/C4/Biblio.pm line 302

With this patch:
[3] Run t/db_dependent/Koha/Pseudonymization.t. Should pass now.
[4] git grep -l mock_config | xargs -i{} prove {}
    Exclude Mocks.pm.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Comment 11 Marcel de Rooy 2023-05-12 06:15:23 UTC
Updated the test plan for pref AutoLinkBiblios.
Comment 12 Katrin Fischer 2023-05-14 11:06:12 UTC
Created attachment 151170 [details] [review]
Bug 33718: Fix the mock of Koha::Config->get in mock_config

It should not mock calls for other sections than 'config' in
koha-conf.xml.

Test plan:
Without this patch:
[1] Enable AutoLinkBiblios, CatalogModuleRelink and LinkerRelink.
This will trigger a SearchAuthorities call when creating a sample biblio.
Note: SearchAuthorities calls Zconn and gets back information from
a wrong part of koha-conf.xml.
[2] Run t/db_dependent/Koha/Pseudonymization.t
You should see something like:
  {UNKNOWN}: Can't use string ("authorities") as a HASH ref while "strict refs" in use at /usr/share/koha/C4/Context.pm line 587. at /usr/share/koha/C4/Biblio.pm line 302

With this patch:
[3] Run t/db_dependent/Koha/Pseudonymization.t. Should pass now.
[4] git grep -l mock_config | xargs -i{} prove {}
    Exclude Mocks.pm.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Marcel de Rooy 2023-05-15 06:27:34 UTC
(In reply to Katrin Fischer from comment #12)
> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Thx
Comment 14 Jonathan Druart 2023-05-16 09:52:43 UTC
Pushed to master for 23.05
Comment 15 Pedro Amorim 2023-06-07 10:16:46 UTC
Nice work everyone!

Pushed to 22.11.x for next release