Bug 20357

Summary: Pod Coverage checking for no public subs
Product: Project Infrastructure Reporter: Mark Tompsett <mtompset>
Component: QA toolsAssignee: Mark Tompsett <mtompset>
Status: Failed QA --- QA Contact:
Severity: minor    
Priority: P5 - low CC: hayleypelham, jonathan.druart, josef.moravec, katrin.fischer, m.de.rooy, martin.renvoize, nick, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19929
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 20357: Correct POD Coverage checking for zero sub case
Bug 20357: Correct POD Coverage checking for zero sub case

Description Mark Tompsett 2018-03-07 19:27:00 UTC
While testing bug 19929, the qa test tools complained of pod coverage. I believe this is in error. Reading the Reports.pm file, it makes no sense to complain about a changed rating when there is nothing in subs before and after.
Comment 1 Mark Tompsett 2018-03-07 19:32:17 UTC
Created attachment 72519 [details] [review]
Bug 20357: Correct POD Coverage checking for zero sub case

Currently, bug 19929 fails QA Test tools, but it
makes no sense.

TEST PLAN
---------
1) in kohaclone branch apply 19929
2) Run the following commands:
    kshell
    /home/vagrant/qa-test-tools/koha-qa.pl -v 2 -c 1
   -- This will fail with POD coverage messages.
   exit
3) apply this patch to qa-test-tools
4) re do step 2
   -- POD coverage messages will be cleared up.
Comment 2 Mark Tompsett 2018-03-07 20:12:26 UTC
Created attachment 72521 [details] [review]
Bug 20357: Correct POD Coverage checking for zero sub case

Currently, bug 19929 fails QA Test tools, but it
makes no sense.

TEST PLAN
---------
1) in kohaclone branch apply 19929
2) Run the following commands:
    kshell
    /home/vagrant/qa-test-tools/koha-qa.pl -v 2 -c 1
   -- This will fail with POD coverage messages.
   exit
3) apply this patch to qa-test-tools
4) re do step 2
   -- POD coverage messages will be cleared up.
Comment 3 Nick Clemens 2018-03-15 13:02:55 UTC
Moved to SeeAlso for 19929 to allow testing, only needed for QA
Comment 4 Marcel de Rooy 2018-03-15 14:26:40 UTC
Mark, what about:

            } elsif ( scalar @{ $after->{subs} } == 0 ) {
                # This is perfectly okay.

If there are no subs anymore, do not complain about pod coverage.
Note that no subs means no 'public' subs; there may still be subs like _type with an underscore.
Comment 5 Mark Tompsett 2018-03-15 14:39:26 UTC
(In reply to Marcel de Rooy from comment #4)
> Mark, what about:
> 
>             } elsif ( scalar @{ $after->{subs} } == 0 ) {
>                 # This is perfectly okay.
> 
> If there are no subs anymore, do not complain about pod coverage.
> Note that no subs means no 'public' subs; there may still be subs like _type
> with an underscore.

Because, if you cut out a function, I rather fall to the next check, then just allow out right cuts. I can't recall the test case I hit that made me want to do both. Probably something about increasing, then decreasing in the same patch set.
Comment 6 Hayley Pelham 2019-03-08 01:05:21 UTC
It looks like this isn't necessary anymore, at step 2 of the test plan the tests did not fail.
Comment 7 Hayley Pelham 2019-03-08 01:07:01 UTC
(In reply to Hayley Mapley from comment #6)
> It looks like this isn't necessary anymore, at step 2 of the test plan the
> tests did not fail.

Actually, on second look the first patch of 19929 did not apply cleanly. I'll request a rebase and then test this again if you deem it still necessary.
Comment 8 Hayley Pelham 2019-03-08 01:08:13 UTC
(In reply to Hayley Mapley from comment #6)
> It looks like this isn't necessary anymore, at step 2 of the test plan the
> tests did not fail.

Actually, on second look the first patch of 19929 did not apply cleanly. I'll request a rebase and then test this again if you deem it still necessary.(In reply to Hayley Mapley from comment #7)
> (In reply to Hayley Mapley from comment #6)
> > It looks like this isn't necessary anymore, at step 2 of the test plan the
> > tests did not fail.
> 
> Actually, on second look the first patch of 19929 did not apply cleanly.
> I'll request a rebase and then test this again if you deem it still
> necessary.

I now see that it's been pushed to master, so nevermind. Sorry for the noise everyone.
Comment 9 Jonathan Druart 2019-03-08 22:35:05 UTC
Please provide tests Mark.