Review needed - GROOVY-3844 (ASTBrowser issue)
|
View:
New views
7 Messages
—
Rating Filter:
Alert me
|
 |
Review needed - GROOVY-3844 (ASTBrowser issue)
by Roshan Dawrani-2
::
Rate this Message:
Hi Hamlet,
Earlier today I had raised an ASTBrowser related issue - GROOVY-3844 (which is about this browser not showing the free-standing methods of a script as it shows its free-standing statements). I have now prepared a fix for this issue but before applying it, I wanted to have it reviewed by you, if you have a few free minutes. I have attached the patch with the JIRA issue. One more thing I wanted to confirm with you was if there are unit tests for ASTBrowser functionality. There are no tests for this UI functionality, right? Thanks, Roshan |
|
|
Re: Review needed - GROOVY-3844 (ASTBrowser issue)
by HamletDRC
::
Rate this Message:
> One more thing I wanted to confirm with you was if there are unit tests for
> ASTBrowser functionality. There are no tests for this UI functionality, > right? Oh yes, there are indeed Unit Tests. Look at ScriptToTreeNodeAdapterTest. Looking at the text of the patch, it looks fine. (I didn't load it into IDEA). But please, if you would, add a unit test to ScriptToTreeNodeAdapterTest. You just need to call "assertASTTree" and pass it your sample script, and then a spec of what you want in the Tree. The other examples are fairly clear I think. I'm going to hack up ScriptToTreeNodeAdapter soon so that it can produce a TreeNode or a TextNode (for command line printing). -- Hamlet D'Arcy hamletdrc@... On Mon, Oct 26, 2009 at 10:04 AM, Roshan Dawrani <roshandawrani@...> wrote: > Hi Hamlet, > Earlier today I had raised an ASTBrowser related issue - GROOVY-3844 (which > is about this browser not showing the free-standing methods of a script as > it shows its free-standing statements). > > I have now prepared a fix for this issue but before applying it, I wanted to > have it reviewed by you, if you have a few free minutes. > > I have attached the patch with the JIRA issue. > > One more thing I wanted to confirm with you was if there are unit tests for > ASTBrowser functionality. There are no tests for this UI functionality, > right? > > Thanks, > Roshan > --------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email --
Hamlet D'Arcy |
|
|
Re: Review needed - GROOVY-3844 (ASTBrowser issue)
by Roshan Dawrani-2
::
Rate this Message:
Thanks for having a look the patch and pointing to ScriptToTreeNodeAdapterTest.
I will surely add a test there to cover the changes I am introducing. rgds, Roshan On Mon, Oct 26, 2009 at 9:38 PM, Hamlet D'Arcy <hamletdrc@...> wrote:
|
|
|
Re: Review needed - GROOVY-3844 (ASTBrowser issue)
by HamletDRC
::
Rate this Message:
> I will surely add a test there to cover the changes I am introducing.
I wouldn't go too far in adding UI test cases, but it is a nice little safety net to have some coverage. One thing to look out for: the TreeNode labels are pulled from AstBrowserProperties.groovy, which can be overridden in $HOME/.groovy ... this is why the assertions are "startsWith" instead of "eq" assertions. -- Hamlet D'Arcy hamletdrc@... On Mon, Oct 26, 2009 at 11:12 AM, Roshan Dawrani <roshandawrani@...> wrote: > Thanks for having a look the patch and pointing to > ScriptToTreeNodeAdapterTest. > > I will surely add a test there to cover the changes I am introducing. > > rgds, > Roshan > > On Mon, Oct 26, 2009 at 9:38 PM, Hamlet D'Arcy <hamletdrc@...> wrote: >> >> > One more thing I wanted to confirm with you was if there are unit tests >> > for >> > ASTBrowser functionality. There are no tests for this UI functionality, >> > right? >> >> Oh yes, there are indeed Unit Tests. Look at ScriptToTreeNodeAdapterTest. >> >> Looking at the text of the patch, it looks fine. (I didn't load it >> into IDEA). But please, if you would, add a unit test to >> ScriptToTreeNodeAdapterTest. You just need to call "assertASTTree" and >> pass it your sample script, and then a spec of what you want in the >> Tree. The other examples are fairly clear I think. >> >> I'm going to hack up ScriptToTreeNodeAdapter soon so that it can >> produce a TreeNode or a TextNode (for command line printing). >> >> -- >> Hamlet D'Arcy >> hamletdrc@... >> >> >> >> On Mon, Oct 26, 2009 at 10:04 AM, Roshan Dawrani >> <roshandawrani@...> wrote: >> > Hi Hamlet, >> > Earlier today I had raised an ASTBrowser related issue - GROOVY-3844 >> > (which >> > is about this browser not showing the free-standing methods of a script >> > as >> > it shows its free-standing statements). >> > >> > I have now prepared a fix for this issue but before applying it, I >> > wanted to >> > have it reviewed by you, if you have a few free minutes. >> > >> > I have attached the patch with the JIRA issue. >> > >> > One more thing I wanted to confirm with you was if there are unit tests >> > for >> > ASTBrowser functionality. There are no tests for this UI functionality, >> > right? >> > >> > Thanks, >> > Roshan >> > >> >> --------------------------------------------------------------------- >> To unsubscribe from this list, please visit: >> >> http://xircles.codehaus.org/manage_email >> >> > > --------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email --
Hamlet D'Arcy |
|
|
Re: Review needed - GROOVY-3844 (ASTBrowser issue)
by Roshan Dawrani-2
::
Rate this Message:
Here is the test I have added following the example of existing tests in there. Do you see any issues in it?
=================================== public void testScriptWithMethods() { // verify the free form script assertTreeStructure( "def foo(String bar) {}", [ eq('Methods'), eq('MethodNode - foo'), eq('Parameter - bar'), ] ) // verify the script's class assertTreeStructure( "def foo(String bar) {}", [ startsWith('ClassNode - script'), eq('Methods'), eq('MethodNode - foo'), eq('Parameter - bar'), ] ) } =================================== thanks, Roshan On Mon, Oct 26, 2009 at 10:37 PM, Hamlet D'Arcy <hamletdrc@...> wrote:
|
|
|
Re: Review needed - GROOVY-3844 (ASTBrowser issue)
by HamletDRC
::
Rate this Message:
> there. Do you see any issues in it?
That's how it be done sir! (Although I would move each assertion method to it's own test method so any failures clearly shows the culprit without consulting the line number, perhaps that makes me a testing zealot) -- Hamlet D'Arcy hamletdrc@... On Mon, Oct 26, 2009 at 12:09 PM, Roshan Dawrani <roshandawrani@...> wrote: > Here is the test I have added following the example of existing tests in > there. Do you see any issues in it? > =================================== > public void testScriptWithMethods() { > // verify the free form script > assertTreeStructure( > "def foo(String bar) {}", > [ > eq('Methods'), > eq('MethodNode - foo'), > eq('Parameter - bar'), > ] > ) > > // verify the script's class > assertTreeStructure( > "def foo(String bar) {}", > [ > startsWith('ClassNode - script'), > eq('Methods'), > eq('MethodNode - foo'), > eq('Parameter - bar'), > ] > ) > } > =================================== > > thanks, > Roshan > > > On Mon, Oct 26, 2009 at 10:37 PM, Hamlet D'Arcy <hamletdrc@...> wrote: >> >> > I will surely add a test there to cover the changes I am introducing. >> >> I wouldn't go too far in adding UI test cases, but it is a nice little >> safety net to have some coverage. >> >> One thing to look out for: the TreeNode labels are pulled from >> AstBrowserProperties.groovy, which can be overridden in $HOME/.groovy >> ... this is why the assertions are "startsWith" instead of "eq" >> assertions. >> >> -- >> Hamlet D'Arcy >> hamletdrc@... >> >> >> >> On Mon, Oct 26, 2009 at 11:12 AM, Roshan Dawrani >> <roshandawrani@...> wrote: >> > Thanks for having a look the patch and pointing to >> > ScriptToTreeNodeAdapterTest. >> > >> > I will surely add a test there to cover the changes I am introducing. >> > >> > rgds, >> > Roshan >> > >> > On Mon, Oct 26, 2009 at 9:38 PM, Hamlet D'Arcy <hamletdrc@...> >> > wrote: >> >> >> >> > One more thing I wanted to confirm with you was if there are unit >> >> > tests >> >> > for >> >> > ASTBrowser functionality. There are no tests for this UI >> >> > functionality, >> >> > right? >> >> >> >> Oh yes, there are indeed Unit Tests. Look at >> >> ScriptToTreeNodeAdapterTest. >> >> >> >> Looking at the text of the patch, it looks fine. (I didn't load it >> >> into IDEA). But please, if you would, add a unit test to >> >> ScriptToTreeNodeAdapterTest. You just need to call "assertASTTree" and >> >> pass it your sample script, and then a spec of what you want in the >> >> Tree. The other examples are fairly clear I think. >> >> >> >> I'm going to hack up ScriptToTreeNodeAdapter soon so that it can >> >> produce a TreeNode or a TextNode (for command line printing). >> >> >> >> -- >> >> Hamlet D'Arcy >> >> hamletdrc@... >> >> >> >> >> >> >> >> On Mon, Oct 26, 2009 at 10:04 AM, Roshan Dawrani >> >> <roshandawrani@...> wrote: >> >> > Hi Hamlet, >> >> > Earlier today I had raised an ASTBrowser related issue - GROOVY-3844 >> >> > (which >> >> > is about this browser not showing the free-standing methods of a >> >> > script >> >> > as >> >> > it shows its free-standing statements). >> >> > >> >> > I have now prepared a fix for this issue but before applying it, I >> >> > wanted to >> >> > have it reviewed by you, if you have a few free minutes. >> >> > >> >> > I have attached the patch with the JIRA issue. >> >> > >> >> > One more thing I wanted to confirm with you was if there are unit >> >> > tests >> >> > for >> >> > ASTBrowser functionality. There are no tests for this UI >> >> > functionality, >> >> > right? >> >> > >> >> > Thanks, >> >> > Roshan >> >> > >> >> >> >> --------------------------------------------------------------------- >> >> To unsubscribe from this list, please visit: >> >> >> >> http://xircles.codehaus.org/manage_email >> >> >> >> >> > >> > >> >> --------------------------------------------------------------------- >> To unsubscribe from this list, please visit: >> >> http://xircles.codehaus.org/manage_email >> >> > > --------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email --
Hamlet D'Arcy |
|
|
Re: Review needed - GROOVY-3844 (ASTBrowser issue)
by Roshan Dawrani-2
::
Rate this Message:
Thanks a lot for your quick assistance on all my queries.
rgds, Roshan On Mon, Oct 26, 2009 at 10:56 PM, Hamlet D'Arcy <hamletdrc@...> wrote:
|
| Free embeddable forum powered by Nabble | Forum Help |
