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] Ability to detect and removed stale code for specific use-cases #104

Open
sivacoder opened this issue Oct 27, 2020 · 5 comments
Open

Comments

@sivacoder
Copy link
Contributor

@sivacoder sivacoder commented Oct 27, 2020

In the past we have made PR's to Piranha to support additional usage patterns. Also, thanks to all the new functionalities added to Piranha Java last few months.

We are in the final stage of putting Piranha to pre-production for our java based micro services. We have hit this two blockers and need your support for this:

Case 1: Tests using the mocked frameworks (mockito) where the same feature flag is used
when(featureUtil.isFlagEnabled("stale_flag").thenReturn(true);

is converted to

when(true).thenReturn(true);

With this change, build is failing as Mockito is classified as "Unnecessary Mocking". Any way to get going on this?

Case 2: Removing Unused Imports (Static and Non-Static) after Piranha cleaning up:

Imagine a code like

import com.test.FlagUtil;
if (flagUtil.isFlagEnabled("stale_flag")) {
// Do some job
}

is converted to
import com.test.FlagUtil;
// Do some job

But can the unused imports also be removed (static and non-static) ?

@mkr-plse
Copy link
Collaborator

@mkr-plse mkr-plse commented Oct 27, 2020

Case 1: Tests using the mocked frameworks (mockito) where the same feature flag is used

In an email, @pranavsb had proposed the following solution:

So a config field to specify strings like "accept" and "when" (usual starting methods for testcases, can use FQNs as well).
And then I'm guessing in "matchMethod" we check for this field and check that the subtree contains a treated or control XP API and delete that line.

To elaborate further on that, update the properties.json file to accept an additional configuration for test methods which will wrap an XP API, and then check for those methods in matchMethod and if those method contains XP API invocation, delete the entire invocation. This is a stop gap until we have multi pass analysis (see here and [here])(https://github.com/uber/piranha/milestone/1).

Case 2: Removing Unused Imports (Static and Non-Static) after Piranha cleaning up:

Yes. Enabling RemoveUnusedImports errorprone check should help here.

@sivacoder
Copy link
Contributor Author

@sivacoder sivacoder commented Oct 28, 2020

Thanks @mkr-plse for the inputs. On #1, we tried some options, but the implementation was getting complicated.

When we intercept matchMethod, we are getting the complete statement.
For example: org.mockito.Mockito.when(experimentation.isToggleDisabled(STALE_FLAG)).thenReturn(false)

From this, we are able to extract the methodName as org.mockito.Mockito.when, but checking if when arguments matches the stale_flag we had to do multiple iterations of getMethodSelect() getExpression() and so on and was becoming very specific to our use-case and not generic.

Any suggestion to take this forward?

@mkr-plse
Copy link
Collaborator

@mkr-plse mkr-plse commented Oct 28, 2020

If you call evalExpr on the argument expression to when and if that evaluation either returns Value.TRUE or Value.FALSE, you can remove the entire method expression.

Specifically, expression representing experimentation.isToggleDisabled(STALE_FLAG) will be passed to evalExpr which internally will then evaluate it to a boolean constant.

@sivacoder
Copy link
Contributor Author

@sivacoder sivacoder commented Oct 28, 2020

Interesting, thanks @mkr-plse . Let me try evalExpr method. Also, is there a standard way to extract the outer method name?

Ie., in this example of org.mockito.Mockito.when(experimentation.isToggleDisabled(STALE_FLAG)).thenReturn(false),
to extract out org.mockito.Mockito.when, is there also a standard way to get the method name as FQN?

@mkr-plse
Copy link
Collaborator

@mkr-plse mkr-plse commented Oct 28, 2020

Also, is there a standard way to extract the outer method name?

See getXPAPI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.