Skip to content

Conversation

@mattrpav
Copy link
Contributor

No description provided.

@mattrpav mattrpav marked this pull request as ready for review March 30, 2023 00:39
Copy link
Contributor

@cshannon cshannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shows that we really need tests. We likely need to use Maven here to and do something where we fork another JVM. We should really run the client against a broker to verify it can connect and work so I think that needs to be included as part of this. We need to verify all the transports work in a real test that isn't ignored.

@mattrpav
Copy link
Contributor Author

mattrpav commented Apr 4, 2023

@cshannon I manually verified the client works for tcp:// and ssl://, and the jar contains the META-INF/services/*.

I agree we need a unit test, but I'm not seeing a quick path to getting a test in-build, since we'd have to implement a new test pattern. The activemq-run maven plugin splits off a thread, not a process on 'fork = true'.

@cshannon
Copy link
Contributor

cshannon commented Apr 4, 2023

Alright, can go ahead and get this merged so we can do 5.18.1 as need to get this fixed but for 5.19.0, especially with more planned changes we definitely can't release without automated testing in some way.

@cshannon cshannon merged commit f8695b5 into apache:main Apr 4, 2023
mattrpav added a commit that referenced this pull request Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants