Bug 321397

Comment 1 Olivier Girardot 2010-07-30 14:55:10 EDT

Created attachment 175605 [details]
BlockingSignal test class

Comment 2 Olivier Girardot 2010-07-30 18:11:19 EDT

Created attachment 175629 [details]
Updated class - passing tests

There was problems with the eclipse @Override annotations, now everything's fine and the class is correctly eecuting during ant test execution

Comment 3 Steve Powell 2010-08-03 05:15:42 EDT

Thank  you Olivier.  I'm just reviewing your potential contribution now.

Please can you: 
----- extract from http://wiki.eclipse.org/Virgo/Community#essential_steps -----
In the bug the contributor (that's you) must confirm that they wrote 100% of the code contributed (in particular that none of it is copied from elsewhere), that they have the right to contribute the code to Eclipse (e.g. the employer agrees or else the code is produced in personal time and contracts do not assign ownership or copyright to the employer), and that any new files contain the appropriate License header with the contributor (or the employer, as appropriate) as the copyright owner and the contributor as the "initial contributor". (See existing files in the Virgo source code repositories for examples of the copyright and license headers.)
----- end of extract -----

Statements confirming these are required in this Bugzilla before I can push your contribution to the code base. I'll check the headers for you (please confirm who the copyright holder is).

If you make changes just submit another attachment (no need to delete the previous one -- I'll flag the one that is the contribution proper).  I'll be putting your email address on the commit under the author's name, so I'll need you to confirm that it is Olivier Giradot <ssaboum@gmail.com>.

This looks tedious, but is actually quite simple in practice.

Comment 4 Steve Powell 2010-08-03 06:43:06 EDT

I have reviewed the contribution.

The tests do drive the code they are to test, but unfortunately, if the code fails the tests do not detect this.  The junit test runner will not detect failure of Assert.* calls when they do not happen in the main (junit) thread. All the assertions in these tests are in new threads you create.

To demonstrate this, I forced one of the tests to fail by inserting:

   throw new FailureSignalledException(new Exception("test"));

after the assertTrue(returnSignal) in signalCompletionAfterNoFailure().  This should be caught by the run() code and trigger a fail() call.

Another way of provoking a failure is to change assertTrue to assertFalse in the same test. You never see the failure. (Actually you do see exceptions in the console log -- but the junit test runner does not detect it.)

It is a good idea to ensure that your tests fail before making them pass, ensuring that the test can detect failure correctly.

When running multiple threads in a junit test, it is important to return the success or failure to the junit runner thread.  Throwing assertion failure exceptions on the other threads will not be caught by junit runner.

I'll leave this to you to correct this and resubmit.

Comment 5 Glyn Normington 2010-08-03 06:56:43 EDT

Hi Olivier

Thanks for thinking of making this contribution - more tests are valuable to Virgo.

I'd like to encourage you to fix the test along the lines Steve suggested. You should be able to use standard Java concurrency constructs to get the main junit thread to wait for the launched thread, receive its results, and then check for the expected outcome using asserts etc.

Taking a look at your code, we noticed some gratuitous "finals", e.g.

    final boolean returnSignal ...

Please take a look at our coding guidelines so that your code looks more like normal Virgo code:

http://wiki.eclipse.org/Virgo/Committers#Coding

Regards,
Glyn

Comment 6 Olivier Girardot 2010-08-04 08:14:41 EDT

thank you for your reviews, so to take care of first things first :

- i assure you that this code (and its future corrected version) is 100% by me,  that i am the only copyright holder and that i have the right to contribute it to Eclipse. 

For the "initial contributor" part (of the license header) should i specify that it's me ?

- You're about the Assert being in other threads, what would be the best approach to check for the correct execution in these other threads :
  - use an Executor with a Future<Boolean> to check for the returnSignal ?
  - or a synchronized access to a common List/Vector of exceptions...

- i confirm that you can use this name for author's name : Olivier Girardot <ssaboum@gmail.com>

I'll fixed the code asap, 
Regards, 

Olivier.

Comment 7 Steve Powell 2010-08-04 12:36:34 EDT

(In reply to comment #6)
Yes, you are the contributor and your name goes in the header. I'll check this before we put the contribution in, so there is no real worry. Your confirmation of originality and ownership is perfectly fine (recorded here). Thanks.

Your question is really interesting, so I created a few pages on the Wiki to stimulate some discussion (http://wiki.eclipse.org/Virgo/Community/Testing_Extensions).

----------

A way forward for you would be a synchronisation mechanism (say a CountdownLatch which the main thread waits on) and a transfer of information mechanism (say a shared volatile variable with results or a boolean with a test evaluation value in it).  This is ad hoc, but see discussion above.

Be careful to use a very >simple< mechanism. We don;t want to have to debug the test harness too much!

Comment 8 Olivier Girardot 2010-08-06 18:43:35 EDT

Created attachment 176073 [details]
Cleaned up contribution file

Comment 9 Olivier Girardot 2010-08-06 18:45:02 EDT

I updated the file according the what was said before, 
cleaned up the dead store variable and final statements, and decided for Thread exception issue to create a custom Thread object that handle these Throwable with the "setUncaughtExceptionHandler" method available for any Thread object.

Thank you for your reviews

Comment 10 Steve Powell 2010-08-09 04:50:21 EDT

Thank you Olivier.  This is a candidate for review in this week's Sprint.

Comment 11 Steve Powell 2010-08-10 09:41:40 EDT

This contribution  is good, thank you.

I have made the following minor changes:
1) Copyright header.
2) Name of test class (ends in Tests -- this is one of our in-house coding standards)
3) Added check that test-thread terminates (t.join(0) could block forever).
4) Minor local name changes for sense.

SHA:	3f475aa14aa9a6b70736345143991471e3400a03
Author:	Olivier Girardot <ssaboum@gmail.com>
Date:	Tue Aug 10 2010 12:41:51 GMT+0100 (BST)
Committer:	Steve Powell <spowell@vmware.com>
Commit Date:	Tue Aug 10 2010 14:38:39 GMT+0100 (BST)
Subject:	New BlockingSignalTests contribution. (Bugzilla 321397).

Thank you for your contribution.

Comment 12 Steve Powell 2010-08-10 09:42:17 EDT

Comment on attachment 176073 [details]
Cleaned up contribution file

Flagged contribution for IP.

Comment 13 Steve Powell 2010-08-10 09:45:35 EDT

Created attachment 176238 [details]
Minor amendments to contribution before commit.

Attached amended version that goes into the code base.

Comment 14 Steve Powell 2010-08-10 09:46:29 EDT

Thank you for your contribution.

Comment 15 Olivier Girardot 2010-08-10 09:56:08 EDT

Thank you for your review

(In reply to comment #14)
> Thank you for your contribution.