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.
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.
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.
Moved to SeeAlso for 19929 to allow testing, only needed for QA
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.
(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.
It looks like this isn't necessary anymore, at step 2 of the test plan the tests did not fail.
(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 #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.
Please provide tests Mark.