Abstract test classes are not managed well in SUnit

Hi,

Today I found a bug in SUnit.

Sometimes when you have an abstract test class and you override it the
tests will be inherit or not.

If a subclass has other tests, the tests will not be inherited. If the
subclass has no other tests, the tests will be inherited.

Here is a little script showing the problem:

TestCase subclass: #A
        slots: {  }
        classVariables: {  }
        category: 'TestProblem'.
       
#A asClass subclass: #B
        slots: {  }
        classVariables: {  }
        category: 'TestProblem'.
       
#B asClass subclass: #C
        slots: {  }
        classVariables: {  }
        category: 'TestProblem'.
       
#A asClass class compile: 'isAbstract
        ^ self = A'.
       
#A asClass compile: 'actualClass
        ^ self subclassResponsibility'.
       
#A asClass compile: 'testTest
        self actualClass new'.
       
#C asClass buildSuiteFromAllSelectors run.
 "1 run, 0 passes, 0 skipped, 0 expected failures, 0 failures, 1 errors,
0 unexpected passes"

#C asClass compile: 'testAnotherTest
        self assert: true'.
       
#C asClass buildSuiteFromAllSelectors run.
  "1 run, 1 passes, 0 skipped, 0 expected failures, 0 failures, 0
errors, 0 unexpected passes"


In the last line I would expect 2 run cases. 1 pass and 1 error.

I think this is important to correct because it weaken the strength of
the tests.

https://pharo.fogbugz.com/f/cases/20118/SUnit-does-not-manage-well-abstract-test-classes


--
Cyril Ferlicot
https://ferlicot.fr

http://www.synectique.eu
2 rue Jacques Prévert 01,
59650 Villeneuve d'ascq France

I've seen this before, and I'm not sure it's a bug.

A testless, non-abstract subclass of a TestCase is useless unless it inherits test methods. But if it defines its own test methods, then it isn't clear if it was made a subclass to inherit test methods, or if it was made a subclass to inherit non-test helper methods, or both. I think to be safe it assumes the second.

Implement #shouldInheritSelectors in the superclass to get your intended behavior.

Le 04/06/2017 à 02:26, monty a écrit :
> I've seen this before, and I'm not sure it's a bug.
>
> A testless, non-abstract subclass of a TestCase is useless unless it inherits test methods. But if it defines its own test methods, then it isn't clear if it was made a subclass to inherit test methods, or if it was made a subclass to inherit non-test helper methods, or both. I think to be safe it assumes the second.
>
> Implement #shouldInheritSelectors in the superclass to get your intended behavior.
>
>

I think that in most case we want the tests from the superclass. At
least this is what I expect.

A subclass of an object keep the behaviour of its superclass when the
methods are not overridden. Why should it be different for tests?

I see your point but if that is the case I think it should inherit the
tests by default and we could had a way to prevent this behaviour in
some cases.

It is very awkward to get less tests after adding a new test in a class.
I think it reduce the power of the tests.

--
Cyril Ferlicot
https://ferlicot.fr

http://www.synectique.eu
2 rue Jacques Prévert 01,
59650 Villeneuve d'ascq France

I second that.

>
> --
> Cyril Ferlicot
> https://ferlicot.fr
>
> http://www.synectique.eu
> 2 rue Jacques Prévert 01,
> 59650 Villeneuve d'ascq France
>

I agree with you, and I would rather subclasses inherit tests by default, but be aware that this isn't a bug[1], and changing it will temporarily break some test hierarchies.

[1] https://github.com/pharo-project/pharo/blob/08fdc8d1712f91b2c22d95349cdf73f83eac66e1/src/SUnit-Core.package/TestCase.class/class/shouldInheritSelectors.st

Le 04/06/2017 à 12:38, monty a écrit :
> I agree with you, and I would rather subclasses inherit tests by default, but be aware that this isn't a bug[1], and changing it will temporarily break some test hierarchies.
>
> [1] https://github.com/pharo-project/pharo/blob/08fdc8d1712f91b2c22d95349cdf73f83eac66e1/src/SUnit-Core.package/TestCase.class/class/shouldInheritSelectors.st
>
>

If most people agree that we should inherit the tests by default I think
this is a change that should be done very early in the Pharo 7 dev. Thus
it would let time to people to adapt their project.

--
Cyril Ferlicot
https://ferlicot.fr

http://www.synectique.eu
2 rue Jacques Prévert 01,
59650 Villeneuve d'ascq France

Hi Cyril,


As Minty points out, this unintuitive behaviour exists fur a reason and there is a work around.  

SUnit is a community wide package existing in all the dialects of Smalltalk I'm aware of.  If you change the semantics then every time a test suite is exchanged it will need to be rewritten, and by someone who understands the divergence, otherwise the test suite won't work as intended.

Is is worth diverging from a community wide standard when there is a rationale for the design and there is a work around?

Is it puss bow to seek a wider consensus and see what e.g. the VisualWorks, VisualAge and GemStone communities think?

Is it possible that better documenting the work around would be better?

>
> --
> Cyril Ferlicot
> https://ferlicot.fr
>
> http://www.synectique.eu
> 2 rue Jacques Prévert 01,
> 59650 Villeneuve d'ascq France

_,,,^..^,,,_ (phone)

Le 04/06/2017 à 18:20, Eliot Miranda a écrit :
> Hi Cyril,
>

Hi Eliot,

>
> As Minty points out, this unintuitive behaviour exists fur a reason and there is a work around.  
>
> SUnit is a community wide package existing in all the dialects of Smalltalk I'm aware of.  If you change the semantics then every time a test suite is exchanged it will need to be rewritten, and by someone who understands the divergence, otherwise the test suite won't work as intended.
>
> Is is worth diverging from a community wide standard when there is a rationale for the design and there is a work around?
>

When I discovered this behaviour I was really surprised by it.
(apparently I am not the only one) And if I did not checked the number
of tests I think I would have never know that my tests are not executed
in all the hierarchy.

Also, abstract tests classes is something I use a lot since I am in the
community. Now I know that I'll need to change many of the projects I
contributed to in order to activate all the tests I wanted. And I think
a lot of newcomers will not know this behaviour because it is counter
intuitive. IMO, it goes against the principle of inheritance.


> Is it puss bow to seek a wider consensus and see what e.g. the VisualWorks, VisualAge and GemStone communities think?
>

For this problem I would like to see all the smalltalks using SUnit
change this behaviour. I really do not want to break compatibility and I
think this change is important for the usability of SUnit. I don't have
much contact with other smalltalk communities but I would be happy if we
can seek a consensus.

If you have an idea of the best way to communicate with all the
communities on this subject I would like to hear it.

> Is it possible that better documenting the work around would be better?
>

Personally I think that even with a good documentation, this is not the
kind of thing everyone will remember. But this is only my opinion. :)

>
> _,,,^..^,,,_ (phone)
>


--
Cyril Ferlicot
https://ferlicot.fr

http://www.synectique.eu
2 rue Jacques Prévert 01,
59650 Villeneuve d'ascq France

Le 04/06/2017 à 21:26, Cyril Ferlicot D. a écrit :
> For this problem I would like to see all the smalltalks using SUnit
> change this behaviour. I really do not want to break compatibility and I
> think this change is important for the usability of SUnit. I don't have
> much contact with other smalltalk communities but I would be happy if we
> can seek a consensus.
>
> If you have an idea of the best way to communicate with all the
> communities on this subject I would like to hear it.
>

Maybe we can talk about it at ESUG?

--
Cyril Ferlicot
https://ferlicot.fr

http://www.synectique.eu
2 rue Jacques Prévert 01,
59650 Villeneuve d'ascq France