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

[Approach PR] [Not ready for review] - Adding support for mocks in test cleanup #107

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[Approach PR] [Not ready for review] - Adding support for mocks in test cleanup #107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ dependencies {
implementation deps.build.googleJsonSimple

testCompile deps.test.junit4
testCompile 'org.mockito:mockito-core:2.7.22'
testCompile deps.test.daggerAnnotations
testCompile(deps.build.errorProneTestHelpers) {
exclude group: "junit", module: "junit"
Expand Down
7 changes: 7 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 @@ -41,5 +41,12 @@
"linkURL": "<provide_your_url>",
"annotations": [
"ToggleTesting"
],
"testMethodProperties" : [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will act as the configuration for the test methods that needs to be cleanup. Structure is the same as methodProperties

Copy link
Contributor

@mkr-plse mkr-plse Oct 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Just change the flagType for the method. So,

{
      "methodName": "when",
      "flagType": "mock",
      "argumentIndex": 0
 }

No separate testMethodProperties.

{
"methodName": "when",
"flagType": "empty",
"argumentIndex": 0
}
]
}
57 changes: 57 additions & 0 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 @@ -69,6 +69,7 @@
import java.util.Set;
import javax.annotation.Nullable;
import javax.lang.model.element.ElementKind;

import org.json.simple.JSONObject;
import org.json.simple.parser.JSONParser;
import org.json.simple.parser.ParseException;
Expand Down Expand Up @@ -186,6 +187,8 @@ enum API {
*/
private ImmutableMultimap<String, PiranhaMethodRecord> configMethodProperties;

private ImmutableMultimap<String, PiranhaMethodRecord> configTestMethodProperties;

private final HashSet<String> handledAnnotations = new HashSet<>();
private String linkURL = PIRANHA_DEFAULT_URL;

Expand Down Expand Up @@ -221,6 +224,7 @@ void init(ErrorProneFlags flags) throws PiranhaConfigurationException {
// No configuration present at all, disable Piranha checker
disabled = true;
configMethodProperties = ImmutableMultimap.of();
configTestMethodProperties = ImmutableMultimap.of();
return;
}

Expand Down Expand Up @@ -274,6 +278,19 @@ void init(ErrorProneFlags flags) throws PiranhaConfigurationException {
builder.put(methodRecord.getMethodName(), methodRecord);
}
configMethodProperties = builder.build();

// Add test method configuration
Set<Map<String, Object>> testMethodProperties = new HashSet<>();
if (propertiesJson.get("testMethodProperties") != null) {
testMethodProperties.addAll((List<Map<String, Object>>) propertiesJson.get("testMethodProperties"));
}
ImmutableMultimap.Builder<String, PiranhaMethodRecord> testPropertiesBuilder = new ImmutableMultimap.Builder<>();
for (Map<String, Object> testMethodProperty : testMethodProperties) {
PiranhaMethodRecord methodRecord = PiranhaMethodRecord.parseFromJSONPropertyEntryMap(testMethodProperty, isArgumentIndexOptional);
testPropertiesBuilder.put(methodRecord.getMethodName(), methodRecord);
}
configTestMethodProperties = testPropertiesBuilder.build();

} catch (IOException fnfe) {
throw new PiranhaConfigurationException(
"Error reading config file " + Paths.get(configFile).toAbsolutePath() + " : " + fnfe);
Expand All @@ -293,6 +310,7 @@ void init(ErrorProneFlags flags) throws PiranhaConfigurationException {
// Already in the right format, re-throw
throw pce;
} catch (Exception e) {
e.getStackTrace();
throw new PiranhaConfigurationException("Some other exception thrown while parsing config");
}
} else {
Expand Down Expand Up @@ -881,6 +899,45 @@ private boolean isCheckedXPFlagName(ExpressionTree tree) {
public Description matchMethod(MethodTree tree, VisitorState state) {
if (disabled) return Description.NO_MATCH;

if (!configTestMethodProperties.isEmpty() && tree != null && tree.getBody() != null && tree.getBody().getStatements() != null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mkr-plse - This entire logic is very specific and I'm not happy with this approach.
But need your inputs to get this better and generic

List<? extends StatementTree> bt = tree.getBody().getStatements();
for (StatementTree st : bt) {
if (st != null && st.getKind().equals(Kind.EXPRESSION_STATEMENT)) {
ExpressionStatementTree est = (ExpressionStatementTree) st;
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Shouldn't the statement count here be just one? Can we avoid the loop?
  2. How about calling evalExpr(st)? If not, evalExpr(mit) below and use the result of that.
  3. Before calling theevalExpr, call getXPAPI and see whether the API is Mock method ? You may also want to update getXPAPI to do the necessary processing to check for when and return the appropriate type.

So, the code will be:

API api = getXPAPI(mit) // may be st too.
if(api.equals(API.MOCK)) {
     Value value = evalExpr(mit) // may be st too and update evalExpr
      if(value != Value.BOT) {
         // code starting at line 922. 
      }
}

if (est != null && est.getExpression() instanceof MethodInvocationTree) {
MethodInvocationTree mit = (MethodInvocationTree) est.getExpression();
if (mit != null && mit.getKind().equals(Kind.METHOD_INVOCATION)) {
ExpressionTree exp = ((MemberSelectTree) mit.getMethodSelect()).getExpression();
if (exp != null && exp.getKind().equals(Kind.METHOD_INVOCATION)) {
ExpressionTree exp2 = ((MethodInvocationTree)exp).getMethodSelect();
if (exp2 != null && exp2.getKind().equals(Kind.MEMBER_SELECT)) {
String methodName = ((MemberSelectTree) exp2).getIdentifier().toString();
if (configTestMethodProperties.containsKey(methodName)) {
ImmutableCollection<PiranhaMethodRecord> methodRecords = configTestMethodProperties.get(methodName);
for (PiranhaMethodRecord methodRecord : methodRecords) {
Optional<Integer> optionalArgumentIdx = methodRecord.getArgumentIdx();
if (optionalArgumentIdx.isPresent()) {
Value value = evalExpr(((MethodInvocationTree) exp).getArguments().get(optionalArgumentIdx.get()), state);
if (value == Value.TRUE || value == Value.FALSE) {
Description.Builder builder = buildDescription(est);
SuggestedFix.Builder fixBuilder = SuggestedFix.builder();
fixBuilder.delete(est);
decrementAllSymbolUsages(est, state, fixBuilder);
builder.addFix(fixBuilder.build());
endPos = state.getEndPosition(est);
return builder.build();
}
}
}
}
}
}
}
}
}
}
}

for (String name : handledAnnotations) {
AnnotationTree at =
ASTHelpers.getAnnotationWithSimpleName(tree.getModifiers().getAnnotations(), name);
Expand Down
1 change: 1 addition & 0 deletions java/piranha/src/test/java/com/uber/piranha/XPFlagCleanerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ public void positiveSpecificTreatmentGroup() throws IOException {
" private XPTest experimentation;",
" public String groupToString() {",
" // BUG: Diagnostic contains: Cleans stale XP flags",
" org.mockito.Mockito.when(experimentation.isToggleDisabled(STALE_FLAG)).thenReturn(false);",
" if (experimentation.isToggleDisabled(STALE_FLAG)) { return \"\"; }",
" else if (experimentation.isToggleInGroup(",
" STALE_FLAG,GROUP_A)) { ",
Expand Down