SONARJAVA-5258 Report secondary location on annotation in S1607. (#4969)

Co-authored-by: Dorian Burihabwa <[email protected]>

java-checks-test-sources/default/src/test/java/checks/tests/IgnoredTestsCheckSample.java

@@ -6,13 +6,17 @@
 
 abstract class IgnoredTestsCheckSample {
   @org.junit.Ignore
+//^^^^^^^^^^^^^^^^^>
   void foo() {} // Noncompliant {{Either add an explanation about why this test is skipped or remove the "@Ignore" annotation.}}
 //     ^^^
 
   @Ignore
+//^^^^^^^>
   void bar() {} // Noncompliant
+//     ^^^
 
   @Disabled
+//^^^^^^^^^>
   void disabledJunit5() {} // Noncompliant {{Either add an explanation about why this test is skipped or remove the "@Disabled" annotation.}}
 //     ^^^^^^^^^^^^^^
 

java-checks/src/main/java/org/sonar/java/checks/helpers/AnnotationsHelper.java

@@ -28,4 +28,10 @@ public static boolean hasUnknownAnnotation(SymbolMetadata symbolMetadata) {
     return symbolMetadata.annotations().stream().anyMatch(annotation -> annotation.symbol().isUnknown());
   }
 
+  /**
+   * Returns the `name` part of a `fully.qualified.name`, that is, the part after the last dot.
+   */
+  public static String annotationTypeIdentifier(String fullyQualified) {
+    return fullyQualified.substring(fullyQualified.lastIndexOf('.') + 1);
+  }
 }

java-checks/src/main/java/org/sonar/java/checks/tests/IgnoredTestsCheck.java

@@ -27,12 +27,15 @@
 import org.sonar.plugins.java.api.semantic.MethodMatchers;
 import org.sonar.plugins.java.api.semantic.SymbolMetadata;
 import org.sonar.plugins.java.api.semantic.Type;
+import org.sonar.plugins.java.api.tree.AnnotationTree;
 import org.sonar.plugins.java.api.tree.BlockTree;
 import org.sonar.plugins.java.api.tree.ExpressionStatementTree;
 import org.sonar.plugins.java.api.tree.MethodInvocationTree;
 import org.sonar.plugins.java.api.tree.MethodTree;
 import org.sonar.plugins.java.api.tree.Tree;
 
+import static org.sonar.java.checks.helpers.AnnotationsHelper.annotationTypeIdentifier;
+
 @Rule(key = "S1607")
 public class IgnoredTestsCheck extends IssuableSubscriptionVisitor {
 
@@ -51,12 +54,20 @@ public List<Tree.Kind> nodesToVisit() {
   public void visitNode(Tree tree) {
     MethodTree methodTree = (MethodTree) tree;
     SymbolMetadata symbolMetadata = methodTree.symbol().metadata();
+
     // check for @Ignore or @Disabled annotations
-    boolean hasIgnoreAnnotation = isSilentlyIgnored(symbolMetadata, "org.junit.Ignore");
-    boolean hasDisabledAnnotation = isSilentlyIgnored(symbolMetadata, "org.junit.jupiter.api.Disabled");
-    if (hasIgnoreAnnotation || hasDisabledAnnotation) {
-      reportIssue(methodTree.simpleName(), String.format("Either add an explanation about why this test is skipped or remove the " +
-        "\"@%s\" annotation.", hasIgnoreAnnotation ? "Ignore" : "Disabled"));
+    for (String annotationName : List.of("org.junit.Ignore", "org.junit.jupiter.api.Disabled")) {
+      getSilentlyIgnoredAnnotation(symbolMetadata, annotationName)
+        .ifPresent(annotationTree -> {
+          String shortName = annotationTypeIdentifier(annotationName);
+          String message = String.format(
+            "Either add an explanation about why this test is skipped or remove the \"@%s\" annotation.",
+            shortName
+          );
+          var secondaryLocation =
+            new JavaFileScannerContext.Location(String.format("@%s annotation skips the test", shortName), annotationTree);
+          context.reportIssue(this, methodTree.simpleName(), message, Collections.singletonList(secondaryLocation), null);
+        });
     }
 
     // check for "assumeFalse(true)" and "assumeTrue(false)"-calls, which may also result in permanent skipping of the given test
@@ -80,21 +91,26 @@ public void visitNode(Tree tree) {
     }
   }
 
-  private static boolean isSilentlyIgnored(SymbolMetadata symbolMetadata, String fullyQualifiedName) {
+  /**
+   * If a test method is silently ignored, returns the annotation that causes this behavior.
+   */
+  private static Optional<AnnotationTree> getSilentlyIgnoredAnnotation(SymbolMetadata symbolMetadata, String fullyQualifiedName) {
     // This code duplicates the behavior of SymbolMetadata.valuesForAnnotation but checks for broken semantics
     for (SymbolMetadata.AnnotationInstance annotation : symbolMetadata.annotations()) {
       Type type = annotation.symbol().type();
       // In case of broken semantics, the annotation may match the fully qualified name but still miss the type binding.
       // As a consequence, fetching the values from the annotation returns an empty list, as if there were no value, even though there might be one or more.
       // In such cases, it is best to consider that the test is not ignored.
       if (type.isUnknown()) {
-        return false;
+        return Optional.empty();
       }
       if (type.is(fullyQualifiedName)) {
-        return annotation.values().isEmpty();
+        return annotation.values().isEmpty()
+          ? Optional.ofNullable(symbolMetadata.findAnnotationTree(annotation))
+          : Optional.empty();
       }
     }
-    return false;
+    return Optional.empty();
   }
 
   private static boolean hasConstantOppositeArg(MethodInvocationTree mit) {

java-checks/src/main/java/org/sonar/java/filters/LombokFilter.java

@@ -22,7 +22,6 @@
 import java.util.Optional;
 import java.util.Set;
 import javax.annotation.Nullable;
-import org.sonar.java.annotations.VisibleForTesting;
 import org.sonar.java.checks.AtLeastOneConstructorCheck;
 import org.sonar.java.checks.CollectionInappropriateCallsCheck;
 import org.sonar.java.checks.ConstantsShouldBeStaticFinalCheck;
@@ -52,6 +51,8 @@
 import org.sonar.plugins.java.api.tree.Tree;
 import org.sonar.plugins.java.api.tree.VariableTree;
 
+import static org.sonar.java.checks.helpers.AnnotationsHelper.annotationTypeIdentifier;
+
 public class LombokFilter extends BaseTreeVisitorIssueFilter {
 
   private static final Set<Class<? extends JavaCheck>> FILTERED_RULES = Set.of(
@@ -201,11 +202,6 @@ private static boolean usesAnnotation(ClassTree classTree, List<String> annotati
     return false;
   }
 
-  @VisibleForTesting
-  static String annotationTypeIdentifier(String fullyQualified) {
-    return fullyQualified.substring(fullyQualified.lastIndexOf('.') + 1);
-  }
-
   private static boolean generatesNonPublicConstructor(ClassTree classTree) {
     if (usesAnnotation(classTree, UTILITY_CLASS, true)) {
       return true;

java-checks/src/test/java/org/sonar/java/checks/helpers/AnnotationsHelperTest.java

@@ -0,0 +1,31 @@
+/*
+ * SonarQube Java
+ * Copyright (C) 2012-2024 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 Sonar Source-Available License Version 1, as published by SonarSource SA.
+ *
+ * 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.helpers;
+
+import org.junit.jupiter.api.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.sonar.java.checks.helpers.AnnotationsHelper.annotationTypeIdentifier;
+
+class AnnotationsHelperTest {
+  @Test
+  void testAnnotationTypeIdentifier() {
+    assertThat(annotationTypeIdentifier("noDot")).isEqualTo("noDot");
+    assertThat(annotationTypeIdentifier("one.dot")).isEqualTo("dot");
+    assertThat(annotationTypeIdentifier("many.many.dots")).isEqualTo("dots");
+  }
+}

java-checks/src/test/java/org/sonar/java/filters/LombokFilterTest.java

@@ -18,9 +18,6 @@
 
 import org.junit.jupiter.api.Test;
 
-import static org.assertj.core.api.Assertions.assertThat;
-import static org.sonar.java.filters.LombokFilter.annotationTypeIdentifier;
-
 class LombokFilterTest {
 
   @Test
@@ -35,11 +32,4 @@ void testWithoutSemantic() {
       .withoutSemantic()
       .verify("src/test/files/filters/LombokFilterWithoutSemantic.java", new LombokFilter());
   }
-
-  @Test
-  void testAnnotationTypeIdentifier() {
-    assertThat(annotationTypeIdentifier("noDot")).isEqualTo("noDot");
-    assertThat(annotationTypeIdentifier("one.dot")).isEqualTo("dot");
-    assertThat(annotationTypeIdentifier("many.many.dots")).isEqualTo("dots");
-  }
 }