Bug 13940

Summary: Testing C4::OAI::Sets
Product: Koha Reporter: Julian FIOL <julian.fiol>
Component: Test SuiteAssignee: Julian FIOL <julian.fiol>
Status: CLOSED FIXED QA Contact: Galen Charlton <gmcharlt>
Severity: trivial    
Priority: P5 - low CC: julian.maurice, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 13940 : Testing C4::OAI::Sets
Bug 13940 : Testing C4::OAI::Sets
Bug 13940 : Testing C4::OAI::Sets
[PASSED QA] Bug 13940 : Testing C4::OAI::Sets
Bug 13940 : Testing C4::OAI::Sets (QA followup)
Bug 13940 : (QA followup) Add tests for the 2 warnings which was not tested
Bug 13940 : (QA followup) Add test for ModOAISet

Description Julian FIOL 2015-04-02 09:12:33 UTC
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
Comment 1 Julian FIOL 2015-04-02 09:16:02 UTC Comment hidden (obsolete)
Comment 2 Julian FIOL 2015-04-02 11:49:49 UTC Comment hidden (obsolete)
Comment 3 Julian FIOL 2015-04-02 12:12:12 UTC
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%
Comment 4 Mark Tompsett 2015-04-02 13:36:39 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2015-04-03 12:39:42 UTC
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 6 Tomás Cohen Arazi 2015-04-08 13:32:23 UTC
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
Comment 7 Julian FIOL 2015-04-09 08:25:52 UTC Comment hidden (obsolete)
Comment 8 Tomás Cohen Arazi 2015-04-09 13:50:58 UTC
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.
Comment 9 Julian FIOL 2015-04-09 14:29:34 UTC
Created attachment 37624 [details] [review]
Bug 13940 : (QA followup) Add tests for the 2 warnings which was not tested
Comment 10 Julian FIOL 2015-04-09 14:54:33 UTC
Created attachment 37625 [details] [review]
Bug 13940 : (QA followup) Add test for ModOAISet
Comment 11 Tomás Cohen Arazi 2015-04-09 17:56:15 UTC
Patches pushed to master.

Good job Julian!