supercollider
UnitTest: Check no 'leaf' classes throw a `SubclassResponsibilityError`
Purpose and Motivation
This PR adds a test that checks no 'node' class throw aSubclassResponsibilityError, as they should implement the method. Subclass responsibility errors are similar to abstract virtual methods in C++, in that they are a part of the interface and the must be defined, however, they may throw if no reasonable implementation is possible.
This is done as a longstanding bug was found in some of the collections where they didn't fulfil their responsibilities.
If a class cannot implement the method, it should redefine the method and throw a ShouldNotImplement error. In future, when creating the help docs, we could remove methods that throw ShouldNotImplement, and more clearly label all methods that throw a SubclassResponsibilityError.
To test whether a method throws this error, an exact bytecode sequence is checked for. This is not full–proof, but in practice works quite well. This is because you would only implement this method in the following way foo { ^this.subclassResponsibility(thisMethod) }. More information about this is written in a comment above the test.
Currently, the following methods produce errors and should be fixed in separate PRs.
a TestSubclassResponsibility: test_all - Archive:isAssociationArray throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - Array2D:isAssociationArray throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - Array2D:remove throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - Array2D:add throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - BinaryOpStream:put throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - BinaryOpXStream:put throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - CSVFileReader:put throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - CleanupStream:put throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - Complex:mod throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - Complex:div throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - ControlSpec:defaultControl throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - EmbedOnce:put throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - EventStreamPlayer:put throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - FuncStream:put throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - FuncStreamAsRoutine:put throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - HIDdef:gui throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - IdentityBag:isAssociationArray throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - Interval:isAssociationArray throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - Interval:remove throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - Library:isAssociationArray throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - MIDIdef:gui throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - NAryOpStream:put throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - OSCdef:gui throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - ObjectTable:isAssociationArray throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - OneShotStream:put throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - OrderedIdentitySet:isAssociationArray throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - Pair:add throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - Pair:remove throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - Pair:isAssociationArray throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - Pipe:pos_ throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - PmonoArticStream:next throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - PmonoArticStream:put throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - Polar:mod throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - Polar:< throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - Polar:pow throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - Polar:div throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - Pretty:pos_ throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - Pretty:next throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - Range:remove throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - Range:isAssociationArray throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - SemiColonFileReader:put throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - StreamClutch:put throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - SynthDefControl:play throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - SynthDefControl:stop throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - TabFileReader:put throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - Task:put throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
a TestSubclassResponsibility: test_all - UnaryOpStream:put throws a subclass responsibility error, please implement it. Use this.shouldNotImplement(thisMethod) if it isn't possible to implement the method.
Types of changes
- New Test
To-do list
- [x] Code is tested
- [ ] All tests are passing — the failures highlight places where things might be broken.
- [x] Updated documentation
- [x] This PR is ready for review
@capital-G something has happened in the CI tests. It says the process exited with error code 124.
https://github.com/supercollider/supercollider/actions/runs/11704337222/job/32596917591?pr=6527#step:6:14308
Do you know what that is?
--- I'm going to just run them again...
~~Seems like a random error in testing. I re-ran the tests again...~~ This didn't help. I don't know what's going on.
BTW there was a brownout period for macOS runner for macOS 12 recently, but that shouldn't affect tests on Linux...
I guess it's possible that it's timing out. Having to run the tests suite for longer than 20 minutes is probably not desirable long run, but for to test you could try raising time limits for both the tests and the posting steps...
The test takes 20 seconds on my machine to run. I've removed the timeout, but he CI test is excruciatingly slow.
It kinda looks like each subsequent test is increasing the amount of the of each test, as if it was O(n^2).... Even simple tests like the ones in TestArray are taking minutes when they should take milliseconds.
This pr test does create several failure cases. Could this be slowing the code down?
I don't know why the tests are slow, of the top of my head. BTW I could recreate locally some failures only when running the test suite on a fully loaded system (see here). You can run test suite by running the code in sclang_test_runner.scd
Do we maybe want to fix the classes before having the failing tests in the CI? We can use the new UnitTest to make sure it is all done.
Do we maybe want to fix the classes before having the failing tests in the CI?
Yes, definitely :)
@JordanHendersonMusic, you can skip the failing tests by adding them here: https://github.com/supercollider/supercollider/blob/develop/testsuite/scripts/test_run_proto_gha.scd
There is a (slightly hidden) already existing issue that I just noticed and just want to mention here.
It has to do with the combination of subclassing and overriding doesNotUnderstand. When a superclass implements subclassResponsibility for a given method, then we cannot catch it anymore with a doesNotUnderstand, but we have to explicitly override the method and call doesNotUnderstand.
For example, when you make a new Number object, you have to explicitly define pow in a subclass (that is fine), but when you want to catch all unknown methods, they need to be forwarded to doesNotUnderstand explicitly.
Not so much of a problem, just mentioning. It limits the utility of subclassing, to a degree.
That's a good point!
I suppose any attempt to reason about the validity of an interface is impossible without considering the content of doesNotUnderstand, additionally the presence of shouldNotImplement breaks interface design so testing for absolute 'correctness' isn't possible anyway — perhaps a feature of smalltalk.
This test just attempts to help catch mistakes. Therefore it doesn't limit the utility of sub-classing, although I don't think adding the extra method deferring to doesNotUnderstand is a limitation, more of a 'explicitation'.
If the doesNotUnderstand approach was to be adopted in the class library (to implement core interfaces) and we wanted to keep this test I can see two options.
- Manually exclude the class/method (and its subclasses) from the test.
- Test whether
Foo:doesNotUnderstanddiffers fromObject:doersNotUnderstand, and post a warning that the specific method might not fulfil its responsibilities.
Do we maybe want to fix the classes before having the failing tests in the CI?
Yes, definitely :)
I am somewhat unsure about this. Many of these methods are pretty obviously unimplementable and should be shouldNotImplement.... but some require more conversation, e.g.,
- Complex:div
- Complex:mod
- FuncStream:put
- PmonoArticStream:next
Should this PR contain all of these fixes?
Should this PR contain all of these fixes?
Could you start another one that focusses on these fixes? I would suggest that we implement the obvious ones.
When we are unsure. what about adding
xyz { ^this.notYetImplemented(thisMethod) }
And thereby exclude them from the test?
I've just provided implementations for everything. The complex things might be possible to implement. I am unsure about the streams.
Okay, I'm lost with the tests... I can't get them to work. This prs test passes though.
@JordanHendersonMusic can you decide whether we should put this on the 3.14 board? Let's start by rebasing on current develop to see whether the tests pass.
In future, when creating the help docs, we could remove methods that throw ShouldNotImplement, and more clearly label all methods that throw a SubclassResponsibilityError.
Could you create an Issue for this?
This approach, while maybe sound (?), should probably be done across the whole class library!
I had never thought of this.subClassResponsibility(thisMethod) as a duty of subclasses, but as an abdication of duty on the side of the superclass. It signals that the superclass cannot know how the method will look in each case.
I therefore thought that when you call it, it throws an error, because you called it and it doesn't exist yet, not because the implementation is buggy.
If we wanted, we could work towards changing this way of treating it. But let's not call it a bug yet.
I therefore thought that when you call it, it throws an error, because you called it and it doesn't exist yet, not because the implementation is buggy.
It signals that the superclass cannot know how the method will look in each case.
but it does signal that it must exist and be defined in the final class in the inheritance chain, presumably, because the implementation of the super class is going to call this method some where, or it is included in the documentation as being as part of the explicit interface and the user might call it.
That was the motivating issue, some classes that the user was supposed to use hadn't fulfilled this requirement.
If we wanted, we could work towards changing this way of treating it.
yes this is a subtle shift in how we think about these things.
Importantly, the test here only runs as a part of the class library in the unit test, so it is really only a contract with class library developers, not everyone else.
I don't have time right now to turn this into something more concrete, hence why I turned it into a draft.