Testing C4::Creators::Lib at 100% Deleting get_column_names subroutine (never used) TEST PLAN --------- 1. Apply patch 2. prove -v t/db_dependent/Creators/Lib.t -- All 644 tests should run successfully without any error or warning TEST PLAN OPTIONAL ------------------ Check with bug 13899 to see the coverage of this module. Coverage BEFORE this patch : Statement : 11,6% Branch : 0,0% Condition : N/A Subroutine : 36,0% Coverage AFTER this patch : Statement : 100,0% Branch : 100,0% Condition : N/A Subroutine : 100,0%
Created attachment 38481 [details] [review] Bug 14059 : Testing C4::Creators::Lib Testing C4::Creators::Lib at 100% Deleting get_column_names subroutine (never used) TEST PLAN --------- 1. Apply patch 2. prove -v t/db_dependent/Creators/Lib.t -- All 644 tests should run successfully without any error or warning TEST PLAN OPTIONAL ------------------ Check with bug 13899 to see the coverage of this module. Coverage BEFORE this patch : Statement : 11,6% Branch : 0,0% Condition : N/A Subroutine : 36,0% Coverage AFTER this patch : Statement : 100,0% Branch : 100,0% Condition : N/A Subroutine : 100,0%
Comment on attachment 38481 [details] [review] Bug 14059 : Testing C4::Creators::Lib Review of attachment 38481 [details] [review]: ----------------------------------------------------------------- ::: C4/Creators/Lib.pm @@ -448,5 @@ > -Return an arrayref of an array containing the column names of the supplied table. > - > -=cut > - > -sub get_column_names { git grep get_column_names shows that C4::Creators exports this function, and this lib still exports it too. Yes, it isn't used, but you forgot to cut the other parts out too.
Failed QA'ing this for reason in comment #2.
I also had a hard time getting 13899 to run well. It runs great the first time (long time to run). But second run after apply doesn't seem to work. I did a rm -rf cover_db and a reboot to get it to work again after the patch. This does improve the coverage.
Created attachment 38801 [details] [review] Bug 14059 : Delete all export of get_column_names
Created attachment 38810 [details] [review] Bug 14059 : Testing C4::Creators::Lib Testing C4::Creators::Lib at 100% Deleting get_column_names subroutine (never used) TEST PLAN --------- 1. Apply patch 2. prove -v t/db_dependent/Creators/Lib.t -- All 644 tests should run successfully without any error or warning TEST PLAN OPTIONAL ------------------ Check with bug 13899 to see the coverage of this module. Coverage BEFORE this patch : Statement : 11,6% Branch : 0,0% Condition : N/A Subroutine : 36,0% Coverage AFTER this patch : Statement : 100,0% Branch : 100,0% Condition : N/A Subroutine : 100,0% Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 38811 [details] [review] Bug 14059 : Delete all export of get_column_names This deals with my concerns raised in comment #2. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Can you please make this a little more fail save? I think isses needs to be emptied out in the tests for them to run: $ prove t/db_dependent/Creators/Lib.t t/db_dependent/Creators/Lib.t .. 1/644 DBD::mysql::db do failed: Cannot delete or update a parent row: a foreign key constraint fails (`koha`.`issues`, CONSTRAINT `issues_ibfk_1` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON UPDATE CASCADE) at t/db_dependent/Creators/Lib.t line 61. DBD::mysql::db do failed: Cannot delete or update a parent row: a foreign key constraint fails (`koha`.`issues`, CONSTRAINT `issues_ibfk_1` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON UPDATE CASCADE) at t/db_dependent/Creators/Lib.t line 61. # Looks like you planned 644 tests but ran 6. # Looks like your test exited with 255 just after 6. t/db_dependent/Creators/Lib.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Created attachment 38840 [details] [review] Bug 14059 : Deleting 'issues' table before processing the tests It's suppose to avoid foreign key trouble
Patch added by comment #9 needs sign off.
Created attachment 38945 [details] [review] [SIGNED-OFF] Bug 14059: Deleting 'issues' table before processing the tests It's suppose to avoid foreign key trouble Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> No error
Created attachment 39426 [details] [review] [PASSED QA] Bug 14059 : Testing C4::Creators::Lib Testing C4::Creators::Lib at 100% Deleting get_column_names subroutine (never used) TEST PLAN --------- 1. Apply patch 2. prove -v t/db_dependent/Creators/Lib.t -- All 644 tests should run successfully without any error or warning TEST PLAN OPTIONAL ------------------ Check with bug 13899 to see the coverage of this module. Coverage BEFORE this patch : Statement : 11,6% Branch : 0,0% Condition : N/A Subroutine : 36,0% Coverage AFTER this patch : Statement : 100,0% Branch : 100,0% Condition : N/A Subroutine : 100,0% Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 39427 [details] [review] [PASSED QA] Bug 14059 : Delete all export of get_column_names This deals with my concerns raised in comment #2. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 39428 [details] [review] [PASSED QA] Bug 14059: Deleting 'issues' table before processing the tests It's suppose to avoid foreign key trouble Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> No error Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Patches pushed to master. Good job Julian!