[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3922: ARTEMIS-3652 - fix test cleanup, allow embedded broker properties to …

[GitBox <gi...@apache.org>]

gemmellr commented on a change in pull request #3922:
URL: https://github.com/apache/activemq-artemis/pull/3922#discussion_r789859571



##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/balancing/ElasticQueueTest.java
##########
@@ -398,7 +407,7 @@ private void prepareNodesAndStartCombinedHeadTail() throws Exception {
          configuration.setName("Node" + nodeId);
          configuration.setBrokerInstance(new File(getTestDirfile(), configuration.getName()));
          configuration.addAcceptorConfiguration("tcp", "tcp://localhost:" + (base_port + (nodeId)) + "?redirect-to=" + balancerConfigName + ";amqpCredits=1000;amqpMinCredits=300");
-         nodes.add(new EmbeddedActiveMQ().setConfiguration(configuration));
+         nodes.add(new EmbeddedActiveMQ().setConfiguration(configuration).setPropertiesResourcePath(null));

Review comment:
       This seems odd...is it actually necessary? Is there a default file on the classpath screwing with this test in some way? That would seem like a general issue needing fixed in itself (also against ARTEMIS-3627 perhaps?), it really doesnt feel like unrelated individual tests should need to be randomly setting this null to work.

##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/embedded/EmbeddedActiveMQ.java
##########
@@ -144,9 +155,11 @@ protected void initStart() throws Exception {
          activeMQServer = new ActiveMQServerImpl(configuration, mbeanServer, securityManager);
       }
 
-      URL brokerPropertiesFromClasspath = this.getClass().getClassLoader().getResource(ActiveMQDefaultConfiguration.BROKER_PROPERTIES_SYSTEM_PROPERTY_NAME);
-      if (brokerPropertiesFromClasspath != null) {
-         activeMQServer.setProperties(new File(brokerPropertiesFromClasspath.toURI()).getAbsolutePath());
+      if (propertiesResourcePath != null) {
+         URL brokerPropertiesFromClasspath = this.getClass().getClassLoader().getResource(propertiesResourcePath);
+         if (brokerPropertiesFromClasspath != null) {
+            activeMQServer.setProperties(new File(brokerPropertiesFromClasspath.toURI()).getAbsolutePath());
+         }

Review comment:
       Feels like this should really be in a separate commit against ARTEMIS-3627 specifically, where the original logic was only just added.




-- 
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: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org