On 4 Jun 2017, at 02:50, Cyril Ferlicot D. <cyril.ferlicot@gmail.com> 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

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/08fdc8d1712f91b2c22d95349cdf73f8...

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

2017-06-04 15:32 GMT+02:00 Denis Kudriashov <dionisiydk@gmail.com>:

2017-06-04 15:03 GMT+02:00 Cyril Ferlicot D. <cyril.ferlicot@gmail.com>:

...

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?

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

2017-06-04 21:28 GMT+02:00 Cyril Ferlicot D. <cyril.ferlicot@gmail.com>:

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