Skip to content

Commit

Permalink
Add a check to reformat poorly formatted EP tests.
Browse files Browse the repository at this point in the history
PiperOrigin-RevId: 678681902
  • Loading branch information
graememorgan authored and Error Prone Team committed Oct 8, 2024
1 parent cd6ae3b commit 63bcc5f
Show file tree
Hide file tree
Showing 4 changed files with 334 additions and 0 deletions.
6 changes: 6 additions & 0 deletions core/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,12 @@
<version>${project.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<!-- Apache 2.0 -->
<groupId>com.google.googlejavaformat</groupId>
<artifactId>google-java-format</artifactId>
<version>1.19.1</version>
</dependency>
<dependency>
<!-- MIT -->
<groupId>org.pcollections</groupId>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
/*
* Copyright 2024 The Error Prone Authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.google.errorprone.bugpatterns;

import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
import static com.google.errorprone.matchers.Description.NO_MATCH;
import static com.google.errorprone.matchers.Matchers.anyOf;
import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod;
import static com.google.errorprone.util.ASTHelpers.getStartPosition;
import static com.google.errorprone.util.ErrorProneTokens.getTokens;
import static com.google.errorprone.util.SourceVersion.supportsTextBlocks;
import static java.util.stream.Collectors.joining;

import com.google.common.base.Splitter;
import com.google.common.escape.Escaper;
import com.google.common.escape.Escapers;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.googlejavaformat.java.Formatter;
import com.google.googlejavaformat.java.FormatterException;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.LiteralTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.tools.javac.parser.Tokens.TokenKind;

/** See the summary. */
@BugPattern(
severity = SUGGESTION,
summary = "This test data will be more readable if correctly formatted.")
public final class MisformattedTestData extends BugChecker implements MethodInvocationTreeMatcher {
@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
// We're not only matching text blocks below, just taking the fact that it's a single literal
// argument containing source code as a sign that it should be formatted in a text block.
if (!supportsTextBlocks(state.context)) {
return NO_MATCH;
}
if (!ADD_SOURCE_CALL.matches(tree, state)) {
return NO_MATCH;
}
if (tree.getArguments().size() != 2) {
return NO_MATCH;
}
var sourceTree = tree.getArguments().get(1);
if (!(sourceTree instanceof LiteralTree)) {
return NO_MATCH;
}
var sourceValue = ((LiteralTree) sourceTree).getValue();
if (!(sourceValue instanceof String)) {
return NO_MATCH;
}

Formatter formatter = new Formatter();
String formattedSource;
try {
formattedSource = formatter.formatSource((String) sourceValue);
} catch (FormatterException exception) {
return NO_MATCH;
}
if (formattedSource.equals(sourceValue)) {
return NO_MATCH;
}
// This is a bit crude: but tokenize between the comma and the 2nd argument in order to work out
// an appropriate indent level for the text block. This is assuming that the source has already
// been formatted so that the arguments are nicely indented.
int startPos = state.getEndPosition(tree.getArguments().get(0));
int endPos = getStartPosition(tree.getArguments().get(1));
var tokens =
getTokens(
state.getSourceCode().subSequence(startPos, endPos).toString(),
startPos,
state.context);
var afterCommaPos =
tokens.reverse().stream()
.filter(t -> t.kind().equals(TokenKind.COMMA))
.findFirst()
.orElseThrow()
.endPos();
var spaces =
state.getSourceCode().subSequence(afterCommaPos, endPos).toString().replace("\n", "");
return describeMatch(
sourceTree,
SuggestedFix.replace(
sourceTree,
"\"\"\"\n"
+ LINE_SPLITTER
.splitToStream(escape(formattedSource))
.map(line -> line.isEmpty() ? "" : spaces + line)
.collect(joining("\n"))
+ spaces
+ "\"\"\""));
}

// TODO(ghm): Consider generalising this via an annotation.
private static final Matcher<ExpressionTree> ADD_SOURCE_CALL =
anyOf(
instanceMethod()
.onExactClass("com.google.errorprone.CompilationTestHelper")
.named("addSourceLines"),
instanceMethod()
.onExactClass("com.google.errorprone.BugCheckerRefactoringTestHelper")
.named("addInputLines"),
instanceMethod()
.onExactClass("com.google.errorprone.BugCheckerRefactoringTestHelper")
.named("addOutputLines"));

private static final Splitter LINE_SPLITTER = Splitter.on('\n');

private static String escape(String line) {
return ESCAPER.escape(line).replace("\"\"\"", "\\\"\"\"");
}

private static final Escaper ESCAPER = Escapers.builder().addEscape('\\', "\\\\").build();
}
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@
import com.google.errorprone.bugpatterns.MemberName;
import com.google.errorprone.bugpatterns.MemoizeConstantVisitorStateLookups;
import com.google.errorprone.bugpatterns.MethodCanBeStatic;
import com.google.errorprone.bugpatterns.MisformattedTestData;
import com.google.errorprone.bugpatterns.MissingBraces;
import com.google.errorprone.bugpatterns.MissingCasesInEnumSwitch;
import com.google.errorprone.bugpatterns.MissingDefault;
Expand Down Expand Up @@ -997,6 +998,7 @@ public static ScannerSupplier warningChecks() {
MalformedInlineTag.class,
MathAbsoluteNegative.class,
MemoizeConstantVisitorStateLookups.class,
MisformattedTestData.class,
MissingCasesInEnumSwitch.class,
MissingFail.class,
MissingImplementsComparable.class,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,194 @@
/*
* Copyright 2024 The Error Prone Authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.google.errorprone.bugpatterns;

import static com.google.common.truth.TruthJUnit.assume;
import static com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH;

import com.google.errorprone.BugCheckerRefactoringTestHelper;
import com.google.errorprone.CompilationTestHelper;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

@RunWith(JUnit4.class)
public final class MisformattedTestDataTest {
private final CompilationTestHelper compilationHelper =
CompilationTestHelper.newInstance(MisformattedTestData.class, getClass());
private final BugCheckerRefactoringTestHelper refactoringHelper =
BugCheckerRefactoringTestHelper.newInstance(MisformattedTestData.class, getClass());

@Test
public void alreadyFormatted_noFinding() {
assume().that(Runtime.version().feature()).isAtLeast(14);

compilationHelper
.addSourceLines(
"Test.java",
"""
import com.google.errorprone.BugCheckerRefactoringTestHelper;
class Test {
void method(BugCheckerRefactoringTestHelper h) {
h.addInputLines(
"Test.java",
\"""
package foo;

class Test {
void method() {
int a = 1;
}
}
\""");
}
}
""")
.doTest();
}

@Test
public void misformatted_suggestsFix() {
assume().that(Runtime.version().feature()).isAtLeast(14);

refactoringHelper
.addInputLines(
"Test.java",
"""
import com.google.errorprone.BugCheckerRefactoringTestHelper;
class Test {
void method(BugCheckerRefactoringTestHelper h) {
h.addInputLines(
"Test.java",
\"""
package foo;
class Test {
void method() {
int a =
1;
}
}
\""");
}
}
""")
.addOutputLines(
"Test.java",
"""
import com.google.errorprone.BugCheckerRefactoringTestHelper;
class Test {
void method(BugCheckerRefactoringTestHelper h) {
h.addInputLines(
"Test.java",
\"""
package foo;

class Test {
void method() {
int a = 1;
}
}
\""");
}
}
""")
.doTest(TEXT_MATCH);
}

@Test
public void onlyDiffersByIndentation_notReindented() {
assume().that(Runtime.version().feature()).isAtLeast(14);

refactoringHelper
.addInputLines(
"Test.java",
"""
import com.google.errorprone.BugCheckerRefactoringTestHelper;
class Test {
void method(BugCheckerRefactoringTestHelper h) {
h.addInputLines(
"Test.java",
\"""
package foo;

class Test {
void method() {
int a = 1;
}
}
""\");
}
}
""")
.expectUnchanged()
.doTest(TEXT_MATCH);
}
@Test
public void escapesSpecialCharacters() {
assume().that(Runtime.version().feature()).isAtLeast(14);
refactoringHelper
.addInputLines(
"Test.java",
"""
import com.google.errorprone.BugCheckerRefactoringTestHelper;

class Test {
void method(BugCheckerRefactoringTestHelper h) {
h.addInputLines(
"Test.java",
\"""
package foo;
class Test {
void method() {
var foo
= "foo\\\\nbar";
}
}
\""");
}
}
""")
.addOutputLines(
"Test.java",
"""
import com.google.errorprone.BugCheckerRefactoringTestHelper;

class Test {
void method(BugCheckerRefactoringTestHelper h) {
h.addInputLines(
"Test.java",
\"""
package foo;
class Test {
void method() {
var foo = "foo\\\\nbar";
}
}
\""");
}
}
""")
.doTest(TEXT_MATCH);
}
}

0 comments on commit 63bcc5f

Please sign in to comment.