Bug 10528 - UT: C4::Bookseller.pm needs unit tests
Summary: UT: C4::Bookseller.pm needs unit tests
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: kenza
QA Contact:
URL:
Keywords:
Depends on: 10556 10557
Blocks: 10630 10640 10641
  Show dependency treegraph
 
Reported: 2013-07-02 12:54 UTC by kenza
Modified: 2014-12-07 20:03 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10528: C4::Booksellers needs unit tests (30.97 KB, patch)
2013-07-11 13:08 UTC, kenza
Details | Diff | Splinter Review
Bug 10528: C4::Booksellers needs unit tests (30.99 KB, patch)
2013-07-17 08:18 UTC, kenza
Details | Diff | Splinter Review
Bug 10528: C4::Booksellers needs unit tests (31.63 KB, patch)
2013-07-18 11:46 UTC, kenza
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10528: C4::Booksellers needs unit tests (31.67 KB, patch)
2013-07-19 03:34 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[PASSED QA] Bug 10528: C4::Booksellers needs unit tests (31.79 KB, patch)
2013-07-22 17:35 UTC, Katrin Fischer
Details | Diff | Splinter Review
C4::Bookseller.pm need unit tests (27.34 KB, patch)
2013-07-23 15:00 UTC, kenza
Details | Diff | Splinter Review
[SIGNED-OFF] C4::Bookseller.pm need unit tests (27.39 KB, patch)
2013-07-25 22:50 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[PASSED QA] C4::Bookseller.pm need unit tests (27.59 KB, patch)
2013-08-25 11:39 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description kenza 2013-07-02 12:54:28 UTC

    
Comment 1 kenza 2013-07-11 13:08:30 UTC Comment hidden (obsolete)
Comment 2 Srdjan Jankovic 2013-07-16 05:45:23 UTC
There's something wrong with the patch:
git am --reject ~/Bug-10528-C4Booksellers-needs-unit-tests.patch
Applying: Bug 10528: C4::Booksellers needs unit tests
Checking patch C4/Bookseller.pm...
error: while searching for:
sub ModBookseller {
    my ($data) = @_;
    my $dbh    = C4::Context->dbh;
    my $query  = q|
        UPDATE aqbooksellers
        SET name = ?,      address1 = ?,     address2 = ?,      address3 = ?,  address4 = ?,

error: patch failed: C4/Bookseller.pm:226
error: while searching for:
        ,$data->{notes}        ,$data->{deliverytime}
        ,$data->{id}
    );
    return;
}

=head2 DelBookseller

error: patch failed: C4/Bookseller.pm:254
Checking patch t/db_dependent/Bookseller.t...
/home/srdjan/koha/dev/.git/rebase-apply/patch:786: new blank line at EOF.
+
Comment 3 Jonathan Druart 2013-07-16 08:36:54 UTC
(In reply to Srdjan Jankovic from comment #2)
> There's something wrong with the patch:

Did you applied dependent patches?
Comment 4 Srdjan Jankovic 2013-07-16 23:11:08 UTC
Yes. 10556 should not be applied any more I guess, it has been pushed to master.
Comment 5 kenza 2013-07-17 08:18:26 UTC Comment hidden (obsolete)
Comment 6 Srdjan Jankovic 2013-07-18 01:06:36 UTC
prove t/db_dependent/Bookseller.t 
t/db_dependent/Bookseller.t .. 1/67 
#   Failed test 'Returns right fields of Supplier1 and Supllier2'
#   at t/db_dependent/Bookseller.t line 128.
#     Structures begin differing at:
#          $got->[0]{listincgst} = '0'
#     $expected->[0]{listincgst} = '1'
Use of uninitialized value $calculated in substitution (s///) at C4/Serials.pm line 988.
Use of uninitialized value $pattern in numeric eq (==) at C4/Serials.pm line 991.
Use of uninitialized value $calculated in substitution (s///) at C4/Serials.pm line 1000.
Use of uninitialized value $calculated in substitution (s///) at C4/Serials.pm line 1003.
Use of uninitialized value $serialseq in string at C4/Serials.pm line 1391.
Use of uninitialized value $calculated in substitution (s///) at C4/Serials.pm line 988.
Use of uninitialized value $pattern in numeric eq (==) at C4/Serials.pm line 991.
Use of uninitialized value $calculated in substitution (s///) at C4/Serials.pm line 1000.
Use of uninitialized value $calculated in substitution (s///) at C4/Serials.pm line 1003.
Use of uninitialized value $serialseq in string at C4/Serials.pm line 1391.
Use of uninitialized value $calculated in substitution (s///) at C4/Serials.pm line 988.
Use of uninitialized value $pattern in numeric eq (==) at C4/Serials.pm line 991.
Use of uninitialized value $calculated in substitution (s///) at C4/Serials.pm line 1000.
Use of uninitialized value $calculated in substitution (s///) at C4/Serials.pm line 1003.
Use of uninitialized value $serialseq in string at C4/Serials.pm line 1391.
WARNING: GetBooksellerWithLateOrders is called with a negative value at C4/Bookseller.pm line 137.
# Looks like you failed 1 test of 67.


Do I need 10337 for this one too?
Comment 7 kenza 2013-07-18 11:46:56 UTC Comment hidden (obsolete)
Comment 8 kenza 2013-07-18 11:53:08 UTC
t/db_dependent/Bookseller.t has been modify. Now, it deletes all the booksellers of the database at the beginning of the transaction in order to be sure the tests pass.
Comment 9 Srdjan Jankovic 2013-07-19 03:34:57 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2013-07-22 17:35:42 UTC Comment hidden (obsolete)
Comment 11 Galen Charlton 2013-07-22 18:54:14 UTC
I have reservations about this patch.  Adding more unit tests is a good thing, obviously, and the proposed changes to functions in C4::Booksellers may be good things.  However, they don't belong squashed together in a single patch, particularly since the patch description provides no test plan for the staff interfaces -- and if you change core routines, there ought to be such a test plan included in the patch, even if the only thing it does is identify the interfaces whose visible behavior is not changed.

Please break it up, for example like this:

[1] A patch for this bug that *only* adds additional unit tests.
[2] An additional bug for each change you propose to routines in C4::Booksellers, along with any changes to the test script.
Comment 12 kenza 2013-07-23 07:25:54 UTC
(In reply to Galen Charlton from comment #11)
> I have reservations about this patch.  Adding more unit tests is a good
> thing, obviously, and the proposed changes to functions in C4::Booksellers
> may be good things.  However, they don't belong squashed together in a
> single patch, particularly since the patch description provides no test plan
> for the staff interfaces -- and if you change core routines, there ought to
> be such a test plan included in the patch, even if the only thing it does is
> identify the interfaces whose visible behavior is not changed.
> 
> Please break it up, for example like this:
> 
> [1] A patch for this bug that *only* adds additional unit tests.
> [2] An additional bug for each change you propose to routines in
> C4::Booksellers, along with any changes to the test script.

Ok I will do that. Thanks for the advices.
Comment 13 kenza 2013-07-23 15:00:11 UTC Comment hidden (obsolete)
Comment 14 Srdjan Jankovic 2013-07-24 03:03:00 UTC
This won't work. Since the first patch is PASSED QA, I suggest the unit test patch is rebased to the first patch.
Comment 15 kenza 2013-07-24 09:41:53 UTC
(In reply to Srdjan Jankovic from comment #14)
> This won't work. Since the first patch is PASSED QA, I suggest the unit test
> patch is rebased to the first patch.

But the first patch is FAILED QA...
Comment 16 Srdjan Jankovic 2013-07-25 02:35:26 UTC
Ok, then it the first patch is FAILED QA, it should be replaced with something?
Comment 17 kenza 2013-07-25 07:28:00 UTC
(In reply to Srdjan Jankovic from comment #16)
> Ok, then it the first patch is FAILED QA, it should be replaced with
> something?

Yes, by the last patch. I put the first on as obsolete. Now it should work...
Comment 18 Srdjan Jankovic 2013-07-25 07:35:04 UTC
Tests passed, however I got
Use of uninitialized value $calculated in substitution (s///) at C4/Serials.pm line 988.
Use of uninitialized value $pattern in numeric eq (==) at C4/Serials.pm line 991.
Use of uninitialized value $calculated in substitution (s///) at C4/Serials.pm line 1000.
Use of uninitialized value $calculated in substitution (s///) at C4/Serials.pm line 1003.
Use of uninitialized value $serialseq in string at C4/Serials.pm line 1391.

Do I need any data from that infamous patch?
Comment 19 kenza 2013-07-25 09:49:40 UTC
(In reply to Srdjan Jankovic from comment #18)
> Tests passed, however I got
> Use of uninitialized value $calculated in substitution (s///) at
> C4/Serials.pm line 988.
> Use of uninitialized value $pattern in numeric eq (==) at C4/Serials.pm line
> 991.
> Use of uninitialized value $calculated in substitution (s///) at
> C4/Serials.pm line 1000.
> Use of uninitialized value $calculated in substitution (s///) at
> C4/Serials.pm line 1003.
> Use of uninitialized value $serialseq in string at C4/Serials.pm line 1391.
> 
> Do I need any data from that infamous patch?

No , these warnings are normal, the initialization of these values is useless for this patch.
Comment 20 Srdjan Jankovic 2013-07-25 22:50:47 UTC Comment hidden (obsolete)
Comment 21 Katrin Fischer 2013-08-25 11:27:55 UTC
All the tests pass nicely, but there are a lot of warnings when you run the test with prove:

Use of uninitialized value $calculated in substitution (s///) at /home/katrin/kohaclone/C4/Serials.pm line 985.
Use of uninitialized value $pattern in numeric eq (==) at /home/katrin/kohaclone/C4/Serials.pm line 988.

Maybe something for another bug? :)
Comment 22 Katrin Fischer 2013-08-25 11:39:43 UTC
Created attachment 20585 [details] [review]
[PASSED QA] C4::Bookseller.pm need unit tests

The tests are wrap in a transaction.

NOTE: some tests should pass but doesn't because of incoherences in the code.
These tests are in comments and preceded by FIXME

Test plan:
prove t/db_dependent/Bookseller.t
t/db_dependent/Bookseller.t .. 15/53
[Some warnings about uninitialised values]
t/db_dependent/Bookseller.t .. ok
All tests successful.
Files=1, Tests=53,  1 wallclock secs ( 0.02 usr  0.00 sys +  0.46 cusr  0.03 csys =  0.51 CPU)
Result: PASS

http://bugs.koha-community.org/show_bug.cgi?id=10528

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
The new tests pass nicely, as do all old tests and the QA script.
All patches marked as dependencies have been pushed to master
already.
Comment 23 Galen Charlton 2013-08-26 16:30:58 UTC
Pushed to master.  Thanks, Kenza!
Comment 24 Tomás Cohen Arazi 2013-09-13 17:28:55 UTC
This patch has been pushed to 3.12.x, will be in 3.12.5.

Thanks Kenza!
Comment 25 Bernardo Gonzalez Kriegel 2013-10-13 13:23:19 UTC
Pushed to 3.10.x, will be in 3.10.12