Skip to content

Simplify annotation handling in SpringJUnit5Check #157

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

Simplify annotation handling in SpringJUnit5Check #157

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
152e98d
Fixed test with bad .java file(Wrong imports and annotations)
Dec 15, 2019
0f93b8c
Removed redundant code(as containsAnnotation doesn't find FQN annotat…
Dec 15, 2019
1437466
Removed other redundant code
Dec 16, 2019
75ac65f
Add junit test const
Feb 20, 2020
6b5df77
Revert "Fixed test with bad .java file(Wrong imports and annotations)…
Feb 26, 2020
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
45 changes: 15 additions & 30 deletions ...mat-checkstyle/src/main/java/io/spring/javaformat/checkstyle/check/SpringJUnit5Check.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,30 +37,23 @@
* @author Phillip Webb
*/
public class SpringJUnit5Check extends AbstractSpringCheck {

private static final String JUNIT4_TEST_ANNOTATION = "org.junit.Test";

private static final List<String> TEST_ANNOTATIONS;
static {
Set<String> annotations = new LinkedHashSet<>();
addAnnotation(annotations, JUNIT4_TEST_ANNOTATION);
addAnnotation(annotations, "org.junit.jupiter.api.RepeatedTest");
addAnnotation(annotations, "org.junit.jupiter.api.Test");
addAnnotation(annotations, "org.junit.jupiter.api.TestFactory");
addAnnotation(annotations, "org.junit.jupiter.api.TestTemplate");
addAnnotation(annotations, "org.junit.jupiter.params.ParameterizedTest");
TEST_ANNOTATIONS = Collections.unmodifiableList(new ArrayList<>(annotations));
}

private static final List<String> LIFECYCLE_ANNOTATIONS;
static {
Set<String> annotations = new LinkedHashSet<>();
addAnnotation(annotations, "org.junit.jupiter.api.BeforeAll");
addAnnotation(annotations, "org.junit.jupiter.api.BeforeEach");
addAnnotation(annotations, "org.junit.jupiter.api.AfterAll");
addAnnotation(annotations, "org.junit.jupiter.api.AfterEach");
LIFECYCLE_ANNOTATIONS = Collections.unmodifiableList(new ArrayList<>(annotations));
}
private static final List<String> TEST_ANNOTATIONS = Collections.unmodifiableList(Arrays.asList(
"RepeatedTest",
"Test",
"TestFactory",
"TestTemplate",
"ParameterizedTest"
));

private static final List<String> LIFECYCLE_ANNOTATIONS = Collections.unmodifiableList(Arrays.asList(
"BeforeAll",
"BeforeEach",
"AfterAll",
"AfterEach"
)
);

private static final Set<String> BANNED_IMPORTS;
static {
Expand All @@ -75,14 +68,6 @@ public class SpringJUnit5Check extends AbstractSpringCheck {
BANNED_IMPORTS = Collections.unmodifiableSet(bannedImports);
}

private static void addAnnotation(Set<String> annotations, String annotation) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that removing this method and the usage of it will introduce a regression. The javadoc for AnnotationUtil.containsAnnotation(DetailAst, List<String>) says that it "accepts both simple and fully-qualified names". If a user has annotated a method with, for example, @org.junit.Test rather than importing org.junit.test and annotating with @Test, this change will cause the usage to be missed.

Copy link
Author

Choose a reason for hiding this comment

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

This method accepts both simple and fully-qualified names, e.g. "Override" will match both java.lang.Override and Override.

Which means that if we are using only the non FQN then we receive a check for both, there is also a test for that:
JUnit5BadAnnotation

Copy link
Contributor

Choose a reason for hiding this comment

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

if we are using only the non FQN

I don't think we can't be certain that this is the case. One user could write @org.junit.Test while another could write @Test along with import org.junit.Test.

Copy link
Author

Choose a reason for hiding this comment

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

What I meant is that I believe it catches both

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I see what you mean now. I agree that just the simple names should be sufficient.

We try to keep issues and pull requests focused on a single change. To that end, would you mind splitting the corrections to JUnit5BadModifier and the simplifications above into separate pull requests?

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I have updated this one to be the refactor, I'll create a new one for the test

annotations.add(annotation);
int lastDot = annotation.lastIndexOf(".");
if (lastDot != -1) {
annotations.add(annotation.substring(lastDot + 1));
}
}

private List<String> unlessImports = new ArrayList<>();

private final List<DetailAST> testMethods = new ArrayList<>();
Expand Down