You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@river.apache.org by Patricia Shanahan <pa...@acm.org> on 2011/04/15 02:40:49 UTC
Test anomaly
I'm attempting to construct QA tests for the RIVER-395 by copying
existing tests of event delivery and adding a special listener that
throws exceptions. If the patch is correct, the modified test should
pass with all expected events going to the existing listeners despite
the exceptions.
I'm running my tests with 'com.sun.jini.qa.harness.level = FINEST'
logging, so that I can assure myself that the exceptions are being
thrown and events delivered after an exception.
I tried to base a test on
com/sun/jini/test/spec/discoverymanager/AddNewDiscoveryListener.td, and
got no events.
I've since run the complete discoverymanager category. From the logs, I
got no change events, and only 4 tests appeared to have non-zero numbers
of initial events.
Could someone else take a look at this and see if this is as expected?
It looked to me from the comments in AddNewDiscoveryListener.java as
though processing a non-zero number of events is important to the test.
Patricia
Re: Test anomaly
Posted by Patricia Shanahan <pa...@acm.org>.
Thanks for the explanation. I see what you mean. I'll add comments to
the methods to clarify the reason for the difference.
Patricia
On 4/25/2011 2:45 PM, Christopher Dolan wrote:
> Hi Patricia,
> Thanks for investing so much time in this issue.
>
> In my patch, I intentionally did not modify the second case (single
> listener) because in that scenario, the failure would only hurt the
> caller's thread, where the first case (all listeners) a failure crashed
> the LookupDiscovery/LookupLocatorDiscovery instance's notify thread. I
> thought that the single listener case deserved fail-fast behavior and it
> should be the caller's responsibility to catch their own broken
> listener's exceptions.
>
> Chris
>
> P.S. sorry for the delay. I'm fresh back from vacation.
>
> -----Original Message-----
> From: Patricia Shanahan [mailto:pats@acm.org]
> Sent: Friday, April 15, 2011 5:46 PM
> To: dev@river.apache.org
> Cc: Christopher Dolan
> Subject: Re: Test anomaly
>
> Chris,
>
> I changed the config file for the specific test to get some events. It
> failed due to my exception. It turns out that LookupDiscoveryManager has
> two notifyListener methods, one for notifying all listeners, the other
> for notifying a specific listener, presumably one that has just been
> added. I've modified the second method based on the patch, and I'll take
> a look at the other notifiers to see if there are any similar
> situations.
>
> I was not trying to test the second method, because I didn't even know
> it existed. It was a result of my fumbling with the test. This confirms
> my feeling that testing is worth doing, and pushes me to a new strategy:
>
> 1. Make sure all listener-related tests really are getting events,
> unless the test comments indicate against it.
>
> 2. Find all the places where the tests add listeners, and throw in a
> BadListener as well.
>
> That should maximize the chances of finding any other notifyListener
> type methods that have not been hardened.
>
> Patricia
>
>
> On 4/15/2011 11:45 AM, Patricia Shanahan wrote:
>> Thanks for looking at this. I agree with you about the impenetrability
>> of the test code - that is why I'm working by modifying existing tests
>> rather than just writing the tests I need from scratch.
>>
>> If this becomes the limiting factor on a release, I'll check in the
>> patch without a QA test.
>>
>> However, I am strongly in favor of QA tests for the sake of regression
>> testing. I want Hudson to tell us if some future change brings back
> the
>> vulnerability to listener exceptions.
>>
>> Patricia
>>
>>
>> On 4/15/2011 11:21 AM, Christopher Dolan wrote:
>>> Patricia,
>>>
>>> I looked at the existing AddNewDiscoveryListener and agree with your
>>> assessment. But the test code is nigh impenetrable, I have to say. It
>>> would be beneficial in the long run if some of the common code in
>>> LookupDiscovery and LookupLocatorDiscovery were broken apart into
>>> separately testable units... I tried writing my own test case, but it
>>> doesn't seem possible to mock any of the providers, so to test it you
>>> need an actual registrar, it appears.
>>>
>>> For what it's worth, today I wrote my own private test that runs in
> my
>>> djinn and verified the patch.
>>>
>>> Chris
>>>
>>>
>>> -----Original Message-----
>>> From: Patricia Shanahan [mailto:pats@acm.org]
>>> Sent: Thursday, April 14, 2011 7:41 PM
>>> To: river-dev@incubator.apache.org
>>> Subject: Test anomaly
>>>
>>> I'm attempting to construct QA tests for the RIVER-395 by copying
>>> existing tests of event delivery and adding a special listener that
>>> throws exceptions. If the patch is correct, the modified test should
>>> pass with all expected events going to the existing listeners despite
>>> the exceptions.
>>>
>>> I'm running my tests with 'com.sun.jini.qa.harness.level = FINEST'
>>> logging, so that I can assure myself that the exceptions are being
>>> thrown and events delivered after an exception.
>>>
>>> I tried to base a test on
>>> com/sun/jini/test/spec/discoverymanager/AddNewDiscoveryListener.td,
> and
>>> got no events.
>>>
>>> I've since run the complete discoverymanager category. From the logs,
> I
>>> got no change events, and only 4 tests appeared to have non-zero
> numbers
>>>
>>> of initial events.
>>>
>>> Could someone else take a look at this and see if this is as
> expected?
>>> It looked to me from the comments in AddNewDiscoveryListener.java as
>>> though processing a non-zero number of events is important to the
> test.
>>>
>>> Patricia
>>>
>>
>
>
RE: Test anomaly
Posted by Christopher Dolan <ch...@avid.com>.
Hi Patricia,
Thanks for investing so much time in this issue.
In my patch, I intentionally did not modify the second case (single
listener) because in that scenario, the failure would only hurt the
caller's thread, where the first case (all listeners) a failure crashed
the LookupDiscovery/LookupLocatorDiscovery instance's notify thread. I
thought that the single listener case deserved fail-fast behavior and it
should be the caller's responsibility to catch their own broken
listener's exceptions.
Chris
P.S. sorry for the delay. I'm fresh back from vacation.
-----Original Message-----
From: Patricia Shanahan [mailto:pats@acm.org]
Sent: Friday, April 15, 2011 5:46 PM
To: dev@river.apache.org
Cc: Christopher Dolan
Subject: Re: Test anomaly
Chris,
I changed the config file for the specific test to get some events. It
failed due to my exception. It turns out that LookupDiscoveryManager has
two notifyListener methods, one for notifying all listeners, the other
for notifying a specific listener, presumably one that has just been
added. I've modified the second method based on the patch, and I'll take
a look at the other notifiers to see if there are any similar
situations.
I was not trying to test the second method, because I didn't even know
it existed. It was a result of my fumbling with the test. This confirms
my feeling that testing is worth doing, and pushes me to a new strategy:
1. Make sure all listener-related tests really are getting events,
unless the test comments indicate against it.
2. Find all the places where the tests add listeners, and throw in a
BadListener as well.
That should maximize the chances of finding any other notifyListener
type methods that have not been hardened.
Patricia
On 4/15/2011 11:45 AM, Patricia Shanahan wrote:
> Thanks for looking at this. I agree with you about the impenetrability
> of the test code - that is why I'm working by modifying existing tests
> rather than just writing the tests I need from scratch.
>
> If this becomes the limiting factor on a release, I'll check in the
> patch without a QA test.
>
> However, I am strongly in favor of QA tests for the sake of regression
> testing. I want Hudson to tell us if some future change brings back
the
> vulnerability to listener exceptions.
>
> Patricia
>
>
> On 4/15/2011 11:21 AM, Christopher Dolan wrote:
>> Patricia,
>>
>> I looked at the existing AddNewDiscoveryListener and agree with your
>> assessment. But the test code is nigh impenetrable, I have to say. It
>> would be beneficial in the long run if some of the common code in
>> LookupDiscovery and LookupLocatorDiscovery were broken apart into
>> separately testable units... I tried writing my own test case, but it
>> doesn't seem possible to mock any of the providers, so to test it you
>> need an actual registrar, it appears.
>>
>> For what it's worth, today I wrote my own private test that runs in
my
>> djinn and verified the patch.
>>
>> Chris
>>
>>
>> -----Original Message-----
>> From: Patricia Shanahan [mailto:pats@acm.org]
>> Sent: Thursday, April 14, 2011 7:41 PM
>> To: river-dev@incubator.apache.org
>> Subject: Test anomaly
>>
>> I'm attempting to construct QA tests for the RIVER-395 by copying
>> existing tests of event delivery and adding a special listener that
>> throws exceptions. If the patch is correct, the modified test should
>> pass with all expected events going to the existing listeners despite
>> the exceptions.
>>
>> I'm running my tests with 'com.sun.jini.qa.harness.level = FINEST'
>> logging, so that I can assure myself that the exceptions are being
>> thrown and events delivered after an exception.
>>
>> I tried to base a test on
>> com/sun/jini/test/spec/discoverymanager/AddNewDiscoveryListener.td,
and
>> got no events.
>>
>> I've since run the complete discoverymanager category. From the logs,
I
>> got no change events, and only 4 tests appeared to have non-zero
numbers
>>
>> of initial events.
>>
>> Could someone else take a look at this and see if this is as
expected?
>> It looked to me from the comments in AddNewDiscoveryListener.java as
>> though processing a non-zero number of events is important to the
test.
>>
>> Patricia
>>
>
Re: Test anomaly
Posted by Patricia Shanahan <pa...@acm.org>.
Chris,
I changed the config file for the specific test to get some events. It
failed due to my exception. It turns out that LookupDiscoveryManager has
two notifyListener methods, one for notifying all listeners, the other
for notifying a specific listener, presumably one that has just been
added. I've modified the second method based on the patch, and I'll take
a look at the other notifiers to see if there are any similar situations.
I was not trying to test the second method, because I didn't even know
it existed. It was a result of my fumbling with the test. This confirms
my feeling that testing is worth doing, and pushes me to a new strategy:
1. Make sure all listener-related tests really are getting events,
unless the test comments indicate against it.
2. Find all the places where the tests add listeners, and throw in a
BadListener as well.
That should maximize the chances of finding any other notifyListener
type methods that have not been hardened.
Patricia
On 4/15/2011 11:45 AM, Patricia Shanahan wrote:
> Thanks for looking at this. I agree with you about the impenetrability
> of the test code - that is why I'm working by modifying existing tests
> rather than just writing the tests I need from scratch.
>
> If this becomes the limiting factor on a release, I'll check in the
> patch without a QA test.
>
> However, I am strongly in favor of QA tests for the sake of regression
> testing. I want Hudson to tell us if some future change brings back the
> vulnerability to listener exceptions.
>
> Patricia
>
>
> On 4/15/2011 11:21 AM, Christopher Dolan wrote:
>> Patricia,
>>
>> I looked at the existing AddNewDiscoveryListener and agree with your
>> assessment. But the test code is nigh impenetrable, I have to say. It
>> would be beneficial in the long run if some of the common code in
>> LookupDiscovery and LookupLocatorDiscovery were broken apart into
>> separately testable units... I tried writing my own test case, but it
>> doesn't seem possible to mock any of the providers, so to test it you
>> need an actual registrar, it appears.
>>
>> For what it's worth, today I wrote my own private test that runs in my
>> djinn and verified the patch.
>>
>> Chris
>>
>>
>> -----Original Message-----
>> From: Patricia Shanahan [mailto:pats@acm.org]
>> Sent: Thursday, April 14, 2011 7:41 PM
>> To: river-dev@incubator.apache.org
>> Subject: Test anomaly
>>
>> I'm attempting to construct QA tests for the RIVER-395 by copying
>> existing tests of event delivery and adding a special listener that
>> throws exceptions. If the patch is correct, the modified test should
>> pass with all expected events going to the existing listeners despite
>> the exceptions.
>>
>> I'm running my tests with 'com.sun.jini.qa.harness.level = FINEST'
>> logging, so that I can assure myself that the exceptions are being
>> thrown and events delivered after an exception.
>>
>> I tried to base a test on
>> com/sun/jini/test/spec/discoverymanager/AddNewDiscoveryListener.td, and
>> got no events.
>>
>> I've since run the complete discoverymanager category. From the logs, I
>> got no change events, and only 4 tests appeared to have non-zero numbers
>>
>> of initial events.
>>
>> Could someone else take a look at this and see if this is as expected?
>> It looked to me from the comments in AddNewDiscoveryListener.java as
>> though processing a non-zero number of events is important to the test.
>>
>> Patricia
>>
>
Re: Test anomaly
Posted by Patricia Shanahan <pa...@acm.org>.
Thanks for looking at this. I agree with you about the impenetrability
of the test code - that is why I'm working by modifying existing tests
rather than just writing the tests I need from scratch.
If this becomes the limiting factor on a release, I'll check in the
patch without a QA test.
However, I am strongly in favor of QA tests for the sake of regression
testing. I want Hudson to tell us if some future change brings back the
vulnerability to listener exceptions.
Patricia
On 4/15/2011 11:21 AM, Christopher Dolan wrote:
> Patricia,
>
> I looked at the existing AddNewDiscoveryListener and agree with your
> assessment. But the test code is nigh impenetrable, I have to say. It
> would be beneficial in the long run if some of the common code in
> LookupDiscovery and LookupLocatorDiscovery were broken apart into
> separately testable units... I tried writing my own test case, but it
> doesn't seem possible to mock any of the providers, so to test it you
> need an actual registrar, it appears.
>
> For what it's worth, today I wrote my own private test that runs in my
> djinn and verified the patch.
>
> Chris
>
>
> -----Original Message-----
> From: Patricia Shanahan [mailto:pats@acm.org]
> Sent: Thursday, April 14, 2011 7:41 PM
> To: river-dev@incubator.apache.org
> Subject: Test anomaly
>
> I'm attempting to construct QA tests for the RIVER-395 by copying
> existing tests of event delivery and adding a special listener that
> throws exceptions. If the patch is correct, the modified test should
> pass with all expected events going to the existing listeners despite
> the exceptions.
>
> I'm running my tests with 'com.sun.jini.qa.harness.level = FINEST'
> logging, so that I can assure myself that the exceptions are being
> thrown and events delivered after an exception.
>
> I tried to base a test on
> com/sun/jini/test/spec/discoverymanager/AddNewDiscoveryListener.td, and
> got no events.
>
> I've since run the complete discoverymanager category. From the logs, I
> got no change events, and only 4 tests appeared to have non-zero numbers
>
> of initial events.
>
> Could someone else take a look at this and see if this is as expected?
> It looked to me from the comments in AddNewDiscoveryListener.java as
> though processing a non-zero number of events is important to the test.
>
> Patricia
>
RE: Test anomaly
Posted by Christopher Dolan <ch...@avid.com>.
Patricia,
I looked at the existing AddNewDiscoveryListener and agree with your
assessment. But the test code is nigh impenetrable, I have to say. It
would be beneficial in the long run if some of the common code in
LookupDiscovery and LookupLocatorDiscovery were broken apart into
separately testable units... I tried writing my own test case, but it
doesn't seem possible to mock any of the providers, so to test it you
need an actual registrar, it appears.
For what it's worth, today I wrote my own private test that runs in my
djinn and verified the patch.
Chris
-----Original Message-----
From: Patricia Shanahan [mailto:pats@acm.org]
Sent: Thursday, April 14, 2011 7:41 PM
To: river-dev@incubator.apache.org
Subject: Test anomaly
I'm attempting to construct QA tests for the RIVER-395 by copying
existing tests of event delivery and adding a special listener that
throws exceptions. If the patch is correct, the modified test should
pass with all expected events going to the existing listeners despite
the exceptions.
I'm running my tests with 'com.sun.jini.qa.harness.level = FINEST'
logging, so that I can assure myself that the exceptions are being
thrown and events delivered after an exception.
I tried to base a test on
com/sun/jini/test/spec/discoverymanager/AddNewDiscoveryListener.td, and
got no events.
I've since run the complete discoverymanager category. From the logs, I
got no change events, and only 4 tests appeared to have non-zero numbers
of initial events.
Could someone else take a look at this and see if this is as expected?
It looked to me from the comments in AddNewDiscoveryListener.java as
though processing a non-zero number of events is important to the test.
Patricia