Created attachment 19338 [details] [review] Bug 10508 : C4::Branch.pm needs unit tests. Unit tests are wrap in a transaction. NOTE: the field "issuing" isn't used in the database To test: prove t/db_dependent/Branches.t t/db_dependent/Branch.t .. ok All tests successful. Files=1, Tests=31, 0 wallclock secs ( 0.03 usr 0.00 sys + 0.13 cusr 0.00 csys = 0.16 CPU) Result: PASS
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.
(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.
Created attachment 19546 [details] [review] Bug 10508: branchprinter will be a foreign key Bug 8034 will introduces a foreign key on branches.branchprinter. This patch set branchprinter values to undef.
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?
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.
(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?
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?
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
Created attachment 19687 [details] [review] Bug 10508 : C4::Branch.pm needs unit tests. Unit tests are wrap in a transaction. NOTE: the field "issuing" isn't used in the database To test: prove t/db_dependent/Branch.t t/db_dependent/Branch.t .. ok All tests successful. Files=1, Tests=31, 0 wallclock secs ( 0.03 usr 0.00 sys + 0.13 cusr 0.00 csys = 0.16 CPU) Result: PASS
Created attachment 19688 [details] [review] Bug 10508: branchprinter will be a foreign key Bug 8034 will introduces a foreign key on branches.branchprinter. This patch set branchprinter values to undef.
(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.
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.
(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).
Well 10515 is in master now. I'll put some debugging and get more details.
Created attachment 19771 [details] [review] Bug 10508 : C4::Branch.pm needs unit tests. Unit tests are wrap in a transaction. NOTE: the field "issuing" isn't used in the database To test: prove t/db_dependent/Branch.t t/db_dependent/Branch.t .. ok All tests successful. Files=1, Tests=31, 0 wallclock secs ( 0.03 usr 0.00 sys + 0.13 cusr 0.00 csys = 0.16 CPU) Result: PASS
The patch now considers the tests which already exist with the patch 10515.
Created attachment 19780 [details] [review] Bug 10508 : C4::Branch.pm needs unit tests. Unit tests are wrap in a transaction. NOTE: the field "issuing" isn't used in the database To test: prove t/db_dependent/Branch.t t/db_dependent/Branch.t .. ok All tests successful. Files=1, Tests=31, 0 wallclock secs ( 0.03 usr 0.00 sys + 0.13 cusr 0.00 csys = 0.16 CPU) Result: PASS Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Created attachment 19781 [details] [review] [SIGNED-OFF] Bug 10508 : C4::Branch.pm needs unit tests. Unit tests are wrap in a transaction. NOTE: the field "issuing" isn't used in the database To test: prove t/db_dependent/Branch.t t/db_dependent/Branch.t .. ok All tests successful. Files=1, Tests=31, 0 wallclock secs ( 0.03 usr 0.00 sys + 0.13 cusr 0.00 csys = 0.16 CPU) Result: PASS Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
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
Created attachment 19853 [details] [review] [PASSED QA] Bug 10508 : C4::Branch.pm needs unit tests. Unit tests are wrap in a transaction. NOTE: the field "issuing" isn't used in the database To test: prove t/db_dependent/Branch.t t/db_dependent/Branch.t .. ok All tests successful. Files=1, Tests=31, 0 wallclock secs ( 0.03 usr 0.00 sys + 0.13 cusr 0.00 csys = 0.16 CPU) Result: PASS Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Passes all tests :)
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.
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.
(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
(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.
> (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.
(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.
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?
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.
Created attachment 19864 [details] [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
(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.
Created attachment 19900 [details] [review] [SIGNED-OFF] 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>
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*
Created attachment 20029 [details] [review] [SIGNED-OFF] 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>
Yes it is ok, my mistake, sorry about that.
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
Ah, I should mention, that I applied 10629 before this patch, following the dependency note.
Created attachment 20626 [details] [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 http://bugs.koha-community.org/show_bug.cgi?id=10508 Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Created attachment 20638 [details] [review] Bug 10508: 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>
Created attachment 20639 [details] [review] Bug 10508 : Follow up Fixing a typo
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.
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!
Pushed to master. Thanks, Kenza!
This patch has been pushed to 3.12.x, will be in 3.12.5. Thanks Kenza!