Mailing-List: contact dev-help@river.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@river.apache.org
Received-SPF: neutral (athena.apache.org: local policy)
Content-class: urn:content-classes:message
MIME-Version: 1.0
Content-Type: text/plain;
	charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Subject: RE: Test anomaly
Date: Mon, 25 Apr 2011 17:45:06 -0400
Message-ID: 
 <77F1E32F67C8D5479858C0C7E93EB46506794F7A@WAL-MAIL.global.avidww.com>
In-Reply-To: <4DA8CAB0.2000506@acm.org>
Thread-Topic: Test anomaly
Thread-Index: Acv7vug6mZW2LEyJR8uKLY+ebifvtQH0l9aw
References: <4DA79411.6090909@acm.org>
 <77F1E32F67C8D5479858C0C7E93EB46506539B7F@WAL-MAIL.global.avidww.com>
 <4DA89258.8010203@acm.org> <4DA8CAB0.2000506@acm.org>
From: "Christopher Dolan" <christopher.dolan@avid.com>
To: "Patricia Shanahan" <pats@acm.org>,
	<dev@river.apache.org>

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]=20
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 =3D 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
>>
>