Addressed ThrottlingLoggingAppenderTest issues (Alternate Solution to #2550)

[isaki]

This class utilizes the DefaultLoggingFactory. Unfortunately that class affects the static logging state of the JVM. This test now forces a full reset of the logging state in any test that touches a default logging factory object.

Problem:

Please refer to #2550 as this does an excellent job of describing the issue as well as how to reliably reproduce it and one possible solution.

Solution:

I have forced a full state reset in the logging factory used by the unit test.

Result:

The intermittent failures due to this issue will stop (as well as the reproduction steps will no longer trigger a failure).

[nickbabcock]

@OrDTesters, did you have any opinions on this implementation? I suppose in the end both are similar.

[isaki]

The reason I prefer this is that we use the same object that was used in the test rather than relying on a side effect of invoking configure on a new instance (that just happens to change the same static state that this version modifies). As such, it is clearer to anyone supporting this in the future what is going on as opposed to having to dig through the implementation of DefaultLoggingFactory to discover that using a new instance does get you where you need to be.

Both are valid solutions; the new instance is fewer lines of code but I personally find counter-intuitive. At the end of the day it purely comes down to preference.

[OrDTesters]

@nickbabcock I don't have much to add; as you've said, @isaki's implementation is similar to mine. Given that I don't have that much experience with this project, perhaps @isaki's fix is preferable.

That being said, I agree that it's strange to create a new instance in a teardown. On the other hand, the lines of code added into the teardown in this fix seem to essentially clean out the whole state of the defaultLoggingFactory, so it might as well be a new object. Given that, it also looks strange to keep using the same object for two purposes (testing as well as clean up).

Perhaps a clearer fix would be to modify (or create a new method) the existing reset method in the DefaultLoggingFactory itself so that it also resets the status printer/does the other setup in the configure method. However, as I said, I am not familiar enough with this project to know if that would cause issues elsewhere.

[isaki]

I think ideally it would be nice to not modify static state with object instance methods (unless that object was a singleton, and even then it's a bit strange). Given the way the integration with SLF4J is currently architected, I don't see a way around that without a major refactoring (and you wouldn't eliminate the static state, which would mean methods to support unit tests regardless).

I like the idea of maybe having some sort of package private (possibly static) reset method that can be used by unit tests (I'd say annotate it with @VisibleForTesting but DropWizard has moved away from Guava). I'll look into that.

[isaki]

I apologize for the delay; my professional responsibilities had me tied up so I didn't get a chance to look at this. I've pushed a new 'clear' concept for unit tests using the default logging factory. I don't love the method name so if you want it renamed I'm open to suggestions. Let me know if this works better for you @nickbabcock and @OrDTesters. Thanks!

[isaki]

It looks like there is some flakiness in the authentication tests; that is outside the scope of this PR. I may try to look at it in the future though.

[isaki]

I can reproduce the AppVeyor error by messing with the Thread.sleep calls; it's most likely a timing issue and not related to this PR. I'll submit a separate PR once I come up with a fix.

[nickbabcock]

Yeah this looks good to me! Dealing with static state in tests is no fun, so I think there couldn't really be a perfect solution. Don't worry about the CI flakiness in this PR.