diff --git a/java/piranha/build.gradle b/java/piranha/build.gradle index f3b4d14f8..ac75d92a7 100644 --- a/java/piranha/build.gradle +++ b/java/piranha/build.gradle @@ -38,6 +38,7 @@ dependencies { testCompile deps.test.junit4 testCompile deps.test.daggerAnnotations testImplementation group: 'org.mockito', name: 'mockito-core', version: '2.1.0' + testImplementation group: 'org.easymock', name: 'easymock', version: '4.3' testCompile(deps.build.errorProneTestHelpers) { exclude group: "junit", module: "junit" } diff --git a/java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java b/java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java index b424f2c3f..1d905bbee 100644 --- a/java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java +++ b/java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java @@ -16,6 +16,8 @@ import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.ALL; import static com.google.errorprone.matchers.Matchers.allOf; +import static com.google.errorprone.matchers.Matchers.anyOf; +import static com.google.errorprone.matchers.Matchers.contains; import static com.google.errorprone.matchers.Matchers.instanceMethod; import static com.google.errorprone.matchers.Matchers.methodInvocation; import static com.google.errorprone.matchers.Matchers.receiverOfInvocation; @@ -705,10 +707,51 @@ private Description updateCode( return Description.NO_MATCH; } + private static final String EASYMOCK_QN = "org.easymock.EasyMock"; + private static final String EASYMOCK_EXPECT = "expect"; + private static final Pattern EASYMOCK_AND = Pattern.compile("and\\W*\\w*"); + private static final String EASYMOCK_ANYTIMES = "anyTimes"; + private static final String EASYMOCK_TIMES = "times"; + private static final String EASYMOCK_ONCE = "once"; + private static final String EASYMOCK_ATLEASTONCE = "atLeastOnce"; + private static final String EASYMOCK_ASSTUB = "asStub"; + private static final String MOCKITO_QN = "org.mockito.Mockito"; private static final String MOCKITO_WHEN = "when"; private static final Pattern MOCKITO_THEN = Pattern.compile("then\\W*\\w*"); + private static final String JUNIT_ASSERT_QN = "org.junit.Assert"; + private static final String JUNIT_ASSERTTRUE = "assertTrue"; + private static final String JUNIT_ASSERTFALSE = "assertFalse"; + + private final Matcher JUNIT_ASSERT_PATTERN = + toType( + MethodInvocationTree.class, + anyOf( + staticMethod().onClass(JUNIT_ASSERT_QN).named(JUNIT_ASSERTTRUE), + staticMethod().onClass(JUNIT_ASSERT_QN).named(JUNIT_ASSERTFALSE))); + + private final Matcher EASYMOCK_PATTERN = + toType( + MethodInvocationTree.class, + allOf( + anyOf( + instanceMethod().anyClass().withNameMatching(EASYMOCK_AND), + instanceMethod().anyClass().named(EASYMOCK_ANYTIMES), + instanceMethod().anyClass().named(EASYMOCK_ONCE), + instanceMethod().anyClass().named(EASYMOCK_TIMES), + instanceMethod().anyClass().named(EASYMOCK_ASSTUB), + instanceMethod().anyClass().named(EASYMOCK_ATLEASTONCE)), + contains( + ExpressionTree.class, + methodInvocation( + staticMethod().onClass(EASYMOCK_QN).named(EASYMOCK_EXPECT), + ALL, + (argument, vs) -> { + Value value = evalExpr(argument, vs); + return value == Value.TRUE || value == Value.FALSE; + })))); + private final Matcher MOCKITO_UNNECESSARY_MOCKING_PATTERN = toType( MethodInvocationTree.class, @@ -726,7 +769,10 @@ private Description updateCode( private SuggestedFix.Builder handleSpecificAPIPatterns(VisitorState state) { ExpressionStatementTree stmt = ASTHelpers.findEnclosingNode(state.getPath(), ExpressionStatementTree.class); - if (stmt != null && MOCKITO_UNNECESSARY_MOCKING_PATTERN.matches(stmt.getExpression(), state)) { + if (stmt != null + && (MOCKITO_UNNECESSARY_MOCKING_PATTERN.matches(stmt.getExpression(), state) + || EASYMOCK_PATTERN.matches(stmt.getExpression(), state) + || JUNIT_ASSERT_PATTERN.matches(stmt.getExpression(), state))) { endPos = state.getEndPosition(stmt); return SuggestedFix.builder().delete(stmt); } diff --git a/java/piranha/src/test/java/com/uber/piranha/CorePiranhaTest.java b/java/piranha/src/test/java/com/uber/piranha/CorePiranhaTest.java index 1f4477221..c217948b2 100644 --- a/java/piranha/src/test/java/com/uber/piranha/CorePiranhaTest.java +++ b/java/piranha/src/test/java/com/uber/piranha/CorePiranhaTest.java @@ -520,9 +520,13 @@ public void testRemoveSpecificAPIpatterns() throws IOException { "import org.mockito.invocation.InvocationOnMock;", "import org.mockito.stubbing.Answer;", "import org.mockito.stubbing.OngoingStubbing;", + "import org.easymock.EasyMock;", + "import static org.junit.Assert.assertFalse;", + "import static org.junit.Assert.assertTrue;", "class MockitoTest {", " private XPTest experimentation;", " public void evaluate() {", + "// Mockito Test Scenarios", " Answer ans = new Answer() {\n" + " public Integer answer(InvocationOnMock invocation) throws Throwable {\n" + " return (Integer) invocation.getArguments()[0];\n" @@ -541,6 +545,27 @@ public void testRemoveSpecificAPIpatterns() throws IOException { " when(experimentation.isToggleEnabled(\"OTHER_FLAG\")).then(ans);", " when(foobar()).thenReturn(false);", " when(foobar()).thenAnswer(ans);", + "// Easymock Test scenarios", + " EasyMock.expect(experimentation.isToggleDisabled(\"STALE_FLAG\")).andReturn(true).anyTimes();", + " EasyMock.expect(experimentation.isToggleEnabled(\"STALE_FLAG\")).andReturn(true).times(5);", + " EasyMock.expect(experimentation.isToggleDisabled(\"STALE_FLAG\")).andReturn(true).times(5, 6);", + " EasyMock.expect(experimentation.isToggleDisabled(\"STALE_FLAG\")).andReturn(true).once();", + " EasyMock.expect(experimentation.isToggleEnabled(\"STALE_FLAG\")).andReturn(true).atLeastOnce();", + " EasyMock.expect(experimentation.isToggleEnabled(\"STALE_FLAG\")).andReturn(true).anyTimes();", + " EasyMock.expect(experimentation.isToggleEnabled(\"STALE_FLAG\")).andReturn(true);", + " EasyMock.expect(experimentation.isToggleEnabled(\"STALE_FLAG\")).asStub();", + " EasyMock.expect(experimentation.isToggleEnabled(\"OTHER_FLAG\")).andReturn(true).anyTimes();", + " EasyMock.expect(foobar()).andReturn(true).times(5);", + " EasyMock.expect(experimentation.isToggleEnabled(\"OTHER_FLAG\")).andReturn(true).times(5, 6);", + " EasyMock.expect(experimentation.isToggleDisabled(\"OTHER_FLAG\")).andReturn(true).once();", + " EasyMock.expect(experimentation.isToggleDisabled(\"OTHER_FLAG\")).andReturn(true).atLeastOnce();", + " EasyMock.expect(experimentation.isToggleDisabled(\"OTHER_FLAG\")).andReturn(true).anyTimes();", + "// JUnit Assert Test scearios", + " assertFalse(experimentation.isToggleDisabled(\"STALE_FLAG\"));", + " assertTrue(experimentation.isToggleEnabled(\"STALE_FLAG\"));", + " assertFalse(experimentation.isToggleEnabled(\"OTHER_FLAG\"));", + " assertTrue(experimentation.isToggleDisabled(\"OTHER_FLAG\"));", + " assertTrue(foobar());", " }", " public boolean foobar() { return true;}", " public boolean someWrapper(Object obj) { return true;}", @@ -553,9 +578,13 @@ public void testRemoveSpecificAPIpatterns() throws IOException { "import org.mockito.invocation.InvocationOnMock;", "import org.mockito.stubbing.Answer;", "import org.mockito.stubbing.OngoingStubbing;", + "import org.easymock.EasyMock;", + "import static org.junit.Assert.assertFalse;", + "import static org.junit.Assert.assertTrue;", "class MockitoTest {", " private XPTest experimentation;", " public void evaluate() {", + "// Mockito Test Scenarios", " Answer ans = new Answer() {\n" + " public Integer answer(InvocationOnMock invocation) throws Throwable {\n" + " return (Integer) invocation.getArguments()[0];\n" @@ -564,6 +593,7 @@ public void testRemoveSpecificAPIpatterns() throws IOException { "", "", "", + "", " boolean b1 = someWrapper(when(true).thenCallRealMethod());", " boolean b2 = someWrapper(when(experimentation.isToggleDisabled(\"OTHER_FLAG\")).thenCallRealMethod());", " someWhenWrapper(when(true)).thenCallRealMethod();", @@ -574,12 +604,30 @@ public void testRemoveSpecificAPIpatterns() throws IOException { " when(experimentation.isToggleEnabled(\"OTHER_FLAG\")).then(ans);", " when(foobar()).thenReturn(false);", " when(foobar()).thenAnswer(ans);", + "// Easymock Test scenarios", + "", + "", + "", + "", + "", + "", + " EasyMock.expect(experimentation.isToggleEnabled(\"OTHER_FLAG\")).andReturn(true).anyTimes();", + " EasyMock.expect(foobar()).andReturn(true).times(5);", + " EasyMock.expect(experimentation.isToggleEnabled(\"OTHER_FLAG\")).andReturn(true).times(5, 6);", + " EasyMock.expect(experimentation.isToggleDisabled(\"OTHER_FLAG\")).andReturn(true).once();", + " EasyMock.expect(experimentation.isToggleDisabled(\"OTHER_FLAG\")).andReturn(true).atLeastOnce();", + " EasyMock.expect(experimentation.isToggleDisabled(\"OTHER_FLAG\")).andReturn(true).anyTimes();", + "// JUnit Assert Test scearios", + "", + "", + " assertFalse(experimentation.isToggleEnabled(\"OTHER_FLAG\"));", + " assertTrue(experimentation.isToggleDisabled(\"OTHER_FLAG\"));", + " assertTrue(foobar());", " }", " public boolean foobar() { return true;}", " public boolean someWrapper(Object obj) { return true;}", " public OngoingStubbing someWhenWrapper(OngoingStubbing x) { return x;}", "}") .doTest(); - // OngoingStubbing w = when(true).the; } }