As discussed on the dev list, we can remove the unit tests in lib/KohaTest. Galen suggested: [I have no objection to a patch that removes them, but it would be a niceness to look over the individual tests and see which ones can be easily ported to the current DB-dependent test framework.] Since these unit tests are not used now, they only generate unnecessary maintenance; they are regularly updated for nothing. I propose to remove them rightaway. But with Galen's suggestion in mind, I will walk through them (placed outside the Koha codebase) in the next couple of months to see if we can still retrieve some additional tests and inject them in the active db_dependent unit tests. Anyone else who wants to join that activity is welcome; you can easily retrieve these files from git. Please let me know if you do. I will open a separate report for that purpose.
Created attachment 19389 [details] [review] Bug 10539: Remove unused unit tests in lib/KohaTest This patch removes all files in lib/KohaTest and the associated script t/db_dependent/database_dependent.pl. The second patch deals with a few consequences. Note that bug 10540 has been opened to save some interesting code from these unit tests and inject them in the currently used t/db_dependent suite. Test plan: See second patch.
Created attachment 19390 [details] [review] Bug 10539: Followup for removing KohaTest unit tests Adds a line to skip testing Koha::Plugins::Base in t/00-load.t. This needs some attention. Makefile.PL: Adds always-false condition to skip section of creating file t/test-config.txt. This file is no longer used. (Next step should be to remove that code.) Removes t/Makefile and t/rewrite-config-test for the same reason. Modifies XISBN.t to remove two commented lines. Test plan: Grep for KohaTest, database_dependent, config-test, t/Makefile. Run perl Makefile.PL and include running the test suite. This may fail on t/00-valid-xml.t and t/QueryParser.t, but that is not the result of this patch. I had this result: Test Summary Report: t/00-valid-xml.t (Wstat: 1792 Tests: 381 Failed: 7) Failed tests: 10, 30-31, 169, 181, 201-202 Non-zero exit status: 7 t/QueryParser.t (Wstat: 512 Tests: 0 Failed: 0) Non-zero exit status: 2 Parse errors: No plan found in TAP output Files=92, Tests=12385, 45 wallclock secs ( 1.74 usr 0.15 sys + 26.61 cusr 1.92 csys = 30.42 CPU) Result: FAIL Failed 2/92 test programs. 7/12385 subtests failed.
Note: I can use some feedback on at least following two points: Can we just remove the section that we now skip with if (0 ) in Makefile.PL? Is skipping Koha::Plugins::Base necessary in 00-load.t? Could be just my problem; it seems that Jenkins does not fail here.
(In reply to M. de Rooy from comment #3) > Note: I can use some feedback on at least following two points: > > Can we just remove the section that we now skip with if (0 ) in Makefile.PL? > > Is skipping Koha::Plugins::Base necessary in 00-load.t? Could be just my > problem; it seems that Jenkins does not fail here. Talking to myself :) I remove the if (0) section now. And will not skip the Koha::Plugins::Base. If you have trouble with it too, just do the same as I did in the obsoleted patch. It seems to be my own problem, perhaps with some perl dependency.. Adjusting/rebasing now.
Created attachment 19564 [details] [review] Bug 10539: Remove unused unit tests in lib/KohaTest This patch removes all files in lib/KohaTest and the associated script t/db_dependent/database_dependent.pl. The second patch deals with a few consequences. Note that bug 10540 has been opened to save some interesting code from these unit tests and inject them in the currently used t/db_dependent suite. Test plan: See second patch.
Created attachment 19565 [details] [review] Bug 10539: Followup for removing KohaTest unit tests Makefile.PL: Section for creating file t/test-config.txt removed. Removes t/Makefile and t/rewrite-config-test too. Modifies XISBN.t to remove two commented lines. Test plan: Grep for KohaTest, database_dependent, config-test, t/Makefile. Run perl Makefile.PL and include running the test suite. This may fail on t/00-valid-xml.t and t/QueryParser.t, but that is not the result of this patch. Same for t/00-load.t with potential error on Koha::Plugins::Base. I had this result: Test Summary Report: t/00-valid-xml.t (Wstat: 1792 Tests: 381 Failed: 7) Failed tests: 10, 30-31, 169, 181, 201-202 Non-zero exit status: 7 t/QueryParser.t (Wstat: 512 Tests: 0 Failed: 0) Non-zero exit status: 2 Parse errors: No plan found in TAP output Files=92, Tests=12385, 45 wallclock secs ( 1.74 usr 0.15 sys + 26.61 cusr 1.92 csys = 30.42 CPU) Result: FAIL Failed 2/92 test programs. 7/12385 subtests failed.
A philosophical question - changes like this, consisting of only removing unused/unwanted code, can those be self-signed, and go to QA directly?
Created attachment 20786 [details] [review] Bug 10539: Remove unused unit tests in lib/KohaTest This patch removes all files in lib/KohaTest and the associated script t/db_dependent/database_dependent.pl. The second patch deals with a few consequences. Note that bug 10540 has been opened to save some interesting code from these unit tests and inject them in the currently used t/db_dependent suite. Test plan: See second patch. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 20787 [details] [review] Bug 10539: Followup for removing KohaTest unit tests Makefile.PL: Section for creating file t/test-config.txt removed. Removes t/Makefile and t/rewrite-config-test too. Modifies XISBN.t to remove two commented lines. Test plan: Grep for KohaTest, database_dependent, config-test, t/Makefile. Run perl Makefile.PL and include running the test suite. This may fail on t/00-valid-xml.t and t/QueryParser.t, but that is not the result of this patch. Same for t/00-load.t with potential error on Koha::Plugins::Base. I had this result: Test Summary Report: t/00-valid-xml.t (Wstat: 1792 Tests: 381 Failed: 7) Failed tests: 10, 30-31, 169, 181, 201-202 Non-zero exit status: 7 t/QueryParser.t (Wstat: 512 Tests: 0 Failed: 0) Non-zero exit status: 2 Parse errors: No plan found in TAP output Files=92, Tests=12385, 45 wallclock secs ( 1.74 usr 0.15 sys + 26.61 cusr 1.92 csys = 30.42 CPU) Result: FAIL Failed 2/92 test programs. 7/12385 subtests failed. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> All tests pass, and I find no mention of the removed test code.
QA comment: This patch is consistent with the current work in progress on unit tests. The KohaTest is obsolete, unused, buggy, etc. The interesting code can be retrieve from the git log. Marked as Passed QA.
Created attachment 20795 [details] [review] Bug 10539: Remove unused unit tests in lib/KohaTest This patch removes all files in lib/KohaTest and the associated script t/db_dependent/database_dependent.pl. The second patch deals with a few consequences. Note that bug 10540 has been opened to save some interesting code from these unit tests and inject them in the currently used t/db_dependent suite. Test plan: See second patch. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 20796 [details] [review] Bug 10539: Followup for removing KohaTest unit tests Makefile.PL: Section for creating file t/test-config.txt removed. Removes t/Makefile and t/rewrite-config-test too. Modifies XISBN.t to remove two commented lines. Test plan: Grep for KohaTest, database_dependent, config-test, t/Makefile. Run perl Makefile.PL and include running the test suite. This may fail on t/00-valid-xml.t and t/QueryParser.t, but that is not the result of this patch. Same for t/00-load.t with potential error on Koha::Plugins::Base. I had this result: Test Summary Report: t/00-valid-xml.t (Wstat: 1792 Tests: 381 Failed: 7) Failed tests: 10, 30-31, 169, 181, 201-202 Non-zero exit status: 7 t/QueryParser.t (Wstat: 512 Tests: 0 Failed: 0) Non-zero exit status: 2 Parse errors: No plan found in TAP output Files=92, Tests=12385, 45 wallclock secs ( 1.74 usr 0.15 sys + 26.61 cusr 1.92 csys = 30.42 CPU) Result: FAIL Failed 2/92 test programs. 7/12385 subtests failed. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> All tests pass, and I find no mention of the removed test code. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
(In reply to Srdjan Jankovic from comment #7) > A philosophical question - changes like this, consisting of only removing > unused/unwanted code, can those be self-signed, and go to QA directly? A philosophical answer -- no, for two reasons. If a patch removes an unused something, having at least two pairs of eyes can help verify that (a) the something was in fact unused and (b) the something was /completely/ removed. Second, it gives folks time to jump in case they question the removal.
Pushed to master. Thanks, Marcel! I note that the RUN_DATABASE_TESTS bits still left in Makefile.PL currently stand unused, but since they provide a hook for work that Jonathan is doing, I think they should stay in.