This is an automated email from the ASF dual-hosted git repository.
hansva pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/hop.git
The following commit(s) were added to refs/heads/main by this push:
new a30fb7c80f harden CI build edge cases, fixes #8890 (#8892)
a30fb7c80f is described below
commit a30fb7c80f70020641da747ade4cefbf6210fcec
Author: Hans Van Akelyen <[email protected]>
AuthorDate: Tue Oct 13 16:57:55 2026 +0200
harden CI build edge cases, fixes #8890 (#8892)
---
.../modules/ROOT/pages/testing/ui-testing.adoc | 4 ++
pom.xml | 70 ++++++++++++++++------
.../hop/ui/hopgui/HopGuiPeekInstanceTest.java | 15 +++++
.../hop/ui/hopgui/file/GraphCanvasTestBase.java | 4 ++
.../org/apache/hop/ui/core/gui/BaseGuiWidgets.java | 57 ++++++++++++++----
.../org/apache/hop/ui/core/gui/GuiMenuWidgets.java | 2 +-
.../apache/hop/ui/core/gui/GuiToolbarWidgets.java | 8 ++-
.../hop/ui/testing/DisabledGuiWidgetsTestBase.java | 5 +-
.../org/apache/hop/ui/testing/SwtBotTestBase.java | 5 ++
9 files changed, 137 insertions(+), 33 deletions(-)
diff --git a/docs/hop-dev-manual/modules/ROOT/pages/testing/ui-testing.adoc
b/docs/hop-dev-manual/modules/ROOT/pages/testing/ui-testing.adoc
index 456639a53f..a5d6ec4c70 100644
--- a/docs/hop-dev-manual/modules/ROOT/pages/testing/ui-testing.adoc
+++ b/docs/hop-dev-manual/modules/ROOT/pages/testing/ui-testing.adoc
@@ -41,6 +41,10 @@ A UI test needs a display, so how you run it matters.
With neither profile the UI tests are included whenever a display is available.
+The UI tests always run in a surefire execution of their own, `ui-test`, after
`default-test` has run the rest of the module.
+That gives them their own JVM: a display, shells or GUI singletons left behind
by a UI test cannot change what an ordinary test sees.
+Keep it that way, and do not write a test that only passes because a UI test
ran before it.
+
On Linux, wrap the command so the test shells do not steal your session:
[source,bash]
diff --git a/pom.xml b/pom.xml
index 61091b5fe5..9002638d34 100644
--- a/pom.xml
+++ b/pom.xml
@@ -200,8 +200,6 @@
<target.jdk.version>21</target.jdk.version>
<tm4e.core.version>0.17.2</tm4e.core.version>
<ui.test.argLine></ui.test.argLine>
- <ui.test.excludedGroups></ui.test.excludedGroups>
- <ui.test.includedGroups></ui.test.includedGroups>
</properties>
<dependencies>
@@ -447,27 +445,41 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
+ <configuration>
+ <argLine>@{argLine}
-Dfile.encoding=${project.build.sourceEncoding}
+ -DHOP_AUDIT_FOLDER=${project.build.directory}/audit
+ ${maven-surefire-plugin.argLine}
${ui.test.argLine}</argLine>
+ <forkCount>${maven-surefire-plugin.forkCount}</forkCount>
+
<forkedProcessTimeoutInSeconds>${maven-surefire-plugin.forkedProcessTimeoutInSeconds}</forkedProcessTimeoutInSeconds>
+
<reuseForks>${maven-surefire-plugin.reuseForks}</reuseForks>
+
<testFailureIgnore>${maven-surefire-plugin.testFailureIgnore}</testFailureIgnore>
+ </configuration>
<executions>
+ <!-- The @Tag("uitest") classes run in an execution, and
so a JVM, of their own.
+ They leave a Display, shells and GUI singletons
behind, and surefire's run
+ order follows the file system, so sharing a JVM made
the tests that ran
+ after them depend on the build agent. -->
<execution>
<id>default-test</id>
<goals>
<goal>test</goal>
</goals>
<configuration>
- <argLine>@{argLine}
-Dfile.encoding=${project.build.sourceEncoding}
-
-DHOP_AUDIT_FOLDER=${project.build.directory}/audit
- ${maven-surefire-plugin.argLine}
${ui.test.argLine}</argLine>
-
<forkCount>${maven-surefire-plugin.forkCount}</forkCount>
-
<forkedProcessTimeoutInSeconds>${maven-surefire-plugin.forkedProcessTimeoutInSeconds}</forkedProcessTimeoutInSeconds>
-
<reuseForks>${maven-surefire-plugin.reuseForks}</reuseForks>
-
<testFailureIgnore>${maven-surefire-plugin.testFailureIgnore}</testFailureIgnore>
- <groups>${ui.test.includedGroups}</groups>
-
<excludedGroups>${ui.test.excludedGroups}</excludedGroups>
+ <excludedGroups>uitest</excludedGroups>
<properties>
<includeTags>junit5</includeTags>
</properties>
</configuration>
</execution>
+ <execution>
+ <id>ui-test</id>
+ <goals>
+ <goal>test</goal>
+ </goals>
+ <configuration>
+ <groups>uitest</groups>
+ </configuration>
+ </execution>
</executions>
</plugin>
<plugin>
@@ -867,22 +879,46 @@
</profile>
<!-- uitest: run ONLY @Tag("uitest") classes (needs a display).
skip-uitest: exclude them from the reactor run.
- Default (neither): include them whenever a display is available.
+ Default (neither): run them whenever a display is available, in
the
+ ui-test execution after the other tests.
On a desktop wrap Maven with tools/with-isolated-display.sh so the
shells do not steal the interactive session. -->
<profile>
<id>uitest</id>
<properties>
<maven-surefire-plugin.forkedProcessTimeoutInSeconds>1200</maven-surefire-plugin.forkedProcessTimeoutInSeconds>
- <ui.test.excludedGroups></ui.test.excludedGroups>
- <ui.test.includedGroups>uitest</ui.test.includedGroups>
</properties>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>default-test</id>
+ <phase>none</phase>
+ </execution>
+ </executions>
+ </plugin>
+ </plugins>
+ </build>
</profile>
<profile>
<id>skip-uitest</id>
- <properties>
- <ui.test.excludedGroups>uitest</ui.test.excludedGroups>
- </properties>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>ui-test</id>
+ <phase>none</phase>
+ </execution>
+ </executions>
+ </plugin>
+ </plugins>
+ </build>
</profile>
<profile>
<id>apache-release</id>
diff --git
a/rcp/src/test/java/org/apache/hop/ui/hopgui/HopGuiPeekInstanceTest.java
b/rcp/src/test/java/org/apache/hop/ui/hopgui/HopGuiPeekInstanceTest.java
index 29201f8438..a2f02dee5e 100644
--- a/rcp/src/test/java/org/apache/hop/ui/hopgui/HopGuiPeekInstanceTest.java
+++ b/rcp/src/test/java/org/apache/hop/ui/hopgui/HopGuiPeekInstanceTest.java
@@ -19,6 +19,7 @@ package org.apache.hop.ui.hopgui;
import static org.junit.jupiter.api.Assertions.assertSame;
+import org.apache.hop.ui.core.gui.GuiToolbarWidgets;
import
org.apache.hop.ui.hopgui.perspective.configuration.ConfigurationPerspective;
import org.apache.hop.ui.hopgui.perspective.execution.ExecutionPerspective;
import org.apache.hop.ui.hopgui.perspective.explorer.ExplorerPerspective;
@@ -39,4 +40,18 @@ class HopGuiPeekInstanceTest {
HopGui.peekInstance(),
"Perspective getInstance() must not start a HopGui (that floods SWT
tests and the display)");
}
+
+ /**
+ * Every TableView builds a toolbar and registers itself with it. Under xvfb
that used to start a
+ * HopGui which then stayed in the test JVM for every test that ran after it.
+ */
+ @Test
+ void toolbarWidgetsDoNotConstructHopGui() {
+ HopGui before = HopGui.peekInstance();
+ GuiToolbarWidgets toolbarWidgets = new GuiToolbarWidgets();
+ toolbarWidgets.registerGuiPluginObject(new Object());
+ toolbarWidgets.dispose();
+ assertSame(
+ before, HopGui.peekInstance(), "Building a toolbar or a table must not
start a HopGui");
+ }
}
diff --git
a/rcp/src/test/java/org/apache/hop/ui/hopgui/file/GraphCanvasTestBase.java
b/rcp/src/test/java/org/apache/hop/ui/hopgui/file/GraphCanvasTestBase.java
index eef8e02ee9..07926d3337 100644
--- a/rcp/src/test/java/org/apache/hop/ui/hopgui/file/GraphCanvasTestBase.java
+++ b/rcp/src/test/java/org/apache/hop/ui/hopgui/file/GraphCanvasTestBase.java
@@ -131,6 +131,10 @@ public abstract class GraphCanvasTestBase extends
SwtBotTestBase {
*/
protected static synchronized HopGui hopGui() {
if (hopGui == null) {
+ // The canvas handlers also reach for the application's HopGui. Build
that one up front too:
+ // created by the first click, its shell would be caught as a dialog
that click opened.
+ //
+ HopGui.getInstance();
hopGui = new MenulessHopGui();
hopGui.setProps(PropsUi.getInstance());
}
diff --git a/ui/src/main/java/org/apache/hop/ui/core/gui/BaseGuiWidgets.java
b/ui/src/main/java/org/apache/hop/ui/core/gui/BaseGuiWidgets.java
index 4a1f72303b..a6e8ab4018 100644
--- a/ui/src/main/java/org/apache/hop/ui/core/gui/BaseGuiWidgets.java
+++ b/ui/src/main/java/org/apache/hop/ui/core/gui/BaseGuiWidgets.java
@@ -35,6 +35,12 @@ import org.eclipse.swt.widgets.Listener;
public class BaseGuiWidgets {
+ /**
+ * The id the GUI plugin objects are registered under when the widgets are
built without a Hop
+ * GUI, for example a dialog in a unit test or a standalone tool.
+ */
+ static final String NO_HOP_GUI_ID = "no-hop-gui";
+
/**
* Every set of widgets (toolbar, composite, menu, ...) gets its own unique
ID It will cause a new
* object to be created per unique ID for the listener or GUI plugins if
this plugin wasn't
@@ -51,6 +57,9 @@ public class BaseGuiWidgets {
/** The plugin object which is registered */
protected Object guiPluginObject;
+ /** The id of the Hop GUI the plugin objects are registered under, see
{@link #getHopGuiId()} */
+ private String hopGuiId;
+
public BaseGuiWidgets(String instanceId) {
this.instanceId = instanceId;
}
@@ -79,29 +88,54 @@ public class BaseGuiWidgets {
this.guiPluginObject = guiPluginObject;
this.guiPluginClassName = guiPluginClassName;
GuiRegistry.getInstance()
- .registerGuiPluginObject(
- HopGui.getInstance().getId(), guiPluginClassName, instanceId,
guiPluginObject);
+ .registerGuiPluginObject(getHopGuiId(), guiPluginClassName,
instanceId, guiPluginObject);
}
protected void addDeRegisterGuiPluginObjectListener(Control control) {
control.addDisposeListener(
- e ->
- GuiRegistry.getInstance()
- .removeGuiPluginObjects(HopGui.getInstance().getId(),
instanceId));
+ e -> GuiRegistry.getInstance().removeGuiPluginObjects(getHopGuiId(),
instanceId));
}
public void dispose() {
- String hopGuiId = HopGui.getInstance().getId();
- GuiRegistry.getInstance().removeGuiPluginObjects(hopGuiId, instanceId);
+ GuiRegistry.getInstance().removeGuiPluginObjects(getHopGuiId(),
instanceId);
+ }
+
+ /**
+ * The id of the Hop GUI the plugin objects of these widgets are registered
under. It is taken
+ * once, so that they are registered, found and removed under the same id,
even when a HopGui only
+ * comes into existence after the widgets were built.
+ *
+ * @return the id of the running Hop GUI, or {@link #NO_HOP_GUI_ID} when
there was none
+ */
+ public String getHopGuiId() {
+ if (hopGuiId == null) {
+ hopGuiId = currentHopGuiId();
+ }
+ return hopGuiId;
+ }
+
+ /**
+ * The id of the running Hop GUI, or {@link #NO_HOP_GUI_ID} when there is
none. This only peeks:
+ * building a toolbar or a table must not start a whole HopGui as a side
effect. In a test JVM
+ * with a display that GUI would outlive the test and change what every
later test sees.
+ */
+ static String currentHopGuiId() {
+ HopGui hopGui = HopGui.peekInstance();
+ return hopGui == null ? NO_HOP_GUI_ID : hopGui.getId();
}
protected static Object findGuiPluginInstance(
ClassLoader classLoader, String listenerClassName, String instanceId)
throws Exception {
+ return findGuiPluginInstance(currentHopGuiId(), classLoader,
listenerClassName, instanceId);
+ }
+
+ protected static Object findGuiPluginInstance(
+ String hopGuiId, ClassLoader classLoader, String listenerClassName,
String instanceId)
+ throws Exception {
try {
// This is the class that owns the listener method
// It's a GuiPlugin class in other words
//
- String hopGuiId = HopGui.getInstance().getId();
Object guiPluginObject =
GuiRegistry.getInstance().findGuiPluginObject(hopGuiId,
listenerClassName, instanceId);
if (guiPluginObject == null) {
@@ -130,7 +164,10 @@ public class BaseGuiWidgets {
try {
Object singleton =
findGuiPluginInstance(
- toolbarItem.getClassLoader(), toolbarItem.getListenerClass(),
instanceId);
+ getHopGuiId(),
+ toolbarItem.getClassLoader(),
+ toolbarItem.getListenerClass(),
+ instanceId);
if (singleton == null) {
LogChannel.UI.logError(
"Could not get instance of class '"
@@ -232,7 +269,7 @@ public class BaseGuiWidgets {
}
Object guiPluginInstance =
- findGuiPluginInstance(classLoader, listenerClassName, instanceId);
+ findGuiPluginInstance(getHopGuiId(), classLoader,
listenerClassName, instanceId);
// Prefer a method that accepts the SWT Event (e.g. to read modifier
keys such as Shift).
try {
Method withEvent =
diff --git a/ui/src/main/java/org/apache/hop/ui/core/gui/GuiMenuWidgets.java
b/ui/src/main/java/org/apache/hop/ui/core/gui/GuiMenuWidgets.java
index c7392c7cc6..aae61caa80 100644
--- a/ui/src/main/java/org/apache/hop/ui/core/gui/GuiMenuWidgets.java
+++ b/ui/src/main/java/org/apache/hop/ui/core/gui/GuiMenuWidgets.java
@@ -80,7 +80,7 @@ public class GuiMenuWidgets extends BaseGuiWidgets {
}
ClassLoader classLoader = pluginRegistry.getClassLoader(plugin);
if (canPreRegister(classLoader, className)) {
- findGuiPluginInstance(classLoader, className, instanceId);
+ findGuiPluginInstance(getHopGuiId(), classLoader, className,
instanceId);
}
} catch (Exception e) {
LogChannel.UI.logDebug(
diff --git a/ui/src/main/java/org/apache/hop/ui/core/gui/GuiToolbarWidgets.java
b/ui/src/main/java/org/apache/hop/ui/core/gui/GuiToolbarWidgets.java
index 79e80afacb..47bbe4e21b 100644
--- a/ui/src/main/java/org/apache/hop/ui/core/gui/GuiToolbarWidgets.java
+++ b/ui/src/main/java/org/apache/hop/ui/core/gui/GuiToolbarWidgets.java
@@ -160,7 +160,8 @@ public class GuiToolbarWidgets extends BaseGuiWidgets
implements IToolbarWidgetR
boolean show = true;
try {
Object guiPluginInstance =
- findGuiPluginInstance(toolbarItem.getClassLoader(),
guiPluginClassName, instanceId);
+ findGuiPluginInstance(
+ getHopGuiId(), toolbarItem.getClassLoader(), guiPluginClassName,
instanceId);
List<GuiToolbarItemFilter> itemFilters =
GuiRegistry.getInstance().getToolbarItemFiltersMap().get(root);
if (!Utils.isEmpty(itemFilters)) {
@@ -632,7 +633,10 @@ public class GuiToolbarWidgets extends BaseGuiWidgets
implements IToolbarWidgetR
// Find the registered GUI plugin object
Object guiPluginInstance =
findGuiPluginInstance(
- toolbarItem.getClassLoader(), toolbarItem.getListenerClass(),
instanceId);
+ getHopGuiId(),
+ toolbarItem.getClassLoader(),
+ toolbarItem.getListenerClass(),
+ instanceId);
imageLocation = (String) imageMethod.invoke(null, guiPluginInstance);
} catch (Exception e) {
imageLocation = null;
diff --git
a/ui/src/test/java/org/apache/hop/ui/testing/DisabledGuiWidgetsTestBase.java
b/ui/src/test/java/org/apache/hop/ui/testing/DisabledGuiWidgetsTestBase.java
index 77d19b48d0..1a4cb7e82e 100644
--- a/ui/src/test/java/org/apache/hop/ui/testing/DisabledGuiWidgetsTestBase.java
+++ b/ui/src/test/java/org/apache/hop/ui/testing/DisabledGuiWidgetsTestBase.java
@@ -39,7 +39,6 @@ import org.apache.hop.ui.core.gui.GuiMenuWidgets;
import org.apache.hop.ui.core.gui.GuiToolbarWidgets;
import org.apache.hop.ui.core.gui.IGuiPluginCompositeWidgetsListener;
import org.apache.hop.ui.core.gui.IToolbarContainer;
-import org.apache.hop.ui.hopgui.HopGui;
import org.apache.hop.ui.hopgui.HopGuiEnvironment;
import org.apache.hop.ui.hopgui.ToolbarFacade;
import org.eclipse.swt.SWT;
@@ -401,13 +400,13 @@ public abstract class DisabledGuiWidgetsTestBase extends
SwtBotTestBase {
/**
* Registers the object Hop hands to this toolbar's callbacks for {@code
className}, under that
- * class's own name and this toolbar's instance id - which is exactly where
{@code
+ * class's own name and this toolbar's Hop GUI and instance ids - which is
exactly where {@code
* BaseGuiWidgets.findGuiPluginInstance} looks before it falls back to
constructing one itself.
*/
private void registerToolbarObject(GuiToolbarWidgets widgets, String
className) {
GuiRegistry.getInstance()
.registerGuiPluginObject(
- HopGui.getInstance().getId(),
+ widgets.getHopGuiId(),
className,
widgets.getInstanceId(),
toolbarSourceObject(className));
diff --git a/ui/src/test/java/org/apache/hop/ui/testing/SwtBotTestBase.java
b/ui/src/test/java/org/apache/hop/ui/testing/SwtBotTestBase.java
index 16c3fc11b1..fdc53bb5a2 100644
--- a/ui/src/test/java/org/apache/hop/ui/testing/SwtBotTestBase.java
+++ b/ui/src/test/java/org/apache/hop/ui/testing/SwtBotTestBase.java
@@ -33,6 +33,8 @@ import org.apache.hop.i18n.BaseMessages;
import org.apache.hop.pipeline.transform.ITransform;
import org.apache.hop.ui.core.PropsUi;
import org.apache.hop.ui.core.gui.GuiResource;
+import org.apache.hop.ui.core.gui.HopNamespace;
+import org.apache.hop.ui.hopgui.HopGui;
import org.eclipse.swt.SWT;
import org.eclipse.swt.SWTException;
import org.eclipse.swt.widgets.Display;
@@ -88,6 +90,9 @@ public abstract class SwtBotTestBase {
HopEnvironment.init();
keepAuditStateOutOfSourceTree();
ensureDisplay();
+ // The namespace a running Hop GUI sets, and that dialogs read their last
used values under.
+ // Do not leave it to whichever test happens to construct a HopGui first.
+ HopNamespace.setNamespace(HopGui.DEFAULT_HOP_GUI_NAMESPACE);
// Warm up the Hop look-and-feel (fonts, zoom factor) against this display.
PropsUi.getInstance();
GuiResource.getInstance();
