Re: [Cppunit-devel] hierarchy sample bug...

[Baptiste L. <bl...@cl...>]

> > >     Humm, just one thing that popup in my mind: who owns the fixture
given
> > > to the test caller ? That's may be the reason why it is done that
way...
> > >
> > > I guess it had something to do with the automatic test registration
thing,
> > > which has been removed anyway.
> >     No, it was done that way in Michael Feather version. And if I
understood
> > thing well, it's also done that way in junit.
> >
>
> Not!
> Please take a look at the JUnit code. JUnit doesn't have a TestCaller: it
doesn't need one because it can use Java Reflection to lookup test methods
and execute them.
> Also, the only container for Tests is the TestSuite class, no Registries
or other things.
> It's simple and I like it.
    They don't have a test caller class because the test caller is the test
case itself. Here is a piece of code extracted from
TestSuite.addTestMethod() (called for each public test method on
construction):

  if (isPublicTestMethod(m)) {
   names.addElement(name);

   Object[] args= new Object[]{name};
   try {
    addTest((Test)constructor.newInstance(args));            //  <=== a new
instance is created which take the name of the method in the constructor !
   } catch (Exception t) {
    addTest(warning("Cannot instantiate test case: "+name));
   }

and in TestCase.runTest() that do what our test caller is doing:
// fname is the name specified on construction of the test case, the name of
the method to run!
   runMethod= getClass().getMethod(fName, new Class[0]);
    [...]
  try {
   runMethod.invoke(this, new Class[0]);
  }

    This exactly what our test caller do... In some way we have even more
flexibility because we can decorate the test caller (adding running time
constraint, expecting some exception...).

>
>
> >
> > > Because I did not have a clear picture of all use cases of TestCaller,
the
> > > current implementation allows the TestCaller to own the Fixture or
not:
> > > If the Fixture is passed through a reference the TestCaller won't own
it.
> > > If the Fixture is passed through a pointer the TestCaller will assume
> > > ownership and destroy it in its destructor.
> >
> >     Things I want to point out:
> >     - in about 95% of the case, you use the default constructor for the
> > fixture.
> >     - Even when subclassing a test case, you override the setUp() and
> > tearOff() method, and would still have a default constructor.
> >     - non default constructor would most likely be used for a test case
with
> > are parameterized (a generic test case that load tests from a file for
> > example). I don't see that happening often. And in all likelyhood, you
would
> > more likely overide runTests() than create a suite using some test
> > callers...
>
> The main problem is not that the old TestCaller uses the default
constructor, but that it is creating the wrong (base) class.
    I'm ok with that.

> >     As I see it the problem is solved by introducing a makeFixture()
virtual method in test case which can be subclassed. You would have
something like:
> >
> >     void registerTest( TestSuite *suite )
> >     {
> >         suite->addTest(
> >                 new TestCaller<MyTestCase>( "test1", test1,
makeFixture() ) );
> >         suite->addTest(
> >                 new TestCaller<MyTestCase>( "test1", test2,
makeFixture() ) );
> >         ...
> >     }
> >
>
> Sorry to be blunt, but I think this sucks, because you need to be very
careful to override makeFixture() in every subclass. Why can't the TestCase
use a reference to itself instead?
    Its depends how you make your code. I use IDE macro to generate and adds
file to the project. I just check a box to say "this class is  a cppunit
test case", and it adds everything I need to make the class a test case
(static suite method, inheritance, includes...). This way I remove all the
problem you have when using cut and paste to do that.

> Ahem, the amended 'hierarchy' example seems like a perfectly valid example
of how to use the same fixture for multiple TestCallers. Any objections?

    There is a very bad thing about the way its done: you enforce the class
owning the test caller to also own a reference to the test case. Any class
you give the test caller to, must also own the instance of the test case
that was used to create the test caller.
    To me this is a big loss of flexibility. It makes it harder to use stuff
such as decorator and break the "ownership" policy that is everywhere in the
framework:
    TestSuite owns all its tests (sub-suite, TestCaller, ...)
    TestCaller owns all its TestCase
    TestCase owns resources needed to run the test.
    Each component of the hierarchy can be taken out of the system and works
on its one. The TestCaller does not need the TestSuite to be able to run.

    Usually, you don't build test case on the stack. You have a static
suite() method in the test case returning a new TestSuite object. With the
new ownership policy, you would have to subclass TestSuite so it could hold
(and own) a reference on the TestCase instance.

    To sum it up:
    - the TestCaller would not be able to be a component running on its one
(you could not pass it to the TestRunner because it need something "above"
owning the test case).
    - you provided a valid example of use of the test caller with a
reference on the test case. Though there are underlying constraints to that
use case (at suite level).
    - this would break one of the constructor of TestSuiteBuilder since it
could not use the generic TestSuite class, but would need to instantiate its
owns subclassed TestSuite.

    Sharing the reference on the test case does not make it easier to use,
and split from the ownership policy that is in the framework. Using the
standard template factory method pattern (makeFixture()) seems a lot easier
to me and much more flexible.

>
> >
> >     The only use I can see for the test caller with fixture by reference
is if the Test Caller is stored by value in another class.
> >
> >     Things to do:
> >         Update TestSuiteBuilder:
> >             - add a addTestCaller() method which take a pointer on the
fixture.
> >             - add a constructor that take a pointer on a "makeFixture()"
methods, to make
> > subclassing easier.

> Before you rush off to add makeFixture() methods, etc. Let us rethink this
issue a bit. My personal feeling is that this registration stuff already is
more complex than necessary, so adding more stuff is unlikely to be the
proper solution. I prefer to steal more from JUnit :-)
    I was not suggesting adding the makeFixture() method in the library.
This is a user choice (just like having a registerTests() method). If the
user make a test case that can be subclassed, he must design the class so it
can be done. I was merely suggesting a way to do that.

    Conclusion:
        - adding the constructor with a pointer on the fixture add a lot of
flexibility to test caller.
        - enforcing the use of the constructor by reference remove a lot of
flexility.

    To do cppunit:
        - remove TestRegistry.cpp/.h
        - implement a TextTestRunner

    To do for the sample:
        - add a suite() method for each test case, which returns a
dynamically allocated suite,
        - add the suite of each test case to the TextTestRunner,
        - run the TextTestRunner.
    => this would make it similar to run suite using a graphic or text
TestRunner, and would show how test case should be factored.


> Please try it again. Also check if you can get shell access with SSH on
cppunit.sourceforge.net.
OK, I'll try that.