Re: [Pharo-dev] Abstract test classes are not managed well in SUnit

[Ben Coman]

Besides other action, in parallel perhaps the first step would be
displaying a QA tip?
cheers -ben

On Mon, Jun 5, 2017 at 3:26 AM, Cyril Ferlicot D. 
wrote:

> 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
>
>

Re: [Pharo-dev] Abstract test classes are not managed well in SUnit

[Nicolas Cellier]

2017-06-04 21:28 GMT+02:00 Cyril Ferlicot D. :

> 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?
>
>
It can be interesting to measure instead of throwing 99% figures like I did
;)
Here is a small snippet and result in Squeak trunk:

| testAll testLocal superIsAbstractOrTestSelectorIsEmpty
superclassIsAbstract selectorsIsEmpty selectorsIsEmptyButNotAbstract |
testAll := Set new.
testLocal := Set new.
superIsAbstractOrTestSelectorIsEmpty := Set new.
superclassIsAbstract := Set new.
selectorsIsEmpty := Set new.
selectorsIsEmptyButNotAbstract := Set new.
TestCase allSubclassesDo: [:c |
c superclass = TestCase
ifFalse: [
(c superclass isAbstract or: [c testSelectors isEmpty])
ifTrue: [superIsAbstractOrTestSelectorIsEmpty add: c].
c superclass isAbstract
ifTrue: [superclassIsAbstract add: c]
ifFalse: [c testSelectors isEmpty ifTrue:
[selectorsIsEmptyButNotAbstract add: c]].
c testSelectors isEmpty ifTrue: [selectorsIsEmpty add: c].
c shouldInheritSelectors
ifTrue: [testAll add: c]
ifFalse: [testLocal add: c]]].
{testAll size. superIsAbstractOrTestSelectorIsEmpty size.
superclassIsAbstract size. selectorsIsEmptyButNotAbstract size.
selectorsIsEmpty size. testLocal size}.
 #(143 142 129 13 17 22)

So 143 classes want to inherit
- 129 because superclass isAbstract,
- 13 because testSelectors isEmpty,
- 1 for another reason - override of shouldInheritSelectors, for which we
should write a snippet too
22 don't want to inherit

But here are the 22 in Squeak:
(testLocal collect: #name) sorted ->
#(#ChangeHooksTest #ClassTraitTest #MessageTraceTest
#MethodHighlightingTests #MorphBugs #MorphicEventDispatcherTests
#MorphicEventFilterTests #MorphicEventTests #PolygonMorphTest
#PureBehaviorTest #StickynessBugz #SystemChangeErrorHandling
#SystemChangeNotifierTest #TestNewParagraphFix #TextAnchorTest
#TextEmphasisTest #TextFontChangeTest #TraitCompositionTest
#TraitFileOutTest #TraitMethodDescriptionTest #TraitSystemTest #TraitTest)

and their superclass
((testLocal collect: #superclass) collect: #name) sorted ->
#(#ClosureCompilerTest #HashAndEqualsTestCase #MessageSetTest #MorphTest
#MorphicUIManagerTest #SystemChangeTestRoot #TestParagraphFix
#TraitsTestCase #UserInputEventTests)

Let's see;
- TraitsTestCase is not abstract, but defines no test! So it is kind of
abstract.
  TraitsTestCase subclasses size -> 7, 15 remain out of 22
- same for SystemChangeTestRoot, 4 subclasses, 11 remain out of 22.
- same for UserInputEventTests. 3 subclasses less, 8 remaining out of 22.
- TestNewParagraphFix does redefine all super test as ^super test... A lack
of knowledge visibly. 7 remaining out of 22.
- HashAndEqualsTestCase is not abstract, has tests, but the tested
prototypes are empty!
  So it is yet another abstract class in disguise.
  But this time, some subclasses won't testHash nor testEquality except if
they redefine it, is it a bug?
  That's 3 subclasses with not empty selectors, so 4 remaining out of 22

The others might be correct, so 161 ou of 165 should inherit... Not 99%,
only near 98%.

That would be 4 (super)classes requiring implementation of a
shouldInheritSelectors ^false.
That's too much, so let's analyze further.

Maybe MethodHighlightingTests does not need to inherit from
ClosureCompilerTest...  (no inst var in super, and no super method used).
That 1 less.
MorphBugs and PolygonMorphTest don't need to be a subclass (it's just for
classifying). There is no specific setUp, and tests could go upward. That's
2 less.
Same for StickynessBugz. 1 less.

After an easy refactoring, nothing in Squeak trunk would really require a
shouldInheritSelectors ^false.
It would be interesting to replay the snippet in Pharo, and in 3rd party
applications too.
That's good elements for opening a discussion.


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

Re: [Pharo-dev] Abstract test classes are not managed well in SUnit

[Cyril Ferlicot D.]

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



signature.asc
Description: OpenPGP digital signature

Re: [Pharo-dev] Abstract test classes are not managed well in SUnit

[Cyril Ferlicot D.]

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



signature.asc
Description: OpenPGP digital signature

Re: [Pharo-dev] Abstract test classes are not managed well in SUnit

[Nicolas Cellier]

2017-06-04 15:32 GMT+02:00 Denis Kudriashov :

>
> 2017-06-04 15:03 GMT+02:00 Cyril Ferlicot D. :
>
>> 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.
>>
>
> I agree :)
>

+1 for inheriting by default.
If ever we don't want to inherit, it's as simple as implementing
shouldInheritSelectors
^false

It's far more simple than implementing isAbstract at class side as we have
to do now.
99% of the time we want to inherit, and what we see in order to avoid
isAbstract dance is
shouldInheritSelectors
^true


One more word about Pharo version.
In original SUnit we control with these simple two methods:

buildSuiteFromSelectors
^self shouldInheritSelectors
ifTrue: [self buildSuiteFromAllSelectors]
ifFalse: [self buildSuiteFromLocalSelectors]

shouldInheritSelectors
^self superclass isAbstract
or: [self testSelectors isEmpty]

In Pharo this has been refactored:
- buildSuiteFromAllSelectors is not sent, but duplicated in
buildSuiteFromSelectors
- buildSuiteFromLocalSelectors is not sent either,
- all happens in allTestSelectors, which has changed of semantic and does
not allways answer all tests selectors :(
But the logic seems to remain the same: if the superclass is abstract OR if
the set of selectors is empty, then the class will inherit...
If my opinion ever counts, what I see is refactoring for refactoring.
I don't see any new feature, just more unsent selectors, more garbage, and
unclear semantic.
I call this runaway refactoring.
There's enough thing to clean in Squeak to avoid "cleaning" what works well
no?

Re: [Pharo-dev] Abstract test classes are not managed well in SUnit

[Eliot Miranda]

Hi Cyril,


> On Jun 3, 2017, at 5:50 PM, Cyril Ferlicot D.  
> wrote:
> 
>> 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.

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)

Re: [Pharo-dev] Abstract test classes are not managed well in SUnit

[Denis Kudriashov]

2017-06-04 15:03 GMT+02:00 Cyril Ferlicot D. :

> 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.
>

I agree :)

Re: [Pharo-dev] Abstract test classes are not managed well in SUnit

[Cyril Ferlicot D.]

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



signature.asc
Description: OpenPGP digital signature

Re: [Pharo-dev] Abstract test classes are not managed well in SUnit

[monty]

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

> Sent: Saturday, June 03, 2017 at 8:50 PM
> From: "Cyril Ferlicot D." <cyril.ferli...@gmail.com>
> To: pharo-dev@lists.pharo.org
> Subject: Re: [Pharo-dev] Abstract test classes are not managed well in SUnit
>
> 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
> 
>

Re: [Pharo-dev] Abstract test classes are not managed well in SUnit

[Max Leske]

> On 4 Jun 2017, at 02:50, Cyril Ferlicot D.  wrote:
> 
> 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.

I second that.

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

Re: [Pharo-dev] Abstract test classes are not managed well in SUnit

[Cyril Ferlicot D.]

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



signature.asc
Description: OpenPGP digital signature

Re: [Pharo-dev] Abstract test classes are not managed well in SUnit

[monty]

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.

> Sent: Saturday, June 03, 2017 at 5:09 PM
> From: "Cyril Ferlicot D." <cyril.ferli...@gmail.com>
> To: Pharo <pharo-dev@lists.pharo.org>
> Subject: [Pharo-dev] 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
> 
>

[Pharo-dev] Abstract test classes are not managed well in SUnit

[Cyril Ferlicot D.]

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



signature.asc
Description: OpenPGP digital signature