gemmellr commented on code in PR #4544:
URL: https://github.com/apache/activemq-artemis/pull/4544#discussion_r1259603913
##########
artemis-junit/artemis-junit-5/pom.xml:
##########
@@ -57,12 +57,7 @@
</dependency>
<dependency>
<groupId>org.apache.activemq</groupId>
- <artifactId>artemis-jms-server</artifactId>
- <version>${project.version}</version>
- </dependency>
- <dependency>
- <groupId>org.apache.activemq</groupId>
- <artifactId>artemis-jms-client</artifactId>
Review Comment:
This one is more reasonable since I believe the junit 5 stuff doesnt
actually supply the bits that use the JMS client, right?
(Which is actually really pretty bizarre, given its how we recommend most
users use the broker. I dont think anyone should use most of this artemis-junit
stuff personally.)
##########
artemis-cdi-client/pom.xml:
##########
@@ -99,15 +94,16 @@
<groupId>org.jboss.shrinkwrap</groupId>
<artifactId>shrinkwrap-api</artifactId>
<version>1.2.6</version>
+ <scope>test</scope>
</dependency>
<dependency>
<groupId>org.jboss.arquillian.junit</groupId>
<artifactId>arquillian-junit-container</artifactId>
</dependency>
<dependency>
- <groupId>org.apache.activemq</groupId>
- <artifactId>artemis-unit-test-support</artifactId>
- <version>${project.version}</version>
Review Comment:
This needs to stay if the logging impl does. The modules are all configured
with a log4j2.properties file that refers to an appender in
artemis-unit-test-support. It spews a stack when not finding it. Again, Maven
wont see a literal dependency, but its there implicitly.
##########
artemis-cli/pom.xml:
##########
@@ -38,6 +38,10 @@
<artifactId>artemis-jms-client</artifactId>
<version>${project.version}</version>
</dependency>
+ <dependency>
+ <groupId>jakarta.jms</groupId>
+ <artifactId>jakarta.jms-api</artifactId>
+ </dependency>
Review Comment:
If defining it, would make more sense to do it before the client dep above
it that would have already brought it in if not for this dep.
##########
tests/integration-tests-isolated/pom.xml:
##########
@@ -302,93 +133,59 @@
</exclusions>
</dependency>
<dependency>
- <groupId>org.apache.directory.server</groupId>
- <artifactId>apacheds-server-annotations</artifactId>
- <version>${directory-version}</version>
+ <groupId>org.apache.directory.api</groupId>
+ <artifactId>api-ldap-model</artifactId>
+ <version>2.0.0.AM1</version>
Review Comment:
We didnt have a direct dep on this before, so presumably it was transitive?
I'd leave it that way rather than hard code a version; add a version prop or
remove?
##########
tests/integration-tests/pom.xml:
##########
@@ -50,56 +56,61 @@
<groupId>org.apache.activemq</groupId>
<artifactId>artemis-jms-client</artifactId>
<version>${project.version}</version>
+ <scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.activemq</groupId>
<artifactId>artemis-jms-server</artifactId>
<version>${project.version}</version>
+ <scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.activemq</groupId>
<artifactId>artemis-ra</artifactId>
<version>${project.version}</version>
+ <scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.activemq</groupId>
<artifactId>artemis-cli</artifactId>
<version>${project.version}</version>
+ <scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.activemq</groupId>
<artifactId>artemis-commons</artifactId>
<version>${project.version}</version>
- </dependency>
- <dependency>
- <groupId>org.apache.activemq</groupId>
- <artifactId>artemis-spring-integration</artifactId>
- <version>${project.version}</version>
Review Comment:
If it isnt even being used here, and says in the module it isnt really
needed...is time to nuke it?
##########
tests/integration-tests/pom.xml:
##########
@@ -340,6 +402,19 @@
</exclusions>
</dependency>
+ <dependency>
+ <groupId>org.codehaus.woodstox</groupId>
+ <artifactId>woodstox-core-asl</artifactId>
+ <version>4.4.0</version>
Review Comment:
While we are 'cleaning up deps' and moving the dep declaration, lets at add
a prop for the version as done for some others.
##########
artemis-cdi-client/pom.xml:
##########
@@ -46,11 +46,6 @@
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>
- <dependency>
- <groupId>org.apache.logging.log4j</groupId>
- <artifactId>log4j-slf4j-impl</artifactId>
- <scope>test</scope>
- </dependency>
Review Comment:
This should probably stay, there is no SLF4J logging impl present otherwise,
prompting a needless warning and perhaps removing the possibility of actually
logging something. There is not meant to be any compile dependency on this or
its deps (besides slfj4-api), so I'd expect Maven generally wont see it as
being 'used' even when it is.
##########
artemis-quorum-ri/src/test/java/org/apache/activemq/artemis/quorum/DistributedLockTest.java:
##########
@@ -222,7 +219,7 @@ public void timedTryLockFailAfterTimeout() throws
ExecutionException, Interrupte
final long timeoutSec = 1;
Assert.assertFalse(manager.getDistributedLock("a").tryLock(timeoutSec,
TimeUnit.SECONDS));
final long elapsed = TimeUnit.NANOSECONDS.toSeconds(System.nanoTime() -
start);
- assertThat(elapsed, greaterThanOrEqualTo(timeoutSec));
+ Assert.assertTrue(elapsed >= timeoutSec);
Review Comment:
In places like this (and some of the other ones) the hamcrest version is far
better because it tells you what the expected+actual values were when it fails.
Without it, you are left to guess at the detail of why it failed. I'd suggest
preparing a failure message for the assert to compensate, here and in some of
the other cases.
##########
artemis-junit/artemis-junit-4/pom.xml:
##########
@@ -84,6 +79,12 @@
<artifactId>jakarta.jms-api</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.activemq</groupId>
+ <artifactId>artemis-jms-client</artifactId>
+ <version>${project.version}</version>
+ <scope>test</scope>
+ </dependency>
Review Comment:
I think this change is probably wrong and a breaking change. Its not
necessarily there as a compile dep, but as a means of supplying the client. A
key intent of the module is to test with the client and broker (still
supplied), and it has always provided the client. It largely makes sense that
it does. Maven cant tell this.
Its a bit like log4j-slf4j-impl. They had always supplied log4j itself along
when declaring that dep. In 2.19.0 for the new slf4j2 variant of the module,
they didnt...which meant you couldnt just swap the dep over, you had had to
declare that module AND add a log4j2 dep yourself in every module you used
log4j-slf4j2-impl. But the whole point of declaring log4j-slf4j* was always to
use log4j, so it was expected that it be supplied even though its not a compile
dep. That behaviour was restored for the slf4j2 variant for 2.20.0.
##########
artemis-server/pom.xml:
##########
@@ -288,12 +295,29 @@
</exclusion>
</exclusions>
</dependency>
+ <dependency>
+ <groupId>org.mock-server</groupId>
+ <artifactId>mockserver-core</artifactId>
+ <version>${mockserver.version}</version>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.mock-server</groupId>
+ <artifactId>mockserver-client-java</artifactId>
+ <version>${mockserver.version}</version>
+ <scope>test</scope>
+ </dependency>
Review Comment:
This seems likely to re-introduce the json-smart dependency excluded from
the mockserver-netty declaration present above, as the exclusion wont apply to
this separate direct declaration.
Either check and fix up the excludes, or just skip adding these. I would
skip.
--
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]
