From 4ac47a7a693a355b584dc93828cd203020d0f3f5 Mon Sep 17 00:00:00 2001 From: Ameya Ketkar Date: Tue, 30 Nov 2021 16:54:21 -0800 Subject: [PATCH 1/7] Delete statement when it matches a specific Mockito API pattern Fixed the Swift CI issue Addressed the issues mentioned in the PR and offline Added support for then* pattern Added delete statement logic for specific patterns menntioned in issue #104 --- .github/workflows/swift.yml | 4 +- java/piranha/build.gradle | 1 + .../java/com/uber/piranha/XPFlagCleaner.java | 63 ++++++++++---- .../com/uber/piranha/CorePiranhaTest.java | 83 +++++++++++++++++++ 4 files changed, 131 insertions(+), 20 deletions(-) diff --git a/.github/workflows/swift.yml b/.github/workflows/swift.yml index e580d8d6a..7a1bacb63 100644 --- a/.github/workflows/swift.yml +++ b/.github/workflows/swift.yml @@ -12,14 +12,14 @@ jobs: runs-on: macos-11 strategy: matrix: - xcode: ['13.0'] + xcode: ['13.1'] name: "PiranhaSwift using ${{ matrix.xcode }} on ${{ matrix.os }}" steps: - name: Check out Piranha sources uses: actions/checkout@v2 - name: Set Xcode ${{ matrix.xcode }} run: | - sudo xcode-select -s /Applications/Xcode_13.0_beta.app + sudo xcode-select -s /Applications/Xcode_13.1.app xcodebuild -version swift --version swift package --version diff --git a/java/piranha/build.gradle b/java/piranha/build.gradle index 9523caaec..f3b4d14f8 100644 --- a/java/piranha/build.gradle +++ b/java/piranha/build.gradle @@ -37,6 +37,7 @@ dependencies { testCompile deps.test.junit4 testCompile deps.test.daggerAnnotations + testImplementation group: 'org.mockito', name: 'mockito-core', version: '2.1.0' 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 771709ba8..b424f2c3f 100644 --- a/java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java +++ b/java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java @@ -14,6 +14,13 @@ package com.uber.piranha; 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.instanceMethod; +import static com.google.errorprone.matchers.Matchers.methodInvocation; +import static com.google.errorprone.matchers.Matchers.receiverOfInvocation; +import static com.google.errorprone.matchers.Matchers.staticMethod; +import static com.google.errorprone.matchers.Matchers.toType; import com.facebook.infer.annotation.Initializer; import com.google.auto.service.AutoService; @@ -27,6 +34,7 @@ import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; import com.google.errorprone.util.ASTHelpers; import com.google.errorprone.util.FindIdentifiers; import com.sun.source.tree.AnnotationTree; @@ -71,6 +79,7 @@ import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.regex.Pattern; import java.util.stream.Collectors; import java.util.stream.Stream; import javax.annotation.Nullable; @@ -683,29 +692,47 @@ private boolean evalTreatmentGroupCheck(MethodInvocationTree methodInvocationTre */ private Description updateCode( Value v, ExpressionTree tree, ExpressionTree expr, VisitorState state) { - boolean update = false; - String replacementString = ""; - - if (v.equals(Value.TRUE)) { - update = true; - replacementString = TRUE; - } else if (v.equals(Value.FALSE)) { - update = true; - replacementString = FALSE; - } - - if (update) { - Description.Builder builder = buildDescription(tree); - SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); - fixBuilder.replace(expr, replacementString); + if (v.equals(Value.TRUE) || v.equals(Value.FALSE)) { + SuggestedFix.Builder fixBuilder = handleSpecificAPIPatterns(state); + if (fixBuilder.isEmpty()) { + String replacementString = v.equals(Value.TRUE) ? TRUE : FALSE; + fixBuilder = SuggestedFix.builder().replace(expr, replacementString); + endPos = state.getEndPosition(expr); + } decrementAllSymbolUsages(expr, state, fixBuilder); - builder.addFix(fixBuilder.build()); - endPos = state.getEndPosition(expr); - return builder.build(); + return buildDescription(tree).addFix(fixBuilder.build()).build(); } return Description.NO_MATCH; } + 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 final Matcher MOCKITO_UNNECESSARY_MOCKING_PATTERN = + toType( + MethodInvocationTree.class, + allOf( + instanceMethod().anyClass().withNameMatching(MOCKITO_THEN), + receiverOfInvocation( + methodInvocation( + staticMethod().onClass(MOCKITO_QN).named(MOCKITO_WHEN), + ALL, + (argument, vs) -> { + Value value = evalExpr(argument, vs); + return value == Value.TRUE || value == Value.FALSE; + })))); + + 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)) { + endPos = state.getEndPosition(stmt); + return SuggestedFix.builder().delete(stmt); + } + return SuggestedFix.builder(); + } + private boolean isTreatmentGroupEnum(Symbol.ClassSymbol enumSym) { // Filter out some generic names, like CONTROL to make sure we don't match the wrong // TreatmentGroup 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 f63e2bbc3..1f4477221 100644 --- a/java/piranha/src/test/java/com/uber/piranha/CorePiranhaTest.java +++ b/java/piranha/src/test/java/com/uber/piranha/CorePiranhaTest.java @@ -499,4 +499,87 @@ public void testIgnoresPrefixMatchFlag() throws IOException { "class TestClassPartialMatch { }") .doTest(); } + + @Test + public void testRemoveSpecificAPIpatterns() throws IOException { + + ErrorProneFlags.Builder b = ErrorProneFlags.builder(); + b.putFlag("Piranha:FlagName", "STALE_FLAG"); + b.putFlag("Piranha:IsTreated", "false"); + b.putFlag("Piranha:Config", "config/properties.json"); + + BugCheckerRefactoringTestHelper bcr = + BugCheckerRefactoringTestHelper.newInstance(new XPFlagCleaner(b.build()), getClass()); + + bcr = bcr.setArgs("-d", temporaryFolder.getRoot().getAbsolutePath()); + bcr = PiranhaTestingHelpers.addHelperClasses(bcr); + bcr.addInputLines( + "XPFlagCleanerSinglePositiveCase.java", + "package com.uber.piranha;", + "import static org.mockito.Mockito.when;", + "import org.mockito.invocation.InvocationOnMock;", + "import org.mockito.stubbing.Answer;", + "import org.mockito.stubbing.OngoingStubbing;", + "class MockitoTest {", + " private XPTest experimentation;", + " public void evaluate() {", + " Answer ans = new Answer() {\n" + + " public Integer answer(InvocationOnMock invocation) throws Throwable {\n" + + " return (Integer) invocation.getArguments()[0];\n" + + " }};", + " when(experimentation.isToggleDisabled(\"STALE_FLAG\")).thenReturn(false);", + " when(experimentation.isToggleEnabled(\"STALE_FLAG\")).thenThrow(new RuntimeException());", + " when(experimentation.isToggleDisabled(\"STALE_FLAG\")).thenCallRealMethod();", + " when(experimentation.isToggleEnabled(\"STALE_FLAG\")).then(ans);", + " boolean b1 = someWrapper(when(experimentation.isToggleDisabled(\"STALE_FLAG\")).thenCallRealMethod());", + " boolean b2 = someWrapper(when(experimentation.isToggleDisabled(\"OTHER_FLAG\")).thenCallRealMethod());", + " someWhenWrapper(when(experimentation.isToggleDisabled(\"STALE_FLAG\"))).thenCallRealMethod();", + " someWhenWrapper(when(experimentation.isToggleDisabled(\"OTHER_FLAG\"))).thenCallRealMethod();", + " when(experimentation.isToggleEnabled(\"OTHER_FLAG\")).thenReturn(false);", + " when(experimentation.isToggleEnabled(\"OTHER_FLAG\")).thenThrow(new RuntimeException());", + " when(experimentation.isToggleDisabled(\"OTHER_FLAG\")).thenCallRealMethod();", + " when(experimentation.isToggleEnabled(\"OTHER_FLAG\")).then(ans);", + " when(foobar()).thenReturn(false);", + " when(foobar()).thenAnswer(ans);", + " }", + " public boolean foobar() { return true;}", + " public boolean someWrapper(Object obj) { return true;}", + " public OngoingStubbing someWhenWrapper(OngoingStubbing x) { return x;}", + "}") + .addOutputLines( + "XPFlagCleanerSinglePositiveCase.java", + "package com.uber.piranha;", + "import static org.mockito.Mockito.when;", + "import org.mockito.invocation.InvocationOnMock;", + "import org.mockito.stubbing.Answer;", + "import org.mockito.stubbing.OngoingStubbing;", + "class MockitoTest {", + " private XPTest experimentation;", + " public void evaluate() {", + " Answer ans = new Answer() {\n" + + " public Integer answer(InvocationOnMock invocation) throws Throwable {\n" + + " return (Integer) invocation.getArguments()[0];\n" + + " }};", + "", + "", + "", + "", + " boolean b1 = someWrapper(when(true).thenCallRealMethod());", + " boolean b2 = someWrapper(when(experimentation.isToggleDisabled(\"OTHER_FLAG\")).thenCallRealMethod());", + " someWhenWrapper(when(true)).thenCallRealMethod();", + " someWhenWrapper(when(experimentation.isToggleDisabled(\"OTHER_FLAG\"))).thenCallRealMethod();", + " when(experimentation.isToggleEnabled(\"OTHER_FLAG\")).thenReturn(false);", + " when(experimentation.isToggleEnabled(\"OTHER_FLAG\")).thenThrow(new RuntimeException());", + " when(experimentation.isToggleDisabled(\"OTHER_FLAG\")).thenCallRealMethod();", + " when(experimentation.isToggleEnabled(\"OTHER_FLAG\")).then(ans);", + " when(foobar()).thenReturn(false);", + " when(foobar()).thenAnswer(ans);", + " }", + " 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; + } } From 2efd810a51869b56568c51c574737e7f67b1a6e6 Mon Sep 17 00:00:00 2001 From: Ameya Ketkar Date: Thu, 2 Dec 2021 12:51:40 -0800 Subject: [PATCH 2/7] Adding support for junit and easy mock issue #47 --- java/piranha/build.gradle | 1 + .../java/com/uber/piranha/XPFlagCleaner.java | 48 +++++++++++++++++- .../com/uber/piranha/CorePiranhaTest.java | 50 ++++++++++++++++++- 3 files changed, 97 insertions(+), 2 deletions(-) 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; } } From fc3d50f6f8f81304c66029699a91450786814744 Mon Sep 17 00:00:00 2001 From: Ameya Ketkar Date: Wed, 8 Dec 2021 10:02:27 +0530 Subject: [PATCH 3/7] Introduced a feature that allows unncessary test methods to be configured via the properties file --- java/piranha/config/properties.json | 22 ++++ .../java/com/uber/piranha/XPFlagCleaner.java | 99 ++++----------- .../java/com/uber/piranha/config/Config.java | 38 +++++- .../com/uber/piranha/config/MethodRecord.java | 115 ++++++++++++++++++ .../piranha/config/PiranhaMethodRecord.java | 53 ++------ .../com/uber/piranha/CorePiranhaTest.java | 2 +- 6 files changed, 204 insertions(+), 125 deletions(-) create mode 100644 java/piranha/src/main/java/com/uber/piranha/config/MethodRecord.java diff --git a/java/piranha/config/properties.json b/java/piranha/config/properties.json index 983765365..a21125191 100644 --- a/java/piranha/config/properties.json +++ b/java/piranha/config/properties.json @@ -38,6 +38,28 @@ "argumentIndex": 0 } ], + "unnecessaryTestMethodProperties" : [ + { + "methodName": "when", + "argumentIndex": 0, + "recieverType":"org.mockito.Mockito" + }, + { + "methodName": "expect", + "argumentIndex": 0, + "recieverType":"org.easymock.EasyMock" + }, + { + "methodName": "assertTrue", + "argumentIndex": 0, + "recieverType":"org.junit.Assert" + }, + { + "methodName": "assertFalse", + "argumentIndex": 0, + "recieverType":"org.junit.Assert" + } + ], "linkURL": "", "annotations": [ "ToggleTesting", 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 36633291d..9be281218 100644 --- a/java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java +++ b/java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java @@ -14,15 +14,7 @@ package com.uber.piranha; 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; import static com.google.errorprone.matchers.Matchers.staticMethod; -import static com.google.errorprone.matchers.Matchers.toType; import com.facebook.infer.annotation.Initializer; import com.google.auto.service.AutoService; @@ -36,7 +28,6 @@ import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; -import com.google.errorprone.matchers.Matcher; import com.google.errorprone.util.ASTHelpers; import com.google.errorprone.util.FindIdentifiers; import com.sun.source.tree.AnnotationTree; @@ -80,8 +71,8 @@ import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.OptionalInt; import java.util.Set; -import java.util.regex.Pattern; import java.util.stream.Collectors; import java.util.stream.Stream; import javax.annotation.Nullable; @@ -371,9 +362,9 @@ private API getXPAPI( for (PiranhaMethodRecord methodRecord : methodRecordsForName) { // when argumentIndex is specified, if mit's argument at argIndex doesn't match xpFlagName, // skip to next method property map - Optional optionalArgumentIdx = methodRecord.getArgumentIdx(); + OptionalInt optionalArgumentIdx = methodRecord.getArgumentIdx(); if (optionalArgumentIdx.isPresent()) { - int argumentIndex = optionalArgumentIdx.get().intValue(); + int argumentIndex = optionalArgumentIdx.getAsInt(); if (argumentIndex < mit.getArguments().size()) { ExpressionTree argTree = mit.getArguments().get(argumentIndex); Symbol argSym = ASTHelpers.getSymbol(argTree); @@ -707,75 +698,27 @@ 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, - allOf( - instanceMethod().anyClass().withNameMatching(MOCKITO_THEN), - receiverOfInvocation( - methodInvocation( - staticMethod().onClass(MOCKITO_QN).named(MOCKITO_WHEN), - ALL, - (argument, vs) -> { - Value value = evalExpr(argument, vs); - return value == Value.TRUE || value == Value.FALSE; - })))); - private SuggestedFix.Builder handleSpecificAPIPatterns(VisitorState state) { - ExpressionStatementTree stmt = + MethodInvocationTree enclosingMit = + ASTHelpers.findEnclosingNode(state.getPath(), MethodInvocationTree.class); + + ExpressionStatementTree enclosingEst = ASTHelpers.findEnclosingNode(state.getPath(), ExpressionStatementTree.class); - 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); + if (enclosingMit != null + && enclosingEst != null + && config + .getUnnecessaryTestMethodRecords() + .stream() + .map( + x -> + x.getReceiverType() + .map(r -> staticMethod().onClass(r)) + .orElseGet(() -> staticMethod().anyClass()) + .named(x.getMethodName())) + .anyMatch(matcher -> matcher.matches(enclosingMit, state))) { + endPos = state.getEndPosition(enclosingMit); + return SuggestedFix.builder().delete(enclosingEst); } return SuggestedFix.builder(); } diff --git a/java/piranha/src/main/java/com/uber/piranha/config/Config.java b/java/piranha/src/main/java/com/uber/piranha/config/Config.java index 313b4163b..d98e123f5 100644 --- a/java/piranha/src/main/java/com/uber/piranha/config/Config.java +++ b/java/piranha/src/main/java/com/uber/piranha/config/Config.java @@ -42,6 +42,7 @@ public final class Config { private static final String LINK_URL_KEY = "linkURL"; private static final String ANNOTATIONS_KEY = "annotations"; private static final String METHODS_KEY = "methodProperties"; + private static final String UNNECESSARY_TEST_METHOD_KEY = "unnecessaryTestMethodProperties"; private static final String CLEANUP_OPTS_KEY = "cleanupOptions"; private static final String ENUMS_KEY = "enumProperties"; @@ -72,6 +73,14 @@ public final class Config { */ private final ImmutableMultimap configMethodProperties; + /** + * TODO: Update this comment configMethodsMap is a map where key is method name and value is a + * list where each item in the list is a map that corresponds to each method property from + * properties.json. In most cases, the list would have only one element. But if someone reuses the + * same method name with different returnType/receiverType/argumentIndex, the list would have each + * method property map as one element. + */ + private ImmutableMultimap unnecessaryTestMethodProperties; /** * configEnumProperties is a map where key is enum name and value is a list where each item in the * list is a map that corresponds to each enum property from properties.json. In most cases, the @@ -100,11 +109,13 @@ public final class Config { // in particular Config.fromJSONFile([properties.json]) private Config( ImmutableMultimap configMethodProperties, + ImmutableMultimap unnecessaryTestMethodProperties, ImmutableMultimap configEnumProperties, TestAnnotationResolver testAnnotationResolver, ImmutableMap cleanupOptions, String linkURL) { this.configMethodProperties = configMethodProperties; + this.unnecessaryTestMethodProperties = unnecessaryTestMethodProperties; this.configEnumProperties = configEnumProperties; this.testAnnotationResolver = testAnnotationResolver; this.cleanupOptions = cleanupOptions; @@ -123,6 +134,15 @@ public ImmutableCollection getMethodRecordsForName(String m ? configMethodProperties.get(methodName) : ImmutableSet.of(); } + /** + * TODO: Update comment Return all configuration method records matching a given method name. + * + * @return A collection of {@link PiranhaMethodRecord} objects, representing each method + * definition in the piranha json configuration file matching {@code methodName}. + */ + public ImmutableCollection getUnnecessaryTestMethodRecords() { + return unnecessaryTestMethodProperties.values(); + } /** * Returns whether any configuration enum records exist. Useful for skipping logic if enum @@ -266,7 +286,9 @@ public static Config fromJSONFile(String configFile, boolean isArgumentIndexOpti } String linkURL = DEFAULT_PIRANHA_URL; - ImmutableMultimap.Builder methodsBuilder = + ImmutableMultimap.Builder unnecessaryTestMethodsBuilder = + ImmutableMultimap.builder(); + ImmutableMultimap.Builder flagMethodsBuilder = ImmutableMultimap.builder(); ImmutableMultimap.Builder enumsBuilder = ImmutableMultimap.builder(); @@ -300,7 +322,15 @@ public static Config fromJSONFile(String configFile, boolean isArgumentIndexOpti PiranhaMethodRecord methodRecord = PiranhaMethodRecord.parseFromJSONPropertyEntryMap( methodProperty, isArgumentIndexOptional); - methodsBuilder.put(methodRecord.getMethodName(), methodRecord); + flagMethodsBuilder.put(methodRecord.getMethodName(), methodRecord); + } + if (propertiesJson.get(UNNECESSARY_TEST_METHOD_KEY) != null) { + for (Map methodProperty : + (List>) propertiesJson.get(UNNECESSARY_TEST_METHOD_KEY)) { + MethodRecord methodRecord = + MethodRecord.parseFromJSONPropertyEntryMap(methodProperty, isArgumentIndexOptional); + unnecessaryTestMethodsBuilder.put(methodRecord.getMethodName(), methodRecord); + } } } else { throw new PiranhaConfigurationException("methodProperties not found, required."); @@ -326,7 +356,8 @@ public static Config fromJSONFile(String configFile, boolean isArgumentIndexOpti }); } return new Config( - methodsBuilder.build(), + flagMethodsBuilder.build(), + unnecessaryTestMethodsBuilder.build(), enumsBuilder.build(), annotationResolverBuilder.build(), cleanupOptionsBuilder.build(), @@ -365,6 +396,7 @@ public static Config fromJSONFile(String configFile, boolean isArgumentIndexOpti */ public static Config emptyConfig() { return new Config( + ImmutableMultimap.of(), ImmutableMultimap.of(), ImmutableMultimap.of(), TestAnnotationResolver.builder().build(), diff --git a/java/piranha/src/main/java/com/uber/piranha/config/MethodRecord.java b/java/piranha/src/main/java/com/uber/piranha/config/MethodRecord.java new file mode 100644 index 000000000..4ed1f8c67 --- /dev/null +++ b/java/piranha/src/main/java/com/uber/piranha/config/MethodRecord.java @@ -0,0 +1,115 @@ +/** + * Copyright (c) 2021 Uber Technologies, Inc. + * + *

Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file + * except in compliance with the License. You may obtain a copy of the License at + * + *

http://www.apache.org/licenses/LICENSE-2.0 + * + *

Unless required by applicable law or agreed to in writing, software distributed under the + * License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.uber.piranha.config; + +import static com.uber.piranha.config.PiranhaRecord.getArgumentIndexFromMap; +import static com.uber.piranha.config.PiranhaRecord.getValueStringFromMap; + +import java.util.Map; +import java.util.Optional; +import java.util.OptionalInt; +import javax.annotation.Nullable; + +/** A class representing a method configuration record from properties.json */ +public class MethodRecord { + + // Allowed fields for a method property in the config file. + // Entered under the top-level "methodProperties" in properties.json. + // By default, the flagType, methodName and argumentIndex fields are mandatory. + // The returnType and receiverType fields are optional. + protected static final String FLAG_TYPE_KEY = "flagType"; + protected static final String METHOD_NAME_KEY = "methodName"; + protected static final String ARGUMENT_INDEX_KEY = "argumentIndex"; + protected static final String RETURN_TYPE_STRING = "returnType"; + protected static final String RECEIVER_TYPE_STRING = "receiverType"; + + /** + * Holds the mapping of flagType string to API. Eg: "treated" -> API.IS_TREATED. Is initialized + * once and then accessed without updating. + */ + private final String methodName; + + @Nullable private final Integer argumentIdx; + @Nullable private final String receiverType; + @Nullable private final String returnType; + // @Nullable + // private final boolean isStatic; + + MethodRecord( + String methodName, + @Nullable Integer argumentIdx, + @Nullable String receiverType, + @Nullable String returnType) { + this.methodName = methodName; + this.argumentIdx = argumentIdx; + this.receiverType = receiverType; + this.returnType = returnType; + } + + public String getMethodName() { + return methodName; + } + + public OptionalInt getArgumentIdx() { + return Optional.ofNullable(argumentIdx).map(OptionalInt::of).orElseGet(OptionalInt::empty); + } + + public Optional getReceiverType() { + return Optional.ofNullable(receiverType); + } + + public Optional getReturnType() { + return Optional.ofNullable(returnType); + } + + /** + * Parse the entry for a single method from piranha.json that has been previously decoded into a + * map + * + * @param methodPropertyEntry The decoded json entry (as a Map of property names to values) + * @param isArgumentIndexOptional Whether argumentIdx should be treated as optional + * @return A PiranhaMethodRecord corresponding to the given map/json record. + * @throws PiranhaConfigurationException if there was any issue reading or parsing the + * configuration file. + */ + static MethodRecord parseFromJSONPropertyEntryMap( + Map methodPropertyEntry, boolean isArgumentIndexOptional) + throws PiranhaConfigurationException { + String methodName = getValueStringFromMap(methodPropertyEntry, METHOD_NAME_KEY); + String flagType = getValueStringFromMap(methodPropertyEntry, FLAG_TYPE_KEY); + Integer argumentIndexInteger = getArgumentIndexFromMap(methodPropertyEntry, ARGUMENT_INDEX_KEY); + if (methodName == null) { + throw new PiranhaConfigurationException( + "methodProperty is missing mandatory methodName field. Check:\n" + methodPropertyEntry); + } else if (!isArgumentIndexOptional && argumentIndexInteger == null) { + throw new PiranhaConfigurationException( + "methodProperty did not have argumentIndex. By default, Piranha requires an argument index for flag " + + "APIs, to which the flag name/symbol will be passed. This is to avoid over-deletion of all " + + "occurrences of a flag API method. If you are sure you want to delete all instances of the " + + "method below, consider using Piranha:ArgumentIndexOptional=true to override this behavior. " + + "Check:\n" + + methodPropertyEntry); + } else if (argumentIndexInteger != null && argumentIndexInteger < 0) { + throw new PiranhaConfigurationException( + "Invalid argumentIndex field. Arguments are zero indexed. Check:\n" + + methodPropertyEntry); + } + + return new MethodRecord( + methodName, + argumentIndexInteger, + getValueStringFromMap(methodPropertyEntry, RECEIVER_TYPE_STRING), + getValueStringFromMap(methodPropertyEntry, RETURN_TYPE_STRING)); + } +} diff --git a/java/piranha/src/main/java/com/uber/piranha/config/PiranhaMethodRecord.java b/java/piranha/src/main/java/com/uber/piranha/config/PiranhaMethodRecord.java index adae61828..3d38719de 100644 --- a/java/piranha/src/main/java/com/uber/piranha/config/PiranhaMethodRecord.java +++ b/java/piranha/src/main/java/com/uber/piranha/config/PiranhaMethodRecord.java @@ -19,20 +19,10 @@ import com.google.common.collect.ImmutableMap; import com.uber.piranha.XPFlagCleaner; import java.util.Map; -import java.util.Optional; +import javax.annotation.Nullable; /** A class representing a method configuration record from properties.json */ -public final class PiranhaMethodRecord { - - // Allowed fields for a method property in the config file. - // Entered under the top-level "methodProperties" in properties.json. - // By default, the flagType, methodName and argumentIndex fields are mandatory. - // The returnType and receiverType fields are optional. - private static final String FLAG_TYPE_KEY = "flagType"; - private static final String METHOD_NAME_KEY = "methodName"; - private static final String ARGUMENT_INDEX_KEY = "argumentIndex"; - private static final String RETURN_TYPE_STRING = "returnType"; - private static final String RECEIVER_TYPE_STRING = "receiverType"; +public final class PiranhaMethodRecord extends MethodRecord { /** * Holds the mapping of flagType string to API. Eg: "treated" -> API.IS_TREATED. Is initialized @@ -41,45 +31,22 @@ public final class PiranhaMethodRecord { private static final ImmutableMap flagTypeToAPIMap = initializeFlagTypeToAPIMap(); - private final String methodName; private final XPFlagCleaner.API apiType; - private final Optional argumentIdx; - private final Optional receiverType; - private final Optional returnType; PiranhaMethodRecord( String methodName, String flagTypeString, - Optional argumentIdx, - Optional receiverType, - Optional returnType) { - this.methodName = methodName; + @Nullable Integer argumentIdx, + @Nullable String receiverType, + @Nullable String returnType) { + super(methodName, argumentIdx, receiverType, returnType); this.apiType = flagTypeToAPIMap.getOrDefault(flagTypeString, XPFlagCleaner.API.UNKNOWN); - this.argumentIdx = argumentIdx; - this.receiverType = receiverType; - this.returnType = returnType; - } - - public String getMethodName() { - return methodName; } public XPFlagCleaner.API getApiType() { return apiType; } - public Optional getArgumentIdx() { - return argumentIdx; - } - - public Optional getReceiverType() { - return receiverType; - } - - public Optional getReturnType() { - return returnType; - } - private static ImmutableMap initializeFlagTypeToAPIMap() { ImmutableMap.Builder builder = new ImmutableMap.Builder<>(); builder.put("treated", XPFlagCleaner.API.IS_TREATED); @@ -121,7 +88,7 @@ static PiranhaMethodRecord parseFromJSONPropertyEntryMap( + "method below, consider using Piranha:ArgumentIndexOptional=true to override this behavior. " + "Check:\n" + methodPropertyEntry); - } else if (argumentIndexInteger != null && argumentIndexInteger.intValue() < 0) { + } else if (argumentIndexInteger != null && argumentIndexInteger < 0) { throw new PiranhaConfigurationException( "Invalid argumentIndex field. Arguments are zero indexed. Check:\n" + methodPropertyEntry); @@ -130,8 +97,8 @@ static PiranhaMethodRecord parseFromJSONPropertyEntryMap( return new PiranhaMethodRecord( methodName, flagType, - Optional.ofNullable(argumentIndexInteger), - Optional.ofNullable(getValueStringFromMap(methodPropertyEntry, RECEIVER_TYPE_STRING)), - Optional.ofNullable(getValueStringFromMap(methodPropertyEntry, RETURN_TYPE_STRING))); + argumentIndexInteger, + getValueStringFromMap(methodPropertyEntry, RECEIVER_TYPE_STRING), + getValueStringFromMap(methodPropertyEntry, RETURN_TYPE_STRING)); } } 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 c217948b2..5441fdd6b 100644 --- a/java/piranha/src/test/java/com/uber/piranha/CorePiranhaTest.java +++ b/java/piranha/src/test/java/com/uber/piranha/CorePiranhaTest.java @@ -596,7 +596,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();", + "", " someWhenWrapper(when(experimentation.isToggleDisabled(\"OTHER_FLAG\"))).thenCallRealMethod();", " when(experimentation.isToggleEnabled(\"OTHER_FLAG\")).thenReturn(false);", " when(experimentation.isToggleEnabled(\"OTHER_FLAG\")).thenThrow(new RuntimeException());", From 4844f6bcddf631a0bcaaa6e40eb374776da7c9df Mon Sep 17 00:00:00 2001 From: Ameya Ketkar Date: Thu, 9 Dec 2021 10:11:46 +0530 Subject: [PATCH 4/7] Configuration for handling static and instance methods --- java/piranha/config/properties.json | 12 ++++++---- .../java/com/uber/piranha/XPFlagCleaner.java | 24 ++++++++++++++----- .../java/com/uber/piranha/config/Config.java | 23 ++++++++++-------- .../com/uber/piranha/config/MethodRecord.java | 17 +++++++++---- .../piranha/config/PiranhaMethodRecord.java | 9 ++++--- .../uber/piranha/config/PiranhaRecord.java | 17 +++++++++++++ 6 files changed, 74 insertions(+), 28 deletions(-) diff --git a/java/piranha/config/properties.json b/java/piranha/config/properties.json index a21125191..376879c9b 100644 --- a/java/piranha/config/properties.json +++ b/java/piranha/config/properties.json @@ -42,22 +42,26 @@ { "methodName": "when", "argumentIndex": 0, - "recieverType":"org.mockito.Mockito" + "recieverType":"org.mockito.Mockito", + "isStatic": "true" }, { "methodName": "expect", "argumentIndex": 0, - "recieverType":"org.easymock.EasyMock" + "recieverType":"org.easymock.EasyMock", + "isStatic": "true" }, { "methodName": "assertTrue", "argumentIndex": 0, - "recieverType":"org.junit.Assert" + "recieverType":"org.junit.Assert", + "isStatic": "true" }, { "methodName": "assertFalse", "argumentIndex": 0, - "recieverType":"org.junit.Assert" + "recieverType":"org.junit.Assert", + "isStatic": "true" } ], "linkURL": "", 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 9be281218..8066570a4 100644 --- a/java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java +++ b/java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java @@ -1,4 +1,4 @@ -/** +/* * Copyright (c) 2019 Uber Technologies, Inc. * *

Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file @@ -14,6 +14,7 @@ package com.uber.piranha; import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; +import static com.google.errorprone.matchers.Matchers.instanceMethod; import static com.google.errorprone.matchers.Matchers.staticMethod; import com.facebook.infer.annotation.Initializer; @@ -698,6 +699,14 @@ private Description updateCode( return Description.NO_MATCH; } + /** + * This method picks up the unnecessary test method as configured in properties.json and converts + * them into a Error-prone AST Matcher and then deletes the containing AST statement. + * + * @param state + * @return Suggestion Fix for deleting the statement containing a unnecessary test method + * invocation + */ private SuggestedFix.Builder handleSpecificAPIPatterns(VisitorState state) { MethodInvocationTree enclosingMit = ASTHelpers.findEnclosingNode(state.getPath(), MethodInvocationTree.class); @@ -711,11 +720,14 @@ private SuggestedFix.Builder handleSpecificAPIPatterns(VisitorState state) { .getUnnecessaryTestMethodRecords() .stream() .map( - x -> - x.getReceiverType() - .map(r -> staticMethod().onClass(r)) - .orElseGet(() -> staticMethod().anyClass()) - .named(x.getMethodName())) + mthd -> + mthd.isStatic() + ? mthd.getReceiverType() + .map(r -> staticMethod().onClass(r)) + .orElseGet(() -> staticMethod().anyClass()) + : mthd.getReceiverType() + .map(r -> instanceMethod().onExactClass(r)) + .orElseGet(() -> instanceMethod().anyClass())) .anyMatch(matcher -> matcher.matches(enclosingMit, state))) { endPos = state.getEndPosition(enclosingMit); return SuggestedFix.builder().delete(enclosingEst); diff --git a/java/piranha/src/main/java/com/uber/piranha/config/Config.java b/java/piranha/src/main/java/com/uber/piranha/config/Config.java index d98e123f5..b0071e343 100644 --- a/java/piranha/src/main/java/com/uber/piranha/config/Config.java +++ b/java/piranha/src/main/java/com/uber/piranha/config/Config.java @@ -65,22 +65,23 @@ public final class Config { private static final boolean DEFAULT_TESTS_CLEAN_BY_SETTERS_IGNORE_OTHERS = false; /** - * configMethodsMap is a map where key is method name and value is a list where each item in the - * list is a map that corresponds to each method property from properties.json. In most cases, the - * list would have only one element. But if someone reuses the same method name with different + * configMethodProperties is a map where key is method name and value is a list where each item in + * the list is a map that corresponds to each method property from properties.json. In most cases, + * the list would have only one element. But if someone reuses the same method name with different * returnType/receiverType/argumentIndex, the list would have each method property map as one * element. */ private final ImmutableMultimap configMethodProperties; /** - * TODO: Update this comment configMethodsMap is a map where key is method name and value is a - * list where each item in the list is a map that corresponds to each method property from + * unnecessaryTestMethodProperties is a map where key is method name and value is a list where + * each item in the list is a map that corresponds to each test method property from * properties.json. In most cases, the list would have only one element. But if someone reuses the * same method name with different returnType/receiverType/argumentIndex, the list would have each - * method property map as one element. + * method property map as one element. Often after the refactoring performed by Piranha, certain + * method invocations in test cases become unnecessary, need to be deleted. */ - private ImmutableMultimap unnecessaryTestMethodProperties; + private final ImmutableMultimap unnecessaryTestMethodProperties; /** * configEnumProperties is a map where key is enum name and value is a list where each item in the * list is a map that corresponds to each enum property from properties.json. In most cases, the @@ -135,10 +136,12 @@ public ImmutableCollection getMethodRecordsForName(String m : ImmutableSet.of(); } /** - * TODO: Update comment Return all configuration method records matching a given method name. + * Return all the configured unnecessary test methods (corresponding to the + * unnecessaryTestMethodProperties field) * - * @return A collection of {@link PiranhaMethodRecord} objects, representing each method - * definition in the piranha json configuration file matching {@code methodName}. + * @return A collection of {@link MethodRecord} objects, representing each method definition in + * the piranha json configuration file (unnecessaryTestMethodProperties field) matching {@code + * methodName}. */ public ImmutableCollection getUnnecessaryTestMethodRecords() { return unnecessaryTestMethodProperties.values(); diff --git a/java/piranha/src/main/java/com/uber/piranha/config/MethodRecord.java b/java/piranha/src/main/java/com/uber/piranha/config/MethodRecord.java index 4ed1f8c67..dc0edf82d 100644 --- a/java/piranha/src/main/java/com/uber/piranha/config/MethodRecord.java +++ b/java/piranha/src/main/java/com/uber/piranha/config/MethodRecord.java @@ -14,6 +14,7 @@ package com.uber.piranha.config; import static com.uber.piranha.config.PiranhaRecord.getArgumentIndexFromMap; +import static com.uber.piranha.config.PiranhaRecord.getValueBooleanFromMap; import static com.uber.piranha.config.PiranhaRecord.getValueStringFromMap; import java.util.Map; @@ -33,6 +34,7 @@ public class MethodRecord { protected static final String ARGUMENT_INDEX_KEY = "argumentIndex"; protected static final String RETURN_TYPE_STRING = "returnType"; protected static final String RECEIVER_TYPE_STRING = "receiverType"; + protected static final String METHOD_IS_STATIC = "isStatic"; /** * Holds the mapping of flagType string to API. Eg: "treated" -> API.IS_TREATED. Is initialized @@ -43,18 +45,19 @@ public class MethodRecord { @Nullable private final Integer argumentIdx; @Nullable private final String receiverType; @Nullable private final String returnType; - // @Nullable - // private final boolean isStatic; + private final Boolean isStatic; MethodRecord( String methodName, @Nullable Integer argumentIdx, @Nullable String receiverType, - @Nullable String returnType) { + @Nullable String returnType, + @Nullable Boolean isStatic) { this.methodName = methodName; this.argumentIdx = argumentIdx; this.receiverType = receiverType; this.returnType = returnType; + this.isStatic = isStatic != null && isStatic; } public String getMethodName() { @@ -87,7 +90,6 @@ static MethodRecord parseFromJSONPropertyEntryMap( Map methodPropertyEntry, boolean isArgumentIndexOptional) throws PiranhaConfigurationException { String methodName = getValueStringFromMap(methodPropertyEntry, METHOD_NAME_KEY); - String flagType = getValueStringFromMap(methodPropertyEntry, FLAG_TYPE_KEY); Integer argumentIndexInteger = getArgumentIndexFromMap(methodPropertyEntry, ARGUMENT_INDEX_KEY); if (methodName == null) { throw new PiranhaConfigurationException( @@ -110,6 +112,11 @@ static MethodRecord parseFromJSONPropertyEntryMap( methodName, argumentIndexInteger, getValueStringFromMap(methodPropertyEntry, RECEIVER_TYPE_STRING), - getValueStringFromMap(methodPropertyEntry, RETURN_TYPE_STRING)); + getValueStringFromMap(methodPropertyEntry, RETURN_TYPE_STRING), + getValueBooleanFromMap(methodPropertyEntry, METHOD_IS_STATIC)); + } + + public boolean isStatic() { + return isStatic; } } diff --git a/java/piranha/src/main/java/com/uber/piranha/config/PiranhaMethodRecord.java b/java/piranha/src/main/java/com/uber/piranha/config/PiranhaMethodRecord.java index 3d38719de..ae0f3e4f2 100644 --- a/java/piranha/src/main/java/com/uber/piranha/config/PiranhaMethodRecord.java +++ b/java/piranha/src/main/java/com/uber/piranha/config/PiranhaMethodRecord.java @@ -14,6 +14,7 @@ package com.uber.piranha.config; import static com.uber.piranha.config.PiranhaRecord.getArgumentIndexFromMap; +import static com.uber.piranha.config.PiranhaRecord.getValueBooleanFromMap; import static com.uber.piranha.config.PiranhaRecord.getValueStringFromMap; import com.google.common.collect.ImmutableMap; @@ -38,8 +39,9 @@ public final class PiranhaMethodRecord extends MethodRecord { String flagTypeString, @Nullable Integer argumentIdx, @Nullable String receiverType, - @Nullable String returnType) { - super(methodName, argumentIdx, receiverType, returnType); + @Nullable String returnType, + @Nullable Boolean isStatic) { + super(methodName, argumentIdx, receiverType, returnType, isStatic); this.apiType = flagTypeToAPIMap.getOrDefault(flagTypeString, XPFlagCleaner.API.UNKNOWN); } @@ -99,6 +101,7 @@ static PiranhaMethodRecord parseFromJSONPropertyEntryMap( flagType, argumentIndexInteger, getValueStringFromMap(methodPropertyEntry, RECEIVER_TYPE_STRING), - getValueStringFromMap(methodPropertyEntry, RETURN_TYPE_STRING)); + getValueStringFromMap(methodPropertyEntry, RETURN_TYPE_STRING), + getValueBooleanFromMap(methodPropertyEntry, METHOD_IS_STATIC)); } } diff --git a/java/piranha/src/main/java/com/uber/piranha/config/PiranhaRecord.java b/java/piranha/src/main/java/com/uber/piranha/config/PiranhaRecord.java index 55b5d90a0..2d5a00af6 100644 --- a/java/piranha/src/main/java/com/uber/piranha/config/PiranhaRecord.java +++ b/java/piranha/src/main/java/com/uber/piranha/config/PiranhaRecord.java @@ -53,4 +53,21 @@ static Integer getArgumentIndexFromMap(Map map, String argumentI } return null; } + + /** + * Utility method. Checks whether the value associated to a given map and given key is a non-empty + * string. + * + * @param map - map corresponding to a method property + * @param key - key to check the corresponding value + * @return the corresponding boolean value + */ + @Nullable + static Boolean getValueBooleanFromMap(Map map, String key) { + Object value = map.get(key); + if (value instanceof String && !value.equals("")) { + return Boolean.parseBoolean(String.valueOf(value)); + } + return null; + } } From 1bd399497881e45ad9329c99f856cff948bef02e Mon Sep 17 00:00:00 2001 From: Ameya Ketkar Date: Fri, 31 Dec 2021 09:29:52 -0800 Subject: [PATCH 5/7] Addressing the comments made in the PR --- java/README.md | 8 + java/piranha/config/properties.json | 8 +- .../java/com/uber/piranha/XPFlagCleaner.java | 12 +- .../uber/piranha/config/PiranhaRecord.java | 5 +- .../com/uber/piranha/CorePiranhaTest.java | 197 ++++++++++++++++++ 5 files changed, 219 insertions(+), 11 deletions(-) diff --git a/java/README.md b/java/README.md index 5931fd9aa..d9b85c76a 100644 --- a/java/README.md +++ b/java/README.md @@ -135,6 +135,14 @@ Additionally, usages of `STALE_FLAG` will be removed as if the enum itself had b Finally, the setting `linkURL` in the properties file is to provide a URL describing the Piranha tooling and any custom configurations associated with the codebase. +Another top-level optional field is `unnecessaryTestMethodProperties`. Within that, there is an array of JSON objects, +having the fields `methodName`, `argumentIndex`, `receiverType`, `returnType` and `isStatic`. + +In practice, often the refactored code produced by `piranha` results in unnecessary method invocations. For instance, +Piranha changes the statement `when(experimentation.isToggleDisabled("STALE_FLAG")).thenReturn(false);` to `when(true).thenReturn(false);`, +where the invocation `when(true)` is unnecessary, and could be deleted. +This field (`unnecessaryTestMethodProperties`) is used to define such potentially unnecessary method invocations. +Piranha will delete a statement if it invokes one of these pre-defined methods with a stale flag as an argument. ## Example refactoring diff --git a/java/piranha/config/properties.json b/java/piranha/config/properties.json index 376879c9b..99131a45d 100644 --- a/java/piranha/config/properties.json +++ b/java/piranha/config/properties.json @@ -42,25 +42,25 @@ { "methodName": "when", "argumentIndex": 0, - "recieverType":"org.mockito.Mockito", + "receiverType":"org.mockito.Mockito", "isStatic": "true" }, { "methodName": "expect", "argumentIndex": 0, - "recieverType":"org.easymock.EasyMock", + "receiverType":"org.easymock.EasyMock", "isStatic": "true" }, { "methodName": "assertTrue", "argumentIndex": 0, - "recieverType":"org.junit.Assert", + "receiverType":"org.junit.Assert", "isStatic": "true" }, { "methodName": "assertFalse", "argumentIndex": 0, - "recieverType":"org.junit.Assert", + "receiverType":"org.junit.Assert", "isStatic": "true" } ], 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 8066570a4..77c069a24 100644 --- a/java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java +++ b/java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java @@ -703,7 +703,7 @@ private Description updateCode( * This method picks up the unnecessary test method as configured in properties.json and converts * them into a Error-prone AST Matcher and then deletes the containing AST statement. * - * @param state + * @param state The visitor state of the statement to be handled * @return Suggestion Fix for deleting the statement containing a unnecessary test method * invocation */ @@ -720,12 +720,14 @@ private SuggestedFix.Builder handleSpecificAPIPatterns(VisitorState state) { .getUnnecessaryTestMethodRecords() .stream() .map( - mthd -> - mthd.isStatic() - ? mthd.getReceiverType() + method -> + method.isStatic() + ? method + .getReceiverType() .map(r -> staticMethod().onClass(r)) .orElseGet(() -> staticMethod().anyClass()) - : mthd.getReceiverType() + : method + .getReceiverType() .map(r -> instanceMethod().onExactClass(r)) .orElseGet(() -> instanceMethod().anyClass())) .anyMatch(matcher -> matcher.matches(enclosingMit, state))) { diff --git a/java/piranha/src/main/java/com/uber/piranha/config/PiranhaRecord.java b/java/piranha/src/main/java/com/uber/piranha/config/PiranhaRecord.java index 2d5a00af6..baf882e2b 100644 --- a/java/piranha/src/main/java/com/uber/piranha/config/PiranhaRecord.java +++ b/java/piranha/src/main/java/com/uber/piranha/config/PiranhaRecord.java @@ -29,8 +29,9 @@ public class PiranhaRecord { @Nullable static String getValueStringFromMap(Map map, String key) { Object value = map.get(key); - if (value instanceof String && !value.equals("")) { - return String.valueOf(value); + if (value instanceof String) { + String valueStr = String.valueOf(value); + if (!valueStr.isEmpty()) return valueStr; } return null; } 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 5441fdd6b..9271f2b1c 100644 --- a/java/piranha/src/test/java/com/uber/piranha/CorePiranhaTest.java +++ b/java/piranha/src/test/java/com/uber/piranha/CorePiranhaTest.java @@ -500,6 +500,203 @@ public void testIgnoresPrefixMatchFlag() throws IOException { .doTest(); } + @Test + public void testRemoveSpecificAPIpatternsMockito() throws IOException { + + ErrorProneFlags.Builder b = ErrorProneFlags.builder(); + b.putFlag("Piranha:FlagName", "STALE_FLAG"); + b.putFlag("Piranha:IsTreated", "false"); + b.putFlag("Piranha:Config", "config/properties.json"); + + BugCheckerRefactoringTestHelper bcr = + BugCheckerRefactoringTestHelper.newInstance(new XPFlagCleaner(b.build()), getClass()); + + bcr = bcr.setArgs("-d", temporaryFolder.getRoot().getAbsolutePath()); + bcr = PiranhaTestingHelpers.addHelperClasses(bcr); + bcr.addInputLines( + "XPFlagCleanerSinglePositiveCase.java", + "package com.uber.piranha;", + "import static org.mockito.Mockito.when;", + "import org.mockito.invocation.InvocationOnMock;", + "import org.mockito.stubbing.Answer;", + "import org.mockito.stubbing.OngoingStubbing;", + "class MockitoTest {", + " private XPTest experimentation;", + " public void evaluate() {", + " Answer ans = new Answer() {\n" + + " public Integer answer(InvocationOnMock invocation) throws Throwable {\n" + + " return (Integer) invocation.getArguments()[0];\n" + + " }};", + " when(experimentation.isToggleDisabled(\"STALE_FLAG\")).thenReturn(false);", + " when(experimentation.isToggleEnabled(\"STALE_FLAG\")).thenThrow(new RuntimeException());", + " when(experimentation.isToggleDisabled(\"STALE_FLAG\")).thenCallRealMethod();", + " when(experimentation.isToggleEnabled(\"STALE_FLAG\")).then(ans);", + " boolean b1 = someWrapper(when(experimentation.isToggleDisabled(\"STALE_FLAG\")).thenCallRealMethod());", + " boolean b2 = someWrapper(when(experimentation.isToggleDisabled(\"OTHER_FLAG\")).thenCallRealMethod());", + " someWhenWrapper(when(experimentation.isToggleDisabled(\"STALE_FLAG\"))).thenCallRealMethod();", + " someWhenWrapper(when(experimentation.isToggleDisabled(\"OTHER_FLAG\"))).thenCallRealMethod();", + " when(experimentation.isToggleEnabled(\"OTHER_FLAG\")).thenReturn(false);", + " when(experimentation.isToggleEnabled(\"OTHER_FLAG\")).thenThrow(new RuntimeException());", + " when(experimentation.isToggleDisabled(\"OTHER_FLAG\")).thenCallRealMethod();", + " when(experimentation.isToggleEnabled(\"OTHER_FLAG\")).then(ans);", + " when(foobar()).thenReturn(false);", + " when(foobar()).thenAnswer(ans);", + " }", + " public boolean foobar() { return true;}", + " public boolean someWrapper(Object obj) { return true;}", + " public OngoingStubbing someWhenWrapper(OngoingStubbing x) { return x;}", + "}") + .addOutputLines( + "XPFlagCleanerSinglePositiveCase.java", + "package com.uber.piranha;", + "import static org.mockito.Mockito.when;", + "import org.mockito.invocation.InvocationOnMock;", + "import org.mockito.stubbing.Answer;", + "import org.mockito.stubbing.OngoingStubbing;", + "class MockitoTest {", + " private XPTest experimentation;", + " public void evaluate() {", + " Answer ans = new Answer() {\n" + + " public Integer answer(InvocationOnMock invocation) throws Throwable {\n" + + " return (Integer) invocation.getArguments()[0];\n" + + " }};", + "", + "", + "", + "", + "", + " boolean b1 = someWrapper(when(true).thenCallRealMethod());", + " boolean b2 = someWrapper(when(experimentation.isToggleDisabled(\"OTHER_FLAG\")).thenCallRealMethod());", + "", + " someWhenWrapper(when(experimentation.isToggleDisabled(\"OTHER_FLAG\"))).thenCallRealMethod();", + " when(experimentation.isToggleEnabled(\"OTHER_FLAG\")).thenReturn(false);", + " when(experimentation.isToggleEnabled(\"OTHER_FLAG\")).thenThrow(new RuntimeException());", + " when(experimentation.isToggleDisabled(\"OTHER_FLAG\")).thenCallRealMethod();", + " when(experimentation.isToggleEnabled(\"OTHER_FLAG\")).then(ans);", + " when(foobar()).thenReturn(false);", + " when(foobar()).thenAnswer(ans);", + " }", + " public boolean foobar() { return true;}", + " public boolean someWrapper(Object obj) { return true;}", + " public OngoingStubbing someWhenWrapper(OngoingStubbing x) { return x;}", + "}") + .doTest(); + } + + @Test + public void testRemoveSpecificAPIpatternsEasyMock() throws IOException { + + ErrorProneFlags.Builder b = ErrorProneFlags.builder(); + b.putFlag("Piranha:FlagName", "STALE_FLAG"); + b.putFlag("Piranha:IsTreated", "false"); + b.putFlag("Piranha:Config", "config/properties.json"); + + BugCheckerRefactoringTestHelper bcr = + BugCheckerRefactoringTestHelper.newInstance(new XPFlagCleaner(b.build()), getClass()); + + bcr = bcr.setArgs("-d", temporaryFolder.getRoot().getAbsolutePath()); + bcr = PiranhaTestingHelpers.addHelperClasses(bcr); + bcr.addInputLines( + "XPFlagCleanerSinglePositiveCase.java", + "package com.uber.piranha;", + "import org.easymock.EasyMock;", + "class EasyMockTest {", + " private XPTest experimentation;", + " public void evaluate() {", + " 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();", + " }", + " public boolean foobar() { return true;}", + " public boolean someWrapper(Object obj) { return true;}", + "}") + .addOutputLines( + "XPFlagCleanerSinglePositiveCase.java", + "package com.uber.piranha;", + "import org.easymock.EasyMock;", + "class EasyMockTest {", + " private XPTest experimentation;", + " public void evaluate() {", + "", + "", + "", + "", + "", + "", + " 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();", + " }", + " public boolean foobar() { return true;}", + " public boolean someWrapper(Object obj) { return true;}", + "}") + .doTest(); + } + + @Test + public void testRemoveSpecificAPIpatternsJUnit() throws IOException { + + ErrorProneFlags.Builder b = ErrorProneFlags.builder(); + b.putFlag("Piranha:FlagName", "STALE_FLAG"); + b.putFlag("Piranha:IsTreated", "false"); + b.putFlag("Piranha:Config", "config/properties.json"); + + BugCheckerRefactoringTestHelper bcr = + BugCheckerRefactoringTestHelper.newInstance(new XPFlagCleaner(b.build()), getClass()); + + bcr = bcr.setArgs("-d", temporaryFolder.getRoot().getAbsolutePath()); + bcr = PiranhaTestingHelpers.addHelperClasses(bcr); + bcr.addInputLines( + "XPFlagCleanerSinglePositiveCase.java", + "package com.uber.piranha;", + "import static org.junit.Assert.assertFalse;", + "import static org.junit.Assert.assertTrue;", + "class JUnitTest {", + " private XPTest experimentation;", + " public void evaluate() {", + " 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;}", + "}") + .addOutputLines( + "XPFlagCleanerSinglePositiveCase.java", + "package com.uber.piranha;", + "import static org.junit.Assert.assertFalse;", + "import static org.junit.Assert.assertTrue;", + "class JUnitTest {", + " private XPTest experimentation;", + " public void evaluate() {", + "", + "", + " assertFalse(experimentation.isToggleEnabled(\"OTHER_FLAG\"));", + " assertTrue(experimentation.isToggleDisabled(\"OTHER_FLAG\"));", + " assertTrue(foobar());", + " }", + " public boolean foobar() { return true;}", + " public boolean someWrapper(Object obj) { return true;}", + "}") + .doTest(); + } + @Test public void testRemoveSpecificAPIpatterns() throws IOException { From 96b7b096e889724117e3d9a51b8ef2dd9f997042 Mon Sep 17 00:00:00 2001 From: Ameya Ketkar Date: Mon, 3 Jan 2022 10:04:13 -0800 Subject: [PATCH 6/7] Attempting to fix macos-latest PiranhaSwift issue --- .github/workflows/swift.yml | 4 ++-- java/README.md | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/swift.yml b/.github/workflows/swift.yml index 7a1bacb63..247c90f9b 100644 --- a/.github/workflows/swift.yml +++ b/.github/workflows/swift.yml @@ -9,10 +9,10 @@ on: jobs: build: - runs-on: macos-11 + runs-on: macos-latest strategy: matrix: - xcode: ['13.1'] + xcode: ['13.0'] name: "PiranhaSwift using ${{ matrix.xcode }} on ${{ matrix.os }}" steps: - name: Check out Piranha sources diff --git a/java/README.md b/java/README.md index d9b85c76a..9dbac2a27 100644 --- a/java/README.md +++ b/java/README.md @@ -138,7 +138,7 @@ Finally, the setting `linkURL` in the properties file is to provide a URL descri Another top-level optional field is `unnecessaryTestMethodProperties`. Within that, there is an array of JSON objects, having the fields `methodName`, `argumentIndex`, `receiverType`, `returnType` and `isStatic`. -In practice, often the refactored code produced by `piranha` results in unnecessary method invocations. For instance, +The refactored code may contain unnecessary method invocations. For instance, Piranha changes the statement `when(experimentation.isToggleDisabled("STALE_FLAG")).thenReturn(false);` to `when(true).thenReturn(false);`, where the invocation `when(true)` is unnecessary, and could be deleted. This field (`unnecessaryTestMethodProperties`) is used to define such potentially unnecessary method invocations. From a70ca2b4d14c27520bd3a2604c6f6e963c71a121 Mon Sep 17 00:00:00 2001 From: Ameya Ketkar Date: Mon, 3 Jan 2022 10:34:50 -0800 Subject: [PATCH 7/7] Removing inconsistency in the XCode versions --- .github/workflows/swift.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/swift.yml b/.github/workflows/swift.yml index 247c90f9b..293a55241 100644 --- a/.github/workflows/swift.yml +++ b/.github/workflows/swift.yml @@ -12,14 +12,14 @@ jobs: runs-on: macos-latest strategy: matrix: - xcode: ['13.0'] + xcode: ['13.1'] name: "PiranhaSwift using ${{ matrix.xcode }} on ${{ matrix.os }}" steps: - name: Check out Piranha sources uses: actions/checkout@v2 - name: Set Xcode ${{ matrix.xcode }} run: | - sudo xcode-select -s /Applications/Xcode_13.1.app + sudo xcode-select -s /Applications/Xcode_13.0.app xcodebuild -version swift --version swift package --version