Runners/Loader API
Status: Beta
Brought to you by:
mca1001
Menu
▾
▴
Brian, while rereading our code I noticed these points:
1) Our existing Runners are named TestRunner, TkTestRunner, HarnessUnit. I think their names should be more consistent, like: TextTestRunner, TkTestRunner, HarnessTestRunner. They should also all support the same external API (start() and such). As there is sure some duplicated code between HarnessUnit and TestRunner, we should factor out a new base class here.
2) Why is UnitHarness not a TestSuite, but a Test? The TestLoader will return a TestSuite object for the first two cases of the load() method (class name or ".pm" file name), but for the third case (Test::Harness type test file), it will return a new UnitHarness object, which is-a TestListener and a Test, in that order (the fourth case (Test::Harness type directory) is not yet implemented). I mean, we don't have typing in Perl, to be sure, but I found that one hard to understand.
"1) Our existing Runners are named TestRunner, TkTestRunner, HarnessUnit. I think their names should be more consistent, like: TextTestRunner, TkTestRunner, HarnessTestRunner."
I agree to some extent. But the name change I'd prefer would be to drop the ubiquitous and redundant 'Test'. e.g. Test::Unit::Case, Test::Unit::Suite.
In fact, I think some of these should be at a higher level still; Test::Unit::Case is meant to be subclassed by end users: why not contract the name down to Test::Case? Test::Runners are also meant to be used with 'old style' tests, and used by end users, so why not: Test::Runner, Test::TkRunner, Test::HarnessRunner ?
On the other hand Test::Unit::Result (for example) is never seen by an end user so the name length doesnt matter.
"They should also all support the same external API (start() and such). As there is sure some duplicated code between HarnessUnit and TestRunner, we should factor out a new base class here."
I agree on the API front. The code layout/base classs is a slightly different issue though. Each of these Runners should not really store much internal state, but make use of the counters in TestResult. This was the GUI mistake I corrected; it hasnt been corrected (yet) in HarnessUnit. Beyond this, very little code is shared between the Runners. What is shared is the timing code, which I still think is an abberation, and should be in TestResult. Its a refactoring, though, not the construction of a new base class.
"Why is UnitHarness not a TestSuite, but a Test?"
Its the operations it can support, and is what the Runners actualy use. Lets look at what what suite supports:
TestSuite.pm:9:# helper subroutines
TestSuite.pm:13:sub is_not_name_of_a_class {
TestSuite.pm:23:sub is_a_test_case_class {
TestSuite.pm:35:sub flatten_inheritance_tree {
All of the above are private.
TestSuite.pm:51:sub new {
This constructor needs to be overridden in UnitHarness anyway
TestSuite.pm:96:sub empty_new {
This is only required because of the untyped 'Class' in perl, as discussed previously.
TestSuite.pm:112:sub name {
This one I should have. I dont understand why this isnt in the Test interface, all of its subclasses implement it!!!
TestSuite.pm:117:sub names {
TestSuite.pm:161:sub test_at {
TestSuite.pm:172:sub tests {
I can't extract the individual tests in UnitHarness.
TestSuite.pm:122:sub add_test {
TestSuite.pm:128:sub add_test_method {
I can't add to the tests in UnitHarness.
TestSuite.pm:143:sub count_test_cases {
TestSuite.pm:152:sub run {
These I do support, but are part of the Test interface.
TestSuite.pm:167:sub test_count {
UnitHarness is 'flat' - it cannot contain sub-Suites - so this method always returns the same as count_test_cases(). From a user point of view, only the latter matters - you always want to know how many tests you have left to run in total, not (usually) how many suites are left to run in a suite of suites. The only client of this interface (in junit) is actually java.swingui.TestTreeModel, where the tree of tests is explicitly displayed - which we dont do. Right now, WeAintGonnaNeedIt, and there are ways of getting at that interface without exposing it directly to the runners (using the Visitor pattern), if we ever come to that.
TestSuite.pm:177:sub to_string {
Should really get promoted into 'Test' (its implicit in the java version). The code should really also be changed to say:
sub to_string {
my $self = shift;
return $self->name();
}
... because otherwise it makes assumptions on how subclasses will implement name().
TestSuite.pm:182:sub warning {
Incidentally this has a bug, there is a missing '}' in the eval'd subroutine. Its only ever used to add a fake test case to a suite, but of course add_test makes no sense to a UnitHarness.
To sum up: there are a couple of methods which I should add (name() and to_string()) but really should go into Test; the rest of the Suite methods do not mean anything to UnitHarness; subclassing Suite would mean me having to override them purely to throw errors. They are never used by the Runners. The Loader should return the bare TestCase when it gets one.
Which brings up an interesting point. Loader should really be returning Test objects, not Suite objects. That's all the interface that is actually used. If this is to be done consistently, the changes required are the addition of the two methods above to Test, and the deletion of the unnecessary TestSuite(new TestClass) code in the Loader.
Another slight inconsistency in the use of the Test interface is that the various implementations of Test::run() don't return the same things - TestCase::run() returns the TestResult, TestSuite::run() returns null (actually it does return the TestResult, but this looks to be a lucky accident - it just so happens the last statement in the loop evaluates to a TestResult)
There are other reasons for not subclassing TestSuite. Subclassing a concrete class is often not a good thing to do: if the superclass behaviour changes, it can have unforseen consequences on the subclasses. Building deep inheritance hierarchies of concrete classes is generally considered harmful for this reason.
- Baz
"I agree to some extent. But the name change I'd prefer would be to drop the ubiquitous and redundant 'Test'. e.g. Test::Unit::Case, Test::Unit::Suite"
Yes, I agree.
"In fact, I think some of these should be at a higher level still; Test::Unit::Case is meant to be subclassed by end users: why not contract the name down to Test::Case? Test::Runners are also meant to be used with 'old style' tests, and used by end users, so why not: Test::Runner, Test::TkRunner, Test::HarnessRunner ?"
No, I don't agree here. Keeping all components of our, hm, well, "application" (or is it still "framework"?), under one namespace is central for easy integration with standard CPAN procedures. Also, if you look at the other stuff in the Test::* tree, it just does not fit this way IMHO (different levels of abstraction). It *might* be an alternative idea to go for a new top-level category on the CPAN (e.g., "PerlUnit"), but will people look there?
Incidentally, I think we are now at a point where we should really discuss this naming thing. What do we want to call it? Test::Unit? PerlUnit? Both (first for the framework part, second for the application part)? Something else?
"TestSuite.pm:112:sub name { ... This one I should have. I dont understand why this isnt in the Test interface, all of its subclasses implement it!!!"
Might be because of the different semantics that this name attribute supports: TestCase uses the name to get the method to be called - while TestSuite uses the name to load the class or to name an empty suite.
I agree with your points that UnitHarness is not a TestSuite - but then don't make the Loader return it together with TestSuite objects (see below).
"TestSuite.pm:177:sub to_string { ... Should really get promoted into 'Test' (its implicit in the java version). ... The code should really also be changed ..."
I will do this, you are right here.
"TestSuite.pm:182:sub warning { ... Incidentally this has a bug, there is a missing '}' in the eval'd subroutine."
Oh oh. Will fix it (and get the SuiteTest to test harder to uncover this ... grumble).
"Which brings up an interesting point. Loader should really be returning Test objects, not Suite
objects. That's all the interface that is actually used. If this is to be done consistently, the changes required are the addition of the two methods above to Test, and the deletion of the unnecessary TestSuite(new TestClass) code in the Loader."
Let's do this.
"Incidentally, I think we are now at a point where we should really discuss this naming thing. What do we want to call it? Test::Unit? PerlUnit? Both (first for the framework part, second for the application part)? Something else?"
I think 'Test::Case' is a good name. Compare (reading as english)
Test::Case::Runner with Test::Unit::TestRunner
Test::Case::Runner with Test::Unit::TkTestRunner
Test::Case::Suite with Test::Unit::TestSuite
Test::Case::Result with Test::Unit::TestResult
Test::Case::Harness with Test::Unit::UnitHarness
Test::Case::Testable with Test::Unit::Test (in this instance a larger name change seems
more appropriate. 'Interface' was a something I thought of as the name here, but its not specific enough, and 'Testable' follows a convention of naming interfaces by the adjective describing the behaviour they add to implementors[1]. Another alternative was 'Runnable' but I wanted to avoid confusion with the Runner classes)
Test::Case::Method with Test::Unit::TestCase (this one doesnt read well. Names I considered were Test::Case - one level up, T::C::Case - which reads worse, T::C::Fixture describing the service that the object actually provides - but the terminology is not widely used, and T::C::Set. I have a difficulty in naming this class because the TestCase constructor returns an object which does a *single* test, but the code for subclasses will contain *multiple* test methods!).
The downside is that the 'Unit' bit isnt in the name, following Kent's naming convention. The question is, though, which is more important - keeping unit in the name or having 'good' names for the classes (whether the above are 'good' or not is of course arguable)
There is another set of names which read even better, and are more in keeping with Kent's 'xUnit' naming, compare:
Unit::Test::Runner with Test::Unit::TestRunner
Unit::Test::Suite with Test::Unit::TestSuite
Unit::Test::Case::Result with Test::Unit::TestResult
Unit::Test::Harness with Test::Unit::UnitHarness
Unit::Test::Testable with Test::Unit::Test
Unit::Test::Case with Test::Unit::TestCase
Unfortunately this is re-rooting the hierarchy in a place thats harder to find (if you're looking for testing software - 'Unit/*' could be for eg, unit conversions like metric to avoirdupois), and creating a new top-level hierarchy in CPAN, which can't be good.
Either set of names roll easier off the tounge, eg if you say 'Use the Test::Case::Runner',
then it sounds like an english sentence ('use the test case runner') as opposed to 'use the Test::Unit::TestRunner' or 'use the Test::Unit::Runner', which sound oddly inverted, as if you meant to say 'use the unit test runner'; hence the second suggested set of names, which are inverted precisely as you would say it.
This is purely an opinion - you did ask! Whether the names sound nice or not isnt as important to me as working code. :o)
It might be worth passing this to Kent. He has an interest in consistent naming, since it affects whether users can converse using the same terminology on the Wiki.
Cheers
Baz
[1] quite common in java, e.g. java.lang.Cloneable, java.io.Serializable. Theres a discussion of pros and cons of this convention here:http://c2.com/cgi/wiki?InterfacesShouldBeAdjectives
"I think 'Test::Case' is a good name ... The downside is that the 'Unit' bit isnt in the name, following Kent's naming convention. The question is, though, which is more important - keeping unit in the name or having 'good' names for the classes (whether the above are 'good' or not is of course arguable)."
At the top level of the namespace, I think the purpose is to make the whole package easy to find. The people I imagine as our users would look for "test" and something with "unit", I think. Therefore I don't like "Test::Case".
Below the top level, the class names should aid understanding of the framework. It is here, in the middle of the names, that I see the most room for changes. There is a good case for leaving them as-is at the lowest level: the names correspond to the JUnit names, which arguably set some kind of "standard" to follow.
"There is another set of names which read even better, and are more in keeping with Kent's 'xUnit' naming, compare: ... Unit::Test::Runner with Test::Unit::TestRunner ... Unfortunately this is re-rooting the hierarchy in a place thats harder to find ... "
Exactly.
"This is purely an opinion - you did ask! Whether the names sound nice or not isnt as important to me as working code."
I don't care at all if they "sound nice", but I *do* care about the namespace, as this will be very important when distributing our finished work (e.g., if we spread out the namespace across multiple second-level tiers, it will open the possibility for people to download only parts of the package, and then it does not work, etc.).
"It might be worth passing this to Kent. He has an interest in consistent naming ..."
Good idea. Discussing our final naming suggestions on the Perl modulelist <modules@perl.org> will also yield good advice.
1) Test now has name() as part of the obligatory interface. It also supports to_string() now.
2) TestSuite has been fixed for the warning() method - this was not detected by the self tests because TestCase covered the error up. TestSuite run() now returns a TestResult, like TestCase run().
3) BTW, we have three more testcases (26 now) as I have added the TestListener interface tests.