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

SONARJAVA-4424 add methods annotated with @Test to the list of testNg test methods #4953

Merged
merged 6 commits into from
Dec 11, 2024

Conversation

erwan-serandour
Copy link
Contributor

@erwan-serandour erwan-serandour commented Dec 6, 2024

SONARJAVA-4424

Part of

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice work. There is a couple of things to revisit here:

  1. Simplify the lambda to find test members in a TestNG class
  2. Clarify which test case is raising a FP here (and if it cannot be fixed, clearly mark it as an FP when semantic is incomplete)

@@ -122,6 +122,16 @@ public class TestNGClassTest { // Noncompliant
private void test1() { }
public static void foo() {}
}

@org.testng.annotations.Test
public class TestNGClassTestUseAnnotation {

Choose a reason for hiding this comment

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

We can change the name of the class to TestNGDoesNotRequirePublicTestMethods or add a compliant comment explaining why this case is not an issue

Comment on lines 116 to 120
if (members.noneMatch(member -> {
boolean annotatedWithTest = isTestFieldOrMethod(member);
boolean publicMethod = member.isMethodSymbol() && member.isPublic() && !member.isStatic() && !"<init>".equals(member.name());
return annotatedWithTest || publicMethod;
})) {

Choose a reason for hiding this comment

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

Can we extract the second test into its own method and simplify the code?

Suggested change
if (members.noneMatch(member -> {
boolean annotatedWithTest = isTestFieldOrMethod(member);
boolean publicMethod = member.isMethodSymbol() && member.isPublic() && !member.isStatic() && !"<init>".equals(member.name());
return annotatedWithTest || publicMethod;
})) {
if (members.noneMatch(member -> isTestFieldOrMethod(member) || isPublicInstanceMethod(member)) {

Comment on lines 123 to 141
boolean annotatedWithTest = member.isMethodSymbol() && member.metadata().annotations().stream().anyMatch(isTestNgAnnotation);
boolean publicMethod = member.isMethodSymbol() && member.isPublic() && !member.isStatic() && !"<init>".equals(member.name());
return annotatedWithTest || publicMethod;

Choose a reason for hiding this comment

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

It looks like we are only testing on methods because we have isMethodSymbol in both branches. Can we return early if the member is not a method?

Suggested change
boolean annotatedWithTest = member.isMethodSymbol() && member.metadata().annotations().stream().anyMatch(isTestNgAnnotation);
boolean publicMethod = member.isMethodSymbol() && member.isPublic() && !member.isStatic() && !"<init>".equals(member.name());
return annotatedWithTest || publicMethod;
if (!member.isMethodSymbol()) {
return false;
}
boolean annotatedWithTest = member.metadata().annotations().stream().anyMatch(isTestNgAnnotation);
boolean publicMethod = member.isPublic() && !member.isStatic() && !"<init>".equals(member.name());
return annotatedWithTest || publicMethod;

@erwan-serandour erwan-serandour merged commit 565282b into master Dec 11, 2024
16 of 17 checks passed
@erwan-serandour erwan-serandour deleted the erwan/S2187 branch December 11, 2024 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants