gnodet-bot commented on code in PR #29083: URL: https://github.com/apache/camel/pull/29083#discussion_r4453452990
########## dsl/camel-jbang/camel-jbang-it/src/test/java/org/apache/camel/dsl/jbang/it/InfraAdvancedITCase.java: ########## @@ -0,0 +1,81 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.dsl.jbang.it; + +import java.time.Duration; + +import org.apache.camel.dsl.jbang.it.support.JBangTestSupport; +import org.assertj.core.api.Assertions; +import org.awaitility.Awaitility; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledIfSystemProperty; +import org.junit.jupiter.api.condition.DisabledOnOs; + +import static org.junit.jupiter.api.condition.OS.WINDOWS; + +@Tag("container-only") +@DisabledOnOs(WINDOWS) +@DisabledIfSystemProperty(named = "ci.env.name", matches = ".*", disabledReason = "Requires too much resources") +public class InfraAdvancedITCase extends JBangTestSupport { + + private static final String SERVICE = "ftp"; + + @AfterEach + public void stopInfraService() { + execInContainer(getMainCommand() + " infra stop " + SERVICE + " || true"); + } + + private String getServicePID(String message) { + return message.split(":")[1].replaceAll("[^0-9]", ""); Review Comment: â ī¸ **Fragile parse â `split(":")[1]` throws `ArrayIndexOutOfBoundsException` if format changes** `message.split(":")[1]` assumes the output of `infra run --background` always contains a colon and that the PID appears after the first one. If the message format changes (or the command fails silently), this throws `AIOOBE` with no diagnostic â the test failure message would point at `getServicePID` rather than the actual problem. Add a defensive guard before the split: ```suggestion private String getServicePID(String message) { org.assertj.core.api.Assertions.assertThat(message) .as("infra run --background output should contain ':'") .contains(":"); return message.split(":")[1].replaceAll("[^0-9]", ""); } ``` ########## dsl/camel-jbang/camel-jbang-it/src/test/java/org/apache/camel/dsl/jbang/it/ProcessListITCase.java: ########## @@ -0,0 +1,45 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.dsl.jbang.it; + +import java.io.IOException; + +import org.apache.camel.dsl.jbang.it.support.JBangTestSupport; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledOnOs; + +import static org.junit.jupiter.api.condition.OS.WINDOWS; + +@DisabledOnOs(WINDOWS) +public class ProcessListITCase extends JBangTestSupport { + + @Test + public void testPsWithRunningIntegration() throws IOException { + copyResourceInDataFolder(TestResources.ROUTE2); + String pid = executeBackground(String.format("run %s/route2.yaml", mountPoint())); + checkLogContains("Hello Camel from custom integration"); + checkCommandOutputs("ps", pid); + checkCommandOutputs("ps", "Running"); + } + + @Test + public void testPsEmpty() { + // no integration running; ps should produce no output (empty table) + execute("stop"); + execute("ps"); Review Comment: đĄ **`testPsEmpty` doesn't assert that `ps` output is actually empty** The comment says _"ps should produce no output (empty table)"_ but the bare `execute("ps")` only validates the exit code (non-zero = fail). The test name implies an empty-result assertion. ```suggestion execute("stop"); Assertions.assertThat(execute("ps").trim()) .as("ps should return empty when no integration is running") .isEmpty(); ``` ########## dsl/camel-jbang/camel-jbang-it/src/test/java/org/apache/camel/dsl/jbang/it/GetHealthITCase.java: ########## @@ -0,0 +1,37 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.dsl.jbang.it; + +import java.io.IOException; + +import org.apache.camel.dsl.jbang.it.support.JBangTestSupport; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledOnOs; + +import static org.junit.jupiter.api.condition.OS.WINDOWS; + +@DisabledOnOs(WINDOWS) +public class GetHealthITCase extends JBangTestSupport { + + @Test + public void testGetHealth() throws IOException { + copyResourceInDataFolder(TestResources.ROUTE2); + executeBackground(String.format("run %s/route2.yaml --health", mountPoint())); + checkLogContains("Hello Camel from custom integration"); + checkCommandOutputsPattern("get health", "UP|DOWN"); Review Comment: â ī¸ **`UP|DOWN` pattern is too permissive â a permanently DOWN health check passes** `"UP|DOWN"` is a regex alternation that matches either `UP` or `DOWN`. Since the route is confirmed as producing messages (via `checkLogContains` on line 34), the health status should be `UP`. Accepting `DOWN` masks a real health failure. ```suggestion checkCommandOutputsPattern("get health", "UP"); ``` ########## dsl/camel-jbang/camel-jbang-it/src/test/java/org/apache/camel/dsl/jbang/it/RestartITCase.java: ########## @@ -0,0 +1,42 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.dsl.jbang.it; + +import java.io.IOException; + +import org.apache.camel.dsl.jbang.it.support.JBangTestSupport; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledOnOs; + +import static org.junit.jupiter.api.condition.OS.WINDOWS; + +@Tag("container-only") +@DisabledOnOs(WINDOWS) +public class RestartITCase extends JBangTestSupport { + + @Test + public void testRestart() throws IOException { + copyResourceInDataFolder(TestResources.ROUTE2); + String pid = executeBackground(String.format("run %s/route2.yaml", mountPoint())); + checkLogContains("Hello Camel from custom integration"); + execute("restart " + pid); + // after restart the integration should come back up + checkLogContains("Hello Camel from custom integration", ASSERTION_WAIT_SECONDS); Review Comment: âšī¸ **`checkLogContains` after restart matches pre-restart log entries** `checkLogContains("Hello Camel from custom integration", ASSERTION_WAIT_SECONDS)` on line 39 searches the _accumulated_ container log â which already contains the pre-restart output from line 34. This assertion will pass trivially even if the integration never restarts successfully. The actual restart validation is `checkCommandOutputs("get context", "Running")` on line 40, which is correct. This is an observation; the test correctness is maintained by that context check. -- 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]
