Skip to content

Commit d53165b

Browse files
rombirlisonartech
rombirli
authored and
sonartech
committed
SONARPY-4312 Port S2187 to python: TestCases should contain tests (#1220)
GitOrigin-RevId: 31d1f65c33e89164346651f3b28f1537eca5dc5d
1 parent 2d78692 commit d53165b

12 files changed

Lines changed: 585 additions & 0 deletions

File tree

‎python-checks/src/main/java/org/sonar/python/checks/OpenSourceCheckList.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@
101101
import org.sonar.python.checks.tests.SingleInvocationRuntimeExceptionCheck;
102102
import org.sonar.python.checks.tests.SkippedTestNoReasonCheck;
103103
import org.sonar.python.checks.tests.SpecificExceptionAssertionCheck;
104+
import org.sonar.python.checks.tests.TestCasesShouldContainTestsCheck;
104105
import org.sonar.python.checks.tests.UnconditionalAssertionCheck;
105106

106107
public class OpenSourceCheckList {
@@ -420,6 +421,7 @@ public Stream<Class<?>> getChecks() {
420421
SpecialMethodParamListCheck.class,
421422
SpecialMethodReturnTypeCheck.class,
422423
SpecificExceptionAssertionCheck.class,
424+
TestCasesShouldContainTestsCheck.class,
423425
SQLQueriesCheck.class,
424426
StartsWithEndsWithTupleCheck.class,
425427
StopIterationInGeneratorCheck.class,

‎python-checks/src/main/java/org/sonar/python/checks/tests/TestCasesShouldContainTestsCheck.java‎

Lines changed: 237 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,237 @@
1+
/*
2+
* SonarQube Python Plugin
3+
* Copyright (C) SonarSource Sàrl
4+
* mailto:info AT sonarsource DOT com
5+
*
6+
* You can redistribute and/or modify this program under the terms of
7+
* the Sonar Source-Available License Version 1, as published by SonarSource Sàrl.
8+
*
9+
* This program is distributed in the hope that it will be useful,
10+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
11+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
12+
* See the Sonar Source-Available License for more details.
13+
*
14+
* You should have received a copy of the Sonar Source-Available License
15+
* along with this program; if not, see https://sonarsource.com/license/ssal/
16+
*/
17+
package org.sonar.python.checks.tests;
18+
19+
import java.util.LinkedHashSet;
20+
import java.util.List;
21+
import java.util.Objects;
22+
import java.util.Set;
23+
import javax.annotation.Nullable;
24+
import org.sonar.check.Rule;
25+
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
26+
import org.sonar.plugins.python.api.SubscriptionContext;
27+
import org.sonar.plugins.python.api.symbols.ClassSymbol;
28+
import org.sonar.plugins.python.api.symbols.Symbol;
29+
import org.sonar.plugins.python.api.tree.ClassDef;
30+
import org.sonar.plugins.python.api.tree.Expression;
31+
import org.sonar.plugins.python.api.tree.FileInput;
32+
import org.sonar.plugins.python.api.tree.FunctionDef;
33+
import org.sonar.plugins.python.api.tree.RegularArgument;
34+
import org.sonar.plugins.python.api.tree.RaiseStatement;
35+
import org.sonar.plugins.python.api.tree.Statement;
36+
import org.sonar.plugins.python.api.tree.StatementList;
37+
import org.sonar.plugins.python.api.tree.Tree;
38+
import org.sonar.plugins.python.api.types.v2.matchers.TypeMatcher;
39+
import org.sonar.plugins.python.api.types.v2.matchers.TypeMatchers;
40+
41+
import static java.util.function.Predicate.not;
42+
import static org.sonar.python.tree.TreeUtils.getClassSymbolFromDef;
43+
44+
@Rule(key = "S2187")
45+
public class TestCasesShouldContainTestsCheck extends PythonSubscriptionCheck {
46+
private static final String CLASS_MESSAGE = "Add some tests to this class.";
47+
private static final String FILE_MESSAGE = "Add some tests to this file.";
48+
private static final TypeMatcher UNITTEST_TEST_CASE_MATCHER = TypeMatchers.any(
49+
TypeMatchers.isOrExtendsType("unittest.TestCase"),
50+
TypeMatchers.isOrExtendsType("unittest.case.TestCase"));
51+
private static final TypeMatcher ABSTRACT_BASE_CLASS_MATCHER = TypeMatchers.isOrExtendsType("abc.ABC");
52+
private static final TypeMatcher NOT_IMPLEMENTED_ERROR_MATCHER = TypeMatchers.any(
53+
TypeMatchers.isType("builtins.NotImplementedError"),
54+
TypeMatchers.isObjectInstanceOf("builtins.NotImplementedError"));
55+
56+
@Override
57+
public void initialize(Context context) {
58+
context.registerSyntaxNodeConsumer(Tree.Kind.FILE_INPUT, TestCasesShouldContainTestsCheck::checkFileInput);
59+
}
60+
61+
private static void checkFileInput(SubscriptionContext ctx) {
62+
FileInput fileInput = (FileInput) ctx.syntaxNode();
63+
StatementList statements = fileInput.statements();
64+
if (statements == null) {
65+
return;
66+
}
67+
68+
boolean pytestStyleFile = isPytestStyleFile(ctx);
69+
List<CandidateClass> candidateClasses = collectCandidateClasses(ctx, statements, pytestStyleFile);
70+
boolean hasCollectedTests = hasModuleLevelPytestTests(statements) || hasClassLevelTests(statements);
71+
72+
if (pytestStyleFile && !hasCollectedTests) {
73+
ctx.addFileIssue(FILE_MESSAGE);
74+
return;
75+
}
76+
77+
candidateClasses
78+
.stream()
79+
.filter(not(CandidateClass::hasCollectedTests))
80+
.filter(candidateClass -> !hasDescendantWithTests(candidateClass, candidateClasses))
81+
.filter(candidateClass -> !isLikelySharedBaseClass(candidateClass, ctx))
82+
.forEach(candidateClass -> ctx.addIssue(candidateClass.classDef().name(), CLASS_MESSAGE));
83+
}
84+
85+
private static List<CandidateClass> collectCandidateClasses(SubscriptionContext ctx, StatementList statements, boolean pytestStyleFile) {
86+
return statements.statements()
87+
.stream().filter(ClassDef.class::isInstance)
88+
.map(ClassDef.class::cast)
89+
.filter(classDef -> isCandidateTestClass(ctx, classDef, pytestStyleFile))
90+
.map(classDef ->
91+
new CandidateClass(classDef, getClassSymbolFromDef(classDef), hasCollectedTests(classDef))
92+
)
93+
.toList();
94+
}
95+
96+
private static boolean hasModuleLevelPytestTests(StatementList statements) {
97+
return statements.statements().stream()
98+
.filter(FunctionDef.class::isInstance)
99+
.map(FunctionDef.class::cast)
100+
.anyMatch(TestCasesShouldContainTestsCheck::isTestMethod);
101+
}
102+
103+
private static boolean hasClassLevelTests(StatementList statements) {
104+
return statements.statements().stream()
105+
.filter(ClassDef.class::isInstance)
106+
.map(ClassDef.class::cast)
107+
.anyMatch(TestCasesShouldContainTestsCheck::hasCollectedTests);
108+
}
109+
110+
private static boolean isCandidateTestClass(SubscriptionContext ctx, ClassDef classDef, boolean pytestStyleFile) {
111+
return (pytestStyleFile && classDef.name().name().startsWith("Test")) || isUnittestTestCaseClass(classDef, ctx);
112+
}
113+
114+
private static boolean isPytestStyleFile(SubscriptionContext ctx) {
115+
String fileName = ctx.pythonFile().fileName();
116+
return fileName.startsWith("test_") || fileName.endsWith("_test.py");
117+
}
118+
119+
private static boolean isUnittestTestCaseClass(ClassDef classDef, SubscriptionContext ctx) {
120+
return hasAncestorMatching(classDef, UNITTEST_TEST_CASE_MATCHER, ctx);
121+
}
122+
123+
private static boolean hasCollectedTests(ClassDef classDef) {
124+
return hasLocalTestMethod(classDef) || hasInheritedTestMethod(classDef);
125+
}
126+
127+
private static boolean hasLocalTestMethod(ClassDef classDef) {
128+
return classDef.body().statements().stream()
129+
.filter(FunctionDef.class::isInstance)
130+
.map(FunctionDef.class::cast)
131+
.anyMatch(TestCasesShouldContainTestsCheck::isTestMethod);
132+
}
133+
134+
private static boolean hasInheritedTestMethod(ClassDef classDef) {
135+
ClassSymbol classSymbol = getClassSymbolFromDef(classDef);
136+
return classSymbol != null && superClassesDeclareTests(classSymbol, new LinkedHashSet<>());
137+
}
138+
139+
private static boolean superClassesDeclareTests(ClassSymbol classSymbol, Set<ClassSymbol> visitedSymbols) {
140+
if (!visitedSymbols.add(classSymbol)) {
141+
return false;
142+
}
143+
return classSymbol.superClasses().stream()
144+
.filter(ClassSymbol.class::isInstance)
145+
.map(ClassSymbol.class::cast)
146+
.anyMatch(superClassSymbol ->
147+
superClassesDeclareTests(superClassSymbol, visitedSymbols) ||
148+
superClassSymbol.declaredMembers().stream()
149+
.filter(symbol -> symbol.is(Symbol.Kind.FUNCTION))
150+
.map(Symbol::name)
151+
.anyMatch(TestCasesShouldContainTestsCheck::isTestMethodName)
152+
);
153+
}
154+
155+
private static boolean hasDescendantWithTests(CandidateClass candidateClass, List<CandidateClass> candidateClasses) {
156+
ClassSymbol classSymbol = candidateClass.classSymbol();
157+
if (classSymbol == null) {
158+
return false;
159+
}
160+
return candidateClasses.stream()
161+
.filter(other -> other != candidateClass)
162+
.filter(CandidateClass::hasCollectedTests)
163+
.map(CandidateClass::classSymbol)
164+
.filter(Objects::nonNull)
165+
.anyMatch(otherClassSymbol -> otherClassSymbol.isOrExtends(classSymbol));
166+
}
167+
168+
private static boolean isLikelySharedBaseClass(CandidateClass candidateClass, SubscriptionContext ctx) {
169+
ClassDef classDef = candidateClass.classDef();
170+
String className = classDef.name().name();
171+
return className.startsWith("Base")
172+
|| className.endsWith("Mixin")
173+
|| isAbstractBaseClass(classDef, candidateClass.classSymbol(), ctx)
174+
|| onlyContainsPlaceholderMethods(classDef, ctx);
175+
}
176+
177+
private static boolean isAbstractBaseClass(ClassDef classDef, @Nullable ClassSymbol classSymbol, SubscriptionContext ctx) {
178+
return classSymbol != null && (classSymbol.hasMetaClass()
179+
|| hasAncestorMatching(classDef, ABSTRACT_BASE_CLASS_MATCHER, ctx));
180+
}
181+
182+
private static boolean hasAncestorMatching(ClassDef classDef, TypeMatcher matcher, SubscriptionContext ctx) {
183+
var args = classDef.args();
184+
if (args == null) {
185+
return false;
186+
}
187+
for (var argument : args.arguments()) {
188+
if (argument instanceof RegularArgument regularArgument && matcher.isTrueFor(regularArgument.expression(), ctx)) {
189+
return true;
190+
}
191+
}
192+
return false;
193+
}
194+
195+
private static boolean onlyContainsPlaceholderMethods(ClassDef classDef, SubscriptionContext ctx) {
196+
return classDef.body().statements().stream()
197+
.filter(FunctionDef.class::isInstance)
198+
.map(FunctionDef.class::cast)
199+
.findAny()
200+
.filter(functionDef -> classDef.body().statements().stream()
201+
.filter(FunctionDef.class::isInstance)
202+
.map(FunctionDef.class::cast)
203+
.allMatch(method -> isPlaceholderMethod(method, ctx)))
204+
.isPresent();
205+
}
206+
207+
private static boolean isPlaceholderMethod(FunctionDef functionDef, SubscriptionContext ctx) {
208+
List<Statement> statements = functionDef.body().statements();
209+
return statements.size() == 1 && isPlaceholderStatement(statements.get(0), ctx);
210+
}
211+
212+
private static boolean isPlaceholderStatement(Statement statement, SubscriptionContext ctx) {
213+
return statement.is(Tree.Kind.PASS_STMT) || isNotImplementedErrorRaise(statement, ctx);
214+
}
215+
216+
private static boolean isNotImplementedErrorRaise(Statement statement, SubscriptionContext ctx) {
217+
if (!(statement instanceof RaiseStatement raiseStatement) || raiseStatement.expressions().size() != 1) {
218+
return false;
219+
}
220+
return isNotImplementedError(raiseStatement.expressions().get(0), ctx);
221+
}
222+
223+
private static boolean isNotImplementedError(Expression expression, SubscriptionContext ctx) {
224+
return NOT_IMPLEMENTED_ERROR_MATCHER.isTrueFor(expression, ctx);
225+
}
226+
227+
private static boolean isTestMethod(FunctionDef functionDef) {
228+
return isTestMethodName(functionDef.name().name());
229+
}
230+
231+
private static boolean isTestMethodName(String name) {
232+
return name.startsWith("test");
233+
}
234+
235+
private record CandidateClass(ClassDef classDef, @Nullable ClassSymbol classSymbol, boolean hasCollectedTests) {
236+
}
237+
}

‎python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S2187.html‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
<h2>Why is this an issue?</h2>
2+
<p>Test files and test classes are meant to verify behavior. When a file looks like a test module, or a class looks like a collected test class,
3+
readers expect it to contain executable test cases. If it only contains helpers, fixtures, or lifecycle code, it gives the false impression that the
4+
covered behavior is actually tested.</p>
5+
<p>In Python, this can happen in different ways depending on the test framework:</p>
6+
<ul>
7+
<li>In <code>pytest</code>, a file such as <code>test_example.py</code> or <code>example_test.py</code> can define fixtures or helper classes but no
8+
collected test functions or test methods.</li>
9+
<li>In <code>unittest</code>, a class can inherit from <code>unittest.TestCase</code> but only define setup helpers and no <code>test_</code>
10+
methods.</li>
11+
</ul>
12+
<p>Such files and classes are misleading because they appear to contribute test coverage while actually verifying nothing.</p>
13+
<h3>Exceptions</h3>
14+
<p>There are scenarios where not having local test cases is acceptable.</p>
15+
<h4>Base classes used only for shared test logic</h4>
16+
<p>Shared helpers or setup code can live in a base class or mixin that is meant to be inherited by real test classes. Those classes are not
17+
problematic by themselves as long as the actual derived test classes contain collected tests.</p>
18+
<h4>Derived classes that inherit tests</h4>
19+
<p>Some test classes inherit test methods from a base class. In that case, the derived class may not declare its own <code>test_</code> methods
20+
locally, but it still participates in real tests and should not be flagged.</p>
21+
<h2>How to fix it in pytest</h2>
22+
<p>To fix this issue in <code>pytest</code>, it is important that all test files contain at least one test case.</p>
23+
<p>Test cases in pytest are identified by:</p>
24+
<ul>
25+
<li>Functions prefixed with <code>test_</code> in files prefixed with <code>test_</code> or suffixed with <code>_test.py</code></li>
26+
<li>Methods prefixed with <code>test_</code> in classes prefixed with <code>Test</code></li>
27+
</ul>
28+
<h3>Code examples</h3>
29+
<h4>Noncompliant code example</h4>
30+
<pre data-diff-id="1" data-diff-type="noncompliant">
31+
# test_example.py
32+
class TestSomeClass:
33+
pass # Noncompliant: no test methods
34+
</pre>
35+
<h4>Compliant solution</h4>
36+
<pre data-diff-id="1" data-diff-type="compliant">
37+
# test_example.py
38+
class TestSomeClass:
39+
def test_some_method_should_return_true(self):
40+
assert some_method() is True
41+
</pre>
42+
<h2>How to fix it in unittest</h2>
43+
<p>To fix this issue in <code>unittest</code>, it is important that all test classes that inherit from <code>unittest.TestCase</code> contain at least
44+
one test method.</p>
45+
<p>Test methods in unittest must:</p>
46+
<ul>
47+
<li>Be prefixed with <code>test_</code></li>
48+
<li>Be part of a class that inherits from <code>unittest.TestCase</code></li>
49+
</ul>
50+
<h3>Code examples</h3>
51+
<h4>Noncompliant code example</h4>
52+
<pre data-diff-id="11" data-diff-type="noncompliant">
53+
import unittest
54+
55+
class TestSomeClass(unittest.TestCase):
56+
pass # Noncompliant: no test methods
57+
</pre>
58+
<h4>Compliant solution</h4>
59+
<pre data-diff-id="11" data-diff-type="compliant">
60+
import unittest
61+
62+
class TestSomeClass(unittest.TestCase):
63+
def test_some_method_should_return_true(self):
64+
self.assertTrue(some_method())
65+
</pre>
66+
<h2>Resources</h2>
67+
<h3>Documentation</h3>
68+
<ul>
69+
<li><a href="https://docs.pytest.org/">pytest: helps you write better programs</a></li>
70+
<li><a href="https://docs.python.org/3/library/unittest.html">unittest — Unit testing framework</a></li>
71+
</ul>
72+

‎python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S2187.json‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
{
2+
"title": "TestCases should contain tests",
3+
"type": "CODE_SMELL",
4+
"code": {
5+
"impacts": {
6+
"MAINTAINABILITY": "BLOCKER"
7+
},
8+
"attribute": "TESTED"
9+
},
10+
"status": "ready",
11+
"remediation": {
12+
"func": "Constant\/Issue",
13+
"constantCost": "5min"
14+
},
15+
"tags": [
16+
"tests",
17+
"pytest",
18+
"unittest"
19+
],
20+
"defaultSeverity": "Blocker",
21+
"ruleSpecification": "RSPEC-2187",
22+
"sqKey": "S2187",
23+
"scope": "Tests",
24+
"quickfix": "unknown"
25+
}

‎python-checks/src/main/resources/org/sonar/l10n/py/rules/python/Sonar_way_profile.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@
5151
"S2092",
5252
"S2115",
5353
"S2159",
54+
"S2187",
5455
"S2190",
5556
"S2201",
5657
"S2208",

0 commit comments

Comments
 (0)