Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1337#discussion_r121922296
--- Diff:
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpClientTestSupport.java
---
@@ -74,6 +79,17 @@
protected MBeanServer mBeanServer =
MBeanServerFactory.createMBeanServer();
+ @Parameterized.Parameters(name = "{index}:
amqpUseCoreSubscriptionNaming={0}")
+ public static Collection<Object[]> data() {
+ return Arrays.asList(new Object[][] {
+ {true}, {false}
--- End diff --
I'm confused by this. Is the effect of adding parameters data here with
'true + false' values, and the related parameters data in JMSSharedConsumerTest
etc subclasses with only 'true', not that almost every test will be run twice
as often and the specific ones for subscriptions will only test the non-default
behaviour? If so it seems like the reverse of what we want on both counts?
Regardless, I also think it would be better all round to avoid
parameterizing every test in the suite for this, and instead restrict it to
only those that specifically need to change it. For example, see the use of
AmqpClientTestSupport#isSecurityEnabled() and how only some specific tests use
it to affect the broker config. Something similar could be added for the
subscription queue naming, or perhaps the general
AmqpClientTestSupport#addConfiguration(server) override could be utilized
instead, so that only the tests of interest need override the default behaviour
and provide the new config (via test paramterization or otherwise).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
