oscerd commented on code in PR #26541: URL: https://github.com/apache/camel/pull/26541#discussion_r4060436684
########## components/camel-vertx/camel-vertx-websocket/src/test/java/org/apache/camel/component/vertx/websocket/VertxWebsocketProducerNoPeerTest.java: ########## @@ -0,0 +1,95 @@ +/* + * 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.component.vertx.websocket; + +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import org.apache.camel.Exchange; +import org.apache.camel.impl.DefaultCamelContext; +import org.apache.camel.support.DefaultExchange; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * An exchange with no peer to send to is finished by the producer itself, so it has to report that it was done + * synchronously. Builds the endpoint directly, so nothing reaches a server. + */ +class VertxWebsocketProducerNoPeerTest { + + private DefaultCamelContext context; + + @AfterEach + void tearDown() { + if (context != null) { + context.stop(); + } + } + + private VertxWebsocketProducer producer() throws Exception { + context = new DefaultCamelContext(); + + VertxWebsocketComponent component = new VertxWebsocketComponent(); + component.setCamelContext(context); + + VertxWebsocketEndpoint endpoint + = (VertxWebsocketEndpoint) component.createEndpoint("vertx-websocket:localhost:1234/test"); + return (VertxWebsocketProducer) endpoint.createProducer(); + } + + @Test + void anExchangeWithNoPeerIsDoneSynchronously() throws Exception { + VertxWebsocketProducer producer = producer(); + + Exchange exchange = new DefaultExchange(context); + exchange.getIn().setBody("a message nobody is listening for"); + // broadcasting to an empty host registry is the one path that reaches no peer without opening a connection + exchange.getIn().setHeader(VertxWebsocketConstants.SEND_TO_ALL, true); + + AtomicInteger callbacks = new AtomicInteger(); + AtomicReference<Boolean> doneSync = new AtomicReference<>(); + + // the callback used to be completed with doneSync=true while the method returned false, which says the + // opposite: that the exchange would be finished from a write handler that never runs + boolean result = producer.process(exchange, sync -> { + callbacks.incrementAndGet(); + doneSync.set(sync); + }); + + assertTrue(result, "process must report that it finished the exchange itself"); + assertEquals(1, callbacks.get()); + assertTrue(doneSync.get()); + assertNull(exchange.getException()); + } + + @Test + void anExchangeWithNoBodyIsDoneSynchronously() throws Exception { + VertxWebsocketProducer producer = producer(); + + Exchange exchange = new DefaultExchange(context); + + AtomicReference<Boolean> doneSync = new AtomicReference<>(); + boolean result = producer.process(exchange, doneSync::set); + + assertTrue(result, "process must report that it finished the exchange itself"); + assertTrue(doneSync.get()); Review Comment: Done, applied as you wrote it — `anExchangeWithNoBodyIsDoneSynchronously` now counts callbacks with an `AtomicInteger` and asserts `assertEquals(1, callbacks.get())`, matching the first test. You were right about why it mattered: `doneSync::set` silently overwrites, so a second `callback.done(true)` left both of the existing assertions green. _Claude Code on behalf of @oscerd_ ########## components/camel-vertx/camel-vertx-websocket/src/test/java/org/apache/camel/component/vertx/websocket/VertxWebsocketProducerNoPeerTest.java: ########## @@ -0,0 +1,95 @@ +/* + * 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.component.vertx.websocket; + +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import org.apache.camel.Exchange; +import org.apache.camel.impl.DefaultCamelContext; +import org.apache.camel.support.DefaultExchange; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * An exchange with no peer to send to is finished by the producer itself, so it has to report that it was done + * synchronously. Builds the endpoint directly, so nothing reaches a server. + */ +class VertxWebsocketProducerNoPeerTest { + + private DefaultCamelContext context; + + @AfterEach + void tearDown() { + if (context != null) { + context.stop(); + } + } + + private VertxWebsocketProducer producer() throws Exception { + context = new DefaultCamelContext(); + + VertxWebsocketComponent component = new VertxWebsocketComponent(); + component.setCamelContext(context); + + VertxWebsocketEndpoint endpoint + = (VertxWebsocketEndpoint) component.createEndpoint("vertx-websocket:localhost:1234/test"); + return (VertxWebsocketProducer) endpoint.createProducer(); + } + + @Test + void anExchangeWithNoPeerIsDoneSynchronously() throws Exception { + VertxWebsocketProducer producer = producer(); + + Exchange exchange = new DefaultExchange(context); + exchange.getIn().setBody("a message nobody is listening for"); + // broadcasting to an empty host registry is the one path that reaches no peer without opening a connection + exchange.getIn().setHeader(VertxWebsocketConstants.SEND_TO_ALL, true); + + AtomicInteger callbacks = new AtomicInteger(); + AtomicReference<Boolean> doneSync = new AtomicReference<>(); + + // the callback used to be completed with doneSync=true while the method returned false, which says the + // opposite: that the exchange would be finished from a write handler that never runs + boolean result = producer.process(exchange, sync -> { + callbacks.incrementAndGet(); + doneSync.set(sync); + }); + + assertTrue(result, "process must report that it finished the exchange itself"); + assertEquals(1, callbacks.get()); + assertTrue(doneSync.get()); + assertNull(exchange.getException()); + } + + @Test + void anExchangeWithNoBodyIsDoneSynchronously() throws Exception { + VertxWebsocketProducer producer = producer(); + + Exchange exchange = new DefaultExchange(context); + + AtomicReference<Boolean> doneSync = new AtomicReference<>(); + boolean result = producer.process(exchange, doneSync::set); + + assertTrue(result, "process must report that it finished the exchange itself"); + assertTrue(doneSync.get()); + } +} Review Comment: Added, but **not in this file** — and I checked why before moving it, because the suggested placement does not reach the branch. `getConnectedPeers()` only takes the unmatched-key path when peers already exist: ```java if (connectionKey != null && ObjectHelper.isNotEmpty(peers)) { ... if (peers.containsKey(key)) { ... } else { LOG.warn(...); } } else { // The producer is invoking an external server not managed by camel connectedPeers.put(UUID.randomUUID().toString(), endpoint.getWebSocket(exchange)); } ``` `VertxWebsocketProducerNoPeerTest` has an empty registry, so a `CONNECTION_KEY` there falls to the **else** branch and tries to open an outbound connection. I ran your test verbatim to be sure rather than argue from the code: ``` result=true callbacks=1 doneSync=true exception=NullPointerException: Cannot invoke "Vertx.createWebSocketClient(...)" ``` So (a) and (b) pass only through the catch-all `catch (Exception e) { exchange.setException(e); callback.done(true); return true; }`, and your own (c) — exception is null — fails. The WARN is never reached. The test is therefore in `VertxWebsocketTest`, which already has the server harness: one real connected peer, a key nobody has, and an assertion that the peer receives nothing. Peers are only added from a live connection handler, so there is no way to populate the registry from the producer unit test without faking internals. It pins behaviour rather than the log line — asserting on a WARN needs an appender and is brittle. To check it pins something real I mutated the branch to `connectedPeers.putAll(peers)`, the plausible wrong fix, and the test fails. Your underlying concern — a future refactor silently dropping the exchange, or worse, fanning it out to whoever is connected — is covered. _Claude Code on behalf of @oscerd_ ########## components/camel-vertx/camel-vertx-websocket/src/test/java/org/apache/camel/component/vertx/websocket/VertxWebsocketProducerNoPeerTest.java: ########## @@ -0,0 +1,93 @@ +/* + * 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.component.vertx.websocket; + +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import org.apache.camel.Exchange; +import org.apache.camel.impl.DefaultCamelContext; +import org.apache.camel.support.DefaultExchange; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * An exchange with no peer to send to is finished by the producer itself, so it has to report that it was done + * synchronously. Builds the endpoint directly, so nothing reaches a server. + */ +class VertxWebsocketProducerNoPeerTest { + + private DefaultCamelContext context; + + @AfterEach + void tearDown() { + if (context != null) { + context.stop(); + } + } + + private VertxWebsocketProducer producer() throws Exception { + context = new DefaultCamelContext(); + + VertxWebsocketComponent component = new VertxWebsocketComponent(); + component.setCamelContext(context); + + VertxWebsocketEndpoint endpoint + = (VertxWebsocketEndpoint) component.createEndpoint("vertx-websocket:localhost:1234/test"); + return (VertxWebsocketProducer) endpoint.createProducer(); + } + + @Test + void anExchangeWithNoPeerIsDoneSynchronously() throws Exception { + VertxWebsocketProducer producer = producer(); + + Exchange exchange = new DefaultExchange(context); + exchange.getIn().setBody("a message nobody is listening for"); + // broadcasting to an empty host registry is the one path that reaches no peer without opening a connection + exchange.getIn().setHeader(VertxWebsocketConstants.SEND_TO_ALL, true); + + AtomicInteger callbacks = new AtomicInteger(); + AtomicReference<Boolean> doneSync = new AtomicReference<>(); + + // the callback used to be completed with doneSync=true while the method returned false, which says the + // opposite: that the exchange would be finished from a write handler that never runs + boolean result = producer.process(exchange, sync -> { + callbacks.incrementAndGet(); + doneSync.set(sync); + }); + + assertThat(result).isTrue(); + assertThat(callbacks).hasValue(1); + assertThat(doneSync).hasValue(true); + assertThat(exchange.getException()).isNull(); + } + + @Test + void anExchangeWithNoBodyIsDoneSynchronously() throws Exception { + VertxWebsocketProducer producer = producer(); + + Exchange exchange = new DefaultExchange(context); + + AtomicReference<Boolean> doneSync = new AtomicReference<>(); + boolean result = producer.process(exchange, doneSync::set); Review Comment: Superseded — this was re-raised on the current diff and is addressed there. Both points are now in: the second no-peer test counts callbacks, and the unmatched `CONNECTION_KEY` path has a test in `VertxWebsocketTest` (it needs a real peer in the registry, which this file cannot provide — see the reply on the newer thread for the evidence). _Claude Code on behalf of @oscerd_ ########## components/camel-vertx/camel-vertx-websocket/src/test/java/org/apache/camel/component/vertx/websocket/VertxWebsocketProducerNoPeerTest.java: ########## @@ -0,0 +1,93 @@ +/* + * 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.component.vertx.websocket; + +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import org.apache.camel.Exchange; +import org.apache.camel.impl.DefaultCamelContext; +import org.apache.camel.support.DefaultExchange; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * An exchange with no peer to send to is finished by the producer itself, so it has to report that it was done + * synchronously. Builds the endpoint directly, so nothing reaches a server. + */ +class VertxWebsocketProducerNoPeerTest { + + private DefaultCamelContext context; + + @AfterEach + void tearDown() { + if (context != null) { + context.stop(); + } + } + + private VertxWebsocketProducer producer() throws Exception { + context = new DefaultCamelContext(); + + VertxWebsocketComponent component = new VertxWebsocketComponent(); + component.setCamelContext(context); + + VertxWebsocketEndpoint endpoint + = (VertxWebsocketEndpoint) component.createEndpoint("vertx-websocket:localhost:1234/test"); + return (VertxWebsocketProducer) endpoint.createProducer(); + } + + @Test + void anExchangeWithNoPeerIsDoneSynchronously() throws Exception { + VertxWebsocketProducer producer = producer(); + + Exchange exchange = new DefaultExchange(context); + exchange.getIn().setBody("a message nobody is listening for"); + // broadcasting to an empty host registry is the one path that reaches no peer without opening a connection + exchange.getIn().setHeader(VertxWebsocketConstants.SEND_TO_ALL, true); + + AtomicInteger callbacks = new AtomicInteger(); + AtomicReference<Boolean> doneSync = new AtomicReference<>(); + + // the callback used to be completed with doneSync=true while the method returned false, which says the + // opposite: that the exchange would be finished from a write handler that never runs + boolean result = producer.process(exchange, sync -> { + callbacks.incrementAndGet(); + doneSync.set(sync); + }); + + assertThat(result).isTrue(); + assertThat(callbacks).hasValue(1); + assertThat(doneSync).hasValue(true); + assertThat(exchange.getException()).isNull(); + } + + @Test + void anExchangeWithNoBodyIsDoneSynchronously() throws Exception { + VertxWebsocketProducer producer = producer(); + + Exchange exchange = new DefaultExchange(context); + + AtomicReference<Boolean> doneSync = new AtomicReference<>(); + boolean result = producer.process(exchange, doneSync::set); + + assertThat(result).isTrue(); + assertThat(doneSync).hasValue(true); Review Comment: Superseded — this was re-raised on the current diff and is addressed there. Both points are now in: the second no-peer test counts callbacks, and the unmatched `CONNECTION_KEY` path has a test in `VertxWebsocketTest` (it needs a real peer in the registry, which this file cannot provide — see the reply on the newer thread for the evidence). _Claude Code on behalf of @oscerd_ ########## components/camel-vertx/camel-vertx-websocket/src/test/java/org/apache/camel/component/vertx/websocket/VertxWebsocketProducerNoPeerTest.java: ########## @@ -0,0 +1,93 @@ +/* + * 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.component.vertx.websocket; + +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import org.apache.camel.Exchange; +import org.apache.camel.impl.DefaultCamelContext; +import org.apache.camel.support.DefaultExchange; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * An exchange with no peer to send to is finished by the producer itself, so it has to report that it was done + * synchronously. Builds the endpoint directly, so nothing reaches a server. + */ +class VertxWebsocketProducerNoPeerTest { + + private DefaultCamelContext context; + + @AfterEach + void tearDown() { + if (context != null) { + context.stop(); + } + } + + private VertxWebsocketProducer producer() throws Exception { + context = new DefaultCamelContext(); + + VertxWebsocketComponent component = new VertxWebsocketComponent(); + component.setCamelContext(context); + + VertxWebsocketEndpoint endpoint + = (VertxWebsocketEndpoint) component.createEndpoint("vertx-websocket:localhost:1234/test"); + return (VertxWebsocketProducer) endpoint.createProducer(); + } + + @Test + void anExchangeWithNoPeerIsDoneSynchronously() throws Exception { + VertxWebsocketProducer producer = producer(); + + Exchange exchange = new DefaultExchange(context); + exchange.getIn().setBody("a message nobody is listening for"); + // broadcasting to an empty host registry is the one path that reaches no peer without opening a connection + exchange.getIn().setHeader(VertxWebsocketConstants.SEND_TO_ALL, true); + + AtomicInteger callbacks = new AtomicInteger(); + AtomicReference<Boolean> doneSync = new AtomicReference<>(); + + // the callback used to be completed with doneSync=true while the method returned false, which says the + // opposite: that the exchange would be finished from a write handler that never runs + boolean result = producer.process(exchange, sync -> { + callbacks.incrementAndGet(); + doneSync.set(sync); + }); + + assertThat(result).isTrue(); + assertThat(callbacks).hasValue(1); + assertThat(doneSync).hasValue(true); + assertThat(exchange.getException()).isNull(); + } Review Comment: Superseded — this was re-raised on the current diff and is addressed there. Both points are now in: the second no-peer test counts callbacks, and the unmatched `CONNECTION_KEY` path has a test in `VertxWebsocketTest` (it needs a real peer in the registry, which this file cannot provide — see the reply on the newer thread for the evidence). _Claude Code on behalf of @oscerd_ -- 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]
