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.

Sign up for GitHub

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] Remove configured test methods #151

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[PiranhaJava] Remove configured test methods #151

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
028fb16
[PiranhaJava] Add configuration for test method cleanup
tnterdan Aug 26, 2021
46ed05a
[PiranhaJava] Remove configured test methods
tnterdan Aug 27, 2021
2866b72
Fix typo in README The -> They
tnterdan Sep 28, 2021
c6bf298
Move shared parseFromJSONPropertyEntryMap code to utility methods
tnterdan Sep 28, 2021
d29883a
Add nullable annotation to parseArgumentIndexFromJSON
tnterdan Sep 28, 2021
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
15 changes: 14 additions & 1 deletion java/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,13 @@ The properties file has the following template:
},
...
],
"testMethodProperties": [
{
"methodName": "when",
"argumentIndex": 0
},
...
],
"enumProperties":
[
{
Expand Down Expand Up @@ -106,7 +113,13 @@ public void some_unit_test() { ... }

when `IsTreated` is `true`, and will be deleted completely when `IsTreated` is `false`.

An optional top-level field is `enumProperties`.
An optional top-level field is `testMethodProperties`.

Copy link
Contributor

Choose a reason for hiding this comment

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

Either no new line here, or add a new line on the other similar property "headers" above and below.

Within that, there is an array of JSON objects, having the required fields `methodName` and `argumentIndex`. The both behave the same as the fields with the same name in `methodProperties`.
Copy link
Contributor

Choose a reason for hiding this comment

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

The -> They


What this field does, is that if we find one of the `methodProperties` fields inside a method that matches one of the methods in `testMethodProperties`, we remove that method. This is useful for removing `mock()` wrappers or `assert()` calls that are no longer useful after a flag is cleaned up.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we refer this to as testMethodWrappers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we keep the pattern so they all still end in properties? testMethodWrapperProperties? Since it behaves so similarly to methodProperties.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not even sure that testMethodWrapperProperties is correct. I wonder if there is a name here that can make it clear at a glance that we mean "Methods from testing/assertion libraries" as opposed to "Methods used to test configuration flags specifically", and that is also not insanely long... if no such name exists, I prefer testMethodProperties than testMethodWrapperProperties...

Perhaps testingLibraryMethodProperties?


Another optional top-level field is `enumProperties`.
Within that, there is an array of JSON objects, having the required fields `enumName` and `argumentIndex`.

What this field does, is if you specify an enum class name, Piranha will remove enum constants that have a constructor with a string argument that matches your `FlagName` value, along with their usages.
Expand Down
14 changes: 14 additions & 0 deletions java/piranha/config/properties.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,20 @@
"argumentIndex": 0
}
],
"testMethodProperties": [
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing I wonder about test library methods vs flag-configuration-system methods, is that they tend to have fairly generic names (like accept). I am wondering if in a large codebase, we might not need to match class name (i.e. FQN of the receiver), in order to avoid false positives here.

Specially if this is going to ship as the default/base config file for Piranha.

{
"methodName": "mock",
"argumentIndex": 0
},
{
"methodName": "accept",
"argumentIndex": 0
},
{
"methodName": "expect",
"argumentIndex": 1
}
],
"linkURL": "<provide_your_url>",
"annotations": [
"ToggleTesting",
Expand Down
53 changes: 48 additions & 5 deletions java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@
import com.uber.piranha.config.Config;
import com.uber.piranha.config.PiranhaConfigurationException;
import com.uber.piranha.config.PiranhaEnumRecord;
import com.uber.piranha.config.PiranhaMethodRecord;
import com.uber.piranha.config.PiranhaFlagMethodRecord;
import com.uber.piranha.config.PiranhaTestMethodRecord;
import com.uber.piranha.testannotations.AnnotationArgument;
import com.uber.piranha.testannotations.ResolvedTestAnnotation;
import java.util.ArrayList;
Expand Down Expand Up @@ -344,7 +345,7 @@ private API getXPAPI(ExpressionTree et, VisitorState state) {
}
MemberSelectTree mst = (MemberSelectTree) mit.getMethodSelect();
String methodName = mst.getIdentifier().toString();
ImmutableCollection<PiranhaMethodRecord> methodRecords =
ImmutableCollection<PiranhaFlagMethodRecord> methodRecords =
this.config.getMethodRecordsForName(methodName);
if (methodRecords.size() > 0) {
return getXPAPI(mit, state, methodRecords);
Expand All @@ -356,8 +357,8 @@ private API getXPAPI(ExpressionTree et, VisitorState state) {
private API getXPAPI(
MethodInvocationTree mit,
VisitorState state,
ImmutableCollection<PiranhaMethodRecord> methodRecordsForName) {
for (PiranhaMethodRecord methodRecord : methodRecordsForName) {
ImmutableCollection<PiranhaFlagMethodRecord> methodRecordsForName) {
for (PiranhaFlagMethodRecord 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();
Expand Down Expand Up @@ -880,19 +881,61 @@ public Description matchBinary(BinaryTree tree, VisitorState state) {
return Description.NO_MATCH;
}

private boolean matchTestMethod(MethodInvocationTree methodTree, VisitorState state) {
Symbol receiverSymbol = ASTHelpers.getSymbol(methodTree.getMethodSelect());
String methodName = receiverSymbol.getSimpleName().toString();
for (PiranhaTestMethodRecord methodRecord : config.getTestMethodRecordsForName(methodName)) {
Optional<Integer> argumentIdx = methodRecord.getArgumentIdx();
if (argumentIdx.isPresent()) {
if (methodTree.getArguments().size() > argumentIdx.get()) {
ExpressionTree argTree = methodTree.getArguments().get(argumentIdx.get());
API api = getXPAPI(argTree, state);
if (api != API.UNKNOWN) {
return true;
}
}
} else {
for (ExpressionTree argTree : methodTree.getArguments()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

does this mean the flagTest API can occur at any index?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but only if the argument index is not defined.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is consistent with other configuration options. Though, if I recall, the index-less config was mostly allowed for backwards compatibility. Maybe it should be index-required for configuration options going forward?

API api = getXPAPI(argTree, state);
if (api != API.UNKNOWN) {
return true;
}
}
}
}
return false;
}

@Override
public Description matchExpressionStatement(ExpressionStatementTree tree, VisitorState state) {
if (shouldSkip(state)) return Description.NO_MATCH;
if (overLaps(tree, state)) {
return Description.NO_MATCH;
}

boolean updateCode = false;

if (tree.getExpression().getKind().equals(Kind.METHOD_INVOCATION)) {
MethodInvocationTree mit = (MethodInvocationTree) tree.getExpression();
ExpressionTree receiver = ASTHelpers.getReceiver(mit);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this logic external to the matchTestMethod, rather than part of it? Why not unconditionally pass mit and check if you can get a receiver within the more specific method? Do we use receiver for something else in the caller?

if (receiver == null) {
if (matchTestMethod(mit, state)) {
updateCode = true;
}
} else if (receiver.getKind() == Kind.METHOD_INVOCATION) {
if (matchTestMethod((MethodInvocationTree) receiver, state)) {
updateCode = true;
}
}

API api = getXPAPI(mit, state);
if (api.equals(API.DELETE_METHOD)
|| api.equals(API.SET_TREATED)
|| api.equals(API.SET_CONTROL)) {
updateCode = true;
}

if (updateCode) {
Description.Builder builder = buildDescription(tree);
SuggestedFix.Builder fixBuilder = SuggestedFix.builder();
fixBuilder.delete(tree);
Expand Down Expand Up @@ -1192,7 +1235,7 @@ private void recursiveScanTestMethodStats(
// only when the flag name matches, and we want to verify that no calls are being made to
// set
// unrelated flags (i.e. count them in counters.allSetters).
for (PiranhaMethodRecord methodRecord : config.getMethodRecordsForName(methodName)) {
for (PiranhaFlagMethodRecord methodRecord : config.getMethodRecordsForName(methodName)) {
if (methodRecord.getApiType().equals(XPFlagCleaner.API.SET_TREATED)) {
counters.allSetters += 1;
// If the test is asking for the flag in treated condition, but we are setting it to
Expand Down
65 changes: 52 additions & 13 deletions java/piranha/src/main/java/com/uber/piranha/config/Config.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ public final class Config {
/* Names of top-level fields within properties.json */
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 FLAG_METHODS_KEY = "methodProperties";
private static final String TEST_METHODS_KEY = "testMethodProperties";
private static final String CLEANUP_OPTS_KEY = "cleanupOptions";
private static final String ENUMS_KEY = "enumProperties";

Expand All @@ -64,13 +65,22 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! 👍

* 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;
private final ImmutableMultimap<String, PiranhaFlagMethodRecord> configMethodProperties;

/**
* configTestMethodProperties 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.
*/
private final ImmutableMultimap<String, PiranhaTestMethodRecord> configTestMethodProperties;

/**
* configEnumProperties is a map where key is enum name and value is a list where each item in the
Expand Down Expand Up @@ -99,12 +109,14 @@ public final class Config {
// Constructor is private, a Config object can be generated using the class' static methods,
// in particular Config.fromJSONFile([properties.json])
private Config(
ImmutableMultimap<String, PiranhaMethodRecord> configMethodProperties,
ImmutableMultimap<String, PiranhaFlagMethodRecord> configMethodProperties,
ImmutableMultimap<String, PiranhaTestMethodRecord> configTestMethodProperties,
ImmutableMultimap<String, PiranhaEnumRecord> configEnumProperties,
TestAnnotationResolver testAnnotationResolver,
ImmutableMap<String, Object> cleanupOptions,
String linkURL) {
this.configMethodProperties = configMethodProperties;
this.configTestMethodProperties = configTestMethodProperties;
this.configEnumProperties = configEnumProperties;
this.testAnnotationResolver = testAnnotationResolver;
this.cleanupOptions = cleanupOptions;
Expand All @@ -115,15 +127,29 @@ private Config(
* Return all configuration method records matching a given method name.
*
* @param methodName the method name to search
* @return A collection of {@link PiranhaMethodRecord} objects, representing each method
* @return A collection of {@link PiranhaFlagMethodRecord} objects, representing each method
* definition in the piranha json configuration file matching {@code methodName}.
*/
public ImmutableCollection<PiranhaMethodRecord> getMethodRecordsForName(String methodName) {
public ImmutableCollection<PiranhaFlagMethodRecord> getMethodRecordsForName(String methodName) {
return configMethodProperties.containsKey(methodName)
? configMethodProperties.get(methodName)
: ImmutableSet.of();
}

/**
* Return all configuration test method records matching a given method name.
*
* @param methodName the method name to search
* @return A collection of {@link PiranhaTestMethodRecord} objects, representing each method
* definition in the piranha json configuration file matching {@code methodName}.
*/
public ImmutableCollection<PiranhaTestMethodRecord> getTestMethodRecordsForName(
String methodName) {
return configTestMethodProperties.containsKey(methodName)
? configTestMethodProperties.get(methodName)
: ImmutableSet.of();
}

/**
* Returns whether any configuration enum records exist. Useful for skipping logic if enum
* properties are not configured.
Expand Down Expand Up @@ -266,7 +292,9 @@ public static Config fromJSONFile(String configFile, boolean isArgumentIndexOpti
}

String linkURL = DEFAULT_PIRANHA_URL;
ImmutableMultimap.Builder<String, PiranhaMethodRecord> methodsBuilder =
ImmutableMultimap.Builder<String, PiranhaFlagMethodRecord> methodsBuilder =
ImmutableMultimap.builder();
ImmutableMultimap.Builder<String, PiranhaTestMethodRecord> testMethodsBuilder =
ImmutableMultimap.builder();
ImmutableMultimap.Builder<String, PiranhaEnumRecord> enumsBuilder =
ImmutableMultimap.builder();
Expand Down Expand Up @@ -294,17 +322,26 @@ public static Config fromJSONFile(String configFile, boolean isArgumentIndexOpti
}
}
}
if (propertiesJson.get(METHODS_KEY) != null) {
if (propertiesJson.get(FLAG_METHODS_KEY) != null) {
for (Map<String, Object> methodProperty :
(List<Map<String, Object>>) propertiesJson.get(METHODS_KEY)) {
PiranhaMethodRecord methodRecord =
PiranhaMethodRecord.parseFromJSONPropertyEntryMap(
(List<Map<String, Object>>) propertiesJson.get(FLAG_METHODS_KEY)) {
PiranhaFlagMethodRecord methodRecord =
PiranhaFlagMethodRecord.parseFromJSONPropertyEntryMap(
methodProperty, isArgumentIndexOptional);
methodsBuilder.put(methodRecord.getMethodName(), methodRecord);
}
} else {
throw new PiranhaConfigurationException("methodProperties not found, required.");
}
if (propertiesJson.get(TEST_METHODS_KEY) != null) {
for (Map<String, Object> methodProperty :
(List<Map<String, Object>>) propertiesJson.get(TEST_METHODS_KEY)) {
PiranhaTestMethodRecord methodRecord =
PiranhaTestMethodRecord.parseFromJSONPropertyEntryMap(
methodProperty, isArgumentIndexOptional);
testMethodsBuilder.put(methodRecord.getMethodName(), methodRecord);
}
}
if (propertiesJson.get(ENUMS_KEY) != null) {
for (Map<String, Object> enumProperty :
(List<Map<String, Object>>) propertiesJson.get(ENUMS_KEY)) {
Expand All @@ -327,6 +364,7 @@ public static Config fromJSONFile(String configFile, boolean isArgumentIndexOpti
}
return new Config(
methodsBuilder.build(),
testMethodsBuilder.build(),
enumsBuilder.build(),
annotationResolverBuilder.build(),
cleanupOptionsBuilder.build(),
Expand Down Expand Up @@ -365,6 +403,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
Loading