SONARPY-765 Rule S5915: Assertions should not be made at the end of blocks expecting an exception (#1180)

Author: rudy-regazzoni-sonarsource

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

@@ -0,0 +1,42 @@
+{
+  'project:docker-compose-1.24.1/tests/unit/config/config_test.py':[
+    641,
+    1482,
+  ],
+  'project:tensorflow/python/data/experimental/kernel_tests/take_while_test.py':[
+    95,
+  ],
+  'project:tensorflow/python/debug/cli/command_parser_test.py':[
+    261,
+    265,
+    269,
+  ],
+  'project:tensorflow/python/debug/lib/session_debug_grpc_test.py':[
+    311,
+    697,
+  ],
+  'project:tensorflow/python/eager/device_placement_test.py':[
+    168,
+  ],
+  'project:tensorflow/python/eager/tensor_test.py':[
+    215,
+  ],
+  'project:tensorflow/python/feature_column/feature_column_v2_test.py':[
+    293,
+  ],
+  'project:tensorflow/python/framework/importer_test.py':[
+    491,
+  ],
+  'project:tensorflow/python/training/monitored_session_test.py':[
+    1979,
+    2008,
+    2243,
+  ],
+  'project:tensorflow/python/util/deprecation_test.py':[
+    941,
+  ],
+  'project:tensorflow/python/util/keyword_args_test.py':[
+    43,
+    48,
+  ],
+}

python-checks/src/main/java/org/sonar/python/checks/CheckList.java

@@ -75,6 +75,7 @@
 import org.sonar.python.checks.regex.UnquantifiedNonCapturingGroupCheck;
 import org.sonar.python.checks.regex.VerboseRegexCheck;
 import org.sonar.python.checks.tests.AssertOnDissimilarTypesCheck;
+import org.sonar.python.checks.tests.AssertAfterRaiseCheck;
 import org.sonar.python.checks.tests.AssertOnTupleLiteralCheck;
 import org.sonar.python.checks.tests.DedicatedAssertionCheck;
 import org.sonar.python.checks.tests.ImplicitlySkippedTestCheck;
@@ -97,6 +98,7 @@ public static Iterable<Class> getChecks() {
       ArgumentNumberCheck.class,
       ArgumentTypeCheck.class,
       AssertOnDissimilarTypesCheck.class,
+      AssertAfterRaiseCheck.class,
       AssertOnTupleLiteralCheck.class,
       BackslashInStringCheck.class,
       BackticksUsageCheck.class,

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

@@ -0,0 +1,127 @@
+/*
+ * SonarQube Python Plugin
+ * Copyright (C) 2011-2022 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.python.checks.tests;
+
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+import org.sonar.check.Rule;
+import org.sonar.plugins.python.api.IssueLocation;
+import org.sonar.plugins.python.api.PythonSubscriptionCheck;
+import org.sonar.plugins.python.api.symbols.Symbol;
+import org.sonar.plugins.python.api.tree.CallExpression;
+import org.sonar.plugins.python.api.tree.ExpressionStatement;
+import org.sonar.plugins.python.api.tree.Name;
+import org.sonar.plugins.python.api.tree.QualifiedExpression;
+import org.sonar.plugins.python.api.tree.RegularArgument;
+import org.sonar.plugins.python.api.tree.Statement;
+import org.sonar.plugins.python.api.tree.Tree;
+import org.sonar.plugins.python.api.tree.WithStatement;
+import org.sonar.python.tests.UnittestUtils;
+import org.sonar.python.tree.TreeUtils;
+
+@Rule(key = "S5915")
+public class AssertAfterRaiseCheck extends PythonSubscriptionCheck {
+  private static final String MESSAGE_MULTIPLE_STATEMENT = "Don’t perform an assertion here; An exception is expected to be raised before its execution.";
+  private static final String MESSAGE_SINGLE_STATEMENT = "Refactor this test; if this assertion’s argument raises an exception, the assertion will never get executed.";
+  private static final String MESSAGE_SECONDARY = "An exception is expected to be raised in this block.";
+
+  private static final String ASSERTION_ERROR = "AssertionError";
+  private static final String PYTEST_RAISE_CALL = "pytest.raises";
+  private static final String PYTEST_ARG_EXCEPTION = "expected_exception";
+  private static final String UNITTEST_ARG_EXCEPTION = "exception";
+
+  @Override
+  public void initialize(Context context) {
+    context.registerSyntaxNodeConsumer(Tree.Kind.WITH_STMT, ctx -> {
+      WithStatement withStatement = (WithStatement) ctx.syntaxNode();
+      if (!isWithStatementItemARaise(withStatement)) {
+        return;
+      }
+
+      List<Statement> statements = withStatement.statements().statements();
+      Statement statement = statements.get(statements.size()-1);
+      if (isAnAssert(statement)) {
+        ctx.addIssue(statement, statements.size() > 1 ? MESSAGE_MULTIPLE_STATEMENT : MESSAGE_SINGLE_STATEMENT)
+          .secondary(IssueLocation.preciseLocation(withStatement.firstToken(), withStatement.colon(), MESSAGE_SECONDARY));
+      }
+    });
+  }
+
+  public boolean isWithStatementItemARaise(WithStatement withStatement) {
+    return withStatement.withItems().stream()
+      .filter(withItem -> withItem.test().is(Tree.Kind.CALL_EXPR))
+      .map(withItem -> (CallExpression) withItem.test())
+      .anyMatch(callExpression -> isValidPytestRaise(callExpression) || isValidUnittestRaise(callExpression));
+  }
+
+  public boolean isValidPytestRaise(CallExpression callExpression) {
+    return Optional.of(callExpression).stream()
+      .map(call -> TreeUtils.getSymbolFromTree(call.callee()))
+      .filter(Optional::isPresent)
+      .map(Optional::get)
+      .map(Symbol::fullyQualifiedName)
+      .filter(Objects::nonNull)
+      .anyMatch(fqn -> fqn.contains(PYTEST_RAISE_CALL))
+    && isNotAssertionErrorArgument(TreeUtils.nthArgumentOrKeyword(0, PYTEST_ARG_EXCEPTION, callExpression.arguments()));
+  }
+
+  public boolean isValidUnittestRaise(CallExpression callExpression) {
+    return Optional.of(callExpression).stream()
+      .filter(call -> call.callee().is(Tree.Kind.QUALIFIED_EXPR))
+      .map(call -> (QualifiedExpression) call.callee())
+      .anyMatch(
+        callee -> callee.qualifier().is(Tree.Kind.NAME)
+        && ((Name) callee.qualifier()).name().equals("self")
+        && UnittestUtils.RAISE_METHODS.contains(callee.name().name()))
+      && isNotAssertionErrorArgument(TreeUtils.nthArgumentOrKeyword(0, UNITTEST_ARG_EXCEPTION, callExpression.arguments()));
+  }
+
+  public boolean isNotAssertionErrorArgument(RegularArgument regularArgument) {
+    return Optional.ofNullable(regularArgument).stream()
+      .filter(Objects::nonNull)
+      .map(arg -> TreeUtils.getSymbolFromTree(arg.expression()))
+      .anyMatch(optSym -> optSym.isEmpty() || !ASSERTION_ERROR.equals(optSym.get().fullyQualifiedName()));
+  }
+
+  public boolean isAnAssert(Statement statement) {
+    if (statement.is(Tree.Kind.ASSERT_STMT)) {
+      return true;
+    }
+
+    return Optional.of(statement).stream()
+      .filter(stat -> stat.is(Tree.Kind.EXPRESSION_STMT))
+      .map(ExpressionStatement.class::cast)
+      .map(ExpressionStatement::expressions)
+      .anyMatch(expressions ->
+        expressions.stream()
+          .filter(expression -> expression.is(Tree.Kind.CALL_EXPR))
+          .map(expression -> ((CallExpression) expression).callee())
+          .filter(callee -> callee.is(Tree.Kind.QUALIFIED_EXPR))
+          .map(QualifiedExpression.class::cast)
+          .anyMatch(this::isUnittestAssert)
+      );
+  }
+
+  public boolean isUnittestAssert(QualifiedExpression callee) {
+    return callee.qualifier().is(Tree.Kind.NAME) && ((Name) callee.qualifier()).name().equals("self")
+       && UnittestUtils.allAssertMethods().contains(callee.name().name());
+  }
+}

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

@@ -94,7 +94,7 @@ private static ReturnStatement getReturnStatementFromFirstIfStatement(FunctionDe
 
   private static boolean containsAssertion(FunctionDef functionDef) {
     Set<String> supportedAssertMethods = UnittestUtils.isWithinUnittestTestCase(functionDef) ?
-      UnittestUtils.ASSERTIONS_METHODS : Collections.emptySet();
+      UnittestUtils.allAssertMethods() : Collections.emptySet();
     AssertionVisitor assertVisitor = new AssertionVisitor(supportedAssertMethods);
     functionDef.accept(assertVisitor);
     return assertVisitor.hasAnAssert;

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

@@ -0,0 +1,29 @@
+<p>Using <code>pytest.raises</code> or <code>unittest.TestCase.assertRaises</code> will assert that an exception is raised in the following block.
+Ending such block in an assertion means that the test can succeed with that last assertion never being executed.</p>
+<h2>Noncompliant Code Example</h2>
+<pre>
+import pytest
+def foo(): return 1 / 0
+def bar(): return 42
+def test_something():
+  with pytest.raises(ZeroDivisionError):
+    foo()
+    assert bar() == 42  # Noncompliant
+</pre>
+<h2>Compliant Solution</h2>
+<pre>
+import pytest
+def foo(): return 1 / 0
+def bar(): return 42
+def test_something():
+  with pytest.raises(ZeroDivisionError):
+    foo()
+  assert bar() == 42
+</pre>
+<h2>See</h2>
+<ul>
+  <li> <a href="https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertRaises">Unittest: assertRaises</a> </li>
+  <li> <a href="https://docs.pytest.org/en/stable/assert.html#assertions-about-expected-exceptions">Pytest: assertions about expected exceptions</a>
+  </li>
+</ul>
+

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

@@ -0,0 +1,19 @@
+{
+  "title": "Assertions should not be made at the end of blocks expecting an exception",
+  "type": "BUG",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "5min"
+  },
+  "tags": [
+    "tests",
+    "unused",
+    "pitfall"
+  ],
+  "defaultSeverity": "Critical",
+  "ruleSpecification": "RSPEC-5915",
+  "sqKey": "S5915",
+  "scope": "Tests",
+  "quickfix": "unknown"
+}

python-checks/src/test/java/org/sonar/python/checks/tests/AssertAfterRaiseCheckTest.java

@@ -0,0 +1,45 @@
+/*
+ * SonarQube Python Plugin
+ * Copyright (C) 2011-2022 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.python.checks.tests;
+
+import org.junit.Test;
+import org.sonar.python.checks.utils.PythonCheckVerifier;
+
+public class AssertAfterRaiseCheckTest {
+  @Test
+  public void test() {
+    PythonCheckVerifier.verify("src/test/resources/checks/tests/assertAfterRaise.py", new AssertAfterRaiseCheck());
+  }
+
+  @Test
+  public void testWithWrapper() {
+    PythonCheckVerifier.verify("src/test/resources/checks/tests/assertAfterRaiseWithWrapper.py", new AssertAfterRaiseCheck());
+  }
+
+  @Test
+  public void testWithAnotherLibraryUnittest() {
+    PythonCheckVerifier.verify("src/test/resources/checks/tests/assertAfterRaiseAnotherLibraryUnittest.py", new AssertAfterRaiseCheck());
+  }
+
+  @Test
+  public void testImportPytestAs() {
+    PythonCheckVerifier.verify("src/test/resources/checks/tests/assertAfterRaiseImportPytestAs.py", new AssertAfterRaiseCheck());
+  }
+}