SUnit and abstract test classes
Last updated at 4:47 am UTC on 5 June 2017
Problem
Cyril Ferlicot D.
<cyril.ferlicot@gmail.com> AttachmentSat, Jun 3, 2017 at 11:09 PM
Reply-To: Pharo Development List <pharo-dev@lists.pharo.org>
To: Pharo <pharo-dev@lists.pharo.org>
Reply | Reply to all | Forward | Print | Delete | Show original
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
Analysis
Nicolas Cellier
<nicolas.cellier.aka.nice@gmail.com> Mon, Jun 5, 2017 at 12:38 AM
Reply-To: Pharo Development List <pharo-dev@lists.pharo.org>
To: Pharo Development List <pharo-dev@lists.pharo.org>
Reply | Reply to all | Forward | Print | Delete | Show original
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.
