Mantis - Squeak
Viewing Issue Advanced Details
6584 System major always 08-01-07 17:24 04-18-10 22:05
pmm any  
nicolas cellier any  
high any  
closed 3.8  
6665 and any newer fixed  
none trunk  
0006584: Symbol allInstances doesn't work anymore
Since the WideString refactoring Symbol is an abstract class like String. It doesn't have any instances anymore. Yet there are still senders of it in ImageSegment related code. They store it into a temprory variable to prevent them from beeing garbage collected. This is clearly broken now. The correct way to fix this is to send Symbol allSymbols.
 SymbolAllintancesFix.1.cs [^] (8,584 bytes) 08-01-07 17:24

08-02-07 01:50   
Can you provide sunit tests with it.

At least one simple one that fails before the patch and passes after

a method that does

self shouldnt: [ something that fails before the patch] raise: Error.

would at least flag if the patch is missing.

If you can think of other ways to check the pieces even better.

Your in service, --Jerome Peace

Note: I didn't test the code.

Just glanced at it to notice that it is several methods long.
08-02-07 11:51   
The patch is not several methods long. The methods that are patched (one line is changed in each) are several methods long. So the patch is only 3 lines.
nicolas cellier   
09-24-07 22:18   
I guaranty this patch as valid and non-regressive.
It seems to me like a non-sense to write long-long-crooked tests for something so obvious and already discussed in Squeak-dev.

If you wait for a test of ImageSegment to appear (that would make sense), you might let these bugs rot in the image for several years. Please include in 3.10!
09-25-07 05:24   
Hi nicolas, hi pmm

I track bugs. The patterns I see are that a patched bug usually receives other patches. Often done by several code cooks.

Tests prevent reversion of fixes by demonstrating a difference. And in that sense they make me feel more comfortable about the future of the image.

Nicolas what made you sure this patch is valid and non-regressive? What stops you from putting that in a simple bug test?

>It seems to me like a non-sense to write long-long-crooked tests for something so obvious and already discussed in Squeak-dev.

Why would the test be long-long-crooked? Testing any simple aspect of the fix that demonstrates the difference is a sufficeint first approximation of what is needed. I read your comments to mean that the methods in question don't have much test coverage at the moment and that the task of giving them some would be onerous, especially to someone who just wants the bug fixed.

I agree, it would be an imposition to ask for repairs that should belong to the code authors. I just ask that patchers take the responsibility of proving their own code is
1) necessary because there is a test that fails if the the code is not included.
2) sufficient because the test passes when the patch is in place.

Even superficial tests with the above two properties would be benificial to the integrity of squeak.

I take your word, Nicolas, at great value. You are particular about things and have high standards.If you vouch for the code I accept that the code is good code.

The tests are to insure it remains so.

I track and fix bugs, I don't harvest them. My requests are just that, requests. They are not demands or preconditions for acceptance.

I have learned that the first rule of trouble shooting is first hand information. Tests are important because they provide that and point to the problem area when they fail.

I believe your word. But it only applies to the here and now. Not for what the code might be patched to in the future. Or what else might be added to squeak that would interact with it.

I figure if I can get the good folks to write tests for their good code that gives us all a place to stand when we ask the same of the stumblers.


Yours in curiosity and service, --Jerome Peace
nicolas cellier   
09-25-07 08:21   
Hi Jerome,
Agree with your general comments, but every rule has exceptions.
I cannot tell if new code is really good, I can only tell it is obviously better than older because it fixes an obvious bug.

Old code call (Symbol allInstances) which is now a NON SENSE because Symbol is now an Abstract class. (Symbol allSymbols) is a well-known-far-better solution.

Maybe the simple test could be to scan source code and check that such a construct is not used, based on Abstract Syntax Tree Analysis.
Should we really test all possible such ill-constructions?

I believe tests should rather concentrate on asserting the functionality and the intention of the messages.
So this lead to the long-long way, that is to test ImageSegment, which very few can understand I guess. So it won't happen.
nicolas cellier   
02-01-08 22:58   
"fix begin"
Installer mantis bug: 6584 fix:'SymbolAllintancesFix.1.cs'.
"fix test"
"fix end"
nicolas cellier   
09-16-09 19:59   
Fixed in [^]
Trunk fix is NOT SymbolAllintancesFix.1.cs above, because eem applied some other changes related to closure.