[PiranhaJava] Remove configured test methods #151

tnterdan · 2021-09-24T22:28:57Z

Fixes #47

This development allows the user of Piranha to specify a list of methods
where if a dynamic flag evaluation is in a certain argument position, we
remove that method.

This is useful for cleaning up mock() wrapper methods or assert()
methods in test code that are useless after a feature flag is removed.

Limitations:

This does not remove unused imports for the test methods because we
have to account for cases of the test methods that will not be
removed.

This development allows the user of Piranha to specify a list of methods where if a dynamic flag evaluation is in a certain argument position, we remove that method. This is useful for cleaning up `mock()` wrapper methods or `assert()` methods in test code that are useless after a feature flag is removed. Limitations: - This does not remove unused imports for the test methods because we have to account for cases of the test methods that will not be removed.

mkr-plse

Overall, looks good. A minor refactoring as suggested in the comment will be helpful.

mkr-plse · 2021-09-27T21:28:06Z

java/README.md

-An optional top-level field is `enumProperties`.
+An optional top-level field is `testMethodProperties`.
+
+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`.


The -> They

mkr-plse · 2021-09-27T21:29:22Z

java/README.md

+
+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`.
+
+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.


Should we refer this to as testMethodWrappers?

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

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?

mkr-plse · 2021-09-27T21:34:55Z

java/piranha/src/test/java/com/uber/piranha/TestCaseCleanUpTest.java

@@ -833,4 +833,196 @@ public void testCleanBySettersHeuristicIgnoreSettersForOtherFlags() throws IOExc
            "}")
        .doTest();
  }
+


nice set of tests.

mkr-plse · 2021-09-27T21:39:17Z

java/piranha/src/main/java/com/uber/piranha/config/PiranhaFlagMethodRecord.java

+  static PiranhaFlagMethodRecord parseFromJSONPropertyEntryMap(
+      Map<String, Object> methodPropertyEntry, boolean isArgumentIndexOptional)
+      throws PiranhaConfigurationException {
+    String methodName = getValueStringFromMap(methodPropertyEntry, METHOD_NAME_KEY);


Can code from line 111 to 132 be hoisted into a utility method and called. The same method can be called from PiranhaTestMethodRecord too. The messaging in the throw and the check on flagType seem to be the only differences between the two methods.

mkr-plse · 2021-09-27T21:43:24Z

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

+          }
+        }
+      } else {
+        for (ExpressionTree argTree : methodTree.getArguments()) {


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

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

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?

lazaroclapp · 2021-10-13T20:25:59Z

Limitations:

This does not remove unused imports for the test methods because we
have to account for cases of the test methods that will not be
removed.

Arguably better in a follow-up, but this sounds like the use case for UsageCounter, which I believe we use in similar cases. Not 100% sure because I haven't yet reviewed the code (starting on it right now :) )

lazaroclapp

A couple very minor nits. Overall this looks good to me. My biggest concern is that the test methods in question tend to have very generic names and wether or not it might make sense in this case to also try to match on declaring class name.

lazaroclapp · 2021-10-13T20:28:37Z

java/README.md

@@ -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`.
+


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

lazaroclapp · 2021-10-13T20:30:59Z

java/README.md

+
+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`.
+
+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.


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?

lazaroclapp · 2021-10-13T20:34:34Z

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

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


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?

lazaroclapp · 2021-10-13T20:37:48Z

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

+          }
+        }
+      } else {
+        for (ExpressionTree argTree : methodTree.getArguments()) {


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?

lazaroclapp · 2021-10-13T20:39:32Z

java/piranha/src/main/java/com/uber/piranha/config/Config.java

-   * 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


Good catch! 👍

lazaroclapp · 2021-10-13T20:48:23Z

java/piranha/config/properties.json

@@ -38,6 +38,20 @@
      "argumentIndex": 0
    }
  ],
+  "testMethodProperties": [


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.

lazaroclapp · 2021-10-13T20:53:05Z

java/piranha/src/test/java/com/uber/piranha/TestCaseCleanUpTest.java

+            "import org.junit.Test;",
+            "import static com.uber.piranha.TestExperimentName.OTHER_FLAG_1;",
+            "import static com.uber.piranha.TestExperimentName.STALE_FLAG;",
+            "import static com.uber.piranha.TestMethods.expect;",


Obvious thing about Java imports that I just thought about seeing this: static imports can actually import a set of methods, rather than a single method, because they match on method name and not method signature. TIL.

(Not a comment on this PR at all, just something I can't believe I just realized... wondering about implications for dead-code removal, etc)

tnterdan added 2 commits September 24, 2021 15:21

[PiranhaJava] Add configuration for test method cleanup

028fb16

mkr-plse reviewed Sep 27, 2021

View reviewed changes

tnterdan added 3 commits September 28, 2021 11:09

Fix typo in README The -> They

2866b72

Move shared parseFromJSONPropertyEntryMap code to utility methods

c6bf298

Add nullable annotation to parseArgumentIndexFromJSON

d29883a

lazaroclapp reviewed Oct 13, 2021

View reviewed changes

mkr-plse mentioned this pull request Nov 23, 2021

maven release #153

Closed

ketkarameya force-pushed the master branch from d802f3e to 3955145 Compare September 7, 2023 02:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PiranhaJava] Remove configured test methods #151

[PiranhaJava] Remove configured test methods #151

tnterdan commented Sep 24, 2021

mkr-plse left a comment •

edited

mkr-plse Sep 27, 2021

mkr-plse Sep 27, 2021

tnterdan Sep 28, 2021

lazaroclapp Oct 13, 2021

mkr-plse Sep 27, 2021

mkr-plse Sep 27, 2021

mkr-plse Sep 27, 2021

tnterdan Sep 28, 2021

lazaroclapp Oct 13, 2021

lazaroclapp commented Oct 13, 2021

lazaroclapp left a comment

lazaroclapp Oct 13, 2021

lazaroclapp Oct 13, 2021

lazaroclapp Oct 13, 2021

lazaroclapp Oct 13, 2021

lazaroclapp Oct 13, 2021

lazaroclapp Oct 13, 2021

lazaroclapp Oct 13, 2021


		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`.

		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.

[PiranhaJava] Remove configured test methods #151

Are you sure you want to change the base?

[PiranhaJava] Remove configured test methods #151

Conversation

tnterdan commented Sep 24, 2021

mkr-plse left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazaroclapp commented Oct 13, 2021

lazaroclapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkr-plse left a comment •

edited