Skip to main content

Please review fix for [ISSUE #294]

4 replies [Last post]
Anonymous

Hi Vladimir,

I'd like to propose to remove ServiceBasedTestSuite.createTestFilter() in order
to fix [ISSUE #294]: "CLDCPrecompileTestSuite filters out some tests". This fix
was already suggested by me some times ago:

http://fisheye4.atlassian.com/changelog/cqme/?cs=1637

I don't see any regressions that can be caused by this fix. Contrary, I don't
see any reasons why service's base test suite should create any filters.

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 11/19/2009 3:52 PM, Alexander Alexeev wrote:
> Hi Vladimir,
>
> I'd like to propose to remove ServiceBasedTestSuite.createTestFilter()
> in order
> to fix [ISSUE #294]: "CLDCPrecompileTestSuite filters out some tests".
> This fix
> was already suggested by me some times ago:
>
> http://fisheye4.atlassian.com/changelog/cqme/?cs=1637
>
> I don't see any regressions that can be caused by this fix. Contrary, I
> don't
> see any reasons why service's base test suite should create any filters.

This makes me a bit uneasy, we're changing the public and documented
behavior of ServiceBasedTestSuite. As a default behavior, it seems
natural to provide the expression filter in the base test suite. And we
even specify that in the docs as well.

Maybe, it would be safer to just override the createTestFilter() in
CLDCPrecompileTestSuite so that it would return null.

That way, we won't change the public behavior of the base test suite
(thus avoiding even the possibility of regressions) and fix the problem
in the PrecomplieTestSuite.

What do you think?

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,

well, it's incompatible fix but I will try to explain my point of view.

The first I don't like an idea to change CLDCPrecompileTestSuite because it
breaks Liskov's principal. Type's behavior will be changed in the subtype.

The second the bug is really a regression introduced by adding
ServiceBaseTestSuite class. This class broke general contract of
CLDCPrecompileTestSuite class by overriding createTestFilter() method. It's not
clear to me why this method exist in ServiceBaseTestSuite. Each class from the
test suite's hierarchy provides specific ability to the suite by design.
ServiceBaseTestSuite adds ability to create services in the suite. Providing
test's filters is deal of J2meBaseTestSuite. Or may be I've missed something?

And why I think it's safe to delete createTestFilter() method. ME Framework
isn't bunch of independent classes. For this particular case it means no one
will use ServiceBaseTestSuite as is since to proper use framework someone needs
to extend J2meBaseTestSuite. So preferably to have the most classes from the
hierarchy to be package-private. Unfortunately it's unachievable now.

Thanks,
Alexander

Vladimir Sizikov wrote:
> Hi Alexander,
>
> On 11/19/2009 3:52 PM, Alexander Alexeev wrote:
>> Hi Vladimir,
>>
>> I'd like to propose to remove ServiceBasedTestSuite.createTestFilter()
>> in order
>> to fix [ISSUE #294]: "CLDCPrecompileTestSuite filters out some tests".
>> This fix
>> was already suggested by me some times ago:
>>
>> http://fisheye4.atlassian.com/changelog/cqme/?cs=1637
>>
>> I don't see any regressions that can be caused by this fix. Contrary,
>> I don't
>> see any reasons why service's base test suite should create any filters.
>
> This makes me a bit uneasy, we're changing the public and documented
> behavior of ServiceBasedTestSuite. As a default behavior, it seems
> natural to provide the expression filter in the base test suite. And we
> even specify that in the docs as well.
>
> Maybe, it would be safer to just override the createTestFilter() in
> CLDCPrecompileTestSuite so that it would return null.
>
> That way, we won't change the public behavior of the base test suite
> (thus avoiding even the possibility of regressions) and fix the problem
> in the PrecomplieTestSuite.
>
> What do you think?
>
> 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,

Yeah, I actually agree with you, but we need to be extra cautious about
not introducing any regressions.

On the other hand, since we're now working towards 2.0 version which is
a major new version, small changes in behavior are OK with me.

So, could you please double-check with all existing test suites that
they are OK with this change and none of them actually relies on the
ServiceBaseTestSuite to provide the expression filter?

If there are no such test suites, then I'm OK with your change.

Thanks,
--Vladimir

On 11/23/2009 3:49 PM, Alexander Alexeev wrote:
> Hi Vladimir,
>
> well, it's incompatible fix but I will try to explain my point of view.
>
> The first I don't like an idea to change CLDCPrecompileTestSuite because it
> breaks Liskov's principal. Type's behavior will be changed in the subtype.
>
> The second the bug is really a regression introduced by adding
> ServiceBaseTestSuite class. This class broke general contract of
> CLDCPrecompileTestSuite class by overriding createTestFilter() method.
> It's not
> clear to me why this method exist in ServiceBaseTestSuite. Each class
> from the
> test suite's hierarchy provides specific ability to the suite by design.
> ServiceBaseTestSuite adds ability to create services in the suite.
> Providing
> test's filters is deal of J2meBaseTestSuite. Or may be I've missed
> something?
>
> And why I think it's safe to delete createTestFilter() method. ME Framework
> isn't bunch of independent classes. For this particular case it means no
> one
> will use ServiceBaseTestSuite as is since to proper use framework
> someone needs
> to extend J2meBaseTestSuite. So preferably to have the most classes from
> the
> hierarchy to be package-private. Unfortunately it's unachievable now.
>
> Thanks,
> Alexander
>
> Vladimir Sizikov wrote:
>> Hi Alexander,
>>
>> On 11/19/2009 3:52 PM, Alexander Alexeev wrote:
>>> Hi Vladimir,
>>>
>>> I'd like to propose to remove
>>> ServiceBasedTestSuite.createTestFilter() in order
>>> to fix [ISSUE #294]: "CLDCPrecompileTestSuite filters out some
>>> tests". This fix
>>> was already suggested by me some times ago:
>>>
>>> http://fisheye4.atlassian.com/changelog/cqme/?cs=1637
>>>
>>> I don't see any regressions that can be caused by this fix. Contrary,
>>> I don't
>>> see any reasons why service's base test suite should create any filters.
>>
>> This makes me a bit uneasy, we're changing the public and documented
>> behavior of ServiceBasedTestSuite. As a default behavior, it seems
>> natural to provide the expression filter in the base test suite. And
>> we even specify that in the docs as well.
>>
>> Maybe, it would be safer to just override the createTestFilter() in
>> CLDCPrecompileTestSuite so that it would return null.
>>
>> That way, we won't change the public behavior of the base test suite
>> (thus avoiding even the possibility of regressions) and fix the
>> problem in the PrecomplieTestSuite.
>>
>> What do you think?
>>
>> 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,

I've checked - there are no such test suites. All test suites rely on
J2meBaseTestSuite.createTestFilter().

Thanks,
Alexander

Vladimir Sizikov wrote:
> Hi Alexander,
>
> Yeah, I actually agree with you, but we need to be extra cautious about
> not introducing any regressions.
>
> On the other hand, since we're now working towards 2.0 version which is
> a major new version, small changes in behavior are OK with me.
>
> So, could you please double-check with all existing test suites that
> they are OK with this change and none of them actually relies on the
> ServiceBaseTestSuite to provide the expression filter?
>
> If there are no such test suites, then I'm OK with your change.
>
> Thanks,
> --Vladimir
>
> On 11/23/2009 3:49 PM, Alexander Alexeev wrote:
>> Hi Vladimir,
>>
>> well, it's incompatible fix but I will try to explain my point of view.
>>
>> The first I don't like an idea to change CLDCPrecompileTestSuite
>> because it
>> breaks Liskov's principal. Type's behavior will be changed in the
>> subtype.
>>
>> The second the bug is really a regression introduced by adding
>> ServiceBaseTestSuite class. This class broke general contract of
>> CLDCPrecompileTestSuite class by overriding createTestFilter() method.
>> It's not
>> clear to me why this method exist in ServiceBaseTestSuite. Each class
>> from the
>> test suite's hierarchy provides specific ability to the suite by design.
>> ServiceBaseTestSuite adds ability to create services in the suite.
>> Providing
>> test's filters is deal of J2meBaseTestSuite. Or may be I've missed
>> something?
>>
>> And why I think it's safe to delete createTestFilter() method. ME
>> Framework
>> isn't bunch of independent classes. For this particular case it means
>> no one
>> will use ServiceBaseTestSuite as is since to proper use framework
>> someone needs
>> to extend J2meBaseTestSuite. So preferably to have the most classes
>> from the
>> hierarchy to be package-private. Unfortunately it's unachievable now.
>>
>> Thanks,
>> Alexander
>>
>> Vladimir Sizikov wrote:
>>> Hi Alexander,
>>>
>>> On 11/19/2009 3:52 PM, Alexander Alexeev wrote:
>>>> Hi Vladimir,
>>>>
>>>> I'd like to propose to remove
>>>> ServiceBasedTestSuite.createTestFilter() in order
>>>> to fix [ISSUE #294]: "CLDCPrecompileTestSuite filters out some
>>>> tests". This fix
>>>> was already suggested by me some times ago:
>>>>
>>>> http://fisheye4.atlassian.com/changelog/cqme/?cs=1637
>>>>
>>>> I don't see any regressions that can be caused by this fix.
>>>> Contrary, I don't
>>>> see any reasons why service's base test suite should create any
>>>> filters.
>>>
>>> This makes me a bit uneasy, we're changing the public and documented
>>> behavior of ServiceBasedTestSuite. As a default behavior, it seems
>>> natural to provide the expression filter in the base test suite. And
>>> we even specify that in the docs as well.
>>>
>>> Maybe, it would be safer to just override the createTestFilter() in
>>> CLDCPrecompileTestSuite so that it would return null.
>>>
>>> That way, we won't change the public behavior of the base test suite
>>> (thus avoiding even the possibility of regressions) and fix the
>>> problem in the PrecomplieTestSuite.
>>>
>>> What do you think?
>>>
>>> Thanks,
>>> --Vladimir
>>

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