Skip to main content

Please Review: fix for [Issue 106] Specific MIDletGUIAgent for test export mode

18 replies [Last post]
Anonymous

Hi all,
please review the fix for [Issue 106] Specific MIDletGUIAgent for test
export mode
https://cqme.dev.java.net/issues/show_bug.cgi?id=106

The changeset: http://fisheye4.cenqua.com/changelog/cqme?cs=921

Also there are some possible features to implement in later releases.
I'm going to file enhancement about this. All comments are welcome.

- May be we need question about interactive agent in the export tests
interview.
- Have the same GUI for the case when previous test results were found
and for the case with no results
- Main screen - current results statistics (tests done, passed,
failed), zeros if no results in RMS
- Title "Tests run finished" or "Previous test
results were found in RMS" or "Test agent"
- Command Exit
- Command Run Tests
- Command View results (exists if there are
results)
- Command clear RMS (exists if there are smth
in RMS)
- Command Settings (to switch on/off writing to
RMS)
- Commands Cancel Test/Bundle if tests are running
- Command Save Status if tests are running
- Tests results tree usability
Up command in the tree
Disable select command on log (substitute by up command)
Show tests statistics in title of tree
Sort results alphabetically
- Create library code to read from RMS (and use it in
ExportMIDletGUIAgent and MIDletRMSReader)

--
Maxim Dolidze (mailto:Maxim.Dolidze@sun.com)

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Reply viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
Maxim Dolidze

All code review issues were fixed. Please review the change
http://fisheye4.cenqua.com/changelog/cqme?cs=970

And see some comments inline.

Maxim Dolidze wrote:
> Vladimir, Alexander,
> thanks for review. Most of your comments I've fixed.
> http://fisheye4.cenqua.com/changelog/cqme?cs=947
>
> For the rest read my comments inline (I've copied both your answers in
> this letter)
>
> Vladimir Sizikov wrote:
>> Minor points:
>>
>> 1. manifest_export_gui
>>
>> a) I wonder, why we keep using this awkward naming convention for
>> manifests. Why ExportMidletGUIAgent.manifest is no good? :) Or,
>> maybe midp_export_guiagent.manifest.
>>
> Actually I have no strong position about this item. Both variants
> (current and proposed by Vladimir) looks good for me. But if finally
> decide to change to .manifest format we should change
> all manifests, shouldn't we?
postponed.
>> b) MIDlet-Name: MIDletAgent, why? Wouldn't it be better to provide
>> more descriptive name?
>>
Done. New name is ExportMIDletGUIAgent.
>
>> On Thu, May 17, 2007 at 08:32:16AM -0700, Vladimir Sizikov wrote:
>>
>>> Hi Maxim,
>>>
>>> 4. MIDletGUIAgent:
>>> d) I'd really like to avoid numbers in method names:
>>> getRecordStore4Read
>>> How about just openRecordStore()?
>>>
> openRecordStore() is not a good solution because in conjunction with
> getRecordStore() it looks confusing.
> So I make them getOutputRecordStore() and getInputRecordStore(). It is
> a little bit strange for whose who know what RecordStore is but quite
> alike java.io.* style.
>>> 5. GenericMIDletAgent:
>>> a) I'd suggest to rename startRun --> startTestExecution.
>>> StartRun is too generic.
>>> b) startApp() javadoc should mention that the default
>>> implementation just invokes
>>> startTestExecution, but the subclasses can provide more
>>> complex behavior.
>>> c) startTestExecution()'s javadoc should be more clear: "Starts
>>> the working thread that
>>> executes the tests via {@link #run()}".
>>> d) doPostRunAction() should be renamed to something more clear.
>>> How about onTestsFinished()?
>>> This clearly shows that it's a "hook", notification method
>>> that can be overriden by subclasses.
>>> The javadoc for this method should be updated as well, stating
>>> that this is a notification method,
>>> that is invoked on the test execution thread when the test
>>> execution is complete. By default, invokes
>>> notifyDestroyed(), but the subclasses can override to provide
>>> more complex behavior and do some cleanup.
Done.
>>> Thanks,
>>> --Vladimir
> Alexander Alexeev wrote:
>
>> 3. I don't like the resultant design of GenericMIDletAgent. Now we have
>> similar methods startApp() and startRun() thats rather confusing. And
>> ExportMIDletGUIAgent overrides startApp() where makes many actions. But
>> the call of startApp() should consume short time. This is statement from
>> spec: "It is intended that these methods return quickly."
>> So I propose to introduce method startTestExecution() called inside
>> run() method of GenericMIDletAgent. MIDletGUIAgent will override it to
>> delete record store and ExportMIDletGUIAgent for check RMS.
> Vladimir, Alexander, both your suggestions looks reasonable. I'll try
> to make some refactoring of GenericMIDletAgent to have same clear
> architecture for batch, passive and active GUI mode.
Partly done. Add method GenericMIDletAgent.onTestsStarted().
>
>>
>> 4. I think more appropriate to add method removeAction() in MenuHadler
>> instead of remove commands from Form directly.
> Good idea. Try to do it.
Done.
>>
>> 5. Folks, what do you think about make this agent default for the export
>> mode?
> I don't think it is right decision. Customers may want to have
> lightweight exported tests for batch mode run.
Declined.
>>
>> Thanks,
>> Alexander
>
>

--
Maxim Dolidze (mailto:Maxim.Dolidze@sun.com)

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Alexander Alexeev

Hi Maxim,

overall looks good, but why to introduce three new API methods to generic agent
if we can do it with one method called inside run() ;-)? Also startApp() of
ExportMIDletGUIAgent consumes much time still.

Thanks,
Alexander

Maxim Dolidze wrote:
> All code review issues were fixed. Please review the change
> http://fisheye4.cenqua.com/changelog/cqme?cs=970
>
> And see some comments inline.
>
>
> Maxim Dolidze wrote:
>> Vladimir, Alexander,
>> thanks for review. Most of your comments I've fixed.
>> http://fisheye4.cenqua.com/changelog/cqme?cs=947
>>
>> For the rest read my comments inline (I've copied both your answers in
>> this letter)
>>
>> Vladimir Sizikov wrote:
>>> Minor points:
>>>
>>> 1. manifest_export_gui
>>>
>>> a) I wonder, why we keep using this awkward naming convention for
>>> manifests. Why ExportMidletGUIAgent.manifest is no good? :) Or,
>>> maybe midp_export_guiagent.manifest.
>>>
>> Actually I have no strong position about this item. Both variants
>> (current and proposed by Vladimir) looks good for me. But if finally
>> decide to change to .manifest format we should change
>> all manifests, shouldn't we?
> postponed.
>>> b) MIDlet-Name: MIDletAgent, why? Wouldn't it be better to provide
>>> more descriptive name?
>>>
> Done. New name is ExportMIDletGUIAgent.
>>
>>> On Thu, May 17, 2007 at 08:32:16AM -0700, Vladimir Sizikov wrote:
>>>
>>>> Hi Maxim,
>>>>
>>>> 4. MIDletGUIAgent:
>>>> d) I'd really like to avoid numbers in method names:
>>>> getRecordStore4Read
>>>> How about just openRecordStore()?
>>>>
>> openRecordStore() is not a good solution because in conjunction with
>> getRecordStore() it looks confusing.
>> So I make them getOutputRecordStore() and getInputRecordStore(). It is
>> a little bit strange for whose who know what RecordStore is but quite
>> alike java.io.* style.
>>>> 5. GenericMIDletAgent:
>>>> a) I'd suggest to rename startRun --> startTestExecution.
>>>> StartRun is too generic.
>>>> b) startApp() javadoc should mention that the default
>>>> implementation just invokes
>>>> startTestExecution, but the subclasses can provide more
>>>> complex behavior.
>>>> c) startTestExecution()'s javadoc should be more clear: "Starts
>>>> the working thread that
>>>> executes the tests via {@link #run()}".
>>>> d) doPostRunAction() should be renamed to something more clear.
>>>> How about onTestsFinished()?
>>>> This clearly shows that it's a "hook", notification method
>>>> that can be overriden by subclasses.
>>>> The javadoc for this method should be updated as well, stating
>>>> that this is a notification method,
>>>> that is invoked on the test execution thread when the test
>>>> execution is complete. By default, invokes
>>>> notifyDestroyed(), but the subclasses can override to provide
>>>> more complex behavior and do some cleanup.
> Done.
>>>> Thanks,
>>>> --Vladimir
>> Alexander Alexeev wrote:
>>
>>> 3. I don't like the resultant design of GenericMIDletAgent. Now we have
>>> similar methods startApp() and startRun() thats rather confusing. And
>>> ExportMIDletGUIAgent overrides startApp() where makes many actions. But
>>> the call of startApp() should consume short time. This is statement from
>>> spec: "It is intended that these methods return quickly."
>>> So I propose to introduce method startTestExecution() called inside
>>> run() method of GenericMIDletAgent. MIDletGUIAgent will override it to
>>> delete record store and ExportMIDletGUIAgent for check RMS.
>> Vladimir, Alexander, both your suggestions looks reasonable. I'll try
>> to make some refactoring of GenericMIDletAgent to have same clear
>> architecture for batch, passive and active GUI mode.
> Partly done. Add method GenericMIDletAgent.onTestsStarted().
>>
>>>
>>> 4. I think more appropriate to add method removeAction() in MenuHadler
>>> instead of remove commands from Form directly.
>> Good idea. Try to do it.
> Done.
>>>
>>> 5. Folks, what do you think about make this agent default for the export
>>> mode?
>> I don't think it is right decision. Customers may want to have
>> lightweight exported tests for batch mode run.
> Declined.
>>>
>>> Thanks,
>>> Alexander
>>
>>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Vladimir Sizikov

Hi Maxim, Alexander,

I actually like the end result. The API is logical and non-surprising,
and flexible. Maxim, thanks a lot for good contribution! :)

I'm OK with committing this to trunk.

One comment below.

On Wed, May 23, 2007 at 05:21:35PM +0400, Alexander Alexeev wrote:
> overall looks good, but why to introduce three new API methods to generic
> agent
> if we can do it with one method called inside run() ;-)?

:) We cannot get away without no new methods, so the three currently
added ones are simple, clearly defined and logical.

> Also startApp() of
> ExportMIDletGUIAgent consumes much time still.

Can you explain what do you mean by that?

Alexander, could you talk to Maxim, since you're in the same building
and close this remaining issue, so that the code could be committed.

Thanks,
--Vladimir

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Maxim Dolidze

[att1.html]

Vladimir Sizikov

Maxim,

On Wed, May 23, 2007 at 06:11:09PM +0400, Maxim Dolidze wrote:
> Alexander already left the office but before it we have a talk. As
> far as I understand Alexander want to move code from if-branch of
> ExportMIDletGUIAgent.startApp() to a thread. He reference to MIDlet
> spec "...It is intended that these methods return quickly." I'm not
> sure it worth creating another thread.

Agree, we cannot do anything useful until the RMS is being read and
the tree of results is constructed. And the read is not supposed to be
_that_ long anyways. For non-consumer apps like our small test results
reader, this should not be a big issue.

So, I suggest to commit the changes to trunk. And if Alexander still
thinks that a new Thread is really necessary, we can add it later, as
an enhancement.

Thanks,
--Vladimir

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Alexander Alexeev

Hi guys,

seems I need to express my opinion about this issue. My objection in the least
concern the reading RMS in startApp(). But look at the code. *Three* new public
methods added and they are not so logical as it would like. For example
onTestFinished() is called always - even in case of Throwable. I don't think any
agent should continue his work in case of internal JVM exception.

So please see that I propose: http://fisheye4.cenqua.com/changelog/cqme/?cs=1004
Only one method added and from my point of view it simple and logical. Also
issues with RMS reading and working after Throwable is gone away.

Thanks,
Alexander

Vladimir Sizikov wrote:
> Maxim,
>
> On Wed, May 23, 2007 at 06:11:09PM +0400, Maxim Dolidze wrote:
>> Alexander already left the office but before it we have a talk. As
>> far as I understand Alexander want to move code from if-branch of
>> ExportMIDletGUIAgent.startApp() to a thread. He reference to MIDlet
>> spec "...It is intended that these methods return quickly." I'm not
>> sure it worth creating another thread.
>
> Agree, we cannot do anything useful until the RMS is being read and
> the tree of results is constructed. And the read is not supposed to be
> _that_ long anyways. For non-consumer apps like our small test results
> reader, this should not be a big issue.
>
> So, I suggest to commit the changes to trunk. And if Alexander still
> thinks that a new Thread is really necessary, we can add it later, as
> an enhancement.
>
> Thanks,
> --Vladimir
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Vladimir Sizikov

Hi Alexander,

Thanks a lot for expressing your opinion and providing the proposed
solution. It's always good to compare ideas and try to select the best
one! :)

My comments below.

On Thu, May 24, 2007 at 04:22:53PM +0400, Alexander Alexeev wrote:
> seems I need to express my opinion about this issue. My objection in the
> least
> concern the reading RMS in startApp(). But look at the code. *Three* new
> public

I appreciate very much this attention to the public API and its size,
and agree that we should not grow it without real need.

Actually, we can make all the new introduced methods package-private,
and not expose them to users at all, and I think that's what we should
do, no matter which way we go (with one ore three methods).

> methods added and they are not so logical as it would like. For
> example onTestFinished() is called always - even in case of
> Throwable. I don't think any agent should continue his work in case
> of internal JVM exception.

While the agent might stop, but that hook could be used for cleanup,
not for extra work.

In general, I find that try-finally is really under-used in our code,
especially where we work with resources that should be cleaned later.

> So please see that I propose:
> http://fisheye4.cenqua.com/changelog/cqme/?cs=1004
> Only one method added and from my point of view it simple and logical. Also
> issues with RMS reading and working after Throwable is gone away.

Very good! I like this. Lots of stuff is removed, and as with any
removal, this is a good feeling. :)

There are few remaining issues:

1.
First, MIDletGUIAgent should not override run() (in order to delete
the record store), but rather runTests(). Also, closeRecordStore()
should be invoke after the tests are finished:

runTests:
deleteRecordStore()
try {
super.runTests()
} finally {
closeRecordStore()
}

2. No javadoc for runTests(). We should make it package-private and
still make some comment.

3. In GenericMIDletAgent, we invoke notifyDestroyed() at the end, but
then destroyApp() will not be invoked, as specified in the
spec. Should we explicitly invoke destroyApp() ourselves?

Thanks,
--Vladimir

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Alexander Alexeev

Hi Vladimir,

I performed some changes based on your comments. Please see:
http://fisheye4.cenqua.com/changelog/cqme/?cs=1005
http://fisheye4.cenqua.com/changelog/cqme/?cs=1006

Thanks,
Alexander

Vladimir Sizikov wrote:
> Hi Alexander,
>
> Thanks a lot for expressing your opinion and providing the proposed
> solution. It's always good to compare ideas and try to select the best
> one! :)
>
> My comments below.
>
> On Thu, May 24, 2007 at 04:22:53PM +0400, Alexander Alexeev wrote:
>> seems I need to express my opinion about this issue. My objection in the
>> least
>> concern the reading RMS in startApp(). But look at the code. *Three* new
>> public
>
> I appreciate very much this attention to the public API and its size,
> and agree that we should not grow it without real need.
>
> Actually, we can make all the new introduced methods package-private,
> and not expose them to users at all, and I think that's what we should
> do, no matter which way we go (with one ore three methods).
>
>> methods added and they are not so logical as it would like. For
>> example onTestFinished() is called always - even in case of
>> Throwable. I don't think any agent should continue his work in case
>> of internal JVM exception.
>
> While the agent might stop, but that hook could be used for cleanup,
> not for extra work.
>
> In general, I find that try-finally is really under-used in our code,
> especially where we work with resources that should be cleaned later.
>
>> So please see that I propose:
>> http://fisheye4.cenqua.com/changelog/cqme/?cs=1004
>> Only one method added and from my point of view it simple and logical. Also
>> issues with RMS reading and working after Throwable is gone away.
>
> Very good! I like this. Lots of stuff is removed, and as with any
> removal, this is a good feeling. :)
>
> There are few remaining issues:
>
> 1.
> First, MIDletGUIAgent should not override run() (in order to delete
> the record store), but rather runTests(). Also, closeRecordStore()
> should be invoke after the tests are finished:
>
>
> runTests:
> deleteRecordStore()
> try {
> super.runTests()
> } finally {
> closeRecordStore()
> }
>
> 2. No javadoc for runTests(). We should make it package-private and
> still make some comment.
>
> 3. In GenericMIDletAgent, we invoke notifyDestroyed() at the end, but
> then destroyApp() will not be invoked, as specified in the
> spec. Should we explicitly invoke destroyApp() ourselves?
>
> Thanks,
> --Vladimir
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Vladimir Sizikov

Alexander,

Looks good. Once you've verified that the changes work, feel free to
commit.

Thanks,
--Vladimir

On Thu, May 24, 2007 at 07:15:43PM +0400, Alexander Alexeev wrote:
> Hi Vladimir,
>
> I performed some changes based on your comments. Please see:
> http://fisheye4.cenqua.com/changelog/cqme/?cs=1005
> http://fisheye4.cenqua.com/changelog/cqme/?cs=1006
>
> Thanks,
> Alexander

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Alexander Alexeev

Vladimir,

sorry, it was missed from my mind the method executeTests() need to be
made package-protected. Please review changes:
http://fisheye4.cenqua.com/changelog/cqme/?cs=1008

Also I've checked everything works fine.

Thanks,
Alexander

Vladimir Sizikov wrote:
> Alexander,
>
> Looks good. Once you've verified that the changes work, feel free to
> commit.
>
> Thanks,
> --Vladimir
>
> On Thu, May 24, 2007 at 07:15:43PM +0400, Alexander Alexeev wrote:
>> Hi Vladimir,
>>
>> I performed some changes based on your comments. Please see:
>> http://fisheye4.cenqua.com/changelog/cqme/?cs=1005
>> http://fisheye4.cenqua.com/changelog/cqme/?cs=1006
>>
>> Thanks,
>> Alexander
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Vladimir Sizikov

Alexander,

Looks good. Please commit.

--VVS

On Thu, May 24, 2007 at 08:55:57PM +0400, Alexander Alexeev wrote:
> Vladimir,
>
> sorry, it was missed from my mind the method executeTests() need to be
> made package-protected. Please review changes:
> http://fisheye4.cenqua.com/changelog/cqme/?cs=1008
>
> Also I've checked everything works fine.

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Alexander Alexeev

Hi Maxim,

there are my comments:

1. As already mentioned by Vladimir we have a huge code duplication in
filelists.mk. Ideally agents file lists should be organized as
hierarchy,i.e.:
JAVAFILES.cldc_agent = \
...
JAVAFILES.midp_generic_agent = \
${JAVAFILES.cldc_agent} \
...
JAVAFILES.midp_agent10 = \
${JAVAFILES.midp_generic_agent} \
...
and so on.

2. I propose do not use class name or use this.getClass() in this
message:
warning = "Uncaught exception in GenericMIDletAgent.run():
"+re+"\n";

3. I don't like the resultant design of GenericMIDletAgent. Now we have
similar methods startApp() and startRun() thats rather confusing. And
ExportMIDletGUIAgent overrides startApp() where makes many actions. But
the call of startApp() should consume short time. This is statement from
spec: "It is intended that these methods return quickly."
So I propose to introduce method startTestExecution() called inside
run() method of GenericMIDletAgent. MIDletGUIAgent will override it to
delete record store and ExportMIDletGUIAgent for check RMS.

4. I think more appropriate to add method removeAction() in MenuHadler
instead of remove commands from Form directly.

5. Folks, what do you think about make this agent default for the export
mode?

Thats all from my side.

Thanks,
Alexander

Maxim Dolidze wrote:
> Hi all,
> please review the fix for [Issue 106] Specific MIDletGUIAgent for test
> export mode
> https://cqme.dev.java.net/issues/show_bug.cgi?id=106
>
> The changeset: http://fisheye4.cenqua.com/changelog/cqme?cs=921
>
>
>
> Also there are some possible features to implement in later releases.
> I'm going to file enhancement about this. All comments are welcome.
>
> - May be we need question about interactive agent in the export tests
> interview.
> - Have the same GUI for the case when previous test results were found
> and for the case with no results
> - Main screen - current results statistics (tests done, passed,
> failed), zeros if no results in RMS
> - Title "Tests run finished" or "Previous test
> results were found in RMS" or "Test agent"
> - Command Exit
> - Command Run Tests
> - Command View results (exists if there are
> results)
> - Command clear RMS (exists if there are smth in
> RMS)
> - Command Settings (to switch on/off writing to
> RMS)
> - Commands Cancel Test/Bundle if tests are running
> - Command Save Status if tests are running
> - Tests results tree usability
> Up command in the tree
> Disable select command on log (substitute by up command)
> Show tests statistics in title of tree
> Sort results alphabetically
> - Create library code to read from RMS (and use it in
> ExportMIDletGUIAgent and MIDletRMSReader)
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Vladimir Sizikov

Hi Maxim,

Thank you very much for all the efforts you've put into this nice feature.
I've played with it a little, and really like it so far.

Please find below some of the comments. (More comments to follow).

I would like to ask Alexander Alexeev to take a deeper look at changes
in GUIAgent, just in case (my comments on this class are below).

0. Most of the changes in the build are OK.

1. filelists.mk

It's better to include filesets previously defined, such as:
JAVAFILES.midp_export_guiagent = ${JAVAFILES.midp_guiagent} \
...

Otherwise, we'll end up with LOTS of code duplication.

2. When previous results are found, the menu contains "Results". It would be better to have
"View saved results".

3. It would be good if "select" button (in the middle of jog dial) would work,
not only the soft button.

4. MIDletGUIAgent:
a) @inheritDocs --> {@inheritDocs} in couple of places
b) atrings --> strings
c) "Opens agent record store" --> "Opens the agent record store".
d) I'd really like to avoid numbers in method names: getRecordStore4Read
How about just openRecordStore()?
e) Some code style issues: lines longer 80 chars, spaces in "catch ( Exception",
should be "catch (Exception".

5. GenericMIDletAgent:
a) I'd suggest to rename startRun --> startTestExecution. StartRun is too generic.
b) startApp() javadoc should mention that the default implementation just invokes
startTestExecution, but the subclasses can provide more complex behavior.
c) startTestExecution()'s javadoc should be more clear: "Starts the working thread that
executes the tests via {@link #run()}".
d) doPostRunAction() should be renamed to something more clear. How about onTestsFinished()?
This clearly shows that it's a "hook", notification method that can be overriden by subclasses.
The javadoc for this method should be updated as well, stating that this is a notification method,
that is invoked on the test execution thread when the test execution is complete. By default, invokes
notifyDestroyed(), but the subclasses can override to provide more complex behavior and do some cleanup.
e) Typos:
- disigned
- stuctures
- @inheritDoc --> {@inheritDoc}
- If found fills --> If found, fills
f) Java style: A lot of places where ( something ), while should be (something) -- no spaces.
g) Class level comment is better to be updated:
/**
* Subclass of a MIDletGUIAgent designed for the export tests mode.
* This MIDlet doesn't terminate after all tests are executed, but
* rather shows the tests results in the test results tree. Also, it
* can read and show the test results of the previous run, if those
* results were stored in the RMS.
*/

Some more comments in the next email.

Thanks,
--Vladimir

On Thu, May 17, 2007 at 02:47:08PM +0400, Maxim Dolidze wrote:
> Hi all,
> please review the fix for [Issue 106] Specific MIDletGUIAgent for test
> export mode
> https://cqme.dev.java.net/issues/show_bug.cgi?id=106
>
> The changeset: http://fisheye4.cenqua.com/changelog/cqme?cs=921
>
>
>
> Also there are some possible features to implement in later releases.
> I'm going to file enhancement about this. All comments are welcome.
>
> - May be we need question about interactive agent in the export tests
> interview.
> - Have the same GUI for the case when previous test results were found
> and for the case with no results
> - Main screen - current results statistics (tests done, passed,
> failed), zeros if no results in RMS
> - Title "Tests run finished" or "Previous test
> results were found in RMS" or "Test agent"
> - Command Exit
> - Command Run Tests
> - Command View results (exists if there are
> results)
> - Command clear RMS (exists if there are smth
> in RMS)
> - Command Settings (to switch on/off writing to
> RMS)
> - Commands Cancel Test/Bundle if tests are running
> - Command Save Status if tests are running
> - Tests results tree usability
> Up command in the tree
> Disable select command on log (substitute by up command)
> Show tests statistics in title of tree
> Sort results alphabetically
> - Create library code to read from RMS (and use it in
> ExportMIDletGUIAgent and MIDletRMSReader)
>
> --
> Maxim Dolidze (mailto:Maxim.Dolidze@sun.com)
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Vladimir Sizikov

Hi Maxim,

I've finished the review.
Sorry, I mixed some file names in my previous message (at the end, but
it's easy to detect from the context). Mostly I was talking about
ExportMIDletGUIAgent, still in he section titled
GenericMIDletAgent. :)

Here's the second part of my comments:

Big general comment. Since the functionality provided by the new
utility classes like SimpleStringTokenizer, TestLogGui, TestLog,
TestLogTree, is really just an internal implementation detail used
only within the single package, I'd suggest to make all these classes
package-private. That way, they will not be a part of public API (and
hence we could more safely improve and change them in the future
without the fear of breaking the public API compatibility).

Minor points:

1. manifest_export_gui

a) I wonder, why we keep using this awkward naming convention for
manifests. Why ExportMidletGUIAgent.manifest is no good? :) Or,
maybe midp_export_guiagent.manifest.

b) MIDlet-Name: MIDletAgent, why? Wouldn't it be better to provide
more descriptive name?

2. TestLogTree:
Typos:
- intergared
- Finishs adding --> Finishes the test results addition process.

Thanks,
--Vladimir

On Thu, May 17, 2007 at 08:32:16AM -0700, Vladimir Sizikov wrote:
> Hi Maxim,
>
> Thank you very much for all the efforts you've put into this nice feature.
> I've played with it a little, and really like it so far.
>
> Please find below some of the comments. (More comments to follow).
>
> I would like to ask Alexander Alexeev to take a deeper look at changes
> in GUIAgent, just in case (my comments on this class are below).
>
> 0. Most of the changes in the build are OK.
>
> 1. filelists.mk
>
> It's better to include filesets previously defined, such as:
> JAVAFILES.midp_export_guiagent = ${JAVAFILES.midp_guiagent} \
> ...
>
> Otherwise, we'll end up with LOTS of code duplication.
>
> 2. When previous results are found, the menu contains "Results". It would be better to have
> "View saved results".
>
> 3. It would be good if "select" button (in the middle of jog dial) would work,
> not only the soft button.
>
> 4. MIDletGUIAgent:
> a) @inheritDocs --> {@inheritDocs} in couple of places
> b) atrings --> strings
> c) "Opens agent record store" --> "Opens the agent record store".
> d) I'd really like to avoid numbers in method names: getRecordStore4Read
> How about just openRecordStore()?
> e) Some code style issues: lines longer 80 chars, spaces in "catch ( Exception",
> should be "catch (Exception".
>
> 5. GenericMIDletAgent:
> a) I'd suggest to rename startRun --> startTestExecution. StartRun is too generic.
> b) startApp() javadoc should mention that the default implementation just invokes
> startTestExecution, but the subclasses can provide more complex behavior.
> c) startTestExecution()'s javadoc should be more clear: "Starts the working thread that
> executes the tests via {@link #run()}".
> d) doPostRunAction() should be renamed to something more clear. How about onTestsFinished()?
> This clearly shows that it's a "hook", notification method that can be overriden by subclasses.
> The javadoc for this method should be updated as well, stating that this is a notification method,
> that is invoked on the test execution thread when the test execution is complete. By default, invokes
> notifyDestroyed(), but the subclasses can override to provide more complex behavior and do some cleanup.
> e) Typos:
> - disigned
> - stuctures
> - @inheritDoc --> {@inheritDoc}
> - If found fills --> If found, fills
> f) Java style: A lot of places where ( something ), while should be (something) -- no spaces.
> g) Class level comment is better to be updated:
> /**
> * Subclass of a MIDletGUIAgent designed for the export tests mode.
> * This MIDlet doesn't terminate after all tests are executed, but
> * rather shows the tests results in the test results tree. Also, it
> * can read and show the test results of the previous run, if those
> * results were stored in the RMS.
> */
>
> Some more comments in the next email.
>
> Thanks,
> --Vladimir
>
> On Thu, May 17, 2007 at 02:47:08PM +0400, Maxim Dolidze wrote:
> > Hi all,
> > please review the fix for [Issue 106] Specific MIDletGUIAgent for test
> > export mode
> > https://cqme.dev.java.net/issues/show_bug.cgi?id=106
> >
> > The changeset: http://fisheye4.cenqua.com/changelog/cqme?cs=921
> >
> >
> >
> > Also there are some possible features to implement in later releases.
> > I'm going to file enhancement about this. All comments are welcome.
> >
> > - May be we need question about interactive agent in the export tests
> > interview.
> > - Have the same GUI for the case when previous test results were found
> > and for the case with no results
> > - Main screen - current results statistics (tests done, passed,
> > failed), zeros if no results in RMS
> > - Title "Tests run finished" or "Previous test
> > results were found in RMS" or "Test agent"
> > - Command Exit
> > - Command Run Tests
> > - Command View results (exists if there are
> > results)
> > - Command clear RMS (exists if there are smth
> > in RMS)
> > - Command Settings (to switch on/off writing to
> > RMS)
> > - Commands Cancel Test/Bundle if tests are running
> > - Command Save Status if tests are running
> > - Tests results tree usability
> > Up command in the tree
> > Disable select command on log (substitute by up command)
> > Show tests statistics in title of tree
> > Sort results alphabetically
> > - Create library code to read from RMS (and use it in
> > ExportMIDletGUIAgent and MIDletRMSReader)
> >
> > --
> > Maxim Dolidze (mailto:Maxim.Dolidze@sun.com)
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Maxim Dolidze

Vladimir, Alexander,
thanks for review. Most of your comments I've fixed.
http://fisheye4.cenqua.com/changelog/cqme?cs=947

For the rest read my comments inline (I've copied both your answers in
this letter)

Vladimir Sizikov wrote:
> Minor points:
>
> 1. manifest_export_gui
>
> a) I wonder, why we keep using this awkward naming convention for
> manifests. Why ExportMidletGUIAgent.manifest is no good? :) Or,
> maybe midp_export_guiagent.manifest.
>
Actually I have no strong position about this item. Both variants
(current and proposed by Vladimir) looks good for me. But if finally
decide to change to .manifest format we should change all
manifests, shouldn't we?
> b) MIDlet-Name: MIDletAgent, why? Wouldn't it be better to provide
> more descriptive name?
>
I don't like this name too. But in my opinion we MIDlet-Name in the
exported test must be . The only problem is that I
don't know for now how to implement this :-)

> On Thu, May 17, 2007 at 08:32:16AM -0700, Vladimir Sizikov wrote:
>
>> Hi Maxim,
>>
>> 4. MIDletGUIAgent:
>> d) I'd really like to avoid numbers in method names: getRecordStore4Read
>> How about just openRecordStore()?
>>
openRecordStore() is not a good solution because in conjunction with
getRecordStore() it looks confusing.
So I make them getOutputRecordStore() and getInputRecordStore(). It is a
little bit strange for whose who know what RecordStore is but quite
alike java.io.* style.
>>
>> 5. GenericMIDletAgent:
>> a) I'd suggest to rename startRun --> startTestExecution. StartRun is too generic.
>> b) startApp() javadoc should mention that the default implementation just invokes
>> startTestExecution, but the subclasses can provide more complex behavior.
>> c) startTestExecution()'s javadoc should be more clear: "Starts the working thread that
>> executes the tests via {@link #run()}".
>> d) doPostRunAction() should be renamed to something more clear. How about onTestsFinished()?
>> This clearly shows that it's a "hook", notification method that can be overriden by subclasses.
>> The javadoc for this method should be updated as well, stating that this is a notification method,
>> that is invoked on the test execution thread when the test execution is complete. By default, invokes
>> notifyDestroyed(), but the subclasses can override to provide more complex behavior and do some cleanup.
>> Thanks,
>> --Vladimir
Alexander Alexeev wrote:

> 3. I don't like the resultant design of GenericMIDletAgent. Now we have
> similar methods startApp() and startRun() thats rather confusing. And
> ExportMIDletGUIAgent overrides startApp() where makes many actions. But
> the call of startApp() should consume short time. This is statement from
> spec: "It is intended that these methods return quickly."
> So I propose to introduce method startTestExecution() called inside
> run() method of GenericMIDletAgent. MIDletGUIAgent will override it to
> delete record store and ExportMIDletGUIAgent for check RMS.
Vladimir, Alexander, both your suggestions looks reasonable. I'll try
to make some refactoring of GenericMIDletAgent to have same clear
architecture for batch, passive and active GUI mode.

>
> 4. I think more appropriate to add method removeAction() in MenuHadler
> instead of remove commands from Form directly.
Good idea. Try to do it.
>
> 5. Folks, what do you think about make this agent default for the export
> mode?
I don't think it is right decision. Customers may want to have
lightweight exported tests for batch mode run.
>
> Thanks,
> Alexander

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Vladimir Sizikov

Hi Maxim,

On Fri, May 18, 2007 at 08:02:10PM +0400, Maxim Dolidze wrote:
> >1. manifest_export_gui
> >
> > a) I wonder, why we keep using this awkward naming convention for
> > manifests. Why ExportMidletGUIAgent.manifest is no good? :) Or,
> > maybe midp_export_guiagent.manifest.
> >
> Actually I have no strong position about this item. Both variants
> (current and proposed by Vladimir) looks good for me. But if finally
> decide to change to .manifest format we should change all
> manifests, shouldn't we?

Let's postpone this. This is not critical and can be done any time
after the main fix is committed.

> > b) MIDlet-Name: MIDletAgent, why? Wouldn't it be better to provide
> > more descriptive name?
> >
> I don't like this name too. But in my opinion we MIDlet-Name in the
> exported test must be . The only problem is that I
> don't know for now how to implement this :-)

But even just simple MIDletGUIAgent would be good. Otherwise, there is
a confusion between this agent and the standard one.

> >>4. MIDletGUIAgent:
> >> d) I'd really like to avoid numbers in method names:
> >> getRecordStore4Read
> >> How about just openRecordStore()?
> >>
> openRecordStore() is not a good solution because in conjunction with
> getRecordStore() it looks confusing.
> So I make them getOutputRecordStore() and getInputRecordStore(). It is a
> little bit strange for whose who know what RecordStore is but quite
> alike java.io.* style.

Agreed.

> >3. I don't like the resultant design of GenericMIDletAgent. Now we have
> >similar methods startApp() and startRun() thats rather confusing. And
> >ExportMIDletGUIAgent overrides startApp() where makes many actions. But
> >the call of startApp() should consume short time. This is statement from
> >spec: "It is intended that these methods return quickly."
> >So I propose to introduce method startTestExecution() called inside
> >run() method of GenericMIDletAgent. MIDletGUIAgent will override it to
> >delete record store and ExportMIDletGUIAgent for check RMS.
> Vladimir, Alexander, both your suggestions looks reasonable. I'll try
> to make some refactoring of GenericMIDletAgent to have same clear
> architecture for batch, passive and active GUI mode.

OK.

> >5. Folks, what do you think about make this agent default for the export
> >mode?
> I don't think it is right decision. Customers may want to have
> lightweight exported tests for batch mode run.

I agree with Maxim. Sometimes users would just want to have the
exported tests to run in environment as close to the certification as
possible.

So, both ways (standard export agent and extended GUI agent) are
reasonable, with the default to be the standard agent, in my opinion.

Thanks,
--Vladimir

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Alexander Alexeev

Hi guys,

> But even just simple MIDletGUIAgent would be good. Otherwise, there is
> a confusion between this agent and the standard one.

I'm afraid it can't be done easily. Looks at DefaultContentHandler, it
produces standard MIDlet name for an attribute of jad file: MIDletAgent.

Thanks,
Alexander

Vladimir Sizikov wrote:
> Hi Maxim,
>
> On Fri, May 18, 2007 at 08:02:10PM +0400, Maxim Dolidze wrote:
>>> 1. manifest_export_gui
>>>
>>> a) I wonder, why we keep using this awkward naming convention for
>>> manifests. Why ExportMidletGUIAgent.manifest is no good? :) Or,
>>> maybe midp_export_guiagent.manifest.
>>>
>> Actually I have no strong position about this item. Both variants
>> (current and proposed by Vladimir) looks good for me. But if finally
>> decide to change to .manifest format we should change all
>> manifests, shouldn't we?
>
> Let's postpone this. This is not critical and can be done any time
> after the main fix is committed.
>
>>> b) MIDlet-Name: MIDletAgent, why? Wouldn't it be better to provide
>>> more descriptive name?
>>>
>> I don't like this name too. But in my opinion we MIDlet-Name in the
>> exported test must be . The only problem is that I
>> don't know for now how to implement this :-)
>
> But even just simple MIDletGUIAgent would be good. Otherwise, there is
> a confusion between this agent and the standard one.
>
>>>> 4. MIDletGUIAgent:
>>>> d) I'd really like to avoid numbers in method names:
>>>> getRecordStore4Read
>>>> How about just openRecordStore()?
>>>>
>> openRecordStore() is not a good solution because in conjunction with
>> getRecordStore() it looks confusing.
>> So I make them getOutputRecordStore() and getInputRecordStore(). It is a
>> little bit strange for whose who know what RecordStore is but quite
>> alike java.io.* style.
>
> Agreed.
>
>>> 3. I don't like the resultant design of GenericMIDletAgent. Now we have
>>> similar methods startApp() and startRun() thats rather confusing. And
>>> ExportMIDletGUIAgent overrides startApp() where makes many actions. But
>>> the call of startApp() should consume short time. This is statement from
>>> spec: "It is intended that these methods return quickly."
>>> So I propose to introduce method startTestExecution() called inside
>>> run() method of GenericMIDletAgent. MIDletGUIAgent will override it to
>>> delete record store and ExportMIDletGUIAgent for check RMS.
>> Vladimir, Alexander, both your suggestions looks reasonable. I'll try
>> to make some refactoring of GenericMIDletAgent to have same clear
>> architecture for batch, passive and active GUI mode.
>
> OK.
>
>>> 5. Folks, what do you think about make this agent default for the export
>>> mode?
>> I don't think it is right decision. Customers may want to have
>> lightweight exported tests for batch mode run.
>
> I agree with Maxim. Sometimes users would just want to have the
> exported tests to run in environment as close to the certification as
> possible.
>
> So, both ways (standard export agent and extended GUI agent) are
> reasonable, with the default to be the standard agent, in my opinion.
>
> Thanks,
> --Vladimir
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Vladimir Sizikov

Alexander,

On Mon, May 21, 2007 at 09:32:37PM +0400, Alexander Alexeev wrote:
> > But even just simple MIDletGUIAgent would be good. Otherwise, there is
> > a confusion between this agent and the standard one.
>
> I'm afraid it can't be done easily. Looks at DefaultContentHandler, it
> produces standard MIDlet name for an attribute of jad file: MIDletAgent.

OK then, this is very small issue. Let's concentrate on the major
functionality changes then. ;)

Thanks,
--Vladimir

> Vladimir Sizikov wrote:
> >Hi Maxim,
> >
> >On Fri, May 18, 2007 at 08:02:10PM +0400, Maxim Dolidze wrote:
> >>>1. manifest_export_gui
> >>>
> >>> a) I wonder, why we keep using this awkward naming convention for
> >>> manifests. Why ExportMidletGUIAgent.manifest is no good? :) Or,
> >>> maybe midp_export_guiagent.manifest.
> >>>
> >>Actually I have no strong position about this item. Both variants
> >>(current and proposed by Vladimir) looks good for me. But if finally
> >>decide to change to .manifest format we should change all
> >>manifests, shouldn't we?
> >
> >Let's postpone this. This is not critical and can be done any time
> >after the main fix is committed.
> >
> >>> b) MIDlet-Name: MIDletAgent, why? Wouldn't it be better to provide
> >>> more descriptive name?
> >>>
> >>I don't like this name too. But in my opinion we MIDlet-Name in the
> >>exported test must be . The only problem is that I
> >>don't know for now how to implement this :-)
> >
> >But even just simple MIDletGUIAgent would be good. Otherwise, there is
> >a confusion between this agent and the standard one.
> >
> >>>>4. MIDletGUIAgent:
> >>>> d) I'd really like to avoid numbers in method names:
> >>>> getRecordStore4Read
> >>>> How about just openRecordStore()?
> >>>>
> >>openRecordStore() is not a good solution because in conjunction with
> >>getRecordStore() it looks confusing.
> >>So I make them getOutputRecordStore() and getInputRecordStore(). It is a
> >>little bit strange for whose who know what RecordStore is but quite
> >>alike java.io.* style.
> >
> >Agreed.
> >
> >>>3. I don't like the resultant design of GenericMIDletAgent. Now we have
> >>>similar methods startApp() and startRun() thats rather confusing. And
> >>>ExportMIDletGUIAgent overrides startApp() where makes many actions. But
> >>>the call of startApp() should consume short time. This is statement from
> >>>spec: "It is intended that these methods return quickly."
> >>>So I propose to introduce method startTestExecution() called inside
> >>>run() method of GenericMIDletAgent. MIDletGUIAgent will override it to
> >>>delete record store and ExportMIDletGUIAgent for check RMS.
> >>Vladimir, Alexander, both your suggestions looks reasonable. I'll try
> >>to make some refactoring of GenericMIDletAgent to have same clear
> >>architecture for batch, passive and active GUI mode.
> >
> >OK.
> >
> >>>5. Folks, what do you think about make this agent default for the export
> >>>mode?
> >>I don't think it is right decision. Customers may want to have
> >>lightweight exported tests for batch mode run.
> >
> >I agree with Maxim. Sometimes users would just want to have the
> >exported tests to run in environment as close to the certification as
> >possible.
> >
> >So, both ways (standard export agent and extended GUI agent) are
> >reasonable, with the default to be the standard agent, in my opinion.
> >
> >Thanks,
> > --Vladimir
> >
> >---------------------------------------------------------------------
> >To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> >For additional commands, e-mail: meframework-help@cqme.dev.java.net
> >
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net