apupier commented on code in PR #24855:
URL: https://github.com/apache/camel/pull/24855#discussion_r3613972889
##########
core/camel-core/src/test/java/org/apache/camel/component/file/FileProduceTempPrefixTest.java:
##########
@@ -74,8 +76,15 @@ public void testTempPrefix() {
}
@Test
- public void testTempPrefixUUIDFilename() {
+ public void testTempPrefixUUIDFilename() throws Exception {
template.sendBody("direct:a", "Bye World");
+
+ // When no FILE_NAME header is set, the producer creates a file with
an auto-generated UUID name
+ File[] files = testDirectory().toFile().listFiles();
+ Assertions.assertNotNull(files, "Test directory should contain files");
+ Assertions.assertTrue(files.length > 0, "A file should have been
created with an auto-generated name");
Review Comment:
we want a single file to be created
##########
core/camel-core/src/test/java/org/apache/camel/impl/CamelContextDeadlockTest.java:
##########
@@ -27,22 +28,24 @@ public class CamelContextDeadlockTest {
@Timeout(5)
@Test
public void testComponentDeadlock() {
- CamelContext context = new DefaultCamelContext();
- context.getRegistry().bind("sql-connector", new DirectComponent() {
- @Override
- protected void doStart() throws Exception {
- Component delegate = new DirectComponent();
+ Assertions.assertDoesNotThrow(() -> {
+ CamelContext context = new DefaultCamelContext();
+ context.getRegistry().bind("sql-connector", new DirectComponent() {
+ @Override
+ protected void doStart() throws Exception {
+ Component delegate = new DirectComponent();
- getCamelContext().removeComponent("sql-connector-component");
- getCamelContext().addService(delegate, true, true);
- getCamelContext().addComponent("sql-connector-component",
delegate);
+
getCamelContext().removeComponent("sql-connector-component");
+ getCamelContext().addService(delegate, true, true);
+ getCamelContext().addComponent("sql-connector-component",
delegate);
- super.doStart();
- }
- });
+ super.doStart();
+ }
+ });
- context.start();
- context.getComponent("sql-connector", true, true);
- context.stop();
+ context.start();
+ context.getComponent("sql-connector", true, true);
+ context.stop();
+ });
Review Comment:
this test sounds not testing much.
it is mentioning to test component deadlock but context.start() is not
blocking, so when the context.getComponent is called right after potentially
the specific actions in regsitry bind has not happened and so nothing is
checked at all. this is something that was already the case before this PR but
these changes are just hiding the fact that the test is not robust. i think it
will be best to keep as-is for now and report an issue to improve this test
##########
core/camel-core/src/test/java/org/apache/camel/issues/AdviceWithTryCatchFinallyTest.java:
##########
@@ -37,6 +38,9 @@ public void testAdviceTryCatchFinally() throws Exception {
.replace().to("mock:replaced"));
context.start();
+
+
assertTrue(context.getRouteController().getRouteStatus("my-route").isStarted(),
+ "Route 'my-route' should be started after advice");
Review Comment:
A more relevant check would be to test that the mock:replaced is called and
not the mock:replace-me
##########
core/camel-core/src/test/java/org/apache/camel/component/file/FileProduceTempPrefixTest.java:
##########
@@ -74,8 +76,15 @@ public void testTempPrefix() {
}
@Test
- public void testTempPrefixUUIDFilename() {
+ public void testTempPrefixUUIDFilename() throws Exception {
template.sendBody("direct:a", "Bye World");
+
+ // When no FILE_NAME header is set, the producer creates a file with
an auto-generated UUID name
+ File[] files = testDirectory().toFile().listFiles();
+ Assertions.assertNotNull(files, "Test directory should contain files");
+ Assertions.assertTrue(files.length > 0, "A file should have been
created with an auto-generated name");
+ String content = new
String(java.nio.file.Files.readAllBytes(files[0].toPath()));
Review Comment:
Unless I missed it, there is no clash of classname for Files, so we can use
an import instead of Full Qualified ClassName
##########
core/camel-core/src/test/java/org/apache/camel/impl/RouteMustHaveOutputOnExceptionTest.java:
##########
@@ -30,16 +31,18 @@ public boolean isUseRouteBuilder() {
}
@Test
- public void testValid() throws Exception {
- context.addRoutes(new RouteBuilder() {
- @Override
- public void configure() {
-
from("direct:start").onException(Exception.class).redeliveryDelay(10).maximumRedeliveries(2)
- .backOffMultiplier(1.5).handled(true).delay(1000)
- .log("Halting for some
time").to("mock:halt").end().end().to("mock:result");
- }
+ public void testValid() {
+ assertDoesNotThrow(() -> {
+ context.addRoutes(new RouteBuilder() {
+ @Override
+ public void configure() {
+
from("direct:start").onException(Exception.class).redeliveryDelay(10).maximumRedeliveries(2)
+ .backOffMultiplier(1.5).handled(true).delay(1000)
+ .log("Halting for some
time").to("mock:halt").end().end().to("mock:result");
+ }
+ });
+ context.start();
Review Comment:
should check that the mock is called instad of just checking no exception is
thrown
--
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]
