SONARPY-1101 Fix FP on S5899 for helper methods (#1196)

Author: rudy-regazzoni-sonarsource

its/ruling/src/test/resources/expected/python-S5899.json

@@ -1,40 +1,56 @@
 {
-'project:django-2.2.3/django/test/testcases.py':[
-292,
-299,
-306,
-437,
-456,
-467,
-522,
-626,
-655,
-690,
-706,
-716,
-762,
-780,
-790,
-802,
-819,
-836,
-856,
-],
-'project:mypy-0.782/test-data/stdlib-samples/3.2/test/test_set.py':[
-741,
-],
-'project:mypy-0.782/test-data/stdlib-samples/3.2/test/test_tempfile.py':[
-50,
-55,
-],
-'project:mypy-0.782/test-data/stdlib-samples/3.2/test/test_textwrap.py':[
-40,
-45,
-],
-'project:numpy-1.16.4/numpy/testing/_private/utils.py':[
-1262,
-],
-'project:tornado-2.3/tornado/testing.py':[
-169,
-],
+  'project:buildbot-0.8.6p1/buildbot/test/unit/test_process_properties.py':[
+    502,
+  ],
+  'project:django-cms-3.7.1/cms/tests/test_cache.py':[
+    257,
+  ],
+  'project:django-cms-3.7.1/cms/tests/test_page_admin.py':[
+    126,
+  ],
+  'project:django-cms-3.7.1/cms/tests/test_plugins.py':[
+    132,
+  ],
+  'project:django-cms-3.7.1/cms/tests/test_toolbar.py':[
+    78,
+    81,
+    86,
+    91,
+    2086,
+  ],
+  'project:mypy-0.782/test-data/stdlib-samples/3.2/test/test_set.py':[
+    741,
+  ],
+  'project:mypy-0.782/test-data/stdlib-samples/3.2/test/test_tempfile.py':[
+    128,
+  ],
+  'project:numpy-1.16.4/numpy/testing/_private/utils.py':[
+    1262,
+  ],
+  'project:tornado-2.3/tornado/test/httpserver_test.py':[
+    124,
+    342,
+  ],
+  'project:tornado-2.3/tornado/test/web_test.py':[
+    23,
+  ],
+  'project:twisted-12.1.0/twisted/conch/test/test_cftp.py':[
+    466,
+    474,
+  ],
+  'project:twisted-12.1.0/twisted/test/test_amp.py':[
+    892,
+    896,
+  ],
+  'project:twisted-12.1.0/twisted/trial/test/sample.py':[
+    69,
+  ],
+  'project:twisted-12.1.0/twisted/trial/test/test_tests.py':[
+    627,
+    631,
+    641,
+  ],
+  'project:twisted-12.1.0/twisted/words/test/test_jabbercomponent.py':[
+    221,
+  ],
 }

python-checks/src/main/java/org/sonar/python/checks/tests/NotDiscoverableTestMethodCheck.java

@@ -23,16 +23,18 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
+import java.util.stream.Collectors;
 import org.sonar.check.Rule;
 import org.sonar.plugins.python.api.PythonSubscriptionCheck;
 import org.sonar.plugins.python.api.SubscriptionContext;
-import org.sonar.plugins.python.api.symbols.ClassSymbol;
 import org.sonar.plugins.python.api.symbols.FunctionSymbol;
 import org.sonar.plugins.python.api.symbols.Symbol;
 import org.sonar.plugins.python.api.symbols.Usage;
 import org.sonar.plugins.python.api.tree.ClassDef;
+import org.sonar.plugins.python.api.tree.Decorator;
 import org.sonar.plugins.python.api.tree.FunctionDef;
 import org.sonar.plugins.python.api.tree.Statement;
 import org.sonar.plugins.python.api.tree.Tree;
@@ -43,26 +45,27 @@
 public class NotDiscoverableTestMethodCheck extends PythonSubscriptionCheck {
 
   private static final String MESSAGE = "Rename this method so that it starts with \"test\" or remove this unused helper.";
+  private static final Set<String> globalFixture = new HashSet<>();
 
   @Override
   public void initialize(Context context) {
+    context.registerSyntaxNodeConsumer(Tree.Kind.FUNCDEF, ctx -> NotDiscoverableTestMethodCheck.lookForGlobalFixture((FunctionDef) ctx.syntaxNode()));
+
     context.registerSyntaxNodeConsumer(Tree.Kind.CLASSDEF, ctx -> {
       ClassDef classDefinition = (ClassDef) ctx.syntaxNode();
 
       if (inheritsOnlyFromUnitTest(classDefinition)) {
         Map<FunctionSymbol, FunctionDef> suspiciousFunctionsAndDefinitions = new HashMap<>();
         Set<Tree> allDefinitions = new HashSet<>();
+        Set<String> classFixtures = getFixturesFromClass(classDefinition);
         // We only consider method definitions, and not nested functions
         for (Statement statement : classDefinition.body().statements()) {
           if (statement.is(Tree.Kind.FUNCDEF)) {
             FunctionDef functionDef = ((FunctionDef) statement);
-            String functionName = functionDef.name().name();
-            Symbol symbol = functionDef.name().symbol();
-            // If it doesn't override existing methods and doesn't start with test, it is added to the map
-            if (!overrideExistingMethod(functionName) && !functionName.startsWith("test")) {
-              Optional.ofNullable(symbol)
-                .filter(s -> s.is(Symbol.Kind.FUNCTION))
-                .ifPresent(s -> suspiciousFunctionsAndDefinitions.put(((FunctionSymbol) s), functionDef));
+            if (!isException(functionDef, classFixtures)) {
+              Optional.ofNullable(functionDef.name().symbol())
+                .filter(symbol -> symbol.is(Symbol.Kind.FUNCTION))
+                .ifPresent(symbol -> suspiciousFunctionsAndDefinitions.put(((FunctionSymbol) symbol), functionDef));
             }
             allDefinitions.add(functionDef);
           }
@@ -73,9 +76,38 @@ public void initialize(Context context) {
     });
   }
 
-  @Override
-  public CheckScope scope() {
-    return CheckScope.ALL;
+  private static void lookForGlobalFixture(FunctionDef functionDef) {
+    if (functionDef.isMethodDefinition()) {
+      return;
+    }
+    if (functionDef.decorators().stream().anyMatch(NotDiscoverableTestMethodCheck::isPytestFixture)) {
+      globalFixture.add(functionDef.name().name());
+    }
+  }
+
+  /** https://docs.pytest.org/en/6.2.x/fixture.html
+   * Retrieve all Fixtures defined in a class.
+   * A fixture is a method which has the specific decorator @pytest.fixture
+   * In those cases, pytest will invoke the method fixture and inject the result in any test method
+   * for which one of their parameter name match with the fixture method name.
+   */
+  private static Set<String> getFixturesFromClass(ClassDef classDefinition) {
+    return classDefinition.body().statements().stream()
+      .filter(statement -> statement.is(Tree.Kind.FUNCDEF))
+      .map(FunctionDef.class::cast)
+      .filter(functionDef -> functionDef.decorators().stream().anyMatch(NotDiscoverableTestMethodCheck::isPytestFixture))
+      .map(functionDef -> functionDef.name().name())
+      .collect(Collectors.toSet());
+  }
+
+  private static boolean isException(FunctionDef functionDef, Set<String> classFixtures) {
+    String functionName = functionDef.name().name();
+    return overrideExistingMethod(functionName) || functionName.startsWith("test") || isHelper(functionDef, classFixtures);
+  }
+
+  private static boolean isPytestFixture(Decorator decorator) {
+    String decoratorName = TreeUtils.decoratorNameFromExpression(decorator.expression());
+    return  "pytest.fixture".equals(decoratorName);
   }
 
   // Only raises issue when the (non-test) method is not used inside the class
@@ -88,19 +120,26 @@ private static void checkSuspiciousFunctionsUsages(SubscriptionContext ctx, Map<
     });
   }
 
-  private static boolean inheritsOnlyFromUnitTest(ClassDef classDefinition) {
-    Optional<ClassSymbol> classSymbolFromDef = Optional.ofNullable(TreeUtils.getClassSymbolFromDef(classDefinition));
-    return classSymbolFromDef.filter(classSymbol -> !classSymbol.superClasses().isEmpty()).isPresent() &&
-      classSymbolFromDef
-      .map(ClassSymbol::superClasses)
-      .stream()
-      .flatMap(List::stream)
-      .map(Symbol::fullyQualifiedName)
-      .allMatch("unittest.case.TestCase"::equals);
+  private static boolean inheritsOnlyFromUnitTest(ClassDef classDef) {
+    return TreeUtils.getParentClassesFQN(classDef).stream().anyMatch(name -> name.contains("unittest") && name.contains("TestCase"))
+      && Optional.ofNullable(TreeUtils.getClassSymbolFromDef(classDef)).stream()
+        .anyMatch(classSym -> classSym.superClasses().size() == 1);
   }
 
   private static boolean overrideExistingMethod(String functionName) {
     return UnittestUtils.allMethods().contains(functionName) || functionName.startsWith("_");
   }
 
+  private static boolean isHelper(FunctionDef functionDef, Set<String> currentClassFixture) {
+    return Optional.ofNullable(TreeUtils.getFunctionSymbolFromDef(functionDef)).stream()
+      .anyMatch(functionSymbol -> functionSymbol.hasDecorators() || !functionSymbol.parameters().stream()
+        .map(FunctionSymbol.Parameter::name)
+        .filter(Objects::nonNull)
+        .allMatch(name -> name.equals("self") || globalFixture.contains(name) || currentClassFixture.contains(name)));
+  }
+
+  @Override
+  public CheckScope scope() {
+    return CheckScope.ALL;
+  }
 }

python-checks/src/test/resources/checks/tests/notDiscoverableTestMethod.py

@@ -96,7 +96,7 @@ def test_something(self):
         self.some_helper()
         ...
 
-    def method_not_in_parent(self):
+    def method_not_in_parent(self): # Noncompliant
         ...
 
 
@@ -157,3 +157,72 @@ def test_encode_decode(self) -> None:
 
     def _not_a_test(self):
         ...
+
+# SONARPY-1101 Fix FP on S5899 for helper methods
+# We ignore helper methods which have arguments excluding the Pytest's fixtures, considering they are real helper methods not to be reported
+# We also ignore methods which have some decorators
+@pytest.fixture
+def outFixture(): ...
+
+class MyParentTestClass(unittest.TestCase):
+    @pytest.fixture
+    def parentFixture(): ...
+
+class MyTestClass(MyParentTestClass):
+    @pytest.fixture
+    def inFixture(): ...
+
+    def helper_method_with_argument(self, arg): ...
+
+    def helper_method_with_arguments(self, arg1, arg2): ...
+
+    def helper_method_with_only_self(self): # Noncompliant
+        ...
+
+    def helper_method_with_no_argument(): # Noncompliant
+        ...
+
+    def helper_method_with_self_and_in_fixture(self, inFixture): # Noncompliant
+        ...
+
+    def helper_method_with_self_and_in_after_fixture(self, inAfterFixture): # Noncompliant
+        ...
+
+    def helper_method_with_self_and_out_fixture(self, outFixture): # Noncompliant
+        ...
+
+    #Accepted FN : we do not check fixture coming from parent class
+    def helper_method_with_self_and_parent_fixture(self, parentFixture):
+        ...
+
+    def helper_method_with_only_in_fixture(inFixture): # Noncompliant
+        ...
+
+    def helper_method_with_only_in_after_fixture(inAfterFixture): # Noncompliant
+        ...
+
+    def helper_method_with_only_out_fixture(outFixture): # Noncompliant
+        ...
+
+    def helper_method_with_self_in_fixture_and_argument(self, inFixture, arg):
+        ...
+
+    def helper_method_with_self_out_fixture_and_argument(self, outFixture, arg):
+        ...
+
+    def helper_method_with_self_parent_fixture_and_argument(self, parentFixture, arg):
+        ...
+
+    @custom_decorator
+    def helper_method_with_decorator():
+        ...
+
+    @pytest.fixture
+    def inAfterFixture(): ...
+
+# Edge case
+class EdgeCaseLookingLikeUnittest1(unittest.fake):
+    def testMethod(): ...
+
+class EdgeCaseLookingLikeUnittest2(fake.TestCase):
+    def testMethod(): ...

python-frontend/src/test/java/org/sonar/python/tests/UnittestUtilsTest.java

@@ -21,7 +21,9 @@
 
 
 import org.junit.Test;
+import org.sonar.plugins.python.api.tree.ClassDef;
 import org.sonar.plugins.python.api.tree.FileInput;
+import org.sonar.plugins.python.api.tree.FunctionDef;
 import org.sonar.plugins.python.api.tree.Tree;
 import org.sonar.python.PythonTestUtils;
 import org.sonar.python.semantic.SymbolTableBuilder;