Bug 10539 - Remove unused unit tests in lib/KohaTest
Summary: Remove unused unit tests in lib/KohaTest
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks: 10426
  Show dependency treegraph
 
Reported: 2013-07-04 06:47 UTC by Marcel de Rooy
Modified: 2014-12-07 20:02 UTC (History)
3 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 10539: Remove unused unit tests in lib/KohaTest (231.44 KB, patch)
2013-07-04 08:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10539: Followup for removing KohaTest unit tests (11.82 KB, patch)
2013-07-04 08:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10539: Remove unused unit tests in lib/KohaTest (231.44 KB, patch)
2013-07-11 12:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10539: Followup for removing KohaTest unit tests (11.99 KB, patch)
2013-07-11 12:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10539: Remove unused unit tests in lib/KohaTest (231.45 KB, patch)
2013-09-04 19:34 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 10539: Followup for removing KohaTest unit tests (12.06 KB, patch)
2013-09-04 19:34 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 10539: Remove unused unit tests in lib/KohaTest (231.51 KB, patch)
2013-09-05 07:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10539: Followup for removing KohaTest unit tests (12.12 KB, patch)
2013-09-05 07:36 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2013-07-04 06:47:24 UTC
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.
Comment 1 Marcel de Rooy 2013-07-04 08:21:22 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2013-07-04 08:21:27 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2013-07-04 08:32:51 UTC
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.
Comment 4 Marcel de Rooy 2013-07-11 12:31:43 UTC
(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.
Comment 5 Marcel de Rooy 2013-07-11 12:32:49 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2013-07-11 12:39:13 UTC Comment hidden (obsolete)
Comment 7 Srdjan Jankovic 2013-09-03 01:43:29 UTC
A philosophical question - changes like this, consisting of only removing unused/unwanted code, can those be self-signed, and go to QA directly?
Comment 8 Jared Camins-Esakov 2013-09-04 19:34:28 UTC Comment hidden (obsolete)
Comment 9 Jared Camins-Esakov 2013-09-04 19:34:38 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2013-09-05 07:34:36 UTC
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.
Comment 11 Jonathan Druart 2013-09-05 07:36:11 UTC
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>
Comment 12 Jonathan Druart 2013-09-05 07:36:17 UTC
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>
Comment 13 Galen Charlton 2013-09-05 15:00:36 UTC
(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.
Comment 14 Galen Charlton 2013-09-05 15:21:32 UTC
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.