gemmellr commented on PR #4544: URL: https://github.com/apache/activemq-artemis/pull/4544#issuecomment-1629245369
I havent actually yet looked at this, but a few comments before I get to it: - The commons-configuration2 upgrade mentioned should be probably done separately. - Yep, I elected not to have the artemis-server test-jar dep declared everywhere, and isolated it to artemis-test-support to give one place to break it later. It sticks out transitively anyway, and its mainly used within the tests tree so it didnt seem like it was really any nicer listing it. - Moving what are really more like integration test base classes such as ActiveMQTestBase into artemis-test-support (or some in artemis-unit-test-support as well) was partof my long term plan yes. Ideally 'actual unit tests' should be in the modules, but these current tests are often wiring so much together I'd question if they they really are even though thats where they live now (not sure 'kind of a mess' really does it justice hehe). One thing to consider is, moving any tests from e.g artemis-server into e.g integration-tests will reduce the set of tests run in the PR check. They could be retained in the PR run tests (if still seeming suitable) by including them in the integration-tests subset run by surefure when enabling the fast-tests profile (see the pom). -- 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]
