VirtualTableViewerTest: improve table update + force active shell #1005

Several test cases of VirtualTableViewerTest randomly fail. This
includes explicit failures of testRenameWithSorter() and testContains()
and silent failures of other test cases that simply return successfully
even if the test actually failed. The reason is a missing processing of
an update of the table data, which is most likely caused by the shell
not having focus.

With this change, the test execution ensures that in cases where an
update of the table data is expected, the shell is forced active and
events are processed until the data is finally set properly. In
addition, early returns producing silent failures are replaced by
assertions leading to explicit failures in case the condition is still
not met.

Contributes to
#1583
May fix #1005

Author: HeikoKlare

tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/viewers/VirtualTableViewerTest.java

@@ -20,6 +20,7 @@
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 
+import java.time.Duration;
 import java.util.Arrays;
 import java.util.HashSet;
 import java.util.Set;
@@ -34,19 +35,14 @@
 import org.eclipse.swt.widgets.TableItem;
 import org.junit.Before;
 import org.junit.Ignore;
-import org.junit.Rule;
 import org.junit.Test;
-import org.junit.rules.TestName;
 
 /**
  * The TableViewerTest is a test of the SWT#VIRTUAL support in TableViewers,
  */
 public class VirtualTableViewerTest extends TableViewerTest {
 
-	@Rule
-	public TestName testName = new TestName();
-
-	Set<TableItem> visibleItems = new HashSet<>();
+	private static final Duration TABLE_DATA_UPDATE_TIMEOUT = Duration.ofSeconds(5);
 
 	/**
 	 * Checks if the virtual tree / table functionality can be tested in the current
@@ -57,13 +53,13 @@ public class VirtualTableViewerTest extends TableViewerTest {
 	 * automated tests to fail. See
 	 * https://bugs.eclipse.org/bugs/show_bug.cgi?id=118919 .
 	 */
-	protected boolean setDataCalled = false;
+	protected volatile boolean setDataCalled = false;
 
 	@Before
 	@Override
 	public void setUp() {
 		super.setUp();
-		processEvents(); // run events for SetData precondition test
+		waitForDataToBeSet();
 	}
 
 	@Override
@@ -73,32 +69,31 @@ protected int getShellStyle() {
 
 	@Override
 	protected TableViewer createTableViewer(Composite parent) {
-		visibleItems = new HashSet<>();
 		TableViewer viewer = new TableViewer(parent, SWT.VIRTUAL | SWT.MULTI);
 		viewer.setUseHashlookup(true);
 		final Table table = viewer.getTable();
-		table.addListener(SWT.SetData, event -> {
-			setDataCalled = true;
-			TableItem item = (TableItem) event.item;
-			visibleItems.add(item);
-		});
+		table.addListener(SWT.SetData, event -> setDataCalled = true);
 		return viewer;
 	}
 
 	/**
-	 * Checks if update occurred. Updates for virtual items will be skipped if, for
-	 * instance, another window is in the foreground.
-	 *
-	 * @return <code>true</code> if update occurred
+	 * Updates the table, also forcing the containing shell to be active in order to
+	 * process according events.
 	 */
-	protected boolean updateTable() {
+	private void updateTable() {
 		setDataCalled = false;
-		((TableViewer) fViewer).getControl().update();
-		if (setDataCalled) {
-			return true;
+		fViewer.refresh();
+		fViewer.getControl().update();
+		waitForDataToBeSet();
+	}
+
+	private void waitForDataToBeSet() {
+		Duration timeoutEnd = Duration.ofMillis(System.currentTimeMillis()).plus(TABLE_DATA_UPDATE_TIMEOUT);
+		while (!setDataCalled && !timeoutEnd.minusMillis(System.currentTimeMillis()).isNegative()) {
+			fShell.forceActive();
+			processEvents();
 		}
-		System.err.println("SWT.SetData is not received. Cancelled test " + testName.getMethodName());
-		return false;
+		assertTrue("waiting for setting table data timed out", setDataCalled);
 	}
 
 	/**
@@ -107,7 +102,7 @@ protected boolean updateTable() {
 	 * @return TableItem[]
 	 */
 	private TableItem[] getVisibleItems() {
-		return visibleItems.toArray(new TableItem[visibleItems.size()]);
+		return ((TableViewer) fViewer).getTable().getItems();
 	}
 
 	@Test
@@ -130,44 +125,24 @@ protected int getItemCount() {
 	@Override
 	public void testFilter() {
 		ViewerFilter filter = new TestLabelFilter();
-		visibleItems = new HashSet<>();
 		fViewer.addFilter(filter);
-		if (!updateTable()) {
-			return;
-		}
 		assertEquals("filtered count", 5, getItemCount());
 
-		visibleItems = new HashSet<>();
 		fViewer.removeFilter(filter);
-		if (!updateTable()) {
-			return;
-		}
 		assertEquals("unfiltered count", 10, getItemCount());
 	}
 
 	@Test
 	@Override
 	public void testSetFilters() {
 		ViewerFilter filter = new TestLabelFilter();
-		visibleItems = new HashSet<>();
 		fViewer.setFilters(filter, new TestLabelFilter2());
-		if (!updateTable()) {
-			return;
-		}
 		assertEquals("2 filters count", 1, getItemCount());
 
-		visibleItems = new HashSet<>();
 		fViewer.setFilters(filter);
-		if (!updateTable()) {
-			return;
-		}
 		assertEquals("1 filtered count", 5, getItemCount());
 
-		visibleItems = new HashSet<>();
 		fViewer.setFilters();
-		if (!updateTable()) {
-			return;
-		}
 		assertEquals("unfiltered count", 10, getItemCount());
 	}
 
@@ -204,21 +179,13 @@ public void testInsertSiblingWithSorter() {
 	@Test
 	@Override
 	public void testRenameWithFilter() {
-		if (!setDataCalled) {
-			System.err.println("SWT.SetData is not received. Cancelled test " + testName.getMethodName());
-			return;
-		}
 		fViewer.addFilter(new TestLabelFilter());
-		if (!updateTable()) {
-			return;
-		}
 		TestElement first = fRootElement.getFirstChild();
 		first.setLabel("name-1111"); // should disappear
-		((TableViewer) fViewer).getControl().update();
-		assertNull("changed sibling is not visible", fViewer.testFindItem(first));
+		updateTable();
+		assertNull("changed sibling is still visible", fViewer.testFindItem(first));
 		first.setLabel("name-2222"); // should reappear
-		fViewer.refresh();
-		((TableViewer) fViewer).getControl().update();
+		updateTable();
 		assertNotNull("changed sibling is not visible", fViewer.testFindItem(first));
 	}
 
@@ -230,14 +197,10 @@ public void testSetInput() {
 	@Test
 	@Override
 	public void testRenameWithSorter() {
-		// Call update to make sure the viewer is in a correct state
-		// At least on MacOSX I get failures without this call
-		((TableViewer) fViewer).getControl().update();
 		fViewer.setComparator(new TestLabelComparator());
 		TestElement first = fRootElement.getFirstChild();
 		first.setLabel("name-9999");
 		String newElementLabel = first.toString();
-		((TableViewer) fViewer).getControl().update();
 		assertEquals("sorted first", newElementLabel, getItemText(0));
 	}
 
@@ -249,15 +212,12 @@ public void testSorter() {
 		String firstLabel = first.toString();
 		String lastLabel = last.toString();
 
-		((TableViewer) fViewer).getControl().update();
 		assertEquals("unsorted", firstLabel, getItemText(0));
 		fViewer.setComparator(new TestLabelComparator());
 
-		((TableViewer) fViewer).getControl().update();
 		assertEquals("reverse sorted", lastLabel, getItemText(0));
 
 		fViewer.setComparator(null);
-		((TableViewer) fViewer).getControl().update();
 		assertEquals("unsorted", firstLabel, getItemText(0));
 	}