Make all Dictionary tests inheritable

[Gabriel-Darbord]

I found 8 tests in DictionaryTest that hardcoded the use of the Dictionary class. This meant that all subclasses of DictionaryTest also ran these tests using dictionaries instead of the class they were supposed to test. This fix replaces those hardcoded occurrences with the use of the classToBeTested method, just like all other tests.

Now about SystemEnvironment, which is a special cookie. This fix works for all classes except for this one. This required implementing SystemEnvironment>>#at:update:initial:, and also adjusting the tests for this class. @jecisc told me about the plans to no longer make this class inherit from Dictionary, so I guess this can be considered a temporary workaround. The API that is tested by these fixed tests is not used anywhere else anyway.

[Ducasse]

Tx!

[demarey]

I'm not fan of adding SystemEnvironment>>#at:update:initial: just for having the same test set passing for dictionaries subclasses. Isn't it possible to disable problematic tests (probably some behavior not needed) for SystemEnvironment?

[Gabriel-Darbord]

It doesn't have to be just for passing the tests, this method has always been part of the SystemEnvironment API by inheritance from Dictionary, it just wasn't well adapted for that class. This part of the API is not used in a base image, but there's no protection against someone (mis)using it. The initial: part would always throw an exception, but the update: part works as intended. Personally, I find it odd to have a half-broken method lying around.
If you'd prefer, this can be a shouldNotImplement and we can change the associated tests to be empty, which seems to be the best we can do to disable them (existing tests do this).

[guillep]

It doesn't have to be just for passing the tests, this method has always been part of the SystemEnvironment API by inheritance from Dictionary, it just wasn't well adapted for that class. This part of the API is not used in a base image, but there's no protection against someone (mis)using it. The initial: part would always throw an exception, but the update: part works as intended. Personally, I find it odd to have a half-broken method lying around. If you'd prefer, this can be a shouldNotImplement and we can change the associated tests to be empty, which seems to be the best we can do to disable them (existing tests do this).

Hi @Gabriel-Darbord, I think the issue is manyfold

• first SystemEnvironment should not inherit from dictionary, and people should not use it as such. Right now it does by chance (or by design). The issue is that if people use it as a dictionary, it will be harder to remove/clean in the future. I think Stef's point is that making a test to "validate wrong behavior" will somehow make this problem worse, as we will have more users of the wrong API, making the code harder to change/remove later

• personally, I prefer, if anything the "should not implement" you propose, but instead of having an empty test I'd have a test asserting that it effectively throws a ShouldNotImplement exception.

[Gabriel-Darbord]

I was thinking more along the lines of using the existing SystemEnvironment with the regular Dictionary API, rather than creating new ones and using them as dictionaries, but both are currently possible, so nothing prevents misuse.
I will change it to shouldNotImplement.

[Ducasse]

Thanks christophe for your review! Tx all.