Fix two error-prone warnings in tests#51
Merged
Conversation
GuiceberryCompatibilityModuleTest.testWrapperExceptionPropagated swallowed the expected exception in an empty catch block. Replace the try/catch idiom with assertThrows, which both states the intent explicitly and silences the EmptyCatch warning. DependenciesTest.arbitraryOrdering passed `new ServiceA()` twice in an ImmutableSet.of varargs, which DistinctVarargsChecker flags as the kind of thing that's almost always a copy-paste bug. The test just asserts that inOrder() returns every input service for an unrelated set, which two distinct services express equally well.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GuiceberryCompatibilityModuleTest.testWrapperExceptionPropagatedwas using the legacy try/catch-and-swallow idiom to assert that aTestExceptionis thrown. Error-prone'sEmptyCatchflagged the swallowed exception. Replaced withassertThrows, which states the intent explicitly.DependenciesTest.arbitraryOrderingpassednew ServiceA()twice in anImmutableSet.ofvarargs call.DistinctVarargsCheckerflags this as the kind of thing that is almost always a copy-paste bug. The test only asserts thatDependencies.inOrderreturns every input service for a set with no@DependsOnrelationships — two distinct services express that equally well.Test plan
mvn -B clean test— 43/43 passEmptyCatchorDistinctVarargsCheckerwarnings in the build output