Comment 1 Tod Creasey 2008-06-05 08:58:33 EDT

This looks like a timing issue with the test as it times out if the build takes too long.

Comment 2 Boris Bokowski 2009-03-12 10:24:46 EDT

We are still seeing this test failure in I builds. Eric and I looked at the test and think that the busy loop is probably not a very good idea, it should be something like

Display display = view.getSite().getShell().getDisplay();
while (waiting[0] && System.currentTimeMillis() < timeOut) {
	while(display.readAndDispatch()) {
		// consume all events
	}
	try {
		Thread.sleep(100);
	} catch {InterruptedException ex) {
		Thread.currentThread().interrupt();
	}
}

Comment 3 Eric Moffatt 2009-03-12 10:43:33 EDT

Created attachment 128567 [details]
Add a 'sleep(100)' stage to the loop


What I did to test this was to lower the timeout until the test failed most of the time (for me it was 105). Then re-ran the tests with the new loop and was successful all the time...

Comment 4 Eric Moffatt 2009-03-12 10:47:32 EDT

Oleg, could you review the patch please? We want to be sure it's safe before committing it this late...

Comment 5 Oleg Besedin 2009-03-12 12:31:02 EDT

It seems like it can help. 

I think the addition of the Thread#sleep() call can help if there are other background processes running when this test is executed.

Comment 6 Eric Moffatt 2009-03-12 12:37:07 EDT

Committed in >20090312. Applied the patch.

We tried this on Paul's linux box with timeout == 100 and it failed the test without the patch but succeeded with it.

Re-open this defect should this test ever fail again...

Comment 7 Boris Bokowski 2009-03-18 12:03:33 EDT

Happened again unfortunately:

http://download.eclipse.org/eclipse/downloads/drops/N20090317-2000/testresults/html/org.eclipse.ui.tests_linux.gtk.x86_6.0.html

Hitesh, can you have a look at this? This test is too brittle - do you have any idea for making it more stable?

Comment 8 Hitesh 2009-03-19 12:28:41 EDT

Created attachment 129371 [details]
steps to reproduce

The attachment show the code to reproduce the problem.

Comment 9 Hitesh 2009-03-19 12:35:34 EDT

Created attachment 129372 [details]
Fix v01

fix.

Comment 10 Hitesh 2009-05-01 04:07:14 EDT

Eric, could you please take a look at the patch.

Comment 11 Eric Moffatt 2009-05-01 10:23:43 EDT

Hitest, the patch looks fine...nice solution to use 'join' and let the manager handle figuring out when the job is done...

The only nits I have are about formatting...please put spaces around operators and assignments (I know it's hard to change styles your fingers just do what they're used to...;-). Also, the loop construction

int i = 0;
for (; i < markers.length; i++) {

is truly odd and should be

for (int i = 0; i < markers.length; i++) {

Comment 12 Eric Moffatt 2009-05-01 10:24:16 EDT

We'll put this in for RC1, we're done M7.

Comment 13 Boris Bokowski 2009-05-01 10:36:21 EDT

(In reply to comment #11)
> Also, the loop construction
> 
> int i = 0;
> for (; i < markers.length; i++) {
> 
> is truly odd and should be
> 
> for (int i = 0; i < markers.length; i++) {

Well, i is used later on in an assert, but maybe a while loop (instead of a for loop) would look more regular.

Comment 14 Eric Moffatt 2009-05-01 11:20:12 EDT

For clarity how about:

boolean markerFound = false;
for (int i = 0; i < markers.length; i++) {
  if(markers[i].equals(marker)){
     markerFound = true;
     break;
  }
}
assertTrue("No markers generated", markerFound);

This way the intent of the test is clearer I think.

Comment 15 Eric Moffatt 2009-05-05 13:28:27 EDT

Paul, the original patch looks alright but take a look at comment #14. Let me know and I'll spin another patch with the changes if you so desire.

Comment 16 Eric Moffatt 2009-05-05 13:29:32 EDT

Also, note that this test failed in last night's I-build...but passes on my box (but then it always does).

Comment 17 Paul Webster 2009-05-05 14:45:58 EDT

Looks good.  I would like the change to the loop added as well, then it is good.

PW

Comment 18 Eric Moffatt 2009-05-05 15:14:58 EDT

Created attachment 134497 [details]
Patch with the test changed


Committed in >20090505. Applied this patch.

Comment 19 Eric Moffatt 2009-05-05 15:15:58 EDT

Marking as fixed, thanks Hitesh!

Comment 20 Eric Moffatt 2009-05-06 11:29:22 EDT

Hitesh, can you check this with last night's I-build and VERIFY ?

Comment 21 Hitesh 2009-05-07 05:24:53 EDT

Verified in I20090506-2000 by running the test :)