Bug 10508 - UT: C4::Branch.pm needs unit tests
Summary: UT: C4::Branch.pm needs unit tests
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: kenza
QA Contact:
URL:
Keywords:
Depends on: 10515 10629
Blocks:
  Show dependency treegraph
 
Reported: 2013-06-26 08:16 UTC by kenza
Modified: 2014-12-07 20:02 UTC (History)
6 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 10508 : C4::Branch.pm needs unit tests. (11.77 KB, patch)
2013-07-02 12:26 UTC, kenza
Details | Diff | Splinter Review
Bug 10508: branchprinter will be a foreign key (1.93 KB, patch)
2013-07-11 07:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10508 : C4::Branch.pm needs unit tests. (11.77 KB, patch)
2013-07-16 07:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10508: branchprinter will be a foreign key (1.93 KB, patch)
2013-07-16 07:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10508 : C4::Branch.pm needs unit tests. (13.32 KB, patch)
2013-07-18 14:43 UTC, kenza
Details | Diff | Splinter Review
Bug 10508 : C4::Branch.pm needs unit tests. (13.33 KB, patch)
2013-07-19 01:56 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10508 : C4::Branch.pm needs unit tests. (13.34 KB, patch)
2013-07-19 01:58 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[PASSED QA] Bug 10508 : C4::Branch.pm needs unit tests. (13.42 KB, patch)
2013-07-22 17:11 UTC, Katrin Fischer
Details | Diff | Splinter Review
C4::Branch.pm needs unit tests (10.88 KB, patch)
2013-07-23 13:02 UTC, kenza
Details | Diff | Splinter Review
[SIGNED-OFF] C4::Branch.pm needs unit tests (1.55 KB, patch)
2013-07-24 03:12 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[SIGNED-OFF] C4::Branch.pm needs unit tests (10.94 KB, patch)
2013-07-31 23:41 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
C4::Branch.pm needs unit tests (10.92 KB, patch)
2013-08-26 08:12 UTC, kenza
Details | Diff | Splinter Review
Bug 10508: C4::Branch.pm needs unit tests (10.92 KB, patch)
2013-08-26 22:30 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10508 : Follow up Fixing a typo (826 bytes, patch)
2013-08-26 22:30 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED QA] C4::Branch.pm needs unit tests (11.02 KB, patch)
2013-08-27 20:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10508 : Follow up Fixing a typo (927 bytes, patch)
2013-08-27 20:10 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-06-26 08:16:58 UTC

    
Comment 1 kenza 2013-07-02 12:26:57 UTC Comment hidden (obsolete)
Comment 2 Srdjan Jankovic 2013-07-11 02:10:24 UTC
t/db_dependent/Branch.t .. 1/31 
#   Failed test 'CAT1 details are right'
#   at t/db_dependent/Branch.t line 170.
#     Structures begin differing at:
#          $got = ARRAY(0x28b3028)
#     $expected = HASH(0x28b2c80)

#   Failed test 'CAT2 doesnt exist'
#   at t/db_dependent/Branch.t line 179.
#          got: 'ARRAY(0x28b3088)'
#     expected: undef
# Looks like you failed 2 tests of 31.
Comment 3 Jonathan Druart 2013-07-11 07:45:09 UTC
(In reply to Srdjan Jankovic from comment #2)
> t/db_dependent/Branch.t .. 1/31 
...
> # Looks like you failed 2 tests of 31.

I Srdjan,

It seems you don't execute this UT file with a fresh DB. I confirm it works against master with a fresh DB.
I think you executed it on your development DB where you applied your patch for bug 8034, which introduces the foreign key on branches.branchprinter.

I will provide a followup to avoid a potential jenkins failure later.
Comment 4 Jonathan Druart 2013-07-11 07:45:28 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2013-07-11 07:47:21 UTC
In fact I don't know if it is your problem, I got the following sql error after applying patch for bug 8034:

DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_coderun`.`branches`, CONSTRAINT `branches_ibfk_1` FOREIGN KEY (`branchprinter`) REFERENCES `printers` (`printqueue`) ON UPDATE CASCADE)

Maybe you forgot to apply the patch of bug 10515?
Comment 6 Srdjan Jankovic 2013-07-11 08:08:03 UTC
I had that problem, but reverted the patch.
I did not run off a fresh DB. In general, are tests supposed to run against a fresh DB only? I would expect not, should run against any data, the only requirement should be database version.
Comment 7 Jonathan Druart 2013-07-11 08:54:31 UTC
(In reply to Srdjan Jankovic from comment #6)
> I had that problem, but reverted the patch.
> I did not run off a fresh DB. In general, are tests supposed to run against
> a fresh DB only? I would expect not, should run against any data, the only
> requirement should be database version.

Do you get the same error after applying patch for bug 10515 and these 2 patches?
Comment 8 Srdjan Jankovic 2013-07-11 22:45:20 UTC
Not sure any more, I was signing off that patch and can't remember what I did. Will try again later.
In general, shall I wait for dependencies to be pushed to master first?
Comment 9 Srdjan Jankovic 2013-07-16 00:21:59 UTC
prove t/db_dependent/Branches.t
Cannot detect source of 't/db_dependent/Branches.t'! at /usr/share/perl/5.14/TAP/Parser/IteratorFactory.pm line 263
        TAP::Parser::IteratorFactory::detect_source('TAP::Parser::IteratorFactory=HASH(0x26c2678)', 'TAP::Parser::Source=HASH(0x25e19c8)') called at /usr/share/perl/5.14/TAP/Parser/IteratorFactory.pm line 213
        TAP::Parser::IteratorFactory::make_iterator('TAP::Parser::IteratorFactory=HASH(0x26c2678)', 'TAP::Parser::Source=HASH(0x25e19c8)') called at /usr/share/perl/5.14/TAP/Parser.pm line 469
        TAP::Parser::_initialize('TAP::Parser=HASH(0x25e17d0)', 'HASH(0x2259ea0)') called at /usr/share/perl/5.14/TAP/Object.pm line 58
        TAP::Object::new('TAP::Parser', 'HASH(0x2259ea0)') called at /usr/share/perl/5.14/TAP/Object.pm line 133
        TAP::Object::_construct('TAP::Harness=HASH(0x245dcc0)', 'TAP::Parser', 'HASH(0x2259ea0)') called at /usr/share/perl/5.14/TAP/Harness.pm line 779
        TAP::Harness::make_parser('TAP::Harness=HASH(0x245dcc0)', 'TAP::Parser::Scheduler::Job=HASH(0x25bbc68)') called at /usr/share/perl/5.14/TAP/Harness.pm line 578
        TAP::Harness::_aggregate_single('TAP::Harness=HASH(0x245dcc0)', 'TAP::Parser::Aggregator=HASH(0x246c7b8)', 'TAP::Parser::Scheduler=HASH(0x25bbd70)') called at /usr/share/perl/5.14/TAP/Harness.pm line 670
        TAP::Harness::aggregate_tests('TAP::Harness=HASH(0x245dcc0)', 'TAP::Parser::Aggregator=HASH(0x246c7b8)', 't/db_dependent/Branches.t') called at /usr/share/perl/5.14/TAP/Harness.pm line 485
        TAP::Harness::__ANON__() called at /usr/share/perl/5.14/TAP/Harness.pm line 498
        TAP::Harness::runtests('TAP::Harness=HASH(0x245dcc0)', 't/db_dependent/Branches.t') called at /usr/share/perl/5.14/App/Prove.pm line 553
        App::Prove::_runtests('App::Prove=HASH(0x202edd8)', 'HASH(0x2430ee8)', 'TAP::Harness', 't/db_dependent/Branches.t') called at /usr/share/perl/5.14/App/Prove.pm line 511
        App::Prove::run('App::Prove=HASH(0x202edd8)') called at /usr/bin/prove line 11
Comment 10 Jonathan Druart 2013-07-16 07:41:38 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2013-07-16 07:41:43 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2013-07-16 07:42:54 UTC
(In reply to Srdjan Jankovic from comment #9)
> prove t/db_dependent/Branches.t
> Cannot detect source of 't/db_dependent/Branches.t'! at

Yes, it is Branch.t, not Branches.t.
I modified the commit message.
Comment 13 Srdjan Jankovic 2013-07-16 23:31:23 UTC
Same result (same 2 failed tests). Do you think I should try on an empty database, or you'd rather make it work on any database. I can see merits in the latter, but if there's consensus that tests should run against some initial data, I'm fine with that.
Comment 14 Jonathan Druart 2013-07-17 07:56:40 UTC
(In reply to Srdjan Jankovic from comment #2)
> t/db_dependent/Branch.t .. 1/31 
> #   Failed test 'CAT1 details are right'
> #   at t/db_dependent/Branch.t line 170.
> #     Structures begin differing at:
> #          $got = ARRAY(0x28b3028)
> #     $expected = HASH(0x28b2c80)

I don't understand how it is possible, GetBranchCategory returns an hashref (if you applied patch for bug 10515).
Comment 15 Srdjan Jankovic 2013-07-17 23:07:20 UTC
Well 10515 is in master now. I'll put some debugging and get more details.
Comment 16 kenza 2013-07-18 14:43:11 UTC Comment hidden (obsolete)
Comment 17 kenza 2013-07-18 14:45:15 UTC
The patch now considers the tests which already exist with the patch 10515.
Comment 18 Srdjan Jankovic 2013-07-19 01:56:32 UTC Comment hidden (obsolete)
Comment 19 Srdjan Jankovic 2013-07-19 01:58:00 UTC Comment hidden (obsolete)
Comment 20 Katrin Fischer 2013-07-22 17:02:08 UTC
12 has no description, but apart from that tests look good :)

ok 11 - A branch has been modified, no new branch added
ok 12
ok 13 - Two categories added
Comment 21 Katrin Fischer 2013-07-22 17:11:55 UTC Comment hidden (obsolete)
Comment 22 Galen Charlton 2013-07-22 18:56:03 UTC
Comment on attachment 19853 [details] [review]
[PASSED QA] Bug 10508 : C4::Branch.pm needs unit tests.

Review of attachment 19853 [details] [review]:
-----------------------------------------------------------------

::: t/db_dependent/Branch.t
@@ +21,4 @@
>  use C4::Context;
>  use Data::Dumper;
>  
> +use Test::More ;#tests => 6;

There is nothing in this test script that makes the number of tests unpredictable.  Consequently, I strongly prefer that the number of tests be declared explicitly, and that done_testing() be used only in cases where it is legitimately uncertain how many tests will be run.
Comment 23 Galen Charlton 2013-07-22 18:56:56 UTC
Similar to the concerns I expressed in bug 10528, I have reservations about a patch that both changes a bunch of core routines and adds tests.  Please split it up.
Comment 24 Srdjan Jankovic 2013-07-22 23:10:23 UTC
(In reply to Galen Charlton from comment #23)
> Similar to the concerns I expressed in bug 10528, I have reservations about
> a patch that both changes a bunch of core routines and adds tests.  Please
> split it up.

Why? I think it is a good thing to add tests that support the change. Makes it kind of complete
Comment 25 Galen Charlton 2013-07-23 00:03:23 UTC
(In reply to Srdjan Jankovic from comment #24)
> (In reply to Galen Charlton from comment #23)
> > Similar to the concerns I expressed in bug 10528, I have reservations about
> > a patch that both changes a bunch of core routines and adds tests.  Please
> > split it up.
> 
> Why? I think it is a good thing to add tests that support the change. Makes
> it kind of complete

My statement, taken out of the context of this bug, was indeed ill-worded.  Of course I want tests to accompany patches that change core routines (although I would prefer that the tests, including regression tests, be in a separate patch from the one that changes the routines).

But consider it in the context of this bug, which advertises itself as "UT: C4::Branch.pm needs unit tests", not "there are inappropriate uses of $sth->finish() in C4::Branches".  In other words, this is part of the ongoing process to improve test coverage, and it's mixing up *that* with changes to core routines that I object to.

Sure, in the process of writing tests for a module, it's quite likely that one will run into cases where a routine ought to be improved -- but those should be handled separately, either via separate bugs or at least separate patches.
Comment 26 Srdjan Jankovic 2013-07-23 00:48:46 UTC
> (although I would prefer that the tests, including regression tests, be in a
> separate patch from the one that changes the routines).

That is exactly what I'm trying to figure out - why do you find it easier that way. I would always want them together. Not arguing at all, as a QA you absolutely have right to ask for it to be delivered in a way that it suits you (it will get merged all together in the end anyway), just finding it interesting. 

> 
> But consider it in the context of this bug, which advertises itself as "UT:
> C4::Branch.pm needs unit tests", not "there are inappropriate uses of
> $sth->finish() in C4::Branches".  In other words, this is part of the
> ongoing process to improve test coverage, and it's mixing up *that* with
> changes to core routines that I object to.

I agree.
Comment 27 Galen Charlton 2013-07-23 00:55:41 UTC
(In reply to Srdjan Jankovic from comment #26)
> That is exactly what I'm trying to figure out - why do you find it easier
> that way. I would always want them together. Not arguing at all, as a QA you
> absolutely have right to ask for it to be delivered in a way that it suits
> you (it will get merged all together in the end anyway), just finding it
> interesting. 

In part, it's a consequence of following the TDD notion of writing the tests first.  Depending on the nature of the bug, having the first patch contain the test cases allows one to apply just that patch, then run the tests, then see from the (presumptive) test failures the essence of the bug being fixed and/or the contract of the new routines being added or the changes to the contract of the routines being changed.  Now that the stage of the little story that the patch series is telling is set and effective *documented* by the first patch, the subsequent patches round it out.
Comment 28 Srdjan Jankovic 2013-07-23 01:20:00 UTC
Ok, I get it, you want to see the test(s) failing first, and then coming up fine. Maybe we should put it on the wiki then?
Comment 29 kenza 2013-07-23 07:45:47 UTC
Ok, I completely understand your point of view. So , I will follow your advices and create new bugs to separate the different parts of this patch.
Comment 30 kenza 2013-07-23 13:02:30 UTC Comment hidden (obsolete)
Comment 31 kenza 2013-07-23 13:04:12 UTC
(In reply to kenza from comment #30)
> Created attachment 19864 [details] [review] [review]
> C4::Branch.pm needs unit tests
> 
> Unit tests are wrap in a transaction.
> 
> To test:
> prove t/db_dependent/Branch.t
> t/db_dependent/Branch.t .. 1/36 Using a hash as a reference is deprecated at
> t/db_dependent/Branch.t line 207.
> t/db_dependent/Branch.t .. ok
> All tests successful.
> Files=1, Tests=36,  1 wallclock secs ( 0.02 usr  0.02 sys +  0.13 cusr  0.01
> csys =  0.18 CPU)
> Result: PASS

Now, the patch only does the tests.
Comment 32 Srdjan Jankovic 2013-07-24 03:12:34 UTC Comment hidden (obsolete)
Comment 33 Katrin Fischer 2013-07-31 20:59:02 UTC
Srdjan, could you confirm the patch works? There were 2 attachements, the second one contained changes that it seems were already part of the first patch, but only the second had your sign-off... *confused*
Comment 34 Srdjan Jankovic 2013-07-31 23:41:29 UTC Comment hidden (obsolete)
Comment 35 Srdjan Jankovic 2013-07-31 23:42:25 UTC
Yes it is ok, my mistake, sorry about that.
Comment 36 Katrin Fischer 2013-08-25 11:19:03 UTC
Hi Kenza,

I see a problem running the QA script - could you take a look at that?

QA SCRIPT
testing 1 commit(s) (applied to b341d4e 'Bug 10629 : Inappropriate uses of $st')

 FAIL	t/db_dependent/Branch.t
   OK	  pod
   OK	  forbidden patterns
   FAIL	  valid
		Using a hash as a reference is deprecated 
   OK	  critic
Comment 37 Katrin Fischer 2013-08-25 11:19:48 UTC
Ah, I should mention, that I applied 10629 before this patch, following the dependency note.
Comment 38 kenza 2013-08-26 08:12:17 UTC Comment hidden (obsolete)
Comment 39 Chris Cormack 2013-08-26 22:30:44 UTC Comment hidden (obsolete)
Comment 40 Chris Cormack 2013-08-26 22:30:57 UTC Comment hidden (obsolete)
Comment 41 Katrin Fischer 2013-08-27 20:10:35 UTC
Created attachment 20663 [details] [review]
[PASSED QA] C4::Branch.pm needs unit tests

Unit tests are wrap in a transaction.

To test:
prove t/db_dependent/Branch.t
t/db_dependent/Branch.t .. 1/36 Using a hash as a reference is deprecated at t/db_dependent/Branch.t line 207.
t/db_dependent/Branch.t .. ok
All tests successful.
Files=1, Tests=36,  1 wallclock secs ( 0.02 usr  0.02 sys +  0.13 cusr  0.01 csys =  0.18 CPU)
Result: PASS

http://bugs.koha-community.org/show_bug.cgi?id=10508
Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script.
Comment 42 Katrin Fischer 2013-08-27 20:10:54 UTC
Created attachment 20664 [details] [review]
[PASSED QA] Bug 10508 : Follow up Fixing a typo

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Thx for the follow-up Chris!
Comment 43 Galen Charlton 2013-08-28 14:27:15 UTC
Pushed to master.  Thanks, Kenza!
Comment 44 Tomás Cohen Arazi 2013-09-13 17:24:44 UTC
This patch has been pushed to 3.12.x, will be in 3.12.5.

Thanks Kenza!