-
Notifications
You must be signed in to change notification settings - Fork 39
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
Introduce BugPatternTestExtractor
with tests
#494
Merged
Merged
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
2004b71
Introduce `BugPatternTestExtractor` with tests
rickie 2a7aeea
Simplify tests
rickie d76a4b0
Further simplify testing setup
rickie d35d606
Kill another mutant and drop unused imports
rickie 3ff0a52
This actually kills the mutant
rickie c0245ea
Suggestions
Stephan202 ee14559
Suggestions
Stephan202 c3131c8
Suggestions
Stephan202 3a1e770
Fix JDK 11 compatibility
Stephan202 3e3fc5f
PSM-1717 Pass `ClassLoader` to `ServiceLoader`
Badbond 35a365b
Add suppressions for `Extractor`s
rickie 4ff42dd
Implement more resilient setup that works for more cases
rickie 2584921
Prefer `List` over `ArrayList`
rickie b52237e
Intermediate state - applied structure changes of `TestEntry`
rickie b4cb79d
Implement `.reverse()` for `TestEntries`
rickie 26dd57f
Add GH issue to suppressions
rickie b35337b
Assorted improvements and cleanup
rickie 037b2a5
Suggestions
Stephan202 7e4fadd
Suggestions (2)
Stephan202 8930710
Suggestions (3)
Stephan202 90f3f53
Address feedback
Stephan202 024fb7e
Resolve JDK 21 compiler warning
Stephan202 bcc4a62
Fix JDK 21 compatibility
Stephan202 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
229 changes: 229 additions & 0 deletions
229
...n-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternTestExtractor.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,229 @@ | ||
package tech.picnic.errorprone.documentation; | ||
|
||
import static com.google.errorprone.matchers.Matchers.instanceMethod; | ||
import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod; | ||
import static java.util.function.Predicate.not; | ||
|
||
import com.google.auto.service.AutoService; | ||
import com.google.auto.value.AutoValue; | ||
import com.google.common.collect.ImmutableList; | ||
import com.google.errorprone.VisitorState; | ||
import com.google.errorprone.annotations.Immutable; | ||
import com.google.errorprone.matchers.Matcher; | ||
import com.google.errorprone.util.ASTHelpers; | ||
import com.sun.source.tree.ClassTree; | ||
import com.sun.source.tree.ExpressionTree; | ||
import com.sun.source.tree.MethodInvocationTree; | ||
import com.sun.source.util.TreeScanner; | ||
import java.util.ArrayList; | ||
import java.util.List; | ||
import java.util.Optional; | ||
import org.jspecify.annotations.Nullable; | ||
import tech.picnic.errorprone.documentation.BugPatternTestExtractor.TestCases; | ||
|
||
/** | ||
* An {@link Extractor} that describes how to extract data from classes that test a {@code | ||
* BugChecker}. | ||
*/ | ||
// XXX: Handle other methods from `{BugCheckerRefactoring,Compilation}TestHelper`: | ||
// - Indicate which custom arguments are specified, if any. | ||
// - For replacement tests, indicate which `FixChooser` is used. | ||
// - ... (We don't use all optional features; TBD what else to support.) | ||
@Immutable | ||
@AutoService(Extractor.class) | ||
@SuppressWarnings("rawtypes" /* See https://github.com/google/auto/issues/870. */) | ||
public final class BugPatternTestExtractor implements Extractor<TestCases> { | ||
/** Instantiates a new {@link BugPatternTestExtractor} instance. */ | ||
public BugPatternTestExtractor() {} | ||
|
||
@Override | ||
public String identifier() { | ||
return "bugpattern-test"; | ||
} | ||
|
||
@Override | ||
public Optional<TestCases> tryExtract(ClassTree tree, VisitorState state) { | ||
BugPatternTestCollector collector = new BugPatternTestCollector(); | ||
|
||
collector.scan(tree, state); | ||
|
||
return Optional.of(collector.getCollectedTests()) | ||
.filter(not(ImmutableList::isEmpty)) | ||
.map( | ||
tests -> | ||
new AutoValue_BugPatternTestExtractor_TestCases( | ||
ASTHelpers.getSymbol(tree).className(), tests)); | ||
} | ||
|
||
private static final class BugPatternTestCollector | ||
extends TreeScanner<@Nullable Void, VisitorState> { | ||
private static final Matcher<ExpressionTree> COMPILATION_HELPER_DO_TEST = | ||
instanceMethod() | ||
.onDescendantOf("com.google.errorprone.CompilationTestHelper") | ||
.named("doTest"); | ||
private static final Matcher<ExpressionTree> TEST_HELPER_NEW_INSTANCE = | ||
staticMethod() | ||
.onDescendantOfAny( | ||
"com.google.errorprone.CompilationTestHelper", | ||
"com.google.errorprone.BugCheckerRefactoringTestHelper") | ||
.named("newInstance") | ||
.withParameters("java.lang.Class", "java.lang.Class"); | ||
private static final Matcher<ExpressionTree> IDENTIFICATION_SOURCE_LINES = | ||
instanceMethod() | ||
.onDescendantOf("com.google.errorprone.CompilationTestHelper") | ||
.named("addSourceLines"); | ||
private static final Matcher<ExpressionTree> REPLACEMENT_DO_TEST = | ||
instanceMethod() | ||
.onDescendantOf("com.google.errorprone.BugCheckerRefactoringTestHelper") | ||
.named("doTest"); | ||
private static final Matcher<ExpressionTree> REPLACEMENT_EXPECT_UNCHANGED = | ||
instanceMethod() | ||
.onDescendantOf("com.google.errorprone.BugCheckerRefactoringTestHelper.ExpectOutput") | ||
.named("expectUnchanged"); | ||
private static final Matcher<ExpressionTree> REPLACEMENT_OUTPUT_SOURCE_LINES = | ||
instanceMethod() | ||
.onDescendantOf("com.google.errorprone.BugCheckerRefactoringTestHelper.ExpectOutput") | ||
.namedAnyOf("addOutputLines", "expectUnchanged"); | ||
|
||
private final List<TestCase> collectedTestCases = new ArrayList<>(); | ||
|
||
private ImmutableList<TestCase> getCollectedTests() { | ||
return ImmutableList.copyOf(collectedTestCases); | ||
} | ||
|
||
@Override | ||
public @Nullable Void visitMethodInvocation(MethodInvocationTree node, VisitorState state) { | ||
boolean isReplacementTest = REPLACEMENT_DO_TEST.matches(node, state); | ||
if (isReplacementTest || COMPILATION_HELPER_DO_TEST.matches(node, state)) { | ||
getClassUnderTest(node, state) | ||
.ifPresent( | ||
classUnderTest -> { | ||
List<TestEntry> entries = new ArrayList<>(); | ||
if (isReplacementTest) { | ||
extractReplacementTestCases(node, entries, state); | ||
} else { | ||
extractIdentificationTestCases(node, entries, state); | ||
} | ||
|
||
if (!entries.isEmpty()) { | ||
collectedTestCases.add( | ||
new AutoValue_BugPatternTestExtractor_TestCase( | ||
classUnderTest, ImmutableList.copyOf(entries).reverse())); | ||
} | ||
}); | ||
} | ||
|
||
return super.visitMethodInvocation(node, state); | ||
Check warning on line 116 in documentation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternTestExtractor.java GitHub Actions / pitestA change can be made to line 116 without causing a test to fail
|
||
} | ||
|
||
private static Optional<String> getClassUnderTest( | ||
MethodInvocationTree tree, VisitorState state) { | ||
if (TEST_HELPER_NEW_INSTANCE.matches(tree, state)) { | ||
return Optional.ofNullable(ASTHelpers.getSymbol(tree.getArguments().get(0))) | ||
.filter(s -> !s.type.allparams().isEmpty()) | ||
.map(s -> s.type.allparams().get(0).tsym.getQualifiedName().toString()); | ||
} | ||
|
||
ExpressionTree receiver = ASTHelpers.getReceiver(tree); | ||
return receiver instanceof MethodInvocationTree | ||
? getClassUnderTest((MethodInvocationTree) receiver, state) | ||
: Optional.empty(); | ||
} | ||
|
||
private static void extractIdentificationTestCases( | ||
MethodInvocationTree tree, List<TestEntry> sink, VisitorState state) { | ||
if (IDENTIFICATION_SOURCE_LINES.matches(tree, state)) { | ||
String path = ASTHelpers.constValue(tree.getArguments().get(0), String.class); | ||
Optional<String> sourceCode = | ||
getSourceCode(tree).filter(s -> s.contains("// BUG: Diagnostic")); | ||
if (path != null && sourceCode.isPresent()) { | ||
sink.add( | ||
new AutoValue_BugPatternTestExtractor_IdentificationTestEntry( | ||
path, sourceCode.orElseThrow())); | ||
} | ||
} | ||
|
||
ExpressionTree receiver = ASTHelpers.getReceiver(tree); | ||
if (receiver instanceof MethodInvocationTree) { | ||
extractIdentificationTestCases((MethodInvocationTree) receiver, sink, state); | ||
} | ||
} | ||
|
||
private static void extractReplacementTestCases( | ||
MethodInvocationTree tree, List<TestEntry> sink, VisitorState state) { | ||
if (REPLACEMENT_OUTPUT_SOURCE_LINES.matches(tree, state)) { | ||
/* | ||
* Retrieve the method invocation that contains the input source code. Note that this cast | ||
* is safe, because this code is guarded by an earlier call to `#getClassUnderTest(..)`, | ||
* which ensures that `tree` is part of a longer method invocation chain. | ||
*/ | ||
MethodInvocationTree inputTree = (MethodInvocationTree) ASTHelpers.getReceiver(tree); | ||
|
||
String path = ASTHelpers.constValue(inputTree.getArguments().get(0), String.class); | ||
Optional<String> inputCode = getSourceCode(inputTree); | ||
if (path != null && inputCode.isPresent()) { | ||
Optional<String> outputCode = | ||
REPLACEMENT_EXPECT_UNCHANGED.matches(tree, state) ? inputCode : getSourceCode(tree); | ||
|
||
if (outputCode.isPresent() && !inputCode.equals(outputCode)) { | ||
sink.add( | ||
new AutoValue_BugPatternTestExtractor_ReplacementTestEntry( | ||
path, inputCode.orElseThrow(), outputCode.orElseThrow())); | ||
} | ||
} | ||
} | ||
|
||
ExpressionTree receiver = ASTHelpers.getReceiver(tree); | ||
if (receiver instanceof MethodInvocationTree) { | ||
extractReplacementTestCases((MethodInvocationTree) receiver, sink, state); | ||
} | ||
} | ||
|
||
// XXX: This logic is duplicated in `ErrorProneTestSourceFormat`. Can we do better? | ||
private static Optional<String> getSourceCode(MethodInvocationTree tree) { | ||
List<? extends ExpressionTree> sourceLines = | ||
tree.getArguments().subList(1, tree.getArguments().size()); | ||
StringBuilder source = new StringBuilder(); | ||
|
||
for (ExpressionTree sourceLine : sourceLines) { | ||
String value = ASTHelpers.constValue(sourceLine, String.class); | ||
if (value == null) { | ||
return Optional.empty(); | ||
} | ||
source.append(value).append('\n'); | ||
} | ||
|
||
return Optional.of(source.toString()); | ||
} | ||
} | ||
|
||
@AutoValue | ||
abstract static class TestCases { | ||
abstract String testClass(); | ||
|
||
abstract ImmutableList<TestCase> testCases(); | ||
} | ||
|
||
@AutoValue | ||
abstract static class TestCase { | ||
abstract String classUnderTest(); | ||
|
||
abstract ImmutableList<TestEntry> entries(); | ||
} | ||
|
||
interface TestEntry { | ||
String path(); | ||
} | ||
|
||
@AutoValue | ||
abstract static class ReplacementTestEntry implements TestEntry { | ||
abstract String input(); | ||
|
||
abstract String output(); | ||
} | ||
|
||
@AutoValue | ||
abstract static class IdentificationTestEntry implements TestEntry { | ||
abstract String code(); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As below, I think this should also be a fully qualified class name. Might not matter now, but it doesn't seem right to throw away this information, while later for reference/URL building purposes we may require it again.
(If this complicates subsequent scripting, then we can opt to additionally expose a
bugCheckerName
field.)