jlengrand/error-prone-support
1
0
Fork 0
mirror of https://github.com/jlengrand/error-prone-support.git synced 2026-03-10 15:49:33 +00:00
Code Issues Packages Projects Releases Wiki Activity

Suggestions and add XXXs

Browse Source
This commit is contained in:
Rick Ossendrijver
2023-01-27 17:15:40 +01:00
parent 2c3ca2f885
commit 8a032bea18
10 changed files with 58 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/testngtojunit/ArgumentMigrator.java
View File

@@ -6,6 +6,21 @@ import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodTree;
import java.util.Optional;
// XXX: General feedback not specific to this class.
// Make sure to:
// - Check if everything needs to be `public`. I suspect most things can actually be package
// private.
// That makes it easier for yourself because public things should be documented with Javadoc :).
// - Make sure there is a completely green build, now `mvn clean install` still gives problems.
// - For some classes it would still be nice to have some Javadoc nonetheless.
// - Maybe we should start working on a README where we document a few things.
// 1. What does is and how it works.
// 2. How to use this.
// 3. The script used to perform the migration?
// 4. Maybe we can think of something else? Things not supported?
// - We have kind of "integration tests" for the `TestNGScanner` which indirectly tests the
// `ArgumentMigrator`s but we should probably add tests for the separate `ArgumentMigrator`s as
// well.
public interface ArgumentMigrator {
Optional<SuggestedFix> createFix(
TestNGMigrationContext context,

4
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/testngtojunit/SupportedArgumentKind.java
View File

@@ -2,20 +2,22 @@ package tech.picnic.errorprone.bugpatterns.testngtojunit;
import static java.util.Arrays.stream;
import com.google.errorprone.annotations.Immutable;
import java.util.Optional;
import tech.picnic.errorprone.bugpatterns.testngtojunit.migrators.DataProviderArgumentMigrator;
import tech.picnic.errorprone.bugpatterns.testngtojunit.migrators.DescriptionArgumentMigrator;
import tech.picnic.errorprone.bugpatterns.testngtojunit.migrators.ExpectedExceptionsArgumentMigrator;
import tech.picnic.errorprone.bugpatterns.testngtojunit.migrators.PriorityArgumentMigrator;
@Immutable
public enum SupportedArgumentKind {
PRIORITY("priority", new PriorityArgumentMigrator()),
DESCRIPTION("description", new DescriptionArgumentMigrator()),
DATAPROVIDER("dataProvider", new DataProviderArgumentMigrator()),
EXPECTED_EXCEPTIONS("expectedExceptions", new ExpectedExceptionsArgumentMigrator());
private final String name;
@SuppressWarnings("ImmutableEnumChecker" /* `SupportedArgumentKind` is effectively immutable. */)
private final ArgumentMigrator argumentMigrator;
SupportedArgumentKind(String name, ArgumentMigrator argumentMigrator) {

5
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/testngtojunit/TestNGJUnitMigration.java
View File

@@ -60,7 +60,7 @@ public final class TestNGJUnitMigration extends BugChecker implements Compilatio
TestNGMigrationContext context =
new TestNGMigrationContext(isAggressiveMode(state), metaData.getClassTree());
// make sure we ALL tests in the class can be migrated
/* Make sure ALL tests in the class can be migrated. */
if (!context.isAggressiveMigration()
&& !metaData.getAnnotations().stream()
.allMatch(annotation -> canMigrateTest(context, annotation))) {
@@ -141,6 +141,9 @@ public final class TestNGJUnitMigration extends BugChecker implements Compilatio
.flatMap(fixer -> fixer.createFix(context, methodTree, argumentContent, state));
}
// XXX: This should happen in the constructor just like `RequestParamType`.
// XXX: Nit: instead of "aggressiveMode" I would flip it and call it "conservativeMode". That's
// how its called in Error Prone as well.
private static boolean isAggressiveMode(VisitorState state) {
return state
.errorProneOptions()

9
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/testngtojunit/TestNGUtil.java → error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/testngtojunit/TestNGMatchers.java
View File

@@ -7,12 +7,15 @@ import com.google.errorprone.matchers.Matcher;
import com.sun.source.tree.AnnotationTree;
import com.sun.source.tree.MethodTree;
public final class TestNGUtil {
// XXX: It would be awesome to have a test for this class as well. It should be very similar to
// `MoreJUnitMatchersTest`.
public final class TestNGMatchers {
public static final Matcher<AnnotationTree> TESTNG_ANNOTATION =
isType("org.testng.annotations.Test");
public static final Matcher<MethodTree> VALUE_FACTORY_METHOD =
// XXX: Should `VALUE` be in this name?
public static final Matcher<MethodTree> TESTNG_VALUE_FACTORY_METHOD =
hasAnnotation("org.testng.annotations.DataProvider");
private TestNGUtil() {}
private TestNGMatchers() {}
}

13
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/testngtojunit/TestNGMetadata.java
View File

@@ -12,11 +12,10 @@ import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import org.jspecify.annotations.Nullable;
public class TestNGMetadata {
public final class TestNGMetadata {
private final ClassTree classTree;
private TestNGMetadata.@Nullable Annotation classLevelAnnotation;
private TestNGMetadata.Annotation classLevelAnnotation;
private final Map<MethodTree, Annotation> methodAnnotations = new HashMap<>();
private final List<DataProvider> dataProviders = new ArrayList<>();
@@ -56,6 +55,14 @@ public class TestNGMetadata {
return ImmutableList.copyOf(dataProviders);
}
// XXX: Hmm not sure if `Annotation` is the ideal name for this. It might be rather confusing IMO.
// Now it's not clear this is our own object and what it actual does? Now it's used with
// `TestNGMetaData.Annotation` however it should also be clear without having to prefix it with
// the qualifier. Maybe adding the `MetaData` suffix to this would clarify a lot (also for
// `DataProvider` class).
// XXX: It would be nice to also use `@AutoValue` for this. This is kind of the "default" for
// creating POJO's in EPS. For other examples, see the PR that is open that is about introducing
// `documentation-support`.
public static class Annotation {
private final AnnotationTree annotationTree;
private final ImmutableMap<String, ExpressionTree> arguments;

4
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/testngtojunit/TestNGMigrationContext.java
View File

@@ -4,7 +4,9 @@ import com.sun.source.tree.ClassTree;
import java.util.HashMap;
import java.util.Map;
public class TestNGMigrationContext {
// XXX: Same goes for this class, it should be using `@AutoValue`. See my other comment about this
// for context.
public final class TestNGMigrationContext {
private final boolean aggressiveMigration;
private final ClassTree classTree;
private final Map<String, MigrationState> migratedDataProviders = new HashMap<>();

8
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/testngtojunit/TestNGScanner.java
View File

@@ -1,8 +1,8 @@
package tech.picnic.errorprone.bugpatterns.testngtojunit;
import static com.google.common.collect.ImmutableMap.toImmutableMap;
import static tech.picnic.errorprone.bugpatterns.testngtojunit.TestNGUtil.TESTNG_ANNOTATION;
import static tech.picnic.errorprone.bugpatterns.testngtojunit.TestNGUtil.VALUE_FACTORY_METHOD;
import static tech.picnic.errorprone.bugpatterns.testngtojunit.TestNGMatchers.TESTNG_ANNOTATION;
import static tech.picnic.errorprone.bugpatterns.testngtojunit.TestNGMatchers.TESTNG_VALUE_FACTORY_METHOD;
import com.google.common.collect.ImmutableMap;
import com.google.errorprone.VisitorState;
@@ -21,7 +21,7 @@ import org.jspecify.annotations.Nullable;
* collect data required for the migration from each class in the compilation unit. <br>
* This data can be retrieved using {@link #buildMetaDataTree()}.
*/
public class TestNGScanner extends TreeScanner<@Nullable Void, TestNGMetadata> {
public final class TestNGScanner extends TreeScanner<@Nullable Void, TestNGMetadata> {
private final VisitorState state;
private final ImmutableMap.Builder<ClassTree, TestNGMetadata> metadataBuilder =
ImmutableMap.builder();
@@ -46,7 +46,7 @@ public class TestNGScanner extends TreeScanner<@Nullable Void, TestNGMetadata> {
return super.visitMethod(tree, testNGMetadata);
}
if (VALUE_FACTORY_METHOD.matches(tree, state)) {
if (TESTNG_VALUE_FACTORY_METHOD.matches(tree, state)) {
testNGMetadata.addDataProvider(tree);
return super.visitMethod(tree, testNGMetadata);
}

2
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/testngtojunit/migrators/DataProviderArgumentMigrator.java
View File

@@ -61,7 +61,7 @@ public class DataProviderArgumentMigrator implements ArgumentMigrator {
}
private static String getDataProviderName(ExpressionTree expressionTree) {
return (String) ((LiteralTree) expressionTree).getValue();
return ((LiteralTree) expressionTree).getValue().toString();
}
private static Optional<SuggestedFix> fixValueFactory(

4
error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/testngtojunit/TestNGJUnitMigrationTest.java
View File

@@ -134,6 +134,10 @@ final class TestNGJUnitMigrationTest {
"import org.junit.jupiter.params.ParameterizedTest;",
"import org.junit.jupiter.params.provider.Arguments;",
"import org.junit.jupiter.params.provider.MethodSource;",
// XXX: Running `mvn clean install` (in this case still with `-Dverification.warn`) will
// show that the `org.testng.annotation.Test` import is still here. I think that makes
// sense, we indeed agreed on not deleting that and letting other checks take care of
// that. So we should probably fully qualify the Jupiter
"",
"@TestMethodOrder(MethodOrderer.OrderAnnotation.class)",
"class A {",

14
error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/testngtojunit/TestNGScannerTest.java
View File

@@ -10,6 +10,7 @@ import com.google.errorprone.BugPattern;
import com.google.errorprone.CompilationTestHelper;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher;
import com.google.errorprone.matchers.Description;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.CompilationUnitTree;
@@ -19,12 +20,14 @@ import org.junit.jupiter.api.Test;
import tech.picnic.errorprone.bugpatterns.util.SourceCode;
final class TestNGScannerTest {
private final CompilationTestHelper compilationTestHelper =
CompilationTestHelper.newInstance(TestNGScannerTest.TestChecker.class, getClass());
// XXX: We are missing some tests here.
// - Nested class.
// - JUnit class that is already migrated.
// - Normal class that shouldn't trigger? Or a negative example at least.
// - Some not supported things as well.
@Test
void classLevelAndMethodLevel() {
compilationTestHelper
CompilationTestHelper.newInstance(TestChecker.class, getClass())
.addSourceLines(
"A.java",
"import org.testng.annotations.DataProvider;",
@@ -76,8 +79,7 @@ final class TestNGScannerTest {
* collected.
*/
@BugPattern(severity = ERROR, summary = "Interacts with `TestNGScanner` for testing purposes")
public static final class TestChecker extends BugChecker
implements BugChecker.CompilationUnitTreeMatcher {
public static final class TestChecker extends BugChecker implements CompilationUnitTreeMatcher {
private static final long serialVersionUID = 1L;
@Override