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.

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

SONARJAVA-4446 Check if the parent class or implemented interface symbols of the test class are known. #4956

Merged
merged 10 commits into from
Dec 10, 2024
Merged

SONARJAVA-4446 Check if the parent class or implemented interface symbols of the test class are known. #4956

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
696dc58
save
erwan-serandour Dec 9, 2024
5272e3e
add test case and don't report errors when parents are not known
erwan-serandour Dec 9, 2024
1d1b1c7
reach full coverage
erwan-serandour Dec 9, 2024
49d8434
fix autoscan
erwan-serandour Dec 9, 2024
f5d1b45
fix autoscan tests
erwan-serandour Dec 10, 2024
95e559d
update after code review
erwan-serandour Dec 10, 2024
221771e
update after code review
erwan-serandour Dec 10, 2024
b1d8595
update after code review
erwan-serandour Dec 10, 2024
c0588fe
update after code review
erwan-serandour Dec 10, 2024
7899e97
update after code review
erwan-serandour Dec 10, 2024
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
2 changes: 1 addition & 1 deletion its/autoscan/src/test/java/org/sonar/java/it/AutoScanTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ public void javaCheckTestSources() throws Exception {
SoftAssertions softly = new SoftAssertions();
softly.assertThat(newDiffs).containsExactlyInAnyOrderElementsOf(knownDiffs.values());
softly.assertThat(newTotal).isEqualTo(knownTotal);
softly.assertThat(rulesCausingFPs).hasSize(11);
softly.assertThat(rulesCausingFPs).hasSize(10);
softly.assertThat(rulesNotReporting).hasSize(10);

/**
Expand Down
4 changes: 2 additions & 2 deletions its/autoscan/src/test/resources/autoscan/diffs/diff_S2187.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"ruleKey": "S2187",
"hasTruePositives": true,
"falseNegatives": 12,
"falsePositives": 1
"falseNegatives": 13,
"falsePositives": 0
}
12 changes: 12 additions & 0 deletions ...est-sources/default/src/main/files/non-compiling/checks/NoTestInTestClassCheckSample.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,18 @@
import com.googlecode.zohhak.api.TestWith;
import com.googlecode.zohhak.api.runners.ZohhakRunner;

class SubclassTest extends ParentTestClass { // Compliant, we cannot know what is in ParentTestClass so we don't raise issues
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we change the name so it suggests that the class is not defined, for example, UndefinedParentTestClass?

}

class Subclass2Test extends myPackage.ParentTestClass {
}

class Subclass3Test implements MyInterface {

}

abstract class ResolvedParent {}
class Subclass4Test extends ResolvedParent {} // Noncompliant
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a good reason to put this new block at the beginning of the file. Could we move it towards the end?


class A extends junit.framework.TestCase {
void testFoo() {
Expand Down
12 changes: 11 additions & 1 deletion java-checks/src/main/java/org/sonar/java/checks/tests/NoTestInTestClassCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.regex.Pattern;
import java.util.stream.Stream;
Expand Down Expand Up @@ -78,7 +79,16 @@ private void resetAnnotationCache() {
}

private void checkClass(ClassTree classTree) {
if (!ModifiersUtils.hasModifier(classTree.modifiers(), Modifier.ABSTRACT)) {
boolean knownParent = Optional.ofNullable(classTree.superClass())
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 a more typical way of doing this would be to define private static knownParent(...) and use if in if-then. Same for knownImplementedInterfaces. You could even put this new check into one method.

.map(parent -> !parent.symbolType().isUnknown())
.orElse(true);
boolean knownImplementedInterfaces = classTree.superInterfaces().stream()
.noneMatch(i -> i.symbolType().isUnknown());

if (!ModifiersUtils.hasModifier(classTree.modifiers(), Modifier.ABSTRACT)
&& knownParent
&& knownImplementedInterfaces
) {
Symbol.TypeSymbol classSymbol = classTree.symbol();
Stream<Symbol> members = getAllMembers(classSymbol, checkRunWith(classSymbol, "Enclosed"));
IdentifierTree simpleName = classTree.simpleName();
Expand Down
Loading