Bug 361121 - [Progress]DetailedProgressViewer's comparator violatesv20111221-1916

its general contract

Enhance the JobInfo compareTo

4 files changed, 383 insertions, 7 deletions

diff --git a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/progress/JobInfo.java b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/progress/JobInfo.java
index 5a59787..4430152 100644
--- a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/progress/JobInfo.java
+++ b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/progress/JobInfo.java
@@ -8,6 +8,7 @@
  * Contributors:
  *     IBM Corporation - initial API and implementation
  *     Brock Janiczak <brockj@tpg.com.au> - Fix for Bug 123169 [Progress] NPE from JobInfo
+ *     Martin W. Kirst <martin.kirst@s1998.tu-chemnitz.de> - jUnit test for Bug 361121 [Progress] DetailedProgressViewer's comparator violates its general contract
  *******************************************************************************/
 package org.eclipse.ui.internal.progress;
 
@@ -53,7 +54,7 @@ public class JobInfo extends JobTreeElement {
      * 
      * @param enclosingJob
      */
-    JobInfo(Job enclosingJob) {
+	protected JobInfo(Job enclosingJob) {
         this.job = enclosingJob;
     }
 
@@ -154,10 +155,12 @@ public class JobInfo extends JobTreeElement {
 			}
         }
 
+        // If equal prio, order by names
         if (job.getPriority() == job2.getPriority()) {
             return job.getName().compareTo(job2.getName());
         }
 
+        // order by priority
         if (job.getPriority() > job2.getPriority()) {
 			return -1;
 		}
@@ -181,15 +184,16 @@ public class JobInfo extends JobTreeElement {
 			return 1;
 		}
 
-        if (element.getJob().getState() == getJob().getState()) {
-			return compareJobs(element);
-		}
+		int thisState = getJob().getState();
+		int anotherState = element.getJob().getState();
 
-        if (getJob().getState() == Job.RUNNING) {
-			return -1;
+		// if equal job state, compare other job attributes
+		if (thisState == anotherState) {
+			return compareJobs(element);
 		}
-        return 1;
 
+		// ordering by job states, Job.RUNNING should be ordered first
+		return (thisState > anotherState ? -1 : (thisState == anotherState ? 0 : 1));
     }
 
     /**
diff --git a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/progress/JobInfoTest.java b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/progress/JobInfoTest.java
new file mode 100644
index 0000000..04e6fc1
--- a/dev/null
+++ b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/progress/JobInfoTest.java
@@ -0,0 +1,235 @@
+/*******************************************************************************
+ * Copyright (c) 2011 IBM Corporation and others.
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ *     IBM Corporation - initial API and implementation
+ *     Martin W. Kirst <martin.kirst@s1998.tu-chemnitz.de> - jUnit test for Bug 361121 [Progress] DetailedProgressViewer's comparator violates its general contract
+ ******************************************************************************/
+
+package org.eclipse.ui.tests.progress;
+
+import java.lang.reflect.Field;
+import java.util.ArrayList;
+import java.util.List;
+
+import junit.framework.TestCase;
+
+import org.eclipse.core.internal.jobs.InternalJob;
+import org.eclipse.core.runtime.IProgressMonitor;
+import org.eclipse.core.runtime.IStatus;
+import org.eclipse.core.runtime.jobs.Job;
+import org.eclipse.ui.internal.progress.JobInfo;
+
+public class JobInfoTest extends TestCase {
+
+	
+	/**
+	 * @see org.eclipse.core.internal.jobs.InternalJob
+	 */
+	static final int ABOUT_TO_RUN = 0x10;
+	/**
+	 * @see org.eclipse.core.internal.jobs.InternalJob
+	 */
+	static final int ABOUT_TO_SCHEDULE = 0x20;
+	/**
+	 * @see org.eclipse.core.internal.jobs.InternalJob
+	 */
+	static final int BLOCKED = 0x08;
+	/**
+	 * @see org.eclipse.core.internal.jobs.InternalJob
+	 */
+	static final int YIELDING = 0x40;
+	
+	private List jobinfos = new ArrayList();
+	
+	/**
+	 * @throws java.lang.Exception
+	 */
+	protected void setUp() throws Exception {
+		int counter = 0;
+		counter = createAndAddJobInfos(false, false, ABOUT_TO_RUN, counter);
+		counter = createAndAddJobInfos(false, true,  ABOUT_TO_RUN, counter);
+		counter = createAndAddJobInfos(true, false,  ABOUT_TO_RUN, counter);
+		counter = createAndAddJobInfos(true, true,  ABOUT_TO_RUN, counter);
+		
+		counter = createAndAddJobInfos(false, false, ABOUT_TO_SCHEDULE, counter);
+		counter = createAndAddJobInfos(false, true, ABOUT_TO_SCHEDULE, counter);
+		counter = createAndAddJobInfos(true, false, ABOUT_TO_SCHEDULE, counter);
+		counter = createAndAddJobInfos(true, true, ABOUT_TO_SCHEDULE, counter);
+
+		counter = createAndAddJobInfos(false, false, Job.SLEEPING, counter);
+		counter = createAndAddJobInfos(false, true, Job.SLEEPING, counter);
+		counter = createAndAddJobInfos(true, false, Job.SLEEPING, counter);
+		counter = createAndAddJobInfos(true, true, Job.SLEEPING, counter);
+		
+		counter = createAndAddJobInfos(false, false, Job.WAITING, counter);
+		counter = createAndAddJobInfos(false, true, Job.WAITING, counter);
+		counter = createAndAddJobInfos(true, false, Job.WAITING, counter);
+		counter = createAndAddJobInfos(true, true, Job.WAITING, counter);
+		
+		counter = createAndAddJobInfos(false, false, Job.RUNNING, counter);
+		counter = createAndAddJobInfos(false, true, Job.RUNNING, counter);
+		counter = createAndAddJobInfos(true, false, Job.RUNNING, counter);
+		counter = createAndAddJobInfos(true, true, Job.RUNNING, counter);
+		
+	}
+
+	/**
+	 * Test that {@link org.eclipse.ui.internal.progress.JobInfo#compareTo(Object)}
+	 * is valid implemented and complies to the interface method contract.
+	 */
+	public void testCompareToContractCompliance() {
+		for(int xi = 0; xi<this.jobinfos.size(); xi++) {
+			JobInfo x = (JobInfo) jobinfos.get(xi);
+
+			for(int yi = 0; yi<this.jobinfos.size(); yi++) {
+				JobInfo y = (JobInfo) jobinfos.get(yi);
+				int xyResult = x.compareTo(y);
+				int yxResult = y.compareTo(x);
+				// sgn(compare(x, y)) == -sgn(compare(y, x)) for all x and y.
+				assertEquals(String.format("sgn(compare(%1$s, %2$s)) != -sgn(compare(%2$s, %1$s))", new Object[] { x, y}),
+						Math.round(Math.signum(xyResult)) , Math.round(-Math.signum(yxResult)));
+
+				for(int zi = 0; zi<this.jobinfos.size(); zi++) {
+					JobInfo z = (JobInfo) jobinfos.get(zi);
+					int xzResult = x.compareTo(z);
+					int yzResult = y.compareTo(z);
+					// ((compare(x, y)>0) && (compare(y, z)>0)) implies compare(x, z)>0.
+					if(xyResult > 0) {
+						if(yzResult > 0) {
+							assertTrue(String.format("((compare(%1$s, %2$s)>0) && (compare(%2$s, %3$s)>0)) but not compare(%1$s, %3$s)>0", new Object[] {x, y, z}),
+									xzResult > 0);
+						}
+					}
+					else if(xyResult == 0) {
+						// compare(x, y)==0 implies that sgn(compare(x, z))==sgn(compare(y, z)) for all z.
+						assertEquals(String.format("compare(%1$s, %2$s)==0 but not that sgn(compare(%1$s, %3$s))==sgn(compare(%2$s, %3$s))", new Object[]{ x, y, z}),
+								Math.round(Math.signum(xzResult)) , Math.round(Math.signum(yzResult)));
+					}
+				}
+
+				boolean consistentWithEquals = true;
+				// Optionally (compare(x, y)==0) == (x.equals(y))
+				if(consistentWithEquals && xyResult == 0) {
+					assertTrue(String.format("compare(%1$s, %2$s)==0) == (%1$s.equals(%2$s)", new Object[] {x, y}),
+							x.equals(y));
+				}
+			}
+		}
+	}
+	
+	/**
+	 * @param user
+	 * @param system
+	 * @param jobstate
+	 * @param counter
+	 * @return
+	 */
+	private int createAndAddJobInfos(boolean user, boolean system, int jobstate, int counter) {
+		TestJob job;
+		JobInfo ji;
+		
+		job = new TestJob("Job" + (counter++));
+		job.setUser(user);
+		job.setSystem(system);
+		job.setPriority(Job.INTERACTIVE);
+		job.setInternalJobState(jobstate);
+		ji = new ExtendedJobInfo(job);
+		jobinfos.add(ji);
+		
+		job = new TestJob("Job" + (counter++));
+		job.setUser(user);
+		job.setSystem(system);
+		job.setPriority(Job.SHORT);
+		job.setInternalJobState(jobstate);
+		ji = new ExtendedJobInfo(job);
+		jobinfos.add(ji);
+		
+		job = new TestJob("Job" + (counter++));
+		job.setUser(user);
+		job.setSystem(system);
+		job.setPriority(Job.LONG);
+		job.setInternalJobState(jobstate);
+		ji = new ExtendedJobInfo(job);
+		jobinfos.add(ji);
+		
+		job = new TestJob("Job" + (counter++));
+		job.setUser(user);
+		job.setSystem(system);
+		job.setPriority(Job.BUILD);
+		job.setInternalJobState(jobstate);
+		ji = new ExtendedJobInfo(job);
+		jobinfos.add(ji);
+		
+		job = new TestJob("Job" + (counter++));
+		job.setUser(user);
+		job.setSystem(system);
+		job.setPriority(Job.DECORATE);
+		job.setInternalJobState(jobstate);
+		ji = new ExtendedJobInfo(job);
+		jobinfos.add(ji);
+		
+		job = new TestJob("Job" + (counter++));
+		job.setUser(user);
+		job.setSystem(system);
+		job.setPriority(Job.LONG);
+		job.setInternalJobState(jobstate);
+		ji = new ExtendedJobInfo(job);
+		jobinfos.add(ji);
+		
+		return counter;
+	}
+
+	/*
+	 * ========================================================================
+	 */
+
+	/**
+	 * Only provides better readable {@link #toString()} method.
+	 */
+	private static class ExtendedJobInfo extends JobInfo {
+
+		public ExtendedJobInfo(Job enclosingJob) {
+			super(enclosingJob);
+		}
+
+		public String toString() {
+			return "ExtendedJobInfo [getName()=" + getJob().getName() + ", getPriority()="
+						+ getJob().getPriority() + ", getState()=" + getJob().getState()
+						+ ", isSystem()=" + getJob().isSystem() + ", isUser()=" + getJob().isUser()
+						+ "]";
+		}
+		
+	}
+	
+	/**
+	 * Enables access to internal state, by using reflection
+	 * Provides better readable {@link #toString()} method.
+	 */
+	private static class TestJob extends Job {
+
+		public TestJob(String name) {
+			super(name);
+		}
+
+		protected IStatus run(IProgressMonitor monitor) {
+			throw new UnsupportedOperationException("Not implemented, because of just a unit test");
+		}
+		
+		public void setInternalJobState(int state) {
+			try {
+				final Field field = InternalJob.class.getDeclaredField("flags");
+				field.setAccessible(true); // hack for testing
+				field.set(this, new Integer(state));
+			} catch (Exception e) {
+				e.printStackTrace();
+				throw new RuntimeException(e);
+			}
+		}
+
+	}
+}
diff --git a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/progress/JobInfoTestOrdering.java b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/progress/JobInfoTestOrdering.java
new file mode 100644
index 0000000..c56f1c7
--- a/dev/null
+++ b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/progress/JobInfoTestOrdering.java
@@ -0,0 +1,135 @@
+/*******************************************************************************
+ * Copyright (c) 2011 IBM Corporation and others.
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ *     IBM Corporation - initial API and implementation
+ *     Martin W. Kirst <martin.kirst@s1998.tu-chemnitz.de> - jUnit test for Bug 361121 [Progress] DetailedProgressViewer's comparator violates its general contract
+ ******************************************************************************/
+
+package org.eclipse.ui.tests.progress;
+
+import java.lang.reflect.Field;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+import junit.framework.TestCase;
+
+import org.eclipse.core.internal.jobs.InternalJob;
+import org.eclipse.core.runtime.IProgressMonitor;
+import org.eclipse.core.runtime.IStatus;
+import org.eclipse.core.runtime.jobs.Job;
+import org.eclipse.ui.internal.progress.JobInfo;
+
+public class JobInfoTestOrdering extends TestCase {
+
+	private List jobinfos = new ArrayList();
+	
+	/**
+	 * @throws java.lang.Exception
+	 */
+	protected void setUp() throws Exception {
+		jobinfos.clear();
+		int counter = 0;
+		TestJob job;
+		JobInfo ji;
+		
+		job = new TestJob("Job" + (counter++));
+		job.setUser(true);
+		job.setSystem(false);
+		job.setPriority(Job.INTERACTIVE);
+		job.setInternalJobState(Job.NONE);  // JOB STATE
+		ji = new ExtendedJobInfo(job);
+		jobinfos.add(ji);
+		
+		job = new TestJob("Job" + (counter++));
+		job.setUser(true);
+		job.setSystem(false);
+		job.setPriority(Job.INTERACTIVE);
+		job.setInternalJobState(Job.SLEEPING);  // JOB STATE
+		ji = new ExtendedJobInfo(job);
+		jobinfos.add(ji);
+		
+		job = new TestJob("Job" + (counter++));
+		job.setUser(true);
+		job.setSystem(false);
+		job.setPriority(Job.INTERACTIVE);
+		job.setInternalJobState(Job.WAITING);  // JOB STATE
+		ji = new ExtendedJobInfo(job);
+		jobinfos.add(ji);
+		
+		job = new TestJob("Job" + (counter++));
+		job.setUser(true);
+		job.setSystem(false);
+		job.setPriority(Job.INTERACTIVE);
+		job.setInternalJobState(Job.RUNNING);  // JOB STATE
+		ji = new ExtendedJobInfo(job);
+		jobinfos.add(ji);
+		
+	}
+
+	/**
+	 * Test that checks when jobs sorted by their state, the running ones
+	 * are ordered to first place
+	 */
+	public void testJobStateOrdering() {
+		Collections.sort(jobinfos);
+		assertEquals(Job.RUNNING,  ((JobInfo)jobinfos.get(0)).getJob().getState());
+		assertEquals(Job.WAITING,  ((JobInfo)jobinfos.get(1)).getJob().getState());
+		assertEquals(Job.SLEEPING, ((JobInfo)jobinfos.get(2)).getJob().getState());
+		assertEquals(Job.NONE,     ((JobInfo)jobinfos.get(3)).getJob().getState());
+	}
+	
+	/*
+	 * ========================================================================
+	 */
+
+	/**
+	 * Only provides better readable {@link #toString()} method.
+	 */
+	private static class ExtendedJobInfo extends JobInfo {
+
+		public ExtendedJobInfo(Job enclosingJob) {
+			super(enclosingJob);
+		}
+
+		public String toString() {
+			return "ExtendedJobInfo [getName()=" + getJob().getName() + ", getPriority()="
+						+ getJob().getPriority() + ", getState()=" + getJob().getState()
+						+ ", isSystem()=" + getJob().isSystem() + ", isUser()=" + getJob().isUser()
+						+ "]";
+		}
+		
+	}
+	
+	/**
+	 * Enables access to internal state, by using reflection
+	 * Provides better readable {@link #toString()} method.
+	 */
+	private static class TestJob extends Job {
+
+		public TestJob(String name) {
+			super(name);
+		}
+
+		protected IStatus run(IProgressMonitor monitor) {
+			throw new UnsupportedOperationException("Not implemented, because of just a unit test");
+		}
+		
+		public void setInternalJobState(int state) {
+			try {
+				final Field field = InternalJob.class.getDeclaredField("flags");
+				field.setAccessible(true); // hack for testing
+				field.set(this, new Integer(state));
+			} catch (Exception e) {
+				e.printStackTrace();
+				throw new RuntimeException(e);
+			}
+		}
+
+	}
+}
diff --git a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/progress/ProgressTestSuite.java b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/progress/ProgressTestSuite.java
index b5185aa..eee8388 100644
--- a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/progress/ProgressTestSuite.java
+++ b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/progress/ProgressTestSuite.java
@@ -33,5 +33,7 @@ public class ProgressTestSuite extends TestSuite {
 	public ProgressTestSuite() {
 		addTest(new TestSuite(ProgressContantsTest.class));
 		addTest(new TestSuite(ProgressViewTests.class));
+		addTest(new TestSuite(JobInfoTest.class));
+		addTest(new TestSuite(JobInfoTestOrdering.class));
 	}
 }