Bug 13940 - Testing C4::OAI::Sets
Summary: Testing C4::OAI::Sets
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial (vote)
Assignee: Julian FIOL
QA Contact: Galen Charlton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-02 09:12 UTC by Julian FIOL
Modified: 2015-12-03 22:11 UTC (History)
2 users (show)

See Also:
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 (23.41 KB, patch)
2015-04-02 09:16 UTC, Julian FIOL
Details | Diff | Splinter Review
Bug 13940 : Testing C4::OAI::Sets (23.29 KB, patch)
2015-04-02 11:49 UTC, Julian FIOL
Details | Diff | Splinter Review
Bug 13940 : Testing C4::OAI::Sets (23.78 KB, patch)
2015-04-02 13:36 UTC, Mark Tompsett
Details | Diff | Splinter Review
[PASSED QA] Bug 13940 : Testing C4::OAI::Sets (23.85 KB, patch)
2015-04-03 12:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13940 : Testing C4::OAI::Sets (QA followup) (23.82 KB, patch)
2015-04-09 08:25 UTC, Julian FIOL
Details | Diff | Splinter Review
Bug 13940 : (QA followup) Add tests for the 2 warnings which was not tested (1.88 KB, patch)
2015-04-09 14:29 UTC, Julian FIOL
Details | Diff | Splinter Review
Bug 13940 : (QA followup) Add test for ModOAISet (1.11 KB, patch)
2015-04-09 14:54 UTC, Julian FIOL
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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!