Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PiranhaJava] Delete statements using EasyMock and JUnit API #156

Merged
merged 8 commits into from
Jan 3, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions java/piranha/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
Expand Down
26 changes: 26 additions & 0 deletions java/piranha/config/properties.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,32 @@
"argumentIndex": 0
}
],
"unnecessaryTestMethodProperties" : [
{
"methodName": "when",
"argumentIndex": 0,
"recieverType":"org.mockito.Mockito",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: receiverType

"isStatic": "true"
},
{
"methodName": "expect",
"argumentIndex": 0,
"recieverType":"org.easymock.EasyMock",
"isStatic": "true"
},
{
"methodName": "assertTrue",
"argumentIndex": 0,
"recieverType":"org.junit.Assert",
"isStatic": "true"
},
{
"methodName": "assertFalse",
"argumentIndex": 0,
"recieverType":"org.junit.Assert",
"isStatic": "true"
}
],
"linkURL": "<provide_your_url>",
"annotations": [
"ToggleTesting",
Expand Down
66 changes: 34 additions & 32 deletions java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My type change tool suggested this :)

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);
Expand Down Expand Up @@ -705,30 +699,38 @@ 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
* @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(
mthd ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expand mthd to method?

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);
}
return SuggestedFix.builder();
}
Expand Down
47 changes: 41 additions & 6 deletions java/piranha/src/main/java/com/uber/piranha/config/Config.java
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand All @@ -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
Expand Down Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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.");
Expand All @@ -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(),
Expand Down Expand Up @@ -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(),
Expand Down
122 changes: 122 additions & 0 deletions java/piranha/src/main/java/com/uber/piranha/config/MethodRecord.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
/**
* Copyright (c) 2021 Uber Technologies, Inc.
*
* <p>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
*
* <p>http://www.apache.org/licenses/LICENSE-2.0
*
* <p>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.getValueBooleanFromMap;
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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I extracted the super class MethodRecord from 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.
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";
protected static final String METHOD_IS_STATIC = "isStatic";

/**
* 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;
private final Boolean isStatic;

MethodRecord(
String methodName,
@Nullable Integer argumentIdx,
@Nullable String receiverType,
@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() {
return methodName;
}

public OptionalInt getArgumentIdx() {
return Optional.ofNullable(argumentIdx).map(OptionalInt::of).orElseGet(OptionalInt::empty);
}

public Optional<String> getReceiverType() {
return Optional.ofNullable(receiverType);
}

public Optional<String> 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<String, Object> methodPropertyEntry, boolean isArgumentIndexOptional)
throws PiranhaConfigurationException {
String methodName = getValueStringFromMap(methodPropertyEntry, METHOD_NAME_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),
getValueBooleanFromMap(methodPropertyEntry, METHOD_IS_STATIC));
}

public boolean isStatic() {
return isStatic;
}
}
Loading