diff --git a/python-checks/src/main/java/org/sonar/python/checks/OpenSourceCheckList.java b/python-checks/src/main/java/org/sonar/python/checks/OpenSourceCheckList.java index 91d54e0b16..dabfdc328d 100644 --- a/python-checks/src/main/java/org/sonar/python/checks/OpenSourceCheckList.java +++ b/python-checks/src/main/java/org/sonar/python/checks/OpenSourceCheckList.java @@ -95,6 +95,7 @@ import org.sonar.python.checks.tests.AssertOnDissimilarTypesCheck; import org.sonar.python.checks.tests.AssertOnTupleLiteralCheck; import org.sonar.python.checks.tests.DedicatedAssertionCheck; +import org.sonar.python.checks.tests.DedicatedExceptionAssertionCheck; import org.sonar.python.checks.tests.ImplicitlySkippedTestCheck; import org.sonar.python.checks.tests.NotDiscoverableTestMethodCheck; import org.sonar.python.checks.tests.SkippedTestNoReasonCheck; @@ -180,6 +181,7 @@ public Stream> getChecks() { DeadStoreCheck.class, DebugModeCheck.class, DedicatedAssertionCheck.class, + DedicatedExceptionAssertionCheck.class, DefaultFactoryArgumentCheck.class, DeprecatedNumpyTypesCheck.class, DictKeysMembershipTestCheck.class, diff --git a/python-checks/src/main/java/org/sonar/python/checks/tests/DedicatedExceptionAssertionCheck.java b/python-checks/src/main/java/org/sonar/python/checks/tests/DedicatedExceptionAssertionCheck.java new file mode 100644 index 0000000000..fdb3651254 --- /dev/null +++ b/python-checks/src/main/java/org/sonar/python/checks/tests/DedicatedExceptionAssertionCheck.java @@ -0,0 +1,152 @@ +/* + * SonarQube Python Plugin + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * 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 Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +package org.sonar.python.checks.tests; + +import java.util.List; +import javax.annotation.Nullable; +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.Symbol; +import org.sonar.plugins.python.api.tree.CallExpression; +import org.sonar.plugins.python.api.tree.ElseClause; +import org.sonar.plugins.python.api.tree.Expression; +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.Statement; +import org.sonar.plugins.python.api.tree.Tree; +import org.sonar.plugins.python.api.tree.TryStatement; +import org.sonar.python.tests.UnittestUtils; + +@Rule(key = "S8714") +public class DedicatedExceptionAssertionCheck extends PythonSubscriptionCheck { + private static final String PYTEST_FAIL_FQN = "pytest.fail"; + private static final String PYTEST_MESSAGE = "Replace this try/except block with a \"pytest.raises\" context manager."; + private static final String UNITTEST_MESSAGE = "Replace this try/except block with \"self.assertRaises()\"."; + private static final String NO_EXCEPTION_MESSAGE = "Remove this try/except block and let the test fail naturally if an exception is raised."; + + @Override + public void initialize(Context context) { + context.registerSyntaxNodeConsumer(Tree.Kind.TRY_STMT, ctx -> checkTryStatement(ctx, (TryStatement) ctx.syntaxNode())); + } + + @Override + public CheckScope scope() { + return CheckScope.ALL; + } + + private static void checkTryStatement(SubscriptionContext ctx, TryStatement tryStatement) { + if (!hasSupportedExceptClauses(tryStatement) || tryStatement.finallyClause() != null) { + return; + } + + if (tryStatement.exceptClauses().size() == 1) { + tryStatement.exceptClauses().stream() + .map(exceptClause -> failCallFromSingleStatementBody(exceptClause.body().statements())) + .filter(callExpression -> callExpression != null) + .forEach(callExpression -> ctx.addIssue(callExpression, NO_EXCEPTION_MESSAGE)); + } + + CallExpression failCall = failCallFromElseClause(tryStatement.elseClause()); + if (failCall == null) { + failCall = failCallFromTryBody(tryStatement.body().statements()); + } + if (failCall == null) { + return; + } + + String message = issueMessage(failCall); + if (message != null) { + ctx.addIssue(failCall, message); + } + } + + private static boolean hasSupportedExceptClauses(TryStatement tryStatement) { + return !tryStatement.exceptClauses().isEmpty() && tryStatement.exceptClauses().stream().allMatch(exceptClause -> + exceptClause.starToken() == null && exceptClause.exception() != null); + } + + @Nullable + private static CallExpression failCallFromElseClause(@Nullable ElseClause elseClause) { + if (elseClause == null) { + return null; + } + return failCallFromSingleStatementBody(elseClause.body().statements()); + } + + @Nullable + private static CallExpression failCallFromTryBody(List statements) { + if (statements.size() < 2) { + return null; + } + return failCallFromStatement(statements.get(statements.size() - 1)); + } + + @Nullable + private static CallExpression failCallFromSingleStatementBody(List statements) { + if (statements.size() != 1) { + return null; + } + return failCallFromStatement(statements.get(0)); + } + + @Nullable + private static CallExpression failCallFromStatement(Statement statement) { + if (!(statement instanceof ExpressionStatement expressionStatement) || expressionStatement.expressions().size() != 1) { + return null; + } + Expression expression = expressionStatement.expressions().get(0); + if (!(expression instanceof CallExpression callExpression)) { + return null; + } + if (!(isPytestFail(callExpression) || isUnittestFail(callExpression))) { + return null; + } + return callExpression; + } + + private static boolean isPytestFail(CallExpression callExpression) { + Symbol calleeSymbol = callExpression.calleeSymbol(); + return calleeSymbol != null && PYTEST_FAIL_FQN.equals(calleeSymbol.fullyQualifiedName()); + } + + private static boolean isUnittestFail(CallExpression callExpression) { + if (!UnittestUtils.isWithinUnittestTestCase(callExpression)) { + return false; + } + Expression callee = callExpression.callee(); + if (!(callee instanceof QualifiedExpression qualifiedExpression)) { + return false; + } + if (!(qualifiedExpression.qualifier() instanceof Name qualifier) || !"self".equals(qualifier.name())) { + return false; + } + return "fail".equals(qualifiedExpression.name().name()); + } + + @Nullable + private static String issueMessage(CallExpression failCall) { + if (isPytestFail(failCall)) { + return PYTEST_MESSAGE; + } + if (isUnittestFail(failCall)) { + return UNITTEST_MESSAGE; + } + return null; + } +} diff --git a/python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S8714.html b/python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S8714.html new file mode 100644 index 0000000000..5a9e54cbde --- /dev/null +++ b/python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S8714.html @@ -0,0 +1,145 @@ +

This rule raises an issue when a test uses a try-except block combined with fail() to verify whether an exception is +raised or not raised.

+

Why is this an issue?

+

Using try-except blocks combined with fail() to test for the presence or absence of exceptions is an anti-pattern.

+

Modern testing frameworks like pytest and Python’s built-in unittest provide dedicated assertion methods that make +exception testing cleaner and more explicit. These methods eliminate boilerplate code, clearly communicate test intent, and make it easier to assert +on exception details.

+

The try-except-fail() pattern has several drawbacks:

+
    +
  • It adds unnecessary boilerplate that obscures the test’s purpose
    • +
  • It requires manual exception handling instead of using framework features
    • +
  • It makes the test harder to maintain and read
    • +
  • It provides no straightforward way to assert on exception attributes when an exception is expected
    • +
+

Dedicated exception assertions solve these problems by providing a declarative way to express exception expectations.

+

What is the potential impact?

+

Using try-except with fail() instead of dedicated exception assertions reduces test code quality and maintainability:

+
    +
  • Readability: Tests become harder to understand at a glance, especially for developers unfamiliar with the codebase
    • +
  • Maintainability: More verbose code means more places for bugs to hide and more effort required to update tests
    • +
  • Intent clarity: The test’s purpose (verifying exception behavior) is buried in control flow rather than being explicit
    • +
  • Lost functionality: It’s harder to assert on exception messages, types, or other attributes when using the + try-except pattern
    • +
+

How to fix it in unittest

+

Code examples

+

Noncompliant code example

+

Replace try-except blocks that use fail() with assertRaises used as a context manager. This built-in method +from Python’s unittest framework provides a clean way to verify exception behavior.

+

When you expect no exception, simply call the code directly without wrapping it in a try-except block. If an unexpected exception is +raised, the test framework will catch it and fail the test automatically.

+

When you expect an exception, use assertRaises as a context manager. This allows you to capture the exception and make additional +assertions on its properties.

+
+import unittest
+
+class UserServiceTest(unittest.TestCase):
+    def test_no_exception_thrown(self):
+        try:
+            self.user_service.register_user(self.valid_user)
+        except ValidationError:
+            self.fail("Should not have thrown any exception")  # Noncompliant
+
+

When verifying that an exception is raised, use assertRaises as a context manager. This provides access to the exception object for +further assertions.

+
+import unittest
+
+class UserServiceTest(unittest.TestCase):
+    def test_exception_is_thrown(self):
+        try:
+            self.user_service.register_user(self.invalid_user)
+            self.fail("Expected ValidationError to be thrown")  # Noncompliant
+        except ValidationError as e:
+            self.assertEqual("Invalid email", str(e))
+
+

Compliant solution

+

Replace try-except blocks that use fail() with assertRaises used as a context manager. This built-in method +from Python’s unittest framework provides a clean way to verify exception behavior.

+

When you expect no exception, simply call the code directly without wrapping it in a try-except block. If an unexpected exception is +raised, the test framework will catch it and fail the test automatically.

+

When you expect an exception, use assertRaises as a context manager. This allows you to capture the exception and make additional +assertions on its properties.

+
+import unittest
+
+class UserServiceTest(unittest.TestCase):
+    def test_no_exception_thrown(self):
+        # Simply call the method - unittest will fail if an exception is raised
+        self.user_service.register_user(self.valid_user)
+
+

When verifying that an exception is raised, use assertRaises as a context manager. This provides access to the exception object for +further assertions.

+
+import unittest
+
+class UserServiceTest(unittest.TestCase):
+    def test_exception_is_thrown(self):
+        with self.assertRaises(ValidationError) as context:
+            self.user_service.register_user(self.invalid_user)
+
+        self.assertEqual("Invalid email", str(context.exception))
+
+

How to fix it in pytest

+

Code examples

+

Noncompliant code example

+

Replace try-except blocks with pytest.raises used as a context manager. Pytest is the most popular Python testing +framework and provides excellent exception testing support.

+

When you expect no exception, simply call the code directly. Pytest will fail the test if an unexpected exception occurs.

+

When you expect an exception, use pytest.raises as a context manager to verify the exception type and optionally inspect its +properties.

+
+import pytest
+
+def test_no_exception_thrown(user_service, valid_user):
+    try:
+        user_service.register_user(valid_user)
+    except ValidationError:
+        pytest.fail("Should not have thrown any exception")  # Noncompliant
+
+

When verifying that an exception is raised with pytest, use pytest.raises as a context manager. This gives you access to the exception +object for additional assertions using the match parameter or the value attribute.

+
+import pytest
+
+def test_exception_is_thrown(user_service, invalid_user):
+    try:
+        user_service.register_user(invalid_user)
+        pytest.fail("Expected ValidationError to be thrown")  # Noncompliant
+    except ValidationError as e:
+        assert str(e) == "Invalid email"
+
+

Compliant solution

+

Replace try-except blocks with pytest.raises used as a context manager. Pytest is the most popular Python testing +framework and provides excellent exception testing support.

+

When you expect no exception, simply call the code directly. Pytest will fail the test if an unexpected exception occurs.

+

When you expect an exception, use pytest.raises as a context manager to verify the exception type and optionally inspect its +properties.

+
+def test_no_exception_thrown(user_service, valid_user):
+    # Simply call the method - pytest will fail if an exception is raised
+    user_service.register_user(valid_user)
+
+

When verifying that an exception is raised with pytest, use pytest.raises as a context manager. This gives you access to the exception +object for additional assertions using the match parameter or the value attribute.

+
+import pytest
+
+def test_exception_is_thrown(user_service, invalid_user):
+    with pytest.raises(ValidationError) as exc_info:
+        user_service.register_user(invalid_user)
+
+    assert str(exc_info.value) == "Invalid email"
+
+

Resources

+

Documentation

+ + diff --git a/python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S8714.json b/python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S8714.json new file mode 100644 index 0000000000..1c41e60595 --- /dev/null +++ b/python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S8714.json @@ -0,0 +1,23 @@ +{ + "title": "Dedicated exception assertions should be used instead of \"try-except\" with \"fail()\"", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [ + "tests" + ], + "defaultSeverity": "Minor", + "ruleSpecification": "RSPEC-8714", + "sqKey": "S8714", + "scope": "Tests", + "quickfix": "unknown", + "code": { + "impacts": { + "MAINTAINABILITY": "LOW" + }, + "attribute": "CONVENTIONAL" + } +} diff --git a/python-checks/src/main/resources/org/sonar/l10n/py/rules/python/Sonar_agentic_AI_profile.json b/python-checks/src/main/resources/org/sonar/l10n/py/rules/python/Sonar_agentic_AI_profile.json index 88ce989453..4f70e2235d 100644 --- a/python-checks/src/main/resources/org/sonar/l10n/py/rules/python/Sonar_agentic_AI_profile.json +++ b/python-checks/src/main/resources/org/sonar/l10n/py/rules/python/Sonar_agentic_AI_profile.json @@ -306,6 +306,7 @@ "S8413", "S8414", "S8415", + "S8714", "S8786" ] } diff --git a/python-checks/src/main/resources/org/sonar/l10n/py/rules/python/Sonar_way_profile.json b/python-checks/src/main/resources/org/sonar/l10n/py/rules/python/Sonar_way_profile.json index 45ffd00ad1..1df1e7138e 100644 --- a/python-checks/src/main/resources/org/sonar/l10n/py/rules/python/Sonar_way_profile.json +++ b/python-checks/src/main/resources/org/sonar/l10n/py/rules/python/Sonar_way_profile.json @@ -353,6 +353,7 @@ "S8521", "S8572", "S8685", + "S8714", "S8786" ] } diff --git a/python-checks/src/test/java/org/sonar/python/checks/tests/DedicatedExceptionAssertionCheckTest.java b/python-checks/src/test/java/org/sonar/python/checks/tests/DedicatedExceptionAssertionCheckTest.java new file mode 100644 index 0000000000..76505896a8 --- /dev/null +++ b/python-checks/src/test/java/org/sonar/python/checks/tests/DedicatedExceptionAssertionCheckTest.java @@ -0,0 +1,36 @@ +/* + * SonarQube Python Plugin + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * 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 Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +package org.sonar.python.checks.tests; + +import org.junit.jupiter.api.Test; +import org.sonar.plugins.python.api.PythonCheck; +import org.sonar.python.checks.utils.PythonCheckVerifier; + +import static org.assertj.core.api.Assertions.assertThat; + +class DedicatedExceptionAssertionCheckTest { + + @Test + void sample() { + PythonCheckVerifier.verify("src/test/resources/checks/tests/dedicatedExceptionAssertion.py", new DedicatedExceptionAssertionCheck()); + } + + @Test + void test_scope() { + assertThat(new DedicatedExceptionAssertionCheck().scope()).isEqualTo(PythonCheck.CheckScope.ALL); + } +} diff --git a/python-checks/src/test/resources/checks/tests/dedicatedExceptionAssertion.py b/python-checks/src/test/resources/checks/tests/dedicatedExceptionAssertion.py new file mode 100644 index 0000000000..8a0536f908 --- /dev/null +++ b/python-checks/src/test/resources/checks/tests/dedicatedExceptionAssertion.py @@ -0,0 +1,120 @@ +import unittest + +import pytest +from pytest import fail as imported_fail + +def explode(): + raise ValueError() + +def test_pytest_reproducer(): + try: + explode() + except ValueError: + pass + else: + pytest.fail("ValueError expected") # Noncompliant {{Replace this try/except block with a "pytest.raises" context manager.}} + +def test_pytest_else_fail(): + try: + explode() + except ValueError: + pass + else: + pytest.fail("ValueError expected") # Noncompliant {{Replace this try/except block with a "pytest.raises" context manager.}} + +def test_pytest_try_fail(): + try: + explode() + pytest.fail("ValueError expected") # Noncompliant {{Replace this try/except block with a "pytest.raises" context manager.}} + except ValueError: + pass + +def test_pytest_imported_fail(): + try: + explode() + except ValueError: + pass + else: + imported_fail("ValueError expected") # Noncompliant {{Replace this try/except block with a "pytest.raises" context manager.}} + +def test_except_body_with_extra_logic(): + try: + explode() + except ValueError as err: + print(err) + else: + pytest.fail("ValueError expected") # Noncompliant + +def test_no_issue_on_non_pytest_fail(): + try: + explode() + except ValueError: + pass + else: + fail("ValueError expected") + +def test_no_issue_with_finally(): + try: + explode() + except ValueError: + pass + else: + pytest.fail("ValueError expected") + finally: + cleanup() + +def test_fail_in_except(): + try: + explode() + except ValueError: + pytest.fail("unexpected") # Noncompliant {{Remove this try/except block and let the test fail naturally if an exception is raised.}} + +class SomeTest(unittest.TestCase): + def __init__(self): + self.user_service = None + self.valid_user = None + self.invalid_user = None + + def test_unittest_else_fail(self): + try: + explode() + except ValueError: + pass + else: + self.fail("ValueError expected") # Noncompliant {{Replace this try/except block with "self.assertRaises()".}} + + def test_unittest_try_fail(self): + try: + explode() + self.fail("ValueError expected") # Noncompliant {{Replace this try/except block with "self.assertRaises()".}} + except ValueError: + pass + + def test_no_exception_expected(self): + try: + self.user_service.register_user(self.valid_user) + except ValidationError: + self.fail("Should not have thrown any exception") # Noncompliant {{Remove this try/except block and let the test fail naturally if an exception is raised.}} + + def test_exception_details(self): + try: + self.user_service.register_user(self.invalid_user) + self.fail("Expected ValidationError to be thrown") # Noncompliant {{Replace this try/except block with "self.assertRaises()".}} + except ValidationError as e: + self.assertEqual("Invalid email", str(e)) + + def test_multiple_except_clauses(self): + try: + self.user_service.register_user(self.valid_user) + except ValidationError: + self.fail("got ValidationError") + except TypeError: + self.assertEqual("bad type", "bad type") + + def test_no_issue_outside_unittest(self): + try: + explode() + except ValueError: + pass + else: + other.fail("ValueError expected")