Testing C4::OAI::Sets in t/db_dependent/OAI/Sets.t Adding &DelOAISetsBiblio in @EXPORT at the beginning of OAI/Sets.pm, it was the only one missing TEST PLAN --------- 1. Apply patch 2. prove -v t/db_dependent/OAI/Sets.t -- All 145 tests should run successfully without any error or warning
Created attachment 37429 [details] [review] Bug 13940 : Testing C4::OAI::Sets Testing C4::OAI::Sets in t/db_dependent/OAI/Sets.t Adding &DelOAISetsBiblio in @EXPORT at the beginning of OAI/Sets.pm, it was the only one missing TEST PLAN --------- 1. Apply patch 2. prove -v t/db_dependent/OAI/Sets.t -- All 145 tests should run successfully without any error or warning
Created attachment 37433 [details] [review] Bug 13940 : Testing C4::OAI::Sets Testing C4::OAI::Sets in t/db_dependent/OAI/Sets.t Adding &DelOAISetsBiblio in @EXPORT at the beginning of OAI/Sets.pm, it was the only one missing TEST PLAN --------- 1. Apply patch 2. prove -v t/db_dependent/OAI/Sets.t -- All 145 tests should run successfully without any error or warning
TEST PLAN OPTIONAL ------------------ Go check bug 13899 and see that coverage is 100% for the statements of this module. Coverage BEFORE this patch : Statement : 6,7% Branch : 0,0% Condition : 0,0% Subroutine : 21,1% Pod : 100,0% Coverage AFTER this patch : Statement : 100,0% Branch : 89,5% Condition : 88,8% Subroutine : 100,0% Pod : 100,0%
Created attachment 37438 [details] [review] Bug 13940 : Testing C4::OAI::Sets Testing C4::OAI::Sets in t/db_dependent/OAI/Sets.t Adding &DelOAISetsBiblio in @EXPORT at the beginning of OAI/Sets.pm, it was the only one missing TEST PLAN --------- 1. Apply patch 2. prove -v t/db_dependent/OAI/Sets.t -- All 145 tests should run successfully without any error or warning 3. Run koha qa test tools -- No issues found. Additionally, I comfirmed why DelOAISetsBiblio was added in the list of exported functions. $ grep "^sub " C4/OAI/Sets.pm | sort > check1 $ for i in `grep "\&[A-Z]" C4/OAI/Sets.pm | xargs `; do echo $i; done | sort > check2 Of course the outputs have a little extra around them, so $ vi check1 $ vi check2 $ diff check1 check2 Lo and behold, it was that one function. Necessary for 100% coverage. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 37454 [details] [review] [PASSED QA] Bug 13940 : Testing C4::OAI::Sets Testing C4::OAI::Sets in t/db_dependent/OAI/Sets.t Adding &DelOAISetsBiblio in @EXPORT at the beginning of OAI/Sets.pm, it was the only one missing TEST PLAN --------- 1. Apply patch 2. prove -v t/db_dependent/OAI/Sets.t -- All 145 tests should run successfully without any error or warning 3. Run koha qa test tools -- No issues found. Additionally, I comfirmed why DelOAISetsBiblio was added in the list of exported functions. $ grep "^sub " C4/OAI/Sets.pm | sort > check1 $ for i in `grep "\&[A-Z]" C4/OAI/Sets.pm | xargs `; do echo $i; done | sort > check2 Of course the outputs have a little extra around them, so $ vi check1 $ vi check2 $ diff check1 check2 Lo and behold, it was that one function. Necessary for 100% coverage. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment on attachment 37454 [details] [review] [PASSED QA] Bug 13940 : Testing C4::OAI::Sets >+#Test to enter in the 'else' case of 'AddOAISet' line 280 >+{ >+ my $dbi_st = Test::MockModule->new('DBI::st', no_auto => 1); # ref($sth) == 'DBI::st' >+ $dbi_st->mock('execute', sub { return 0; }); >+ >+ my $setWrong = { >+ 'spec' => 'specWrong', >+ 'name' => 'nameWrong', >+ }; >+ my $setWrong_id = AddOAISet($setWrong); >+ >+ DelOAISet($setWrong_id); >+} It seems to me that you forgot to actually test behaviour here. You are traversing that branch in the code, but still need to test for: - Side effects (no side effects should be tested if such was the case) - The code raises a warning. It whould be tested with a block like this: my $setWrong_id; warning_is ( $setWrong_id = AddOAISet($setWrong) ) 'AddOAISet failed', 'AddOAISet raises warning in XXX condition'; There's also another warning not being tested. You need to add use Test::Warn; for this purpose. I leave you the task to put the pieces together :-D
Created attachment 37602 [details] [review] Bug 13940 : Testing C4::OAI::Sets (QA followup) Testing C4::OAI::Sets in t/db_dependent/OAI/Sets.t Adding &DelOAISetsBiblio in @EXPORT at the beginning of OAI/Sets.pm, it was the only one missing To Tomás Cohen Arazi on comment #6 : Adding tests on the 2 warnings not tested with 'warning_is' like you suggested it. Hope it's good enough :) TEST PLAN --------- 1. Apply patch 2. prove -v t/db_dependent/OAI/Sets.t -- All 145 tests should run successfully without any error or warning
Maybe I wasn't clear enough. Sorry: The regular workflow when we ask for QA followups is to leave the currently testted patch as-is (with its signoffs, for instance) and provide a "new" patch to be applied on top of the original one. *That* new patch should have "(QA followup)" on its subject. Take a look at bug 13075 for example. You'll see an original bug, and a followup (this time made by me). So, I suggest you obsolete the new one, de-obsolete de passed-qa one, and provide just a small followup with the last changes.
Created attachment 37624 [details] [review] Bug 13940 : (QA followup) Add tests for the 2 warnings which was not tested
Created attachment 37625 [details] [review] Bug 13940 : (QA followup) Add test for ModOAISet
Patches pushed to master. Good job Julian!