Skip to main content

Please review fixes

8 replies [Last post]
Anonymous

Hi Vladimir,

please review fixes for ME Framework.
[ISSUE #299]: Test export functionality is broken under JT Harness 4.1:
http://fisheye4.atlassian.com/browse/cqme/branches/users/skavas/issue299...

[ISSUE #300]: Client verbose doesn't work sometimes:
http://fisheye4.atlassian.com/changelog/cqme/?cs=1660

[ISSUE #175]: Distributed tests in CLDC mode are not supported:
http://fisheye4.atlassian.com/changelog/cqme/?cs=1662

Thanks,
Alexander

---------------------------------------------------------------------
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.
Vladimir Sizikov

Hi Alexander,

On 5/26/2009 4:18 PM, Alexander Alexeev wrote:
> Just in case, have you looked at
> [ISSUE #299]: Test export functionality is broken under JT Harness 4.1:

Just finished reviewing.

Everything looks good, except for one small comment. Why did you add at
line 106+ the overridden save() method which does nothing, only
delegating to the super implementation? It seems, this should be
completely removed, no?

Thanks,
--Vladimir
[vladimir_sizikov.vcf]
---------------------------------------------------------------------
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,

thanks for catching this. Eliminated.

Thanks,
Alexander

Vladimir Sizikov wrote:
>
> Just finished reviewing.
>
> Everything looks good, except for one small comment. Why did you add at
> line 106+ the overridden save() method which does nothing, only
> delegating to the super implementation? It seems, this should be
> completely removed, no?
>
> Thanks,
> --Vladimir

---------------------------------------------------------------------
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,

Here are my comments for:

> [ISSUE #175]: Distributed tests in CLDC mode are not supported:
> http://fisheye4.atlassian.com/changelog/cqme/?cs=1662

1. In DTFInterview.java, you probably should remove the whole
StringBuilder sb related code, since it's not used anymore anywhere.

2. CldcBaseCommInterview.java:

919 - data.put("script", "com.sun.tck.cldc.javatest.CldcTCKScript");
919 + data.put("script", "com.sun.tck.j2me.javatest.DistributedScript");

I'm a bit concerned about this change, since this means that even for
regular CLDC-based TCKs (no distributed tests), we now specify the
distributed script.

Would it be better, if we make it conditional, depending on
isDistMode(), so that standard TCKs won't be affected at all, and only
those TCKs that return yes to isDistrMode() would get the distributed
script?

Thanks,
--Vladimir

[vladimir_sizikov.vcf]
---------------------------------------------------------------------
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,

thank you for the comments. Please see my answers inlined.

Vladimir Sizikov wrote:
> Hi Alexander,
>
> Here are my comments for:
>
>> [ISSUE #175]: Distributed tests in CLDC mode are not supported:
>> http://fisheye4.atlassian.com/changelog/cqme/?cs=1662
>
> 1. In DTFInterview.java, you probably should remove the whole
> StringBuilder sb related code, since it's not used anymore anywhere.
>
I removed StringBuilder.

> 2. CldcBaseCommInterview.java:
>
> 919 - data.put("script", "com.sun.tck.cldc.javatest.CldcTCKScript");
> 919 + data.put("script", "com.sun.tck.j2me.javatest.DistributedScript");
>
> I'm a bit concerned about this change, since this means that even for
> regular CLDC-based TCKs (no distributed tests), we now specify the
> distributed script.
>
> Would it be better, if we make it conditional, depending on
> isDistMode(), so that standard TCKs won't be affected at all, and only
> those TCKs that return yes to isDistrMode() would get the distributed
> script?
>
After some thoughts I found it make sense. It will lead to more understandable code at least (will
not confuse by script name). I will update CldcBaseCommInterview.
I didn't make this conditional by the first time by analogy with MIDP. We do not have special
non-distributed script for MIDP mode since this mode was design to support distributed tests from
the beginning.

Thanks,
Alexander

> 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,

may I integrate the branch to the trunk?

Just in case, have you looked at
[ISSUE #299]: Test export functionality is broken under JT Harness 4.1:
http://fisheye4.atlassian.com/browse/cqme/branches/users/skavas/issue299...
?

Thanks,
Alexander

Alexander Alexeev wrote:
> Hi Vladimir,
>
> thank you for the comments. Please see my answers inlined.
>
> Vladimir Sizikov wrote:
>> Hi Alexander,
>>
>> Here are my comments for:
>>
>>> [ISSUE #175]: Distributed tests in CLDC mode are not supported:
>>> http://fisheye4.atlassian.com/changelog/cqme/?cs=1662
>>
>> 1. In DTFInterview.java, you probably should remove the whole
>> StringBuilder sb related code, since it's not used anymore anywhere.
>>
> I removed StringBuilder.
>
>> 2. CldcBaseCommInterview.java:
>>
>> 919 - data.put("script", "com.sun.tck.cldc.javatest.CldcTCKScript");
>> 919 + data.put("script", "com.sun.tck.j2me.javatest.DistributedScript");
>>
>> I'm a bit concerned about this change, since this means that even for
>> regular CLDC-based TCKs (no distributed tests), we now specify the
>> distributed script.
>>
>> Would it be better, if we make it conditional, depending on
>> isDistMode(), so that standard TCKs won't be affected at all, and only
>> those TCKs that return yes to isDistrMode() would get the distributed
>> script?
>>
> After some thoughts I found it make sense. It will lead to more
> understandable code at least (will
> not confuse by script name). I will update CldcBaseCommInterview.
> I didn't make this conditional by the first time by analogy with MIDP.
> We do not have special
> non-distributed script for MIDP mode since this mode was design to
> support distributed tests from
> the beginning.
>
> Thanks,
> Alexander
>
>> 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,

Feel free to integrate the two previously reviewed fixed.

As for the 3rd one, I'll try to review it during this week. Sorry for
the delay.

Thanks,
--Vladimir

On 5/26/2009 4:18 PM, Alexander Alexeev wrote:
> Hi Vladimir,
>
> may I integrate the branch to the trunk?
>
> Just in case, have you looked at
> [ISSUE #299]: Test export functionality is broken under JT Harness 4.1:
> http://fisheye4.atlassian.com/browse/cqme/branches/users/skavas/issue299...
[vladimir_sizikov.vcf]
---------------------------------------------------------------------
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,

Will do, one by one, but that will take some (more) time...

Thanks,
--Vladimir

On 5/21/2009 3:18 PM, Alexander Alexeev wrote:
> [ISSUE #299]: Test export functionality is broken under JT Harness 4.1:
> http://fisheye4.atlassian.com/browse/cqme/branches/users/skavas/issue299...
>
> [ISSUE #300]: Client verbose doesn't work sometimes:
> http://fisheye4.atlassian.com/changelog/cqme/?cs=1660
>
> [ISSUE #175]: Distributed tests in CLDC mode are not supported:
> http://fisheye4.atlassian.com/changelog/cqme/?cs=1662

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

Vladimir Sizikov

While we're at it. I've reviewed the fix for ISSUE #300, it was simple
enough to review it right away. :)

>> [ISSUE #300]: Client verbose doesn't work sometimes:
>> http://fisheye4.atlassian.com/changelog/cqme/?cs=1660

Looks good. Please commit.

Thanks,
--Vladimir
[vladimir_sizikov.vcf]
---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net