diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S1874.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S1874.json index 54f9c6aecab..dd83ce440c0 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S1874.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S1874.json @@ -1,6 +1,6 @@ { "ruleKey": "S1874", "hasTruePositives": true, - "falseNegatives": 245, + "falseNegatives": 246, "falsePositives": 0 -} \ No newline at end of file +} diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S5960.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S5960.json index 68b30cb1649..d5ead00a529 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S5960.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S5960.json @@ -1,6 +1,6 @@ { "ruleKey": "S5960", "hasTruePositives": false, - "falseNegatives": 3, + "falseNegatives": 4, "falsePositives": 0 -} \ No newline at end of file +} diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S8714.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S8714.json new file mode 100644 index 00000000000..458ea610c32 --- /dev/null +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S8714.json @@ -0,0 +1,6 @@ +{ + "ruleKey": "S8714", + "hasTruePositives": false, + "falseNegatives": 33, + "falsePositives": 0 +} diff --git a/its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S8714.json b/its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S8714.json new file mode 100644 index 00000000000..5a10deffebb --- /dev/null +++ b/its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S8714.json @@ -0,0 +1,28 @@ +{ +"org.eclipse.jetty:jetty-project:jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java": [ +107 +], +"org.eclipse.jetty:jetty-project:jetty-http/src/test/java/org/eclipse/jetty/http/SyntaxTest.java": [ +72, +126 +], +"org.eclipse.jetty:jetty-project:jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/ServletPathSpecTest.java": [ +37 +], +"org.eclipse.jetty:jetty-project:jetty-io/src/test/java/org/eclipse/jetty/io/ByteArrayEndPointTest.java": [ +90, +206, +297 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartFormInputStreamTest.java": [ +945 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/test/java/org/eclipse/jetty/server/NotAcceptingTest.java": [ +134, +251 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/test/java/org/eclipse/jetty/server/PartialRFC2616Test.java": [ +102, +692 +] +} diff --git a/its/ruling/src/test/resources/eclipse-jetty/java-S8714.json b/its/ruling/src/test/resources/eclipse-jetty/java-S8714.json new file mode 100644 index 00000000000..4a03b212701 --- /dev/null +++ b/its/ruling/src/test/resources/eclipse-jetty/java-S8714.json @@ -0,0 +1,47 @@ +{ +"org.eclipse.jetty:jetty-project:jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java": [ +107 +], +"org.eclipse.jetty:jetty-project:jetty-http/src/test/java/org/eclipse/jetty/http/SyntaxTest.java": [ +72, +126 +], +"org.eclipse.jetty:jetty-project:jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/ServletPathSpecTest.java": [ +37 +], +"org.eclipse.jetty:jetty-project:jetty-io/src/test/java/org/eclipse/jetty/io/ByteArrayEndPointTest.java": [ +90, +206, +297 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartFormInputStreamTest.java": [ +945 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/test/java/org/eclipse/jetty/server/NotAcceptingTest.java": [ +134, +251 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/test/java/org/eclipse/jetty/server/PartialRFC2616Test.java": [ +102, +692 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java": [ +1255, +1263 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/test/java/org/eclipse/jetty/server/StopTest.java": [ +497 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/test/java/org/eclipse/jetty/util/BlockingArrayQueueTest.java": [ +192 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/test/java/org/eclipse/jetty/util/SharedBlockingCallbackTest.java": [ +116, +161, +235 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/test/java/org/eclipse/jetty/util/thread/AbstractThreadPoolTest.java": [ +59, +81 +] +} diff --git a/its/ruling/src/test/resources/sonar-server/java-S8714.json b/its/ruling/src/test/resources/sonar-server/java-S8714.json new file mode 100644 index 00000000000..b76399a5b57 --- /dev/null +++ b/its/ruling/src/test/resources/sonar-server/java-S8714.json @@ -0,0 +1,332 @@ +{ +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/app/EmbeddedTomcatTest.java": [ +75 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/app/StartupLogsTest.java": [ +74 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/app/TomcatConnectorsTest.java": [ +84 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/batch/ProjectDataLoaderMediumTest.java": [ +489 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/batch/ProjectDataLoaderTest.java": [ +123 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/computation/task/projectanalysis/component/ComponentImplTest.java": [ +99, +114 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/computation/task/projectanalysis/component/ComponentRootBuilderTest.java": [ +90 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/computation/task/projectanalysis/event/EventRepositoryImplTest.java": [ +77 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/computation/task/projectanalysis/filemove/MutableMovedFilesRepositoryImplTest.java": [ +72 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/computation/task/projectanalysis/issue/ScmAccountToUserLoaderTest.java": [ +91 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/computation/task/projectanalysis/measure/MapBasedRawMeasureRepositoryTest.java": [ +191, +238, +248 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/computation/task/projectanalysis/measure/MeasureRepositoryImplTest.java": [ +116, +126, +248, +295, +305 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/computation/task/projectanalysis/qualitygate/ConditionEvaluatorTest.java": [ +69, +77, +85 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/computation/task/projectanalysis/step/ComputeQProfileMeasureStepTest.java": [ +142 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/computation/task/projectanalysis/step/PersistProjectLinksStepTest.java": [ +209 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/computation/task/step/ComputationStepExecutorTest.java": [ +137 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/debt/DebtModelPluginRepositoryTest.java": [ +94 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/debt/DebtRulesXMLImporterTest.java": [ +230 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/email/ws/SendActionTest.java": [ +111 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/es/IndexDefinitionContextTest.java": [ +45 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/es/NewIndexTest.java": [ +55 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/es/SearchOptionsTest.java": [ +78 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/es/SortingTest.java": [ +130 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/es/request/ProxyClearCacheRequestBuilderTest.java": [ +79, +89, +99 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/es/request/ProxyClusterHealthRequestBuilderTest.java": [ +72, +82, +92 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/es/request/ProxyClusterStateRequestBuilderTest.java": [ +68, +78, +88 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/es/request/ProxyClusterStatsRequestBuilderTest.java": [ +67, +77, +87 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/es/request/ProxyCreateIndexRequestBuilderTest.java": [ +68, +78, +88 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/es/request/ProxyDeleteRequestBuilderTest.java": [ +63, +73, +83 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/es/request/ProxyFlushRequestBuilderTest.java": [ +64, +75, +85, +95 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/es/request/ProxyGetRequestBuilderTest.java": [ +71, +82, +92, +102 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/es/request/ProxyIndexRequestBuilderTest.java": [ +67, +79, +90, +100, +110 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/es/request/ProxyIndicesExistsRequestBuilderTest.java": [ +62, +78, +88, +98 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/es/request/ProxyIndicesStatsRequestBuilderTest.java": [ +65, +76, +86, +96 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/es/request/ProxyMultiGetRequestBuilderTest.java": [ +78, +88, +98 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/es/request/ProxyPutMappingRequestBuilderTest.java": [ +77, +88, +98, +108 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/es/request/ProxyRefreshRequestBuilderTest.java": [ +67, +78, +88, +98 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/es/request/ProxySearchRequestBuilderTest.java": [ +67, +78, +88, +98 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/es/request/ProxySearchScrollRequestBuilderTest.java": [ +73, +84, +94, +104 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/issue/CommentActionTest.java": [ +72 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/issue/IssueQueryFactoryTest.java": [ +297 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/issue/index/IssueIndexTest.java": [ +1301 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/issue/index/IssueIndexerTest.java": [ +118 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/issue/workflow/IssueWorkflowTest.java": [ +125, +216 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/issue/workflow/TransitionTest.java": [ +73, +83, +93, +103 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/notification/email/EmailNotificationChannelTest.java": [ +86, +181 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/organization/DefaultOrganizationProviderImplTest.java": [ +123 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/organization/OrganizationCreationImplTest.java": [ +160 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/organization/OrganizationValidationImplTest.java": [ +81, +96, +144, +178, +207, +236 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/permission/GroupPermissionChangerTest.java": [ +92, +322, +339, +356 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/permission/ws/AddGroupActionTest.java": [ +378 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/permission/ws/template/DeleteTemplateActionTest.java": [ +134, +155 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/platform/db/EmbeddedDatabaseTest.java": [ +166 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/platform/web/requestid/RequestIdFilterTest.java": [ +84 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/plugins/PluginDownloaderTest.java": [ +174, +214, +232 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/plugins/ServerPluginRepositoryTest.java": [ +133, +221, +297, +304 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/project/ws/UpdateVisibilityActionTest.java": [ +157, +195 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/qualityprofile/QProfileBackuperImplTest.java": [ +189, +202, +215 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/qualityprofile/QProfileCopierTest.java": [ +94 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/qualityprofile/QProfileExportersTest.java": [ +133, +143 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/qualityprofile/RuleActivatorTest.java": [ +1152 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/qualityprofile/ws/QProfilesWsMediumTest.java": [ +243 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/qualityprofile/ws/RestoreBuiltInActionTest.java": [ +56 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/rule/DeprecatedRulesDefinitionLoaderTest.java": [ +205 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/rule/RuleCreatorTest.java": [ +294 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/rule/RuleTagHelperTest.java": [ +66 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/rule/RuleUpdaterTest.java": [ +99, +517, +542, +560, +578, +596 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/rule/index/RuleIndexTest.java": [ +934 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/search/BaseDocTest.java": [ +85 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/source/ws/HashActionTest.java": [ +92 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/user/DoPrivilegedTest.java": [ +69 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/user/SecurityRealmFactoryTest.java": [ +69, +103, +126 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/user/UserSessionFilterTest.java": [ +116, +142, +159, +172, +198, +215, +228 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/user/UserUpdaterTest.java": [ +400 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/util/ObjectInputStreamIteratorTest.java": [ +51, +70 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/util/RubyUtilsTest.java": [ +56, +62 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/util/TypeValidationsTest.java": [ +64 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/util/cache/DiskCacheTest.java": [ +59, +75 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/util/cache/MemoryCacheTest.java": [ +65 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/ws/RemovedWebServiceHandlerTest.java": [ +45 +] +} diff --git a/java-checks-test-sources/default/src/main/java/checks/AssertThrowsInsteadOfTryCatchFailCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/AssertThrowsInsteadOfTryCatchFailCheckSample.java new file mode 100644 index 00000000000..366e29a46c7 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/AssertThrowsInsteadOfTryCatchFailCheckSample.java @@ -0,0 +1,84 @@ +package checks; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.*; + +public class AssertThrowsInsteadOfTryCatchFailCheckSample { + @Test + void tests() { + try { + raise(); + fail(); // Noncompliant {{Use assertThrows() instead of try/catch and fail() in the try block.}} +// ^^^^^^ + } catch (Exception _) { + // test passed + } + + try { + dontRaise(); + } catch (Exception _) { + fail(); // Noncompliant {{Use assertDoesNotThrow() instead of try/catch and fail() in the catch block.}} +// ^^^^^^ + } + + try { + raise(); + org.junit.Assert.fail("expected exception"); // Noncompliant {{Use assertThrows() instead of try/catch and fail() in the try block.}} +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + } catch (Exception _) { + // test passed + } + + try { + raise(); + junit.framework.Assert.fail("expected exception"); // Noncompliant {{Use assertThrows() instead of try/catch and fail() in the try block.}} +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + } catch (Exception _) { + // test passed + } + + try { + raise(); + org.fest.assertions.Fail.fail("expected exception"); // Noncompliant {{Use assertThrows() instead of try/catch and fail() in the try block.}} +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + } catch (Exception _) { + // test passed + } + + try { + raise(); + org.assertj.core.api.Fail.fail("expected exception"); // Noncompliant {{Use assertThrows() instead of try/catch and fail() in the try block.}} +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + } catch (Exception _) { + // test passed + } + + try { + raise(); + org.assertj.core.api.Assertions.fail("expected exception"); // Noncompliant {{Use assertThrows() instead of try/catch and fail() in the try block.}} +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + } catch (Exception _) { + // test passed + } + + try { + raise(); + org.assertj.core.api.Assertions.failBecauseExceptionWasNotThrown(IllegalStateException.class); // Noncompliant {{Use assertThrows() instead of try/catch and fail() in the try block.}} +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + } catch (Exception _) { + // test passed + } + + assertThrows(IllegalStateException.class, AssertThrowsInsteadOfTryCatchFailCheckSample::raise); // compliant + assertDoesNotThrow(AssertThrowsInsteadOfTryCatchFailCheckSample::dontRaise); // compliant + } + + private static void raise() { + throw new IllegalStateException(); + } + + private static void dontRaise() { + // do nothing + } +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/AssertThrowsInsteadOfTryCatchFailCheck.java b/java-checks/src/main/java/org/sonar/java/checks/AssertThrowsInsteadOfTryCatchFailCheck.java new file mode 100644 index 00000000000..293e2c069c9 --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/AssertThrowsInsteadOfTryCatchFailCheck.java @@ -0,0 +1,67 @@ +/* + * SonarQube Java + * 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.java.checks; + +import org.sonar.check.Rule; +import org.sonar.java.checks.helpers.UnitTestUtils; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.tree.*; + +import java.util.List; + +import static org.sonar.java.checks.helpers.UnitTestUtils.getJUnitVersion; + +@Rule(key = "S8714") +public class AssertThrowsInsteadOfTryCatchFailCheck extends IssuableSubscriptionVisitor { + + @Override + public List nodesToVisit() { + return List.of(Tree.Kind.CLASS, Tree.Kind.ENUM, Tree.Kind.INTERFACE, Tree.Kind.IMPLICIT_CLASS, Tree.Kind.RECORD, Tree.Kind.ANNOTATION_TYPE); + } + + @Override + public void visitNode(Tree tree) { + ClassTree classTree = (ClassTree) tree; + + List methods = classTree.members().stream() + .filter(member -> member.is(Tree.Kind.METHOD)) + .map(MethodTree.class::cast) + .toList(); + + int jUnitVersion = getJUnitVersion(methods); + if (jUnitVersion < 5) { + return; + } + + methods.forEach(method -> method.accept(tryStatementsVisitor)); + } + + private final BaseTreeVisitor tryStatementsVisitor = new BaseTreeVisitor() { + @Override + public void visitTryStatement(TryStatementTree tree) { + checkBlock(tree.block(), "Use assertThrows() instead of try/catch and fail() in the try block."); + tree.catches().forEach(c -> checkBlock(c.block(), "Use assertDoesNotThrow() instead of try/catch and fail() in the catch block.")); + super.visitTryStatement(tree); + } + + private void checkBlock(BlockTree block, String message) { + UnitTestUtils.findFail(block).ifPresent(fail -> + reportIssue(fail, message) + ); + } + }; +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/helpers/UnitTestUtils.java b/java-checks/src/main/java/org/sonar/java/checks/helpers/UnitTestUtils.java index ead40f817eb..b30e8c07b57 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/helpers/UnitTestUtils.java +++ b/java-checks/src/main/java/org/sonar/java/checks/helpers/UnitTestUtils.java @@ -19,6 +19,7 @@ import java.util.HashSet; import java.util.List; import java.util.Objects; +import java.util.Optional; import java.util.Set; import java.util.function.Predicate; import java.util.regex.Pattern; @@ -67,8 +68,8 @@ public final class UnitTestUtils { "io.restassured.response.ValidatableResponseOptions", // restassured 3.x and 4.x "io.restassured.module.mockmvc.response.ValidatableMockMvcResponse" // spring mock mvc extending the io.restassured library ) - .name(name -> "body" .equals(name) || - "time" .equals(name) || + .name(name -> "body".equals(name) || + "time".equals(name) || name.startsWith("time") || name.startsWith("content") || name.startsWith("status") || @@ -247,24 +248,44 @@ public static boolean methodNameMatchesAssertionMethodPattern(String methodName, if (TEST_METHODS_PATTERN.matcher(methodName).matches()) { return !REACTIVE_X_TEST_METHODS.matches(methodSymbol); } - if ("verify" .equals(methodName) || "failing" .equals(methodName)) { + if ("verify".equals(methodName) || "failing".equals(methodName)) { return !VERTX_TEST_CONTEXT_METHODS.matches(methodSymbol); } return ASSERTION_METHODS_PATTERN.matcher(methodName).matches(); } - public static boolean isTryCatchFail(BlockTree block) { + /** + * Checks if the given block tree's last statement is a call to a fail method, and if so, returns the corresponding method invocation tree. + * @param block the block tree to check + * @return an optional containing the method invocation tree if the last statement is a call to a fail method, or an empty optional otherwise + */ + public static Optional findFail(BlockTree block) { List statements = block.body(); if (statements.isEmpty()) { - return false; + return Optional.empty(); } StatementTree lastStatement = statements.get(statements.size() - 1); if (lastStatement.is(Tree.Kind.EXPRESSION_STATEMENT)) { ExpressionTree expression = ((ExpressionStatementTree) lastStatement).expression(); - return expression.is(Tree.Kind.METHOD_INVOCATION) && FAIL_METHOD_MATCHER.matches((MethodInvocationTree) expression); + return expression.is(Tree.Kind.METHOD_INVOCATION) && FAIL_METHOD_MATCHER.matches((MethodInvocationTree) expression) + ? Optional.of(expression) + : Optional.empty(); } - return false; + return Optional.empty(); } + public static int getJUnitVersion(List methods) { + boolean containsJUnit4Tests = false; + for (MethodTree methodTree : methods) { + SymbolMetadata metadata = methodTree.symbol().metadata(); + containsJUnit4Tests |= metadata.isAnnotatedWith("org.junit.Test"); + if (hasJUnit5TestAnnotation(methodTree)) { + // While migrating from JUnit4 to JUnit5, classes might end up in mixed state of having tests using both versions. + // If it's the case, we consider the test classes as ultimately targeting 5 + return 5; + } + } + return containsJUnit4Tests ? 4 : -1; + } } diff --git a/java-checks/src/main/java/org/sonar/java/checks/tests/AbstractOneExpectedExceptionRule.java b/java-checks/src/main/java/org/sonar/java/checks/tests/AbstractOneExpectedExceptionRule.java index 5411f979918..d9899a27da4 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/tests/AbstractOneExpectedExceptionRule.java +++ b/java-checks/src/main/java/org/sonar/java/checks/tests/AbstractOneExpectedExceptionRule.java @@ -34,8 +34,7 @@ import org.sonar.plugins.java.api.tree.MethodInvocationTree; import org.sonar.plugins.java.api.tree.Tree; import org.sonar.plugins.java.api.tree.TryStatementTree; - -import static org.sonar.java.checks.helpers.UnitTestUtils.isTryCatchFail; +import static org.sonar.java.checks.helpers.UnitTestUtils.findFail; public abstract class AbstractOneExpectedExceptionRule extends IssuableSubscriptionVisitor { @@ -120,7 +119,7 @@ private void visitMethodInvocation(MethodInvocationTree mit) { } private void visitTryStatement(TryStatementTree tryStatementTree) { - if (isTryCatchFail(tryStatementTree.block())) { + if (findFail(tryStatementTree.block()).isPresent()) { List expectedTypes = tryStatementTree.catches().stream().map(c -> c.parameter().type().symbolType()).toList(); reportMultipleCallInTree(expectedTypes, tryStatementTree.block(), tryStatementTree.tryKeyword(), "body of this try/catch"); } diff --git a/java-checks/src/main/java/org/sonar/java/checks/tests/JUnit45MethodAnnotationCheck.java b/java-checks/src/main/java/org/sonar/java/checks/tests/JUnit45MethodAnnotationCheck.java index e5ac57be591..171c2f7776b 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/tests/JUnit45MethodAnnotationCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/tests/JUnit45MethodAnnotationCheck.java @@ -25,7 +25,7 @@ import java.util.Set; import org.sonar.check.Rule; import org.sonarsource.analyzer.commons.collections.MapBuilder; -import org.sonar.java.checks.helpers.UnitTestUtils; +import static org.sonar.java.checks.helpers.UnitTestUtils.getJUnitVersion; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; import org.sonar.plugins.java.api.semantic.Symbol; import org.sonar.plugins.java.api.semantic.SymbolMetadata; @@ -71,20 +71,6 @@ public void visitNode(Tree tree) { } } - private static int getJUnitVersion(List methods) { - boolean containsJUnit4Tests = false; - for (MethodTree methodTree : methods) { - SymbolMetadata metadata = methodTree.symbol().metadata(); - containsJUnit4Tests |= metadata.isAnnotatedWith("org.junit.Test"); - if (UnitTestUtils.hasJUnit5TestAnnotation(methodTree)) { - // While migrating from JUnit4 to JUnit5, classes might end up in mixed state of having tests using both versions. - // If it's the case, we consider the test classes as ultimately targeting 5 - return 5; - } - } - return containsJUnit4Tests ? 4 : -1; - } - private void checkJUnitMethod(MethodTree methodTree, int jUnitVersion) { if (isSetupTearDownSignature(methodTree) || (jUnitVersion == 5 && isAnnotatedWith(methodTree, ORG_JUNIT_BEFORE, ORG_JUNIT_AFTER))) { checkSetupTearDownSignature(methodTree, jUnitVersion); diff --git a/java-checks/src/test/java/org/sonar/java/checks/AssertThrowsInsteadOfTryCatchFailCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/AssertThrowsInsteadOfTryCatchFailCheckTest.java new file mode 100644 index 00000000000..7038d9b293e --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/AssertThrowsInsteadOfTryCatchFailCheckTest.java @@ -0,0 +1,43 @@ +/* + * SonarQube Java + * 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.java.checks; + +import org.junit.jupiter.api.Test; +import org.sonar.java.checks.verifier.CheckVerifier; + +import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; + +class AssertThrowsInsteadOfTryCatchFailCheckTest { + + @Test + void detected() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/AssertThrowsInsteadOfTryCatchFailCheckSample.java")) + .withCheck(new AssertThrowsInsteadOfTryCatchFailCheck()) + .verifyIssues(); + } + + @Test + void undetected() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/AssertThrowsInsteadOfTryCatchFailCheckSample.java")) + .withCheck(new AssertThrowsInsteadOfTryCatchFailCheck()) + .withoutSemantic() + .verifyNoIssues(); + } + +} diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S8714.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S8714.html new file mode 100644 index 00000000000..0b39e6e4160 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S8714.html @@ -0,0 +1,106 @@ +

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

+

Why is this an issue?

+

Modern assertion libraries have made the clunky try-fail-catch pattern obsolete by introducing cleaner alternatives. For example, +starting with JUnit 5, JUnit Jupiter provides assertThrows and assertDoesNotThrow. AssertJ offers similar methods.

+

Using try-catch with fail() for the same purpose adds boilerplate, makes the test intent less explicit, and is harder to +maintain. Dedicated exception assertions also make it straightforward to keep asserting on the exception when one is expected.

+

How to fix it

+

Replace try-catch blocks that rely on fail() to verify exception behavior with dedicated exception assertions from your +testing library:

+
    +
  • Starting with JUnit 5, JUnit Jupiter provides assertDoesNotThrow and assertThrows.
    • +
  • AssertJ provides assertThatCode, assertThatThrownBy, and assertThatExceptionOfType.
    • +
+

Code examples

+

Noncompliant code example

+
+@Test
+void testNoExceptionThrown() {
+  try {
+    userService.registerUser(validUser);
+  } catch (ValidationException e) {
+    fail("Should not have thrown any exception");
+  }
+}
+
+

Compliant solution

+
+// JUnit 5
+@Test
+void testNoExceptionWithAssertion() {
+  assertDoesNotThrow(() -> userService.registerUser(validUser));
+}
+
+

Noncompliant code example

+
+@Test
+void testExceptionIsThrown() {
+  try {
+    userService.registerUser(invalidUser);
+    fail("Expected ValidationException to be thrown");
+  } catch (ValidationException e) {
+    // Test passes, but code is verbose
+    assertEquals("Invalid email", e.getMessage());
+  }
+}
+
+

Compliant solution

+
+// JUnit 5
+@Test
+void testExceptionWithAssertion() {
+  ValidationException exception = assertThrows(ValidationException.class, () -> userService.registerUser(invalidUser));
+
+  assertEquals("Invalid email", exception.getMessage());
+}
+
+

Noncompliant code example

+
+@Test
+void testNoExceptionThrown() {
+  try {
+    userService.registerUser(validUser);
+  } catch (ValidationException e) {
+    fail("Should not have thrown any exception");
+  }
+}
+
+

Compliant solution

+
+// AssertJ
+@Test
+void testNoExceptionWithAssertion() {
+  assertThatCode(() -> userService.registerUser(validUser)).doesNotThrowAnyException();
+}
+
+

Noncompliant code example

+
+@Test
+void testExceptionIsThrown() {
+  try {
+    userService.registerUser(invalidUser);
+    fail("Expected ValidationException to be thrown");
+  } catch (ValidationException e) {
+    assertThat(e).hasMessage("Invalid email");
+  }
+}
+
+

Compliant solution

+
+// AssertJ
+@Test
+void testExceptionWithAssertion() {
+  assertThatThrownBy(() -> userService.registerUser(invalidUser))
+    .isInstanceOf(ValidationException.class)
+    .hasMessage("Invalid email");
+}
+
+

Resources

+

Documentation

+ + diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S8714.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S8714.json new file mode 100644 index 00000000000..44d6ad1abed --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S8714.json @@ -0,0 +1,23 @@ +{ + "title": "Dedicated exception assertions should be used instead of \"try-catch\" with \"fail()\"", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [ + "tests" + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-8714", + "sqKey": "S8714", + "scope": "Tests", + "quickfix": "unknown", + "code": { + "impacts": { + "MAINTAINABILITY": "MEDIUM" + }, + "attribute": "CONVENTIONAL" + } +} diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_agentic_AI_profile.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_agentic_AI_profile.json index 49cea8eb4c2..f753df99887 100644 --- a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_agentic_AI_profile.json +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_agentic_AI_profile.json @@ -468,6 +468,7 @@ "S8450", "S8465", "S8469", - "S8696" + "S8696", + "S8714" ] } diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_way_profile.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_way_profile.json index c4ea4993f94..17a7f7bc53c 100644 --- a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_way_profile.json +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_way_profile.json @@ -536,6 +536,7 @@ "S8694", "S8695", "S8696", - "S8700" + "S8700", + "S8714" ] } diff --git a/sonar-java-plugin/src/test/java/org/sonar/plugins/java/JavaAgenticWayProfileTest.java b/sonar-java-plugin/src/test/java/org/sonar/plugins/java/JavaAgenticWayProfileTest.java index 07b8e02b4e2..d890a914649 100644 --- a/sonar-java-plugin/src/test/java/org/sonar/plugins/java/JavaAgenticWayProfileTest.java +++ b/sonar-java-plugin/src/test/java/org/sonar/plugins/java/JavaAgenticWayProfileTest.java @@ -53,7 +53,7 @@ void profile_is_registered_as_expected() { BuiltInQualityProfilesDefinition.BuiltInQualityProfile actualProfile = profilesPerLanguages.get("java").get("Sonar agentic AI"); assertThat(actualProfile.isDefault()).isFalse(); assertThat(actualProfile.rules()) - .hasSize(468) + .hasSize(469) .extracting(BuiltInQualityProfilesDefinition.BuiltInActiveRule::ruleKey) .doesNotContainAnyElementsOf(List.of( "S101",