Copilot commented on code in PR #4178:
URL:
https://github.com/apache/incubator-kie-kogito-runtimes/pull/4178#discussion_r2798330031
##########
quarkus/addons/tracing-decision/deployment/src/main/java/org/kie/kogito/tracing/decision/quarkus/deployment/KogitoDevServicesBuildTimeConfig.java:
##########
@@ -20,32 +20,33 @@
import java.util.Optional;
-import io.quarkus.runtime.annotations.ConfigGroup;
-import io.quarkus.runtime.annotations.ConfigItem;
+import io.smallrye.config.WithDefault;
-@ConfigGroup
-public class KogitoDevServicesBuildTimeConfig {
+/**
+ * Nested configuration group for Kogito Trusty Service DevServices build-time
settings.
+ * Note: @ConfigGroup is not used here because this interface is nested within
a @ConfigMapping interface.
+ * In Quarkus 3.x, @ConfigGroup is only for legacy @ConfigRoot + @ConfigItem
style configurations.
+ */
+public interface KogitoDevServicesBuildTimeConfig {
/**
* If Dev Services for Kogito has been explicitly enabled or disabled. Dev
Services are generally enabled
* by default, unless there is an existing configuration present.
*/
- @ConfigItem
- public Optional<Boolean> enabled = Optional.empty();
+ Optional<Boolean> enabled();
/**
* Optional fixed port the dev service will listen to.
* <p>
* If not defined, 8081 will be used.
*/
- @ConfigItem(defaultValue = "8081")
- public Optional<Integer> port;
+ @WithDefault("8081")
+ Optional<Integer> port();
/**
* The TrustyService image to use.
*/
- @ConfigItem
- public String imageName;
+ Optional<String> imageName();
Review Comment:
`imageName()` is defined as `Optional<String>` with no default. Since
DevServices container startup requires an image name, consider providing a
default via `@WithDefault` (and returning `String`) or ensure callers handle
the empty case explicitly (rather than propagating null and failing later).
##########
quarkus/addons/kubernetes/test-utils/src/main/java/org/kie/kogito/addons/quarkus/k8s/test/utils/KubernetesMockServerTestResource.java:
##########
@@ -55,15 +75,20 @@ public Map<String, String> start() {
@Override
public void stop() {
+ if (client != null) {
+ client.close();
+ }
if (server != null) {
- server.after(); // Stop the mock server
+ server.destroy();
+ server = null;
}
Review Comment:
`start()` sets the global system property
`Config.KUBERNETES_MASTER_SYSTEM_PROPERTY`, but `stop()` doesn’t restore/clear
it. This can affect other tests running in the same JVM after this resource
stops. Save the previous value in `start()` and restore it (or clear it) in
`stop()`.
##########
quarkus/addons/tracing-decision/deployment/src/main/java/org/kie/kogito/tracing/decision/quarkus/deployment/KogitoDevServicesProcessor.java:
##########
@@ -319,12 +319,12 @@ private static final class TrustyServiceDevServiceConfig {
private final int portUsedByTest;
public TrustyServiceDevServiceConfig(final
KogitoDevServicesBuildTimeConfig config) {
- this.devServicesEnabled = config.enabled.orElse(true);
- this.imageName = config.imageName;
- this.fixedExposedPort = config.port.orElse(0);
- this.shared = config.shared;
- this.serviceName = config.serviceName;
- this.portUsedByTest = config.portUsedByTest;
+ this.devServicesEnabled = config.enabled().orElse(true);
+ this.imageName = config.imageName().orElse(null);
+ this.fixedExposedPort = config.port().orElse(0);
+ this.shared = config.shared();
+ this.serviceName = config.serviceName();
+ this.portUsedByTest = config.portUsedByTest();
Review Comment:
`imageName` is now populated via `config.imageName().orElse(null)`. If the
property isn’t set, this will later cause a NullPointerException when
`DockerImageName.parse(config.imageName)` is called during container startup.
Consider making `imageName` required (non-Optional) or providing a safe default
/ explicit error when absent instead of storing null here.
##########
quarkus/extensions/kogito-quarkus-serverless-workflow-extension/kogito-quarkus-serverless-workflow-extension-live-reload-test/src/test/java/org/kie/kogito/quarkus/serverless/workflow/deployment/livereload/LiveReloadProcessorTest.java:
##########
@@ -51,7 +54,11 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.is;
+import static org.junit.jupiter.api.parallel.ExecutionMode.SAME_THREAD;
+@TestInstance(TestInstance.Lifecycle.PER_CLASS)
+@Execution(SAME_THREAD)
+@Disabled("Disabled temporarly - when doing quarkus upgrade.")
public class LiveReloadProcessorTest {
Review Comment:
Disabling the entire live-reload test class will hide regressions in
dev-mode hot reload behavior. If this is only to address upgrade-related
flakiness, prefer re-enabling and fixing the underlying issue, or at least
limiting the disablement to the specific failing test(s) and linking to a
tracking issue so it’s not left permanently disabled.
##########
quarkus/extensions/kogito-quarkus-serverless-workflow-extension/kogito-quarkus-serverless-workflow-extension-live-reload-test/src/test/java/org/kie/kogito/quarkus/serverless/workflow/deployment/livereload/LiveReloadProcessorTest.java:
##########
@@ -190,5 +200,8 @@ void testAsyncApi() throws IOException {
.extract().path("id");
assertThat(id).isNotBlank();
+
+ // Add a small delay before test cleanup to avoid
ConcurrentModificationException
+ Thread.sleep(100);
Review Comment:
Using fixed `Thread.sleep(...)` delays to wait for hot reload makes this
test timing-dependent and flaky across environments. Prefer waiting on an
observable condition (e.g., polling the endpoint until it returns 201, or using
Awaitility with a timeout) so the test is resilient to slower/faster CI
machines.
##########
quarkus/extensions/kogito-quarkus-serverless-workflow-extension/kogito-quarkus-serverless-workflow-extension-live-reload-test/src/test/java/org/kie/kogito/quarkus/serverless/workflow/deployment/livereload/LiveReloadProcessorTest.java:
##########
@@ -51,7 +54,11 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.is;
+import static org.junit.jupiter.api.parallel.ExecutionMode.SAME_THREAD;
+@TestInstance(TestInstance.Lifecycle.PER_CLASS)
+@Execution(SAME_THREAD)
+@Disabled("Disabled temporarly - when doing quarkus upgrade.")
Review Comment:
Typo in the @Disabled reason string: "temporarly" → "temporarily".
##########
quarkus/addons/persistence/kafka/runtime/src/test/java/org/kie/kogito/persistence/kafka/KafkaProcessInstancesIT.java:
##########
@@ -234,6 +237,12 @@ Properties getStreamsConfig() {
Properties properties = new Properties();
properties.put(StreamsConfig.APPLICATION_ID_CONFIG, "kogito");
properties.put(StreamsConfig.BOOTSTRAP_SERVERS_CONFIG,
kafka.getBootstrapServers());
+ try {
+ Path tempDir = Files.createTempDirectory("kafka-streams-test");
+ properties.put(StreamsConfig.STATE_DIR_CONFIG, tempDir.toString());
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to create temp directory for
Kafka Streams state", e);
+ }
Review Comment:
`Files.createTempDirectory(...)` creates a new state dir for Kafka Streams
but it’s never cleaned up, which can leave many temp folders behind on
developer machines / CI workers. Consider registering it for deletion (e.g.,
`deleteOnExit()` and/or deleting recursively in `@AfterEach`), or use JUnit’s
`@TempDir` so lifecycle cleanup is automatic.
##########
quarkus/addons/kubernetes/test-utils/src/main/java/org/kie/kogito/addons/quarkus/k8s/test/utils/KubernetesMockServerTestResource.java:
##########
@@ -55,15 +75,20 @@ public Map<String, String> start() {
@Override
public void stop() {
+ if (client != null) {
+ client.close();
+ }
if (server != null) {
- server.after(); // Stop the mock server
+ server.destroy();
+ server = null;
}
}
- /**
- * Expose the Fabric8 Kubernetes mock server instance for advanced use in
tests.
- */
- public KubernetesServer getServer() {
+ public static KubernetesMockServer getServer() {
return server;
}
Review Comment:
`server` is now a static field but there are no usages of the static
`getServer()` in the codebase. Keeping the mock server static increases the
risk of state leaking between test classes (and makes parallel test execution
unsafe). Prefer making `server` an instance field and `getServer()` non-static
(or remove the accessor entirely if not needed).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
