Skip to main content

Please review fix for issue 112

5 replies [Last post]
Anonymous

Hi Vladimir,

please review fix for [Issue 112]: "Class.forName() should not be used for
loading custom interview". https://cqme.dev.java.net/issues/show_bug.cgi?id=112
The changeset is http://fisheye4.cenqua.com/changelog/cqme/?cs=872

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,

I might be missing something, but I do not understand why such
significant changes were done to fix this problem.

You've changed public API for CustomPermissionsInterview (constructor
parameters changed), introduced some new (not documented :) ) public
methods into the base test suite, which is confusing -
getPermissions() that return ResourceBundle, etc.

Do I understand correctly that all we would like to do is to select
proper classloader in the CustomPermissionsInterview:

Instead of :

perms = new Permissions(root.getTestSuite().getClass().getClassLoader(), permsLocation);

Something like:

perms = new
Permissions(root.getTestSuite().getClassLoader(), permsLocation);

All we need to do in order for this to work is to override
getClassLoader in J2meBasetTestSuite and make it public instead of
protected.

So, the whole fix will be significantly reduced and no new public API would
be added. Just 3-4 lines of changes is much better then 50-60.

Maybe I've missed something here?

Thanks,
--Vladimir

On Fri, May 11, 2007 at 06:26:16PM +0400, Alexander Alexeev wrote:
> Hi Vladimir,
>
> please review fix for [Issue 112]: "Class.forName() should not be used for
> loading custom interview".
> https://cqme.dev.java.net/issues/show_bug.cgi?id=112
> The changeset is http://fisheye4.cenqua.com/changelog/cqme/?cs=872
>
> 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

Hi Vladimir,

you are right, your suggested fix will solve the problem. But I was trying to
solve yet another problem. We already changed this public API while was fixing
the runtime-pack issue and introduced dependencies between test suite and
interview. See the chageset:
http://fisheye4.cenqua.com/changelog/cqme/?cs=415

So I tried to reduce dependencies between test suite and interviews. Now
CustomPermissionsInterview doesn't depend on VmInterview and test suite.

But I'm agree method getPermissions() is confusing. So I propose two
options:
1. Rename method to getPermissionsBundle() or
2. Remove method, make getClassLoader() public and implement method
functionality in TrustedInterview.

Thanks,
Alexander

Vladimir Sizikov wrote:
> Hi Alexander,
>
> I might be missing something, but I do not understand why such
> significant changes were done to fix this problem.
>
> You've changed public API for CustomPermissionsInterview (constructor
> parameters changed), introduced some new (not documented :) ) public
> methods into the base test suite, which is confusing -
> getPermissions() that return ResourceBundle, etc.
>
> Do I understand correctly that all we would like to do is to select
> proper classloader in the CustomPermissionsInterview:
>
> Instead of :
>
> perms = new Permissions(root.getTestSuite().getClass().getClassLoader(), permsLocation);
>
> Something like:
>
> perms = new
> Permissions(root.getTestSuite().getClassLoader(), permsLocation);
>
> All we need to do in order for this to work is to override
> getClassLoader in J2meBasetTestSuite and make it public instead of
> protected.
>
> So, the whole fix will be significantly reduced and no new public API would
> be added. Just 3-4 lines of changes is much better then 50-60.
>
> Maybe I've missed something here?
>
> Thanks,
> --Vladimir
>
> On Fri, May 11, 2007 at 06:26:16PM +0400, Alexander Alexeev wrote:
>> Hi Vladimir,
>>
>> please review fix for [Issue 112]: "Class.forName() should not be used for
>> loading custom interview".
>> https://cqme.dev.java.net/issues/show_bug.cgi?id=112
>> The changeset is http://fisheye4.cenqua.com/changelog/cqme/?cs=872
>>
>> 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

Hi Alexander,

On Mon, May 14, 2007 at 05:35:39PM +0400, Alexander Alexeev wrote:
> So I tried to reduce dependencies between test suite and interviews. Now
> CustomPermissionsInterview doesn't depend on VmInterview and test suite.
>
> But I'm agree method getPermissions() is confusing. So I propose two
> options:
> 1. Rename method to getPermissionsBundle() or

This methods is not very generic and does not make much sense from
user's point of view. What permisisons? Why? When and how should I,
the user of the Framework's base test suite, call and use this method?

For non-MIDP test suites the whole thing does not make much sense.

> 2. Remove method, make getClassLoader() public and implement method
> functionality in TrustedInterview.

I like this one better. Ideally, the getClassLoader() method should
have been pubilc in the core JTHarness. This is very generic and
common functionality - to load something from the TestSuite's
classloader. Actually, why not file a RFE against the jtharness and
ask to make this method public? Meanwhile, we can make it public in
our most base TestSuite.

Thanks,
--Vladimir

> Vladimir Sizikov wrote:
> >Hi Alexander,
> >
> >I might be missing something, but I do not understand why such
> >significant changes were done to fix this problem.
> >
> >You've changed public API for CustomPermissionsInterview (constructor
> >parameters changed), introduced some new (not documented :) ) public
> >methods into the base test suite, which is confusing -
> >getPermissions() that return ResourceBundle, etc.
> >
> >Do I understand correctly that all we would like to do is to select
> >proper classloader in the CustomPermissionsInterview:
> >
> >Instead of :
> >
> >perms = new Permissions(root.getTestSuite().getClass().getClassLoader(),
> >permsLocation);
> >
> >Something like:
> >
> >perms = new
> >Permissions(root.getTestSuite().getClassLoader(), permsLocation);
> >
> >All we need to do in order for this to work is to override
> >getClassLoader in J2meBasetTestSuite and make it public instead of
> >protected.
> >
> >So, the whole fix will be significantly reduced and no new public API
> >would
> >be added. Just 3-4 lines of changes is much better then 50-60.
> >
> >Maybe I've missed something here?
> >
> >Thanks,
> > --Vladimir
> >
> >On Fri, May 11, 2007 at 06:26:16PM +0400, Alexander Alexeev wrote:
> >>Hi Vladimir,
> >>
> >>please review fix for [Issue 112]: "Class.forName() should not be used for
> >>loading custom interview".
> >>https://cqme.dev.java.net/issues/show_bug.cgi?id=112
> >>The changeset is http://fisheye4.cenqua.com/changelog/cqme/?cs=872
> >>
> >>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

Alexander Alexeev

Hi Vladimir,

please see changes on http://fisheye4.cenqua.com/changelog/cqme/?cs=878
and http://fisheye4.cenqua.com/changelog/cqme/?cs=879

Thanks,
Alexander

Vladimir Sizikov wrote:
> Hi Alexander,
>
> On Mon, May 14, 2007 at 05:35:39PM +0400, Alexander Alexeev wrote:
>> So I tried to reduce dependencies between test suite and interviews. Now
>> CustomPermissionsInterview doesn't depend on VmInterview and test suite.
>>
>> But I'm agree method getPermissions() is confusing. So I propose two
>> options:
>> 1. Rename method to getPermissionsBundle() or
>
> This methods is not very generic and does not make much sense from
> user's point of view. What permisisons? Why? When and how should I,
> the user of the Framework's base test suite, call and use this method?
>
> For non-MIDP test suites the whole thing does not make much sense.
>
>> 2. Remove method, make getClassLoader() public and implement method
>> functionality in TrustedInterview.
>
> I like this one better. Ideally, the getClassLoader() method should
> have been pubilc in the core JTHarness. This is very generic and
> common functionality - to load something from the TestSuite's
> classloader. Actually, why not file a RFE against the jtharness and
> ask to make this method public? Meanwhile, we can make it public in
> our most base TestSuite.
>
> Thanks,
> --Vladimir
>
>> Vladimir Sizikov wrote:
>>> Hi Alexander,
>>>
>>> I might be missing something, but I do not understand why such
>>> significant changes were done to fix this problem.
>>>
>>> You've changed public API for CustomPermissionsInterview (constructor
>>> parameters changed), introduced some new (not documented :) ) public
>>> methods into the base test suite, which is confusing -
>>> getPermissions() that return ResourceBundle, etc.
>>>
>>> Do I understand correctly that all we would like to do is to select
>>> proper classloader in the CustomPermissionsInterview:
>>>
>>> Instead of :
>>>
>>> perms = new Permissions(root.getTestSuite().getClass().getClassLoader(),
>>> permsLocation);
>>>
>>> Something like:
>>>
>>> perms = new
>>> Permissions(root.getTestSuite().getClassLoader(), permsLocation);
>>>
>>> All we need to do in order for this to work is to override
>>> getClassLoader in J2meBasetTestSuite and make it public instead of
>>> protected.
>>>
>>> So, the whole fix will be significantly reduced and no new public API
>>> would
>>> be added. Just 3-4 lines of changes is much better then 50-60.
>>>
>>> Maybe I've missed something here?
>>>
>>> Thanks,
>>> --Vladimir
>>>
>>> On Fri, May 11, 2007 at 06:26:16PM +0400, Alexander Alexeev wrote:
>>>> Hi Vladimir,
>>>>
>>>> please review fix for [Issue 112]: "Class.forName() should not be used for
>>>> loading custom interview".
>>>> https://cqme.dev.java.net/issues/show_bug.cgi?id=112
>>>> The changeset is http://fisheye4.cenqua.com/changelog/cqme/?cs=872
>>>>
>>>> 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
>

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

The changes look good, with a couple of minor points:

1. In CustomPermissionsInterview, you've changed the name of the
parameter in the constructor, but the javadoc was not changed.

2. I'd suggest to add some javadoc (copy from jtharness) to the overridden
getClassLoader() method, _and_ explanation why overridden (to make
method public).

Once you've done with this changes, feel free to commit.

Thanks,
--Vladimir

On Mon, May 14, 2007 at 08:05:02PM +0400, Alexander Alexeev wrote:
> Hi Vladimir,
>
> please see changes on http://fisheye4.cenqua.com/changelog/cqme/?cs=878
> and http://fisheye4.cenqua.com/changelog/cqme/?cs=879
>
> Thanks,
> Alexander
>
> Vladimir Sizikov wrote:
> >Hi Alexander,
> >
> >On Mon, May 14, 2007 at 05:35:39PM +0400, Alexander Alexeev wrote:
> >>So I tried to reduce dependencies between test suite and interviews. Now
> >>CustomPermissionsInterview doesn't depend on VmInterview and test suite.
> >>
> >>But I'm agree method getPermissions() is confusing. So I propose two
> >>options:
> >>1. Rename method to getPermissionsBundle() or
> >
> >This methods is not very generic and does not make much sense from
> >user's point of view. What permisisons? Why? When and how should I,
> >the user of the Framework's base test suite, call and use this method?
> >
> >For non-MIDP test suites the whole thing does not make much sense.
> >
> >>2. Remove method, make getClassLoader() public and implement method
> >>functionality in TrustedInterview.
> >
> >I like this one better. Ideally, the getClassLoader() method should
> >have been pubilc in the core JTHarness. This is very generic and
> >common functionality - to load something from the TestSuite's
> >classloader. Actually, why not file a RFE against the jtharness and
> >ask to make this method public? Meanwhile, we can make it public in
> >our most base TestSuite.
> >
> >Thanks,
> > --Vladimir
> >
> >>Vladimir Sizikov wrote:
> >>>Hi Alexander,
> >>>
> >>>I might be missing something, but I do not understand why such
> >>>significant changes were done to fix this problem.
> >>>
> >>>You've changed public API for CustomPermissionsInterview (constructor
> >>>parameters changed), introduced some new (not documented :) ) public
> >>>methods into the base test suite, which is confusing -
> >>>getPermissions() that return ResourceBundle, etc.
> >>>
> >>>Do I understand correctly that all we would like to do is to select
> >>>proper classloader in the CustomPermissionsInterview:
> >>>
> >>>Instead of :
> >>>
> >>>perms = new Permissions(root.getTestSuite().getClass().getClassLoader(),
> >>>permsLocation);
> >>>
> >>>Something like:
> >>>
> >>>perms = new
> >>>Permissions(root.getTestSuite().getClassLoader(), permsLocation);
> >>>
> >>>All we need to do in order for this to work is to override
> >>>getClassLoader in J2meBasetTestSuite and make it public instead of
> >>>protected.
> >>>
> >>>So, the whole fix will be significantly reduced and no new public API
> >>>would
> >>>be added. Just 3-4 lines of changes is much better then 50-60.
> >>>
> >>>Maybe I've missed something here?
> >>>
> >>>Thanks,
> >>> --Vladimir
> >>>
> >>>On Fri, May 11, 2007 at 06:26:16PM +0400, Alexander Alexeev wrote:
> >>>>Hi Vladimir,
> >>>>
> >>>>please review fix for [Issue 112]: "Class.forName() should not be used
> >>>>for
> >>>>loading custom interview".
> >>>>https://cqme.dev.java.net/issues/show_bug.cgi?id=112
> >>>>The changeset is http://fisheye4.cenqua.com/changelog/cqme/?cs=872
> >>>>
> >>>>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
> >
>

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