cshannon commented on PR #1484: URL: https://github.com/apache/activemq/pull/1484#issuecomment-3784972754
@mattrpav - The biggest thing now is I think the test coverage is lacking. 1. Some of the new methods could be unit tested . The new hasActiveConsumers() and canGcConsumer predicate can probably be tested in isolation as they are protected so maybe with mocks or something. 2. I think we need to double check there is sufficient test cover for the different combinations of the gcWithOnlyWildcardConsumers and gcWithOnlyWildcardConsumers flags being set. You added a test if gcWithOnlyWildcardConsumers is true but we should also add a quick test where both flags are true (easy enough to change in the policy and can probably just reuse the same test). We should also verify there's a test somewhere that still works for gc network consumers flag (i would hope so). 3. Verify that there's good GC test coverage with both flags false as the default, i would expect there should already be some GC tests but I haven't looked in a while. I think better test coverage is important because it would be bad to start deleting destinations that should not be deleted as that leads to permanent data loss. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected] For further information, visit: https://activemq.apache.org/contact
