Introduce BugPatternTestExtractor with tests
#494
Conversation
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
| return scanBugPatternTest.hasTestUsingClassInstance(bugPatternName); | ||
| } | ||
|
|
||
| private static final class ScanBugPatternTest extends TreeScanner<@Nullable Void, VisitorState> { |
There was a problem hiding this comment.
The name of this Scanner can probably be improved.... The same holds for the encounteredClasses and hasTestUsingClassInstance.... Idk why, but really struggled with these names 🤔. Definitely open to suggestions 😄.
There was a problem hiding this comment.
I this we can side-step this issue; will propose something.
|
|
||
| @AutoValue | ||
| abstract static class BugPatternReplacementTestDocumentation { | ||
| static BugPatternReplacementTestDocumentation create(String sourceLines, String outputLines) { |
There was a problem hiding this comment.
Couldn't find a way around this, the way we instantiate the BugPatternTestDocumentation earlier on wasn't possible with this class...
| "import com.google.errorprone.CompilationTestHelper;", | ||
| "", | ||
| "final class IdentityConversionTest {", | ||
| " private static class IdentityConversion extends BugChecker {}", |
There was a problem hiding this comment.
Came up with this way to make testing a little easier... It's hard to make sure that the IdentityConversion is available without having to do an import. I figured this was "closest" to our actual testing setup...
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Stephan202
force-pushed
the
rossendrijver/PSM-1716
branch
from
302274e to
485c85c
Compare
rickie
force-pushed
the
rossendrijver/PSM-1717
branch
from
ccfaca2 to
0755f54
Compare
|
Applied the feedback @Stephan202 gave offline. It's ready for review 😄. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
| } | ||
|
|
||
| // XXX: Duplicate from `ErrorProneTestSourceFormat`, should we move this to `SourceCode` util? | ||
| private static String getSourceLines(MethodInvocationTree tree) { |
There was a problem hiding this comment.
should we move this to
SourceCodeutil?
Didn't apply something like this yet, to not introduce a dependency on error-prone-contrib. I think we should have a more generic "utilities" module.
There was a problem hiding this comment.
Hmm, this logic is very specific to the {BugCheckerRefactoring,Compilation}TestHelper APIs, so moving it to SourceCode feels out of place. I guess duplicating this isn't so bad. Or perhaps we need "something" specific to Error Prone testing. 🤔
rickie
force-pushed
the
rossendrijver/PSM-1717
branch
from
fff2b03 to
2dad974
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
rickie
force-pushed
the
rossendrijver/PSM-1717
branch
from
be9a611 to
673f444
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Stephan202
force-pushed
the
rossendrijver/PSM-1717
branch
from
673f444 to
f43aa12
Compare
|
|
||
| @AutoValue | ||
| abstract static class BugPatternReplacementTestDocumentation { | ||
| static BugPatternReplacementTestDocumentation create(String sourceLines, String outputLines) { |
| StringBuilder source = new StringBuilder(); | ||
|
|
||
| for (ExpressionTree sourceLine : sourceLines) { | ||
| Object value = ASTHelpers.constValue(sourceLine); |
There was a problem hiding this comment.
| Object value = ASTHelpers.constValue(sourceLine); | |
| String value = ASTHelpers.constValue(sourceLine, String.class); |
Has no functional impact, but arguably makes for clearer code. Will also change the other code.
| } | ||
|
|
||
| // XXX: Duplicate from `ErrorProneTestSourceFormat`, should we move this to `SourceCode` util? | ||
| private static String getSourceLines(MethodInvocationTree tree) { |
There was a problem hiding this comment.
Hmm, this logic is very specific to the {BugCheckerRefactoring,Compilation}TestHelper APIs, so moving it to SourceCode feels out of place. I guess duplicating this isn't so bad. Or perhaps we need "something" specific to Error Prone testing. 🤔
| for (ExpressionTree sourceLine : sourceLines) { | ||
| Object value = ASTHelpers.constValue(sourceLine); | ||
| if (value == null) { | ||
| return ""; |
There was a problem hiding this comment.
We shouldn't add the empty string to the result collection.
| } | ||
|
|
||
| // XXX: Duplicate from `ErrorProneTestSourceFormat`, should we move this to `SourceCode` util? | ||
| private static String getSourceLines(MethodInvocationTree tree) { |
There was a problem hiding this comment.
| private static String getSourceLines(MethodInvocationTree tree) { | |
| private static String getSourceCode(MethodInvocationTree tree) { |
(That the input is represented as a sequence of lines is an implementation detail.)
| "outputLines": "class A {}\n" | ||
| } | ||
| ] | ||
| } |
| "replacementTests": [ | ||
| { | ||
| "inputLines": "class A {}\n", | ||
| "outputLines": "" | ||
| } | ||
| ] |
There was a problem hiding this comment.
This is not an accurate representation of expectUnchanged(): either input and output should be the same, or we should omit the test case. From a documentation POV omitting makes more sense, I suppose. (An alternative is to express such a test as an identification test. Which then raises the further question: should we perhaps have a separate category of "unchanged code examples" for source code that doesn't contain a // BUG: Diagnostic (contains|matches) marker?)
There was a problem hiding this comment.
My current thinking leans towards leaving the unchanged examples out.
| private final List<String> identificationTests = new ArrayList<>(); | ||
| private final List<BugPatternReplacementTestDocumentation> replacementTests = new ArrayList<>(); | ||
|
|
||
| @Var private String replacementOutputLines = ""; |
There was a problem hiding this comment.
This stateful solution makes the code more brittle than necessary; will propose an alternative.
| .onDescendantOf("com.google.errorprone.BugCheckerRefactoringTestHelper.ExpectOutput") | ||
| .named("addOutputLines"); | ||
|
|
||
| private final VisitorState state; |
There was a problem hiding this comment.
This could be passed around instead of one of the Void params. (Like for ScanBugPatternTest.)
| @Override | ||
| public @Nullable Void visitMethodInvocation(MethodInvocationTree node, VisitorState state) { | ||
| if (BUG_PATTERN_TEST_METHOD.matches(node, state)) { | ||
| MemberSelectTree firstArgumentTree = (MemberSelectTree) node.getArguments().get(0); |
There was a problem hiding this comment.
While not likely in our code base, this could cause a ClassCastException. Can be fixed by augmenting the Matcher.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Stephan202
force-pushed
the
rossendrijver/PSM-1717
branch
from
f43aa12 to
2ad41dd
Compare
|
|
||
| String className = tree.getSimpleName().toString(); | ||
| return new AutoValue_BugPatternTestExtractor_BugPatternTestDocumentation( | ||
| className.substring(0, className.lastIndexOf("Test")), |
There was a problem hiding this comment.
This code is duplicated below 👀
There was a problem hiding this comment.
In part this is due to how Extractor is defined. Perhaps we should combine the two methods into a single Optional-returning method. That mirrors approaches we take elsewhere.
| return scanBugPatternTest.hasTestUsingClassInstance(bugPatternName); | ||
| } | ||
|
|
||
| private static final class ScanBugPatternTest extends TreeScanner<@Nullable Void, VisitorState> { |
There was a problem hiding this comment.
I this we can side-step this issue; will propose something.
| for (Tree m : tree.getMembers()) { | ||
| if (JUNIT_TEST_METHOD.matches(m, state)) { | ||
| scanner.scan(m, state); | ||
| } | ||
| } |
There was a problem hiding this comment.
We make a number of assumptions in this extractor class:
FooBugPatternis tested (only) in a class namesFooBugPatternTest.- All
CompilationTestHelpers andBugCheckerRefactoringTestHelperinsideFooBugPatternTestexerciseFooBugPattern.
This assumptions are certainly valid for our code, but since {Compilation,BugCheckerRefactoring}TestHelper explicitly mention which bug pattern they exercise, we shouldn't need to make these assumptions. Generalization may require a change to the way in which we persist the resultant JSON, though.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Stephan202
force-pushed
the
rossendrijver/PSM-1717
branch
from
2ad41dd to
6ebb265
Compare
rickie
force-pushed
the
rossendrijver/PSM-1717
branch
from
8f8f858 to
bcc4a62
Compare
|
Will rebase and merge afterwards 😄! |
|
Kudos, SonarCloud Quality Gate passed! |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
❗
This is draft PR on top of #428❗It looks like a huge PR but it consists mostly of tests and associated resources!
There are some open points that we should discuss before we mark this as 100% ready for review:
error-prone-contrib. It would be nice to be able to use theSourceCodeutil indocumentation-supportsuch that we can de-duplicate code (as well as anMatcherthat lives in our utils). Let's discuss how to approach this.websitebranch. If we want to further improve the heuristics, we can discuss that maybe in a follow up PR. I also added an XXX for this.BugPatternExtractor#extractis not as optimal as it can be. However, again, the goal of this ticket is not to implement the most optimal solution now. I added some XXXs to indicate where we can further improve.Let's discuss this and hopefully continue with this approach.