[update 1.4] #14356
Locked
 
|
14356
----- Issue 5376: AbstractTool fix removeMethods method http://code.google.com/p/pharo/issues/detail?id=5376 Issue 5371: XTableForUnicodeFont is not used anywhere (and an internal class) http://code.google.com/p/pharo/issues/detail?id=5371 Issue 5372: Remove old Mac font reading in StrikeFont http://code.google.com/p/pharo/issues/detail?id=5372 Issue 5370: AppRegistry class >> #default: dependent on Morphic http://code.google.com/p/pharo/issues/detail?id=5370 -- Marcus Denker -- http://marcusdenker.de |
|
|
Great, The basic Pharo Kernel Jobs can be build again! Leaving aside
the fact that number of failing tests significantly increased because someone made compiler tests dependent on TextMorph (setUpForErrorsIn:) :-) -- Pavel On Sat, Feb 25, 2012 at 8:43 AM, Marcus Denker <[hidden email]> wrote: > 14356 > ----- > > Issue 5376: AbstractTool fix removeMethods method > http://code.google.com/p/pharo/issues/detail?id=5376 > > Issue 5371: XTableForUnicodeFont is not used anywhere (and an internal class) > http://code.google.com/p/pharo/issues/detail?id=5371 > > Issue 5372: Remove old Mac font reading in StrikeFont > http://code.google.com/p/pharo/issues/detail?id=5372 > > Issue 5370: AppRegistry class >> #default: dependent on Morphic > http://code.google.com/p/pharo/issues/detail?id=5370 > > > > -- > Marcus Denker -- http://marcusdenker.de > > |
|
|
Ah yes, that was me, since one feature of the Compiler is to insert
notifications into source text editor, this has to be tested. - does the Compiler insert the right notification ? - does it insert the notification at the right place ? As soon as TextMorph and SmalltalkEditor provide a hook for evaluating/compiling source, and a callback for notifications they become inter-dependent. And we'd better write a test documenting the interdependency. Of course, we can replace the TextEditor with a mock up, like the one in CompilerTest and split the interdependency in two parts. And then assert on the notifier side that: - the compiler emit notifications with 1-based insertion point indices (1 means insert before first character) (Though I don't like the hardcoded positions in CompilerTest, it's not readble, but could be changed). Right now, the notified side of interdependency is missing, I see no test asserting that: - SmalltalkEditor notify:at:in: insert notifications with 1-based indices If I change TextEditor insertion points to be 0-based, this will get unnoticed... And this decomposition being less immediate than my simple tests, we forgot one feature which is to evaluate a sub-selection. This feature makes the interdependency much more complex - is notifier aware of sub-selection location ? If yes, does it provide a location related to beginning of sub-selection of beginning of text ? - same for the notified side... I'm sure that decomposing the dependency explicitly with tests would help reducing the complexity... That was one of my questions: - what if we change usage of ReadWriteStream on:from:to: in selectionAsStream? Since I wrote tests in Squeak and simply ported to Pharo, and since I missed CompilerTest (which is in Test-Compiler not in CompilerTests, why the hell having two packages?), we could push refactoring further. I made a class for testing a pair (Compiler notifying a SmalltalkEditor), and a subclass for testing another pair (Compiler notifying nil), but I think Pharo has very new capabilities, like working with console like I/O, so this is another variant to test, and maybe in this case splitting interdependency in two is not an over-engineered feature anymore ;) Nicolas Le 25 février 2012 14:23, Pavel Krivanek <[hidden email]> a écrit : > Great, The basic Pharo Kernel Jobs can be build again! Leaving aside > the fact that number of failing tests significantly increased because > someone made compiler tests dependent on TextMorph (setUpForErrorsIn:) > :-) > > -- Pavel > > On Sat, Feb 25, 2012 at 8:43 AM, Marcus Denker <[hidden email]> wrote: >> 14356 >> ----- >> >> Issue 5376: AbstractTool fix removeMethods method >> http://code.google.com/p/pharo/issues/detail?id=5376 >> >> Issue 5371: XTableForUnicodeFont is not used anywhere (and an internal class) >> http://code.google.com/p/pharo/issues/detail?id=5371 >> >> Issue 5372: Remove old Mac font reading in StrikeFont >> http://code.google.com/p/pharo/issues/detail?id=5372 >> >> Issue 5370: AppRegistry class >> #default: dependent on Morphic >> http://code.google.com/p/pharo/issues/detail?id=5370 >> >> >> >> -- >> Marcus Denker -- http://marcusdenker.de >> >> > |
|
|
And still, one thing that is no obvious with split interdependency, is
that the tests testing the two sides are inter-dependent. How do we express this interdependency? If we don't that let us free to modify one single side and break the contract. Nicolas Le 25 février 2012 15:37, Nicolas Cellier <[hidden email]> a écrit : > Ah yes, that was me, since one feature of the Compiler is to insert > notifications into source text editor, this has to be tested. > - does the Compiler insert the right notification ? > - does it insert the notification at the right place ? > > As soon as TextMorph and SmalltalkEditor provide a hook for > evaluating/compiling source, and a callback for notifications they > become inter-dependent. And we'd better write a test documenting the > interdependency. > > Of course, we can replace the TextEditor with a mock up, like the one > in CompilerTest and split the interdependency in two parts. > And then assert on the notifier side that: > - the compiler emit notifications with 1-based insertion point indices > (1 means insert before first character) > (Though I don't like the hardcoded positions in CompilerTest, it's not > readble, but could be changed). > > Right now, the notified side of interdependency is missing, I see no > test asserting that: > - SmalltalkEditor notify:at:in: insert notifications with 1-based indices > > If I change TextEditor insertion points to be 0-based, this will get > unnoticed... > > And this decomposition being less immediate than my simple tests, we > forgot one feature which is to evaluate a sub-selection. > This feature makes the interdependency much more complex > - is notifier aware of sub-selection location ? If yes, does it > provide a location related to beginning of sub-selection of beginning > of text ? > - same for the notified side... > > I'm sure that decomposing the dependency explicitly with tests would > help reducing the complexity... > That was one of my questions: > - what if we change usage of ReadWriteStream on:from:to: in selectionAsStream? > > Since I wrote tests in Squeak and simply ported to Pharo, and since I > missed CompilerTest (which is in Test-Compiler not in CompilerTests, > why the hell having two packages?), we could push refactoring further. > I made a class for testing a pair (Compiler notifying a > SmalltalkEditor), and a subclass for testing another pair (Compiler > notifying nil), but I think Pharo has very new capabilities, like > working with console like I/O, so this is another variant to test, and > maybe in this case splitting interdependency in two is not an > over-engineered feature anymore ;) > > Nicolas > > Le 25 février 2012 14:23, Pavel Krivanek <[hidden email]> a écrit : >> Great, The basic Pharo Kernel Jobs can be build again! Leaving aside >> the fact that number of failing tests significantly increased because >> someone made compiler tests dependent on TextMorph (setUpForErrorsIn:) >> :-) >> >> -- Pavel >> >> On Sat, Feb 25, 2012 at 8:43 AM, Marcus Denker <[hidden email]> wrote: >>> 14356 >>> ----- >>> >>> Issue 5376: AbstractTool fix removeMethods method >>> http://code.google.com/p/pharo/issues/detail?id=5376 >>> >>> Issue 5371: XTableForUnicodeFont is not used anywhere (and an internal class) >>> http://code.google.com/p/pharo/issues/detail?id=5371 >>> >>> Issue 5372: Remove old Mac font reading in StrikeFont >>> http://code.google.com/p/pharo/issues/detail?id=5372 >>> >>> Issue 5370: AppRegistry class >> #default: dependent on Morphic >>> http://code.google.com/p/pharo/issues/detail?id=5370 >>> >>> >>> >>> -- >>> Marcus Denker -- http://marcusdenker.de >>> >>> >> |
|
|
In reply to this post by Nicolas Cellier
The least we can do is to move such tests to a package that will not
be loaded. Something like CompilerToolsNotificationsTests or so... -- Pavel On Sat, Feb 25, 2012 at 3:37 PM, Nicolas Cellier <[hidden email]> wrote: > Ah yes, that was me, since one feature of the Compiler is to insert > notifications into source text editor, this has to be tested. > - does the Compiler insert the right notification ? > - does it insert the notification at the right place ? > > As soon as TextMorph and SmalltalkEditor provide a hook for > evaluating/compiling source, and a callback for notifications they > become inter-dependent. And we'd better write a test documenting the > interdependency. > > Of course, we can replace the TextEditor with a mock up, like the one > in CompilerTest and split the interdependency in two parts. > And then assert on the notifier side that: > - the compiler emit notifications with 1-based insertion point indices > (1 means insert before first character) > (Though I don't like the hardcoded positions in CompilerTest, it's not > readble, but could be changed). > > Right now, the notified side of interdependency is missing, I see no > test asserting that: > - SmalltalkEditor notify:at:in: insert notifications with 1-based indices > > If I change TextEditor insertion points to be 0-based, this will get > unnoticed... > > And this decomposition being less immediate than my simple tests, we > forgot one feature which is to evaluate a sub-selection. > This feature makes the interdependency much more complex > - is notifier aware of sub-selection location ? If yes, does it > provide a location related to beginning of sub-selection of beginning > of text ? > - same for the notified side... > > I'm sure that decomposing the dependency explicitly with tests would > help reducing the complexity... > That was one of my questions: > - what if we change usage of ReadWriteStream on:from:to: in selectionAsStream? > > Since I wrote tests in Squeak and simply ported to Pharo, and since I > missed CompilerTest (which is in Test-Compiler not in CompilerTests, > why the hell having two packages?), we could push refactoring further. > I made a class for testing a pair (Compiler notifying a > SmalltalkEditor), and a subclass for testing another pair (Compiler > notifying nil), but I think Pharo has very new capabilities, like > working with console like I/O, so this is another variant to test, and > maybe in this case splitting interdependency in two is not an > over-engineered feature anymore ;) > > Nicolas > > Le 25 février 2012 14:23, Pavel Krivanek <[hidden email]> a écrit : >> Great, The basic Pharo Kernel Jobs can be build again! Leaving aside >> the fact that number of failing tests significantly increased because >> someone made compiler tests dependent on TextMorph (setUpForErrorsIn:) >> :-) >> >> -- Pavel >> >> On Sat, Feb 25, 2012 at 8:43 AM, Marcus Denker <[hidden email]> wrote: >>> 14356 >>> ----- >>> >>> Issue 5376: AbstractTool fix removeMethods method >>> http://code.google.com/p/pharo/issues/detail?id=5376 >>> >>> Issue 5371: XTableForUnicodeFont is not used anywhere (and an internal class) >>> http://code.google.com/p/pharo/issues/detail?id=5371 >>> >>> Issue 5372: Remove old Mac font reading in StrikeFont >>> http://code.google.com/p/pharo/issues/detail?id=5372 >>> >>> Issue 5370: AppRegistry class >> #default: dependent on Morphic >>> http://code.google.com/p/pharo/issues/detail?id=5370 >>> >>> >>> >>> -- >>> Marcus Denker -- http://marcusdenker.de >>> >>> >> > |
«
Return to Pharo Smalltalk Developers
|
1 view|%1 views
| Free forum by Nabble | Edit this page |