http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 kenza <kenza.zaki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@biblibre.co | |m Depends on| |10515 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 --- Comment #1 from kenza <kenza.zaki@biblibre.com> --- Created attachment 19338 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19338&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 --- Comment #3 from Jonathan Druart <jonathan.druart@biblibre.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 --- Comment #5 from Jonathan Druart <jonathan.druart@biblibre.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 --- Comment #7 from Jonathan Druart <jonathan.druart@biblibre.com> --- (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? -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 --- Comment #9 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- 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 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19546|0 |1 is obsolete| | --- Comment #11 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 19688 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19688&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 --- Comment #13 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 --- Comment #15 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Well 10515 is in master now. I'll put some debugging and get more details. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 --- Comment #17 from kenza <kenza.zaki@biblibre.com> --- The patch now considers the tests which already exist with the patch 10515. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19688|0 |1 is obsolete| | Attachment #19771|0 |1 is obsolete| | --- Comment #18 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 19780 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19780&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #20 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19781|0 |1 is obsolete| | --- Comment #21 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 19853 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19853&action=edit [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 :) -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA CC| |gmcharlt@gmail.com --- Comment #23 from Galen Charlton <gmcharlt@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 --- Comment #25 from Galen Charlton <gmcharlt@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 --- Comment #27 from Galen Charlton <gmcharlt@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 --- Comment #29 from kenza <kenza.zaki@biblibre.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 --- Comment #30 from kenza <kenza.zaki@biblibre.com> --- Created attachment 19864 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19864&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 kenza <kenza.zaki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |10629 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 kenza <kenza.zaki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |10629 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19864|0 |1 is obsolete| | --- Comment #32 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 19900 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19900&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19864|1 |0 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 --- Comment #33 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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* -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 --- Comment #35 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Yes it is ok, my mistake, sorry about that. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 --- Comment #37 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Ah, I should mention, that I applied 10629 before this patch, following the dependency note. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 kenza <kenza.zaki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20029|0 |1 is obsolete| | Attachment #20626|0 |1 is obsolete| | --- Comment #39 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 20638 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20638&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 --- Comment #42 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 20664 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20664&action=edit [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! -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10508 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable CC| |tomascohen@gmail.com --- Comment #44 from Tomás Cohen Arazi <tomascohen@gmail.com> --- This patch has been pushed to 3.12.x, will be in 3.12.5. Thanks Kenza! -- You are receiving this mail because: You are watching all bug changes.