gnodet commented on code in PR #24858:
URL: https://github.com/apache/camel/pull/24858#discussion_r3655163574
##########
components/camel-ai/camel-langchain4j-embeddingstore/src/test/java/org/apache/camel/component/langchain4j/embeddings/LangChain4jEmbeddingStoreComponentTest.java:
##########
@@ -51,16 +53,16 @@ protected CamelContext createCamelContext() throws
Exception {
}
@Test
- public void testSimpleEmbedding() {
-
+ public void testSimpleEmbedding() throws Exception {
EmbeddingModel embeddingModel = new AllMiniLmL6V2EmbeddingModel();
TextSegment segment1 = TextSegment.from("I like football.");
Embedding testEmbedding = embeddingModel.embed(segment1).content();
+ assertNotNull(testEmbedding, "embedding model should produce a
non-null embedding");
Message first = fluentTemplate.to("langchain4j-embeddingstore:first")
.withBody(testEmbedding)
.request(Message.class);
-
+ assertNotNull(first, "response message should not be null");
Review Comment:
_Claude Code on behalf of gnodet_
Good point — there is indeed a Weaviate test container available. Looking at
the codebase, `LangChain4jEmbeddingStoreComponentWeaviateTargetIT` already
covers store-level testing with Weaviate test infra. The disabled
`testStoreRouting` test was redundant, so it has been removed. This unit test
now focuses only on the embedding model (no infra needed), while the IT covers
the full store routing.
##########
components/camel-mllp/src/test/java/org/apache/camel/component/mllp/MllpTcpClientProducerRequiredEndOfDataWithoutValidationTest.java:
##########
Review Comment:
_Claude Code on behalf of gnodet_
You are right — the try-catch wrapping in `RuntimeCamelException` was worse
than the original. `assertDoesNotThrow` IS a valid assertion for S2699 and was
already more expressive. Reverted back to `assertDoesNotThrow` style.
##########
components/camel-mllp/src/test/java/org/apache/camel/test/executor/PooledExecutorTest.java:
##########
@@ -54,6 +57,9 @@ public void testAddRunnable() throws Exception {
log.info("Starting second set of runnables");
startRunnables(runnableCount, runCount);
+
+ assertNotNull(instance.executor, "Executor should still be active
after submitting runnables");
+ assertFalse(instance.executor.isShutdown(), "Executor should not be
shut down after submitting runnables");
Review Comment:
_Claude Code on behalf of gnodet_
Addressed — `startRunnables` now returns the list of accepted runnables, and
the test verifies the pool behavior: first batch accepts up to THREAD_COUNT
runnables (SynchronousQueue constraint), second batch is fully rejected because
threads are still busy.
##########
components/camel-netty/src/test/java/org/apache/camel/component/netty/MainNettyCustomCodecTest.java:
##########
@@ -56,7 +59,11 @@ public void configure() {
});
main.configure().withDurationMaxMessages(2);
main.configure().withDurationMaxSeconds(5);
+
main.run();
+ // run() blocks until duration/message limit is reached, then returns.
+ // The route processor inside validates data equality and throws if
mismatched.
+ assertNotNull(main.getCamelContext(), "CamelContext should have been
created during run");
Review Comment:
_Claude Code on behalf of gnodet_
Addressed — added an `AtomicBoolean routeProcessed` that the route processor
sets after validating the custom-codec data. The test now asserts
`assertTrue(routeProcessed.get())` to verify the route actually processed and
validated the message.
##########
components/camel-plc4x/src/test/java/org/apache/camel/component/plc4x/Plc4XEndpointTest.java:
##########
@@ -56,11 +56,14 @@ public void isSingleton() {
}
@Test
- public void doStopBadConnection() throws Exception {
+ void doStopBadConnection() throws Exception {
PlcConnection plcConnectionMock = mock(PlcConnection.class);
sut.connection = plcConnectionMock;
doThrow(new RuntimeException("oh
noes")).when(plcConnectionMock).close();
sut.doStop();
+
+ // isConnected() returns false (mock default), so close is skipped
+ verify(plcConnectionMock, never()).close();
Review Comment:
_Claude Code on behalf of gnodet_
Good catch — the `doThrow` setup was misleading since `isConnected()`
returns false on the mock, so `close()` is never called anyway. Removed the
confusing `doThrow` and renamed the test to `doStopDisconnectedConnection` to
clarify intent: when `isConnected()` is false, `doStop()` should skip closing.
--
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]
