[PiranhaJava] Delete statements using EasyMock and JUnit API

.github/workflows/swift.yml

@@ -9,7 +9,7 @@ on:
 
 jobs:
   build:
-    runs-on: macos-11
+    runs-on: macos-latest
     strategy:
         matrix:
           xcode: ['13.1']
@@ -19,7 +19,7 @@ jobs:
         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

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`.
+
+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.
+Piranha will delete a statement if it invokes one of these pre-defined methods with a stale flag as an argument.  
 
 ## Example refactoring
 

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"
     }

java/piranha/config/properties.json

@@ -38,6 +38,32 @@
       "argumentIndex": 0
     }
   ],
+  "unnecessaryTestMethodProperties" : [
+    {
+      "methodName": "when",
+      "argumentIndex": 0,
+      "receiverType":"org.mockito.Mockito",
+      "isStatic": "true"
+    },
+    {
+      "methodName": "expect",
+      "argumentIndex": 0,
+      "receiverType":"org.easymock.EasyMock",
+      "isStatic": "true"
+    },
+    {
+      "methodName": "assertTrue",
+      "argumentIndex": 0,
+      "receiverType":"org.junit.Assert",
+      "isStatic": "true"
+    },
+    {
+      "methodName": "assertFalse",
+      "argumentIndex": 0,
+      "receiverType":"org.junit.Assert",
+      "isStatic": "true"
+    }
+  ],
   "linkURL": "<provide_your_url>",
   "annotations": [
     "ToggleTesting",

java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java

@@ -1,4 +1,4 @@
-/**
+/*
  * Copyright (c) 2019 Uber Technologies, Inc.
  *
  * <p>Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file
@@ -14,13 +14,8 @@
 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;
@@ -34,7 +29,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;
@@ -78,8 +72,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;
@@ -369,9 +363,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<Integer> 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);
@@ -705,30 +699,40 @@ private Description updateCode(
     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<Tree> 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;
-                      }))));
-
+  /**
+   * 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 The visitor state of the statement to be handled
+   * @return Suggestion Fix for deleting the statement containing a unnecessary test method
+   *     invocation
+   */
   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)) {
-      endPos = state.getEndPosition(stmt);
-      return SuggestedFix.builder().delete(stmt);
+
+    if (enclosingMit != null
+        && enclosingEst != null
+        && config
+            .getUnnecessaryTestMethodRecords()
+            .stream()
+            .map(
+                method ->
+                    method.isStatic()
+                        ? method
+                            .getReceiverType()
+                            .map(r -> staticMethod().onClass(r))
+                            .orElseGet(() -> staticMethod().anyClass())
+                        : method
+                            .getReceiverType()
+                            .map(r -> instanceMethod().onExactClass(r))
+                            .orElseGet(() -> instanceMethod().anyClass()))
+            .anyMatch(matcher -> matcher.matches(enclosingMit, state))) {
+      endPos = state.getEndPosition(enclosingMit);
+      return SuggestedFix.builder().delete(enclosingEst);
     }
     return SuggestedFix.builder();
   }

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";
 
@@ -64,14 +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<String, PiranhaMethodRecord> configMethodProperties;
 
+  /**
+   * 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. Often after the refactoring performed by Piranha, certain
+   * method invocations in test cases become unnecessary, need to be deleted.
+   */
+  private final ImmutableMultimap<String, MethodRecord> 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 +110,13 @@ public final class Config {
   // in particular Config.fromJSONFile([properties.json])
   private Config(
       ImmutableMultimap<String, PiranhaMethodRecord> configMethodProperties,
+      ImmutableMultimap<String, MethodRecord> unnecessaryTestMethodProperties,
       ImmutableMultimap<String, PiranhaEnumRecord> configEnumProperties,
       TestAnnotationResolver testAnnotationResolver,
       ImmutableMap<String, Object> cleanupOptions,
       String linkURL) {
     this.configMethodProperties = configMethodProperties;
+    this.unnecessaryTestMethodProperties = unnecessaryTestMethodProperties;
     this.configEnumProperties = configEnumProperties;
     this.testAnnotationResolver = testAnnotationResolver;
     this.cleanupOptions = cleanupOptions;
@@ -123,6 +135,17 @@ public ImmutableCollection<PiranhaMethodRecord> getMethodRecordsForName(String m
         ? configMethodProperties.get(methodName)
         : ImmutableSet.of();
   }
+  /**
+   * Return all the configured unnecessary test methods (corresponding to the
+   * unnecessaryTestMethodProperties field)
+   *
+   * @return A collection of {@link MethodRecord} objects, representing each method definition in
+   *     the piranha json configuration file (unnecessaryTestMethodProperties field) matching {@code
+   *     methodName}.
+   */
+  public ImmutableCollection<MethodRecord> getUnnecessaryTestMethodRecords() {
+    return unnecessaryTestMethodProperties.values();
+  }
 
   /**
    * Returns whether any configuration enum records exist. Useful for skipping logic if enum
@@ -266,7 +289,9 @@ public static Config fromJSONFile(String configFile, boolean isArgumentIndexOpti
       }
 
       String linkURL = DEFAULT_PIRANHA_URL;
-      ImmutableMultimap.Builder<String, PiranhaMethodRecord> methodsBuilder =
+      ImmutableMultimap.Builder<String, MethodRecord> unnecessaryTestMethodsBuilder =
+          ImmutableMultimap.builder();
+      ImmutableMultimap.Builder<String, PiranhaMethodRecord> flagMethodsBuilder =
           ImmutableMultimap.builder();
       ImmutableMultimap.Builder<String, PiranhaEnumRecord> enumsBuilder =
           ImmutableMultimap.builder();
@@ -300,7 +325,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<String, Object> methodProperty :
+              (List<Map<String, Object>>) 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 +359,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 +399,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(),