Fix AbstractJobTest#waitForCompletion() and Bug_32039#testBug() #1115

The test case Bug_32039#testBug() randomly fails because
* AbstractJobTest#waitForCompletion() is can succeed even if the job is
not complete and
* Bug_32039#testBug() does not ensure a proper execution order of rule
acquisitions

This change fixes the waitForCompletion() method in terms of enforcing a
Duration instead of an integer to be passed as a timeout to avoid faulty
units and in terms of really throwing an exception of the job does not
complete (in time). It also ensures that the rule acquisition in the
Bug_32039#testBug() method always happens in the same order.
Since the correction of waitForCompletion() reveals that
IJobManagerTest#testBug57656() and IJobManagerTest#testScheduleRace()
contained bugs that made the test rely on waitForCompletion() not
working properly, they are fixed as well by correcting the job execution
times and terminate conditions.

Fixes #1115

Author: HeikoKlare

runtime/tests/org.eclipse.core.tests.runtime/src/org/eclipse/core/tests/runtime/jobs/AbstractJobTest.java

@@ -14,9 +14,10 @@
  *******************************************************************************/
 package org.eclipse.core.tests.runtime.jobs;
 
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
 
+import java.time.Duration;
 import org.eclipse.core.internal.jobs.JobListeners;
 import org.eclipse.core.internal.jobs.JobManager;
 import org.eclipse.core.runtime.jobs.IJobManager;
@@ -43,28 +44,25 @@ protected void sleep(long duration) {
 
 	/**
 	 * Ensures job completes within the given time.
-	 * @param waitTime time in milliseconds
 	 */
-	protected void waitForCompletion(Job job, int waitTime) {
-		int i = 0;
-		int tickLength = 1;
-		int ticks = waitTime / tickLength;
-		long start = now();
-		while (job.getState() != Job.NONE && now() - start < waitTime) {
-			sleep(tickLength);
-			// sanity test to avoid hanging tests
-			if (i++ > ticks && now() - start > waitTime) {
-				dumpState();
-				assertTrue("Timeout waiting for job to complete", false);
-			}
+	protected void waitForCompletion(Job job, Duration timeoutDuration) {
+		Duration startTime = Duration.ofMillis(now());
+		Duration timeout = startTime.plus(timeoutDuration);
+		while (job.getState() != Job.NONE && !timeout.minusMillis(now()).isNegative()) {
+			Thread.yield();
+		}
+		int finalJobState = job.getState();
+		if (finalJobState != Job.NONE) {
+			dumpState();
+			assertThat(finalJobState).as("timeout waiting for job to complete").isEqualTo(Job.NONE);
 		}
 	}
 
 	/**
 	 * Ensures given job completes within a second.
 	 */
 	protected void waitForCompletion(Job job) {
-		waitForCompletion(job, 1000);
+		waitForCompletion(job, Duration.ofSeconds(1));
 	}
 
 	/**

runtime/tests/org.eclipse.core.tests.runtime/src/org/eclipse/core/tests/runtime/jobs/Bug_320329.java

@@ -13,10 +13,14 @@
  *******************************************************************************/
 package org.eclipse.core.tests.runtime.jobs;
 
+import java.time.Duration;
+import org.eclipse.core.runtime.IProgressMonitor;
+import org.eclipse.core.runtime.IStatus;
 import org.eclipse.core.runtime.NullProgressMonitor;
 import org.eclipse.core.runtime.jobs.ISchedulingRule;
 import org.eclipse.core.runtime.jobs.Job;
 import org.eclipse.core.runtime.jobs.MultiRule;
+import org.eclipse.core.tests.harness.TestBarrier2;
 import org.eclipse.core.tests.harness.TestJob;
 import org.junit.Test;
 
@@ -27,22 +31,30 @@ public class Bug_320329 extends AbstractJobTest {
 
 	@Test
 	public void testBug() {
+		TestBarrier2 job2State = new TestBarrier2();
 		Job j1 = new TestJob("job1", 10, 5);// 50 ms
-		Job j2 = new TestJob("job2");
+		Job j2 = new TestJob("job2") {
+			@Override
+			public IStatus run(IProgressMonitor monitor) {
+				job2State.upgradeTo(TestBarrier2.STATUS_RUNNING);
+				return super.run(monitor);
+			}
+		};
 		ISchedulingRule rule1 = new IdentityRule();
 		ISchedulingRule rule2 = new IdentityRule();
 
 		j1.setRule(rule1);
 		j2.setRule(MultiRule.combine(rule1, rule2));
 		j1.schedule();
 		j2.schedule();
+		job2State.waitForStatus(TestBarrier2.STATUS_RUNNING);
 
 		// busy wait here
 		Job.getJobManager().beginRule(rule2, new NullProgressMonitor());
 
 		// Clean up
 		Job.getJobManager().endRule(rule2);
-		waitForCompletion(j1, 60);
-		waitForCompletion(j2, 60);
+		waitForCompletion(j1, Duration.ofMillis(60));
+		waitForCompletion(j2, Duration.ofMillis(60));
 	}
 }

runtime/tests/org.eclipse.core.tests.runtime/src/org/eclipse/core/tests/runtime/jobs/IJobManagerTest.java

@@ -21,6 +21,7 @@
 import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 
+import java.time.Duration;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
@@ -397,16 +398,19 @@ public void testBug48073() {
 	 */
 	@Test
 	public void testBug57656() {
-		TestJob jobA = new TestJob("Job1");
-		TestJob jobB = new TestJob("Job2");
+		TestJob jobA = new TestJob("Job1", 10000, 10);
+		TestJob jobB = new TestJob("Job2", 1, 1);
 		//schedule jobA
-		jobA.schedule(50);
+		jobA.schedule(100);
 		//schedule jobB so it gets behind jobA in the queue
-		jobB.schedule(100);
+		jobB.schedule(101);
 		//now put jobA to sleep indefinitely
 		jobA.sleep();
-		//jobB should still run within ten seconds
-		waitForCompletion(jobB, 300);
+		// jobB should still run even though jobA scheduled before did not start
+		waitForCompletion(jobB, Duration.ofSeconds(1));
+		jobA.terminate();
+		jobA.cancel();
+		waitForCompletion(jobA, Duration.ofSeconds(5));
 	}
 
 	/**
@@ -1744,10 +1748,13 @@ public void testReverseOrder() throws InterruptedException {
 	}
 
 	/**
-	 * Tests conditions where there is a race to schedule the same job multiple times.
+	 * Tests conditions where there is a race to schedule the same job multiple
+	 * times.
+	 *
+	 * @throws InterruptedException
 	 */
 	@Test
-	public void testScheduleRace() {
+	public void testScheduleRace() throws InterruptedException {
 		final int[] count = new int[1];
 		final boolean[] running = new boolean[] {false};
 		final boolean[] failure = new boolean[] {false};
@@ -1784,7 +1791,8 @@ public void scheduled(IJobChangeEvent event) {
 			}
 		});
 		testJob.schedule();
-		waitForCompletion(testJob, 5000);
+		testJob.join();
+		waitForCompletion(testJob, Duration.ofSeconds(5));
 		assertTrue("1.0", !failure[0]);
 	}
 

runtime/tests/org.eclipse.core.tests.runtime/src/org/eclipse/core/tests/runtime/jobs/JobGroupTest.java

@@ -24,6 +24,7 @@
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
+import java.time.Duration;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -1441,7 +1442,7 @@ public IStatus run(IProgressMonitor monitor) {
 		};
 		job.setJobGroup(jobGroup);
 		job.schedule();
-		waitForCompletion(job, 100);
+		waitForCompletion(job, Duration.ofMillis(100));
 
 		boolean completed = jobGroup.join(1000, null);
 		assertTrue("2.0", completed);

runtime/tests/org.eclipse.core.tests.runtime/src/org/eclipse/core/tests/runtime/jobs/YieldTest.java

@@ -19,6 +19,7 @@
 import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 
+import java.time.Duration;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
@@ -430,7 +431,7 @@ public void run() {
 		barrier.waitForStatus(TestBarrier2.STATUS_START);
 		t.start();
 
-		waitForCompletion(yielding, 5000);
+		waitForCompletion(yielding, Duration.ofSeconds(5));
 		assertTrue(yielding.getResult().isOK());
 	}
 
@@ -518,7 +519,7 @@ protected IStatus run(IProgressMonitor monitor) {
 		barrier.waitForStatus(TestBarrier2.STATUS_START);
 		conflicting.schedule();
 
-		waitForCompletion(conflicting, 5000);
+		waitForCompletion(conflicting, Duration.ofSeconds(5));
 		assertTrue(conflicting.getResult().isOK());
 		barrier.waitForStatus(TestBarrier2.STATUS_BLOCKED);
 	}