Bug 41271

Summary: pod_coverage.t unintentionally attempts to launch a SIP server when checking SIPServer.pm
Product: Koha Reporter: Pedro Amorim <pedro.amorim>
Component: Architecture, internals, and plumbingAssignee: Pedro Amorim <pedro.amorim>
Status: Pushed to main --- QA Contact: Jonathan Druart <jonathan.druart>
Severity: normal    
Priority: P5 - low CC: jonathan.druart, lucas, martin.renvoize, pedro.amorim
Version: Main   
Hardware: All   
OS: All   
GIT URL: Initiative type: ---
Sponsorship status: --- Comma delimited list of Sponsors:
Crowdfunding goal: 0 Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.11.00
Circulation function:
Bug Depends on: 37893    
Bug Blocks:    
Attachments: Bug 41271: Make pod_coverage.t die verbosely instead of silently
Bug 41271: Dont run Net::Server::PreFork->run on 'require' or 'use'
Bug 41271: Fix pod_coverage for SIPServer.pm
Bug 41271: Revert follow-up from bug 37893
Bug 41271: Dont run Net::Server::PreFork->run on 'require' or 'use'
Bug 41271: Fix pod_coverage for SIPServer.pm
Bug 41271: Revert follow-up from bug 37893

Description Pedro Amorim 2025-11-19 15:47:08 UTC

    
Comment 1 Pedro Amorim 2025-11-19 16:17:20 UTC
Created attachment 189713 [details] [review]
Bug 41271: Make pod_coverage.t die verbosely instead of silently

To test, ktd:
1) Rollback to commit before bug 37893:
   git reset --hard 3f59557d2ac
2) Apply this patch
3) Reset:
   reset_all
4) Run the pod_coverage.t test:
   prove -v /kohadevbox/koha/xt/author/pod_coverage.t

You get error:
"Module died while loading 'C4::SIP::SIPServer': Could not find pod_coverage.xml in /kohadevbox/koha/xt/author.
Comment 2 Pedro Amorim 2025-11-19 16:24:43 UTC
Hi all, I don't know what the fix should be here, but SIPServer.pm is seemingly being checked for pod_coverage.t, except it isn't. It's dying and pod_coverage.t skips over it.

Questions:
A) Should we push this patch (or similar)? To ensure that pod_coverage.t actually does its thing or dies verbosely.
B) Do we exclude SIPServer.pm from pod_coverage altogether? 
C) You can skip step 1) of the test plan in the patch (that was just to highlight that issue precedes bug 37893), and you'll still reproduce the issue, for SIPServer.pm (with latest bug 37893 follow-up applied but for the reason that it dies intentionally now) and for misc/translator/TmplTokenizer.pm:
Can't locate VerboseWarnings.pm
Comment 3 Pedro Amorim 2025-11-20 15:59:05 UTC
Created attachment 189797 [details] [review]
Bug 41271: Dont run Net::Server::PreFork->run on 'require' or 'use'

By using 'unless caller' we ensure the class is not executed when Pod::Coverage->new does 'require C4/SIP/SIPServer.pm

Ensure no regressions are added:
prove t/db_dependent/SIP/*

koha-sip --restart kohadev
koha-sip --status kohadev
Comment 4 Pedro Amorim 2025-11-20 15:59:07 UTC
Created attachment 189798 [details] [review]
Bug 41271: Fix pod_coverage for SIPServer.pm

Because the file is now being properly checked for pod_coverage, it fails (because a lot of methods dont have pod coverage. Add here
Comment 5 Pedro Amorim 2025-11-20 15:59:10 UTC
Created attachment 189799 [details] [review]
Bug 41271: Revert follow-up from bug 37893

This was a bad workaround.
It was ugly and is no longer relevant now that pod_coverage.t no longer attempts to launch a SIP server.

prove xt/author/pod_coverage.t
Comment 6 Pedro Amorim 2025-11-20 16:00:47 UTC
Had a chat with Joubu about this and I agree with his opinion that the 'die' patch from bug 37893 was ugly. I believe we both agree that adding 'unless caller' to SIPServer.pm is the most sensible approach here.
Comment 7 Pedro Amorim 2025-11-20 16:03:53 UTC
I don't think this should be backported as it's cleaning up an inconsequential bug only made evident by bug 37893 - which is not backportable.
Comment 8 Jonathan Druart 2025-11-21 08:47:06 UTC
Created attachment 189828 [details] [review]
Bug 41271: Dont run Net::Server::PreFork->run on 'require' or 'use'

By using 'unless caller' we ensure the class is not executed when Pod::Coverage->new does 'require C4/SIP/SIPServer.pm

Ensure no regressions are added:
prove t/db_dependent/SIP/*

koha-sip --restart kohadev
koha-sip --status kohadev

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 9 Jonathan Druart 2025-11-21 08:47:07 UTC
Created attachment 189829 [details] [review]
Bug 41271: Fix pod_coverage for SIPServer.pm

Because the file is now being properly checked for pod_coverage, it fails (because a lot of methods dont have pod coverage. Add here

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Jonathan Druart 2025-11-21 08:47:09 UTC
Created attachment 189830 [details] [review]
Bug 41271: Revert follow-up from bug 37893

This was a bad workaround.
It was ugly and is no longer relevant now that pod_coverage.t no longer attempts to launch a SIP server.

prove xt/author/pod_coverage.t

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Lucas Gass (lukeg) 2025-11-21 15:43:19 UTC
Nice work everyone!

Pushed to main for 25.11