Skip to content

Commit

Permalink
Fix AbstractJobTest#waitForCompletion() and Bug_32039#testBug() #1115
Browse files Browse the repository at this point in the history
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
  • Loading branch information
HeikoKlare committed Jan 24, 2024
1 parent e9f9536 commit 583ce4a
Show file tree
Hide file tree
Showing 5 changed files with 50 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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));
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
}

/**
Expand Down Expand Up @@ -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};
Expand Down Expand Up @@ -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]);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
}

Expand Down Expand Up @@ -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);
}
Expand Down

0 comments on commit 583ce4a

Please sign in to comment.