gemmellr commented on code in PR #5431:
URL: https://github.com/apache/activemq-artemis/pull/5431#discussion_r1910426210
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/routing/RedirectTest.java:
##########
@@ -45,37 +45,30 @@
import org.apache.activemq.artemis.tests.extensions.parameterized.Parameters;
import org.apache.activemq.artemis.core.server.routing.KeyType;
import
org.apache.activemq.artemis.core.server.cluster.impl.MessageLoadBalancingType;
-import org.apache.activemq.artemis.tests.util.Wait;
import org.junit.jupiter.api.TestTemplate;
import org.junit.jupiter.api.extension.ExtendWith;
@ExtendWith(ParameterizedTestExtension.class)
public class RedirectTest extends RoutingTestBase {
+ static final String[] protocols = new String[] {AMQP_PROTOCOL,
CORE_PROTOCOL, OPENWIRE_PROTOCOL};
+ static final String[] pools = new String[] {CLUSTER_POOL, DISCOVERY_POOL,
STATIC_POOL};
@Parameters(name = "protocol: {0}, pool: {1}")
public static Collection<Object[]> data() {
- final String[] protocols = new String[] {AMQP_PROTOCOL, CORE_PROTOCOL,
OPENWIRE_PROTOCOL};
- final String[] pools = new String[] {CLUSTER_POOL, DISCOVERY_POOL,
STATIC_POOL};
Collection<Object[]> data = new ArrayList<>();
- for (String protocol : Arrays.asList(protocols)) {
- for (String pool : Arrays.asList(pools)) {
- data.add(new Object[] {protocol, pool});
- }
+ for (String pool : Arrays.asList(pools)) {
+ data.add(new Object[] {AMQP_PROTOCOL, pool});
Review Comment:
The protocol parameter isnt being used, the field it populated was removed,
so seems like it can just go away rather than be passed a fixed unused value of
AMQP_PROTOCOL, which will make for misleading test display names in results.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/routing/RedirectTest.java:
##########
@@ -105,39 +98,41 @@ public void testSimpleRedirect() throws Exception {
QueueControl queueControl1 =
(QueueControl)getServer(1).getManagementService()
.getResource(ResourceNames.QUEUE + queueName);
- assertEquals(0, queueControl0.countMessages());
- assertEquals(0, queueControl1.countMessages());
+ for (String protocol : Arrays.asList(protocols)) {
Review Comment:
As set out here, many failure scenarios will be difficult to distinguish
from each other, e.g any of the assertions failing will give the exact same
stack+message for all of the protocols.
Originally I was suggesting just extracting a method, and explicitly calling
it in turn for each protocol of interest...in that case at least the stacks
would differ for each failure.
In this style of repetition, I'd suggest adding messages to the assertions
which denote the protocol, thena failure will be more immediately
understandable. Also, the body of TextMessage sent could also indicate the
protocol (and the test name?) rather than just "TEST", helping ensure the truly
correct matching message is actually the one that was received, and not another
of the other identical messages sent during the test.
( + Much the same for the other tests below)
--
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]
For further information, visit: https://activemq.apache.org/contact
