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

Introduce RefasterRuleTestExtractor for documentation generation #1317

Merged
merged 5 commits into from
Sep 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 9 additions & 0 deletions documentation-support/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,15 @@
<artifactId>error_prone_test_helpers</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>error-prone-utils</artifactId>
</dependency>
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>refaster-test-support</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-annotations</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
import java.util.List;
import java.util.Optional;
import org.jspecify.annotations.Nullable;
import tech.picnic.errorprone.documentation.BugPatternTestExtractor.TestCases;
import tech.picnic.errorprone.documentation.BugPatternTestExtractor.BugPatternTestCases;

/**
* An {@link Extractor} that describes how to extract data from classes that test a {@code
Expand All @@ -40,7 +40,7 @@
@Immutable
@AutoService(Extractor.class)
@SuppressWarnings("rawtypes" /* See https://github.com/google/auto/issues/870. */)
public final class BugPatternTestExtractor implements Extractor<TestCases> {
public final class BugPatternTestExtractor implements Extractor<BugPatternTestCases> {
Copy link
Member Author

Choose a reason for hiding this comment

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

Renaming for disambiguation.

/** Instantiates a new {@link BugPatternTestExtractor} instance. */
public BugPatternTestExtractor() {}

Expand All @@ -50,7 +50,7 @@ public String identifier() {
}

@Override
public Optional<TestCases> tryExtract(ClassTree tree, VisitorState state) {
public Optional<BugPatternTestCases> tryExtract(ClassTree tree, VisitorState state) {
BugPatternTestCollector collector = new BugPatternTestCollector();

collector.scan(tree, state);
Expand All @@ -59,7 +59,7 @@ public Optional<TestCases> tryExtract(ClassTree tree, VisitorState state) {
.filter(not(ImmutableList::isEmpty))
.map(
tests ->
new AutoValue_BugPatternTestExtractor_TestCases(
new AutoValue_BugPatternTestExtractor_BugPatternTestCases(
state.getPath().getCompilationUnit().getSourceFile().toUri(),
ASTHelpers.getSymbol(tree).className(),
tests));
Expand Down Expand Up @@ -95,10 +95,10 @@ private static final class BugPatternTestCollector
.onDescendantOf("com.google.errorprone.BugCheckerRefactoringTestHelper.ExpectOutput")
.namedAnyOf("addOutputLines", "expectUnchanged");

private final List<TestCase> collectedTestCases = new ArrayList<>();
private final List<BugPatternTestCase> collectedBugPatternTestCases = new ArrayList<>();

private ImmutableList<TestCase> getCollectedTests() {
return ImmutableList.copyOf(collectedTestCases);
private ImmutableList<BugPatternTestCase> getCollectedTests() {
return ImmutableList.copyOf(collectedBugPatternTestCases);
}

@Override
Expand All @@ -110,14 +110,14 @@ private ImmutableList<TestCase> getCollectedTests() {
classUnderTest -> {
List<TestEntry> entries = new ArrayList<>();
if (isReplacementTest) {
extractReplacementTestCases(node, entries, state);
extractReplacementBugPatternTestCases(node, entries, state);
} else {
extractIdentificationTestCases(node, entries, state);
extractIdentificationBugPatternTestCases(node, entries, state);
}

if (!entries.isEmpty()) {
collectedTestCases.add(
new AutoValue_BugPatternTestExtractor_TestCase(
collectedBugPatternTestCases.add(
new AutoValue_BugPatternTestExtractor_BugPatternTestCase(
classUnderTest, ImmutableList.copyOf(entries).reverse()));
}
});
Expand All @@ -140,7 +140,7 @@ private static Optional<String> getClassUnderTest(
: Optional.empty();
}

private static void extractIdentificationTestCases(
private static void extractIdentificationBugPatternTestCases(
MethodInvocationTree tree, List<TestEntry> sink, VisitorState state) {
if (IDENTIFICATION_SOURCE_LINES.matches(tree, state)) {
String path = ASTHelpers.constValue(tree.getArguments().get(0), String.class);
Expand All @@ -155,11 +155,11 @@ private static void extractIdentificationTestCases(

ExpressionTree receiver = ASTHelpers.getReceiver(tree);
if (receiver instanceof MethodInvocationTree methodInvocation) {
extractIdentificationTestCases(methodInvocation, sink, state);
extractIdentificationBugPatternTestCases(methodInvocation, sink, state);
}
}

private static void extractReplacementTestCases(
private static void extractReplacementBugPatternTestCases(
MethodInvocationTree tree, List<TestEntry> sink, VisitorState state) {
if (REPLACEMENT_OUTPUT_SOURCE_LINES.matches(tree, state)) {
/*
Expand All @@ -185,7 +185,7 @@ private static void extractReplacementTestCases(

ExpressionTree receiver = ASTHelpers.getReceiver(tree);
if (receiver instanceof MethodInvocationTree methodInvocation) {
extractReplacementTestCases(methodInvocation, sink, state);
extractReplacementBugPatternTestCases(methodInvocation, sink, state);
}
}

Expand All @@ -208,24 +208,26 @@ private static Optional<String> getSourceCode(MethodInvocationTree tree) {
}

@AutoValue
@JsonDeserialize(as = AutoValue_BugPatternTestExtractor_TestCases.class)
abstract static class TestCases {
static TestCases create(URI source, String testClass, ImmutableList<TestCase> testCases) {
return new AutoValue_BugPatternTestExtractor_TestCases(source, testClass, testCases);
@JsonDeserialize(as = AutoValue_BugPatternTestExtractor_BugPatternTestCases.class)
abstract static class BugPatternTestCases {
static BugPatternTestCases create(
URI source, String testClass, ImmutableList<BugPatternTestCase> testCases) {
return new AutoValue_BugPatternTestExtractor_BugPatternTestCases(
source, testClass, testCases);
}

abstract URI source();

abstract String testClass();

abstract ImmutableList<TestCase> testCases();
abstract ImmutableList<BugPatternTestCase> testCases();
}

@AutoValue
@JsonDeserialize(as = AutoValue_BugPatternTestExtractor_TestCase.class)
abstract static class TestCase {
static TestCase create(String classUnderTest, ImmutableList<TestEntry> entries) {
return new AutoValue_BugPatternTestExtractor_TestCase(classUnderTest, entries);
@JsonDeserialize(as = AutoValue_BugPatternTestExtractor_BugPatternTestCase.class)
abstract static class BugPatternTestCase {
static BugPatternTestCase create(String classUnderTest, ImmutableList<TestEntry> entries) {
return new AutoValue_BugPatternTestExtractor_BugPatternTestCase(classUnderTest, entries);
}

abstract String classUnderTest();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,176 @@
package tech.picnic.errorprone.documentation;

import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.errorprone.matchers.Matchers.isSubtypeOf;
import static java.util.stream.Collectors.joining;

import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.google.auto.service.AutoService;
import com.google.auto.value.AutoValue;
import com.google.common.base.Splitter;
import com.google.common.base.Supplier;
import com.google.common.base.VerifyException;
import com.google.common.collect.ImmutableList;
import com.google.errorprone.VisitorState;
import com.google.errorprone.annotations.FormatMethod;
import com.google.errorprone.annotations.Immutable;
import com.google.errorprone.matchers.Matcher;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.MethodTree;
import java.net.URI;
import java.util.Optional;
import java.util.regex.Pattern;
import tech.picnic.errorprone.documentation.RefasterRuleCollectionTestExtractor.RefasterTestCases;
import tech.picnic.errorprone.utils.SourceCode;

/**
* An {@link Extractor} that describes how to extract data from Refaster rule input and output test
* classes.
*/
// XXX: Drop this extractor if/when the Refaster test framework is reimplemented such that tests can
// be located alongside rules, rather than in two additional resource files as currently required by
// `RefasterRuleCollection`.
@Immutable
@AutoService(Extractor.class)
@SuppressWarnings("rawtypes" /* See https://github.com/google/auto/issues/870. */)
public final class RefasterRuleCollectionTestExtractor implements Extractor<RefasterTestCases> {
private static final Matcher<ClassTree> IS_REFASTER_RULE_COLLECTION_TEST_CASE =
isSubtypeOf("tech.picnic.errorprone.refaster.test.RefasterRuleCollectionTestCase");
private static final Pattern TEST_CLASS_NAME_PATTERN = Pattern.compile("(.*)Test");
private static final Pattern TEST_CLASS_FILE_NAME_PATTERN =
Pattern.compile(".*(Input|Output)\\.java");
private static final Pattern TEST_METHOD_NAME_PATTERN = Pattern.compile("test(.*)");
private static final String LINE_SEPARATOR = "\n";
private static final Splitter LINE_SPLITTER = Splitter.on(LINE_SEPARATOR);

/** Instantiates a new {@link RefasterRuleCollectionTestExtractor} instance. */
public RefasterRuleCollectionTestExtractor() {}

@Override
public String identifier() {
return "refaster-rule-collection-test";
}

@Override
public Optional<RefasterTestCases> tryExtract(ClassTree tree, VisitorState state) {
if (!IS_REFASTER_RULE_COLLECTION_TEST_CASE.matches(tree, state)) {
return Optional.empty();
}

URI sourceFile = state.getPath().getCompilationUnit().getSourceFile().toUri();
return Optional.of(
RefasterTestCases.create(
sourceFile,
getRuleCollectionName(tree),
isInputFile(sourceFile),
getRefasterTestCases(tree, state)));
}

private static String getRuleCollectionName(ClassTree tree) {
String className = tree.getSimpleName().toString();

// XXX: Instead of throwing an error here, it'd be nicer to have a bug checker validate key
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I am missing something, you can also argue that throwing an error provides faster feedback when developing / running tests locally.

Copy link
Member Author

Choose a reason for hiding this comment

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

The bug checker would also run at compile time, but would provide a (much) better error message.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense 👍

// aspects of `RefasterRuleCollectionTestCase` subtypes.
return tryExtractPatternGroup(className, TEST_CLASS_NAME_PATTERN)
.orElseThrow(
violation(
"Refaster rule collection test class name '%s' does not match '%s'",
className, TEST_CLASS_NAME_PATTERN));
}

private static boolean isInputFile(URI sourceFile) {
String path = sourceFile.getPath();

// XXX: Instead of throwing an error here, it'd be nicer to have a bug checker validate key
// aspects of `RefasterRuleCollectionTestCase` subtypes.
return "Input"
.equals(
tryExtractPatternGroup(path, TEST_CLASS_FILE_NAME_PATTERN)
.orElseThrow(
violation(
"Refaster rule collection test file name '%s' does not match '%s'",
path, TEST_CLASS_FILE_NAME_PATTERN)));
}

private static ImmutableList<RefasterTestCase> getRefasterTestCases(
ClassTree tree, VisitorState state) {
return tree.getMembers().stream()
.filter(MethodTree.class::isInstance)
.map(MethodTree.class::cast)
.flatMap(m -> tryExtractRefasterTestCase(m, state).stream())
.collect(toImmutableList());
}

private static Optional<RefasterTestCase> tryExtractRefasterTestCase(
MethodTree method, VisitorState state) {
return tryExtractPatternGroup(method.getName().toString(), TEST_METHOD_NAME_PATTERN)
.map(name -> RefasterTestCase.create(name, getFormattedSource(method, state)));
}

/**
* Returns the source code for the specified method.
*
* @implNote This operation attempts to trim leading whitespace, such that the start and end of
* the method declaration are aligned. The implemented heuristic assumes that the code is
* formatted using Google Java Format.
*/
// XXX: Leading Javadoc and other comments are currently not extracted. Consider fixing this.
private static String getFormattedSource(MethodTree method, VisitorState state) {
String source = SourceCode.treeToString(method, state);
int finalNewline = source.lastIndexOf(LINE_SEPARATOR);
if (finalNewline < 0) {

Check warning on line 121 in documentation-support/src/main/java/tech/picnic/errorprone/documentation/RefasterRuleCollectionTestExtractor.java

View workflow job for this annotation

GitHub Actions / pitest

2 different changes can be made to line 121 without causing a test to fail

changed conditional boundary (covered by 2 tests ConditionalsBoundaryMutator) removed conditional - replaced comparison check with false (covered by 2 tests RemoveConditionalMutator_ORDER_ELSE)
Copy link
Member Author

Choose a reason for hiding this comment

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

Pitest flags that this case isn't covered. There is actually an empty method test, but I suppose that source code does have a trailing newline (didn't check).

Copy link
Member

Choose a reason for hiding this comment

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

Usually with checks like this -1 is returned if it is not found, it might be slightly more inuitive is we say == -1 instead of < 0, although it's the same, WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I generally prefer < 0, because any negative value isn't a valid index. We do this in a bunch of other places too:

$ git grep -i 'index.*< 0' | wc -l
8

Copy link
Member Author

Choose a reason for hiding this comment

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

Rebased and reverted this change as discussed offline.

return source;
}

int indentation = Math.max(0, source.lastIndexOf(' ') - finalNewline);
String prefixToStrip = " ".repeat(indentation);

Check warning on line 126 in documentation-support/src/main/java/tech/picnic/errorprone/documentation/RefasterRuleCollectionTestExtractor.java

View workflow job for this annotation

GitHub Actions / pitest

A change can be made to line 126 without causing a test to fail

removed call to repeat (covered by 2 tests RemoveChainedCallsMutator)
Copy link
Member Author

Choose a reason for hiding this comment

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

Pitest flags that the .repeat(indentation) logic isn't covered. This is because the test code is properly formatted; not an issue I suppose. (I did manually check that unformatted code doesn't break the extractor.)


return LINE_SPLITTER
.splitToStream(source)
.map(line -> line.startsWith(prefixToStrip) ? line.substring(indentation) : line)
.collect(joining(LINE_SEPARATOR));
}

private static Optional<String> tryExtractPatternGroup(String input, Pattern pattern) {
java.util.regex.Matcher matcher = pattern.matcher(input);
return matcher.matches() ? Optional.of(matcher.group(1)) : Optional.empty();
}

@FormatMethod
private static Supplier<VerifyException> violation(String format, Object... args) {
return () -> new VerifyException(String.format(format, args));
}

@AutoValue
@JsonDeserialize(as = AutoValue_RefasterRuleCollectionTestExtractor_RefasterTestCases.class)
abstract static class RefasterTestCases {
static RefasterTestCases create(
URI source,
String ruleCollection,
boolean isInput,
ImmutableList<RefasterTestCase> testCases) {
return new AutoValue_RefasterRuleCollectionTestExtractor_RefasterTestCases(
source, ruleCollection, isInput, testCases);
}

abstract URI source();

abstract String ruleCollection();

abstract boolean isInput();

abstract ImmutableList<RefasterTestCase> testCases();
}

@AutoValue
@JsonDeserialize(as = AutoValue_RefasterRuleCollectionTestExtractor_RefasterTestCase.class)
abstract static class RefasterTestCase {
static RefasterTestCase create(String name, String content) {
return new AutoValue_RefasterRuleCollectionTestExtractor_RefasterTestCase(name, content);

Check warning on line 169 in documentation-support/src/main/java/tech/picnic/errorprone/documentation/RefasterRuleCollectionTestExtractor.java

View workflow job for this annotation

GitHub Actions / pitest

A change can be made to line 169 without causing a test to fail

swapped parameters 1 and 2 in call to <init> (covered by 2 tests ParamSwapMutator)
Copy link
Member Author

Choose a reason for hiding this comment

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

Pitest flags that these parameters can be swapped without failing a test. That's because we only validate that the type can be (de)serialized, without asserting the exact serialization format. That's a conscious choice. The final PR will improve coverage in this respect.

}

abstract String name();

abstract String content();
}
}
Loading