Move check for leaked shells to CloseTestWindowsRule

The UITestCase contains a leaked shell check that would be better places
in a test rule and fits to the already existing CloseTestWindowsRule.

This change integrates the leaked shell check into the
CloseTestWindowsRule.

Author: HeikoKlare

tests/org.eclipse.ui.tests.harness/src/org/eclipse/ui/tests/harness/util/CloseTestWindowsRule.java

@@ -16,30 +16,40 @@
 package org.eclipse.ui.tests.harness.util;
 
 import static org.eclipse.ui.tests.harness.util.UITestUtil.processEvents;
+import static org.junit.Assert.assertEquals;
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Set;
 
+import org.eclipse.swt.widgets.Shell;
 import org.eclipse.ui.IWindowListener;
 import org.eclipse.ui.IWorkbenchWindow;
 import org.eclipse.ui.PlatformUI;
 import org.junit.rules.ExternalResource;
 
+/**
+ * Rule that close windows opened during the test case and checks for shells
+ * unintentionally leaked from the test case.
+ */
 public class CloseTestWindowsRule extends ExternalResource {
 
-	private boolean enabled = true;
-
 	private final List<IWorkbenchWindow> testWindows;
 
 	private TestWindowListener windowListener;
 
+	private Set<Shell> initialShells;
+
+	private boolean leakChecksDisabled;
+
 	public CloseTestWindowsRule() {
 		testWindows = new ArrayList<>(3);
 	}
 
 	@Override
 	protected void before() throws Exception {
 		addWindowListener();
+		storeInitialShells();
 	}
 
 	@Override
@@ -48,6 +58,7 @@ protected void after() {
 		processEvents();
 		closeAllTestWindows();
 		processEvents();
+		checkForLeakedShells();
 	}
 
 	/**
@@ -78,11 +89,7 @@ private void closeAllTestWindows() {
 		testWindows.clear();
 	}
 
-	public void setEnabled(boolean enabled) {
-		this.enabled = enabled;
-	}
-
-	class TestWindowListener implements IWindowListener {
+	private class TestWindowListener implements IWindowListener {
 		@Override
 		public void windowActivated(IWorkbenchWindow window) {
 			// do nothing
@@ -95,16 +102,36 @@ public void windowDeactivated(IWorkbenchWindow window) {
 
 		@Override
 		public void windowClosed(IWorkbenchWindow window) {
-			if (enabled) {
-				testWindows.remove(window);
-			}
+			testWindows.remove(window);
 		}
 
 		@Override
 		public void windowOpened(IWorkbenchWindow window) {
-			if (enabled) {
-				testWindows.add(window);
+			testWindows.add(window);
+		}
+	}
+
+	private void storeInitialShells() {
+		this.initialShells = Set.of(PlatformUI.getWorkbench().getDisplay().getShells());
+	}
+
+	private void checkForLeakedShells() {
+		List<String> leakedModalShellTitles = new ArrayList<>();
+		Shell[] shells = PlatformUI.getWorkbench().getDisplay().getShells();
+		for (Shell shell : shells) {
+			if (!shell.isDisposed() && !initialShells.contains(shell)) {
+				leakedModalShellTitles.add(shell.getText());
+				shell.close();
 			}
 		}
+		if (!leakChecksDisabled) {
+			assertEquals("Test leaked modal shell: [" + String.join(", ", leakedModalShellTitles) + "]", 0,
+					leakedModalShellTitles.size());
+		}
 	}
+
+	public void disableLeakChecks() {
+		this.leakChecksDisabled = true;
+	}
+
 }

tests/org.eclipse.ui.tests.harness/src/org/eclipse/ui/tests/harness/util/UITestCase.java

@@ -16,12 +16,6 @@
  *******************************************************************************/
 package org.eclipse.ui.tests.harness.util;
 
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Set;
-
-import org.eclipse.swt.widgets.Shell;
-import org.eclipse.ui.PlatformUI;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Rule;
@@ -46,8 +40,6 @@ public abstract class UITestCase extends TestCase {
 	 */
 	private final CloseTestWindowsRule closeTestWindows = new CloseTestWindowsRule();
 
-	private Set<Shell> preExistingShells;
-
 	/**
 	 * Required to preserve the existing logging output when running tests with
 	 * {@link BlockJUnit4ClassRunner}.
@@ -97,11 +89,9 @@ protected void trace(String msg) {
 	public final void setUp() throws Exception {
 		super.setUp();
 		closeTestWindows.before();
-		this.preExistingShells = Set.of(PlatformUI.getWorkbench().getDisplay().getShells());
 		String name = runningTest != null ? runningTest : this.getName();
 		trace(TestRunLogUtil.formatTestStartMessage(name));
 		doSetUp();
-
 	}
 
 	/**
@@ -129,18 +119,7 @@ public final void tearDown() throws Exception {
 		String name = runningTest != null ? runningTest : this.getName();
 		trace(TestRunLogUtil.formatTestFinishedMessage(name));
 		doTearDown();
-
-		// Check for shell leak.
-		List<String> leakedModalShellTitles = new ArrayList<>();
-		Shell[] shells = PlatformUI.getWorkbench().getDisplay().getShells();
-		for (Shell shell : shells) {
-			if (!shell.isDisposed() && !preExistingShells.contains(shell)) {
-				leakedModalShellTitles.add(shell.getText());
-				shell.close();
-			}
-		}
-		assertEquals("Test leaked modal shell: [" + String.join(", ", leakedModalShellTitles) + "]", 0,
-				leakedModalShellTitles.size());
+		closeTestWindows.after();
 	}
 
 	/**
@@ -151,7 +130,6 @@ public final void tearDown() throws Exception {
 	 * Subclasses may extend.
 	 */
 	protected void doTearDown() throws Exception {
-		closeTestWindows.after();
 	}
 
 }

tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/stress/OpenCloseTest.java

@@ -118,6 +118,9 @@ public void testOpenCloseWorkbenchWindow() throws WorkbenchException {
 	 */
 	@Test
 	public void testOpenClosePerspective() {
+		// Reopening the perspective will create a new popup shell that would be
+		// detected as leak
+		closeTestWindows.disableLeakChecks();
 		ICommandService commandService = workbench.getService(ICommandService.class);
 		Command command = commandService.getCommand("org.eclipse.ui.window.closePerspective");
 
@@ -131,11 +134,11 @@ public void testOpenClosePerspective() {
 
 		for (int index = 0; index < numIterations; index++) {
 			try {
-				PlatformUI.getWorkbench().showPerspective(ORG_ECLIPSE_RESOURCE_PERSPECTIVE, workbenchWindow);
 				try {
 					handlerService.executeCommand(pCommand, null);
 				} catch (ExecutionException | NotDefinedException | NotEnabledException | NotHandledException e1) {
 				}
+				PlatformUI.getWorkbench().showPerspective(ORG_ECLIPSE_RESOURCE_PERSPECTIVE, workbenchWindow);
 			} catch (WorkbenchException e) {
 				e.printStackTrace();
 			}
@@ -148,8 +151,6 @@ public void testOpenClosePerspective() {
 	@Test
 	public void testOpenCloseView() throws WorkbenchException {
 		IViewPart consoleView;
-		IWorkbenchPage page = PlatformUI.getWorkbench().showPerspective(ORG_ECLIPSE_RESOURCE_PERSPECTIVE,
-				workbenchWindow);
 		for (int index = 0; index < numIterations; index++) {
 			consoleView = page.showView(IPageLayout.ID_MINIMAP_VIEW);
 			page.hideView(consoleView);