Skip to content

Fix #1984 where TestMethodNameDetector does not scan some tests #1985

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
drawers wants to merge 1 commit into
base: main
Choose a base branch
from
+27 −8
Open

Fix #1984 where TestMethodNameDetector does not scan some tests #1985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion lint/src/main/kotlin/com/google/samples/apps/nowinandroid/lint/TestMethodNameDetector.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package com.google.samples.apps.nowinandroid.lint

import com.android.tools.lint.detector.api.AnnotationInfo
import com.android.tools.lint.detector.api.AnnotationUsageInfo
import com.android.tools.lint.detector.api.AnnotationUsageType
import com.android.tools.lint.detector.api.Category.Companion.TESTING
import com.android.tools.lint.detector.api.Detector
import com.android.tools.lint.detector.api.Implementation
Expand All @@ -31,6 +32,7 @@ import com.android.tools.lint.detector.api.SourceCodeScanner
import com.android.tools.lint.detector.api.TextFormat.RAW
import com.intellij.psi.PsiMethod
import org.jetbrains.uast.UElement
import org.jetbrains.uast.UMethod
import java.util.EnumSet
import kotlin.io.path.Path

Expand All @@ -43,13 +45,18 @@ class TestMethodNameDetector : Detector(), SourceCodeScanner {

override fun applicableAnnotations() = listOf("org.junit.Test")

// Restrict this detector to `AnnotationUsageType.DEFINITION` following
// similar examples in AOSP like `IgnoreWithoutReasonDetector`.
override fun isApplicableAnnotationUsage(type: AnnotationUsageType): Boolean =
Copy link
Contributor Author

@drawers drawers Nov 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't override this function from the superclass, we get the following implementation which skips AnnotationUsageType.DEFINITION:

  open fun isApplicableAnnotationUsage(type: AnnotationUsageType): Boolean {
    // Some annotation usages are off by default (typically because they were introduced
    // later and might introduce false positives in third party checks; checks should
    // opt into these.
    return when (type) {
      AnnotationUsageType.BINARY,
      AnnotationUsageType.EQUALITY,
      AnnotationUsageType.DEFINITION,
      AnnotationUsageType.IMPLICIT_CONSTRUCTOR,
      AnnotationUsageType.IMPLICIT_CONSTRUCTOR_CALL -> false
      else -> true
    }
  }

Hence the problem where test functions with an annotation that is not AnnotationUsageType.METHOD_RETURN aren't scanned.

Docs for AnnotationUsageType.DEFINITION suggest opting in like this:

override fun isApplicableAnnotationUsage(type: AnnotationUsageType): Boolean {
    return type == AnnotationUsageType.DEFINITION || super.isApplicableAnnotationUsage(type)
}

but if I do that, I get duplicate reports. In other words, the same problem is reported twice. So I have based the solution on this one that restricts annotations to just AnnotationUsageType.DEFINITION

type == AnnotationUsageType.DEFINITION

override fun visitAnnotationUsage(
context: JavaContext,
element: UElement,
annotationInfo: AnnotationInfo,
usageInfo: AnnotationUsageInfo,
) {
val method = usageInfo.referenced as? PsiMethod ?: return
val method = annotationInfo.annotation.uastParent as? UMethod ?: return
Copy link
Contributor Author

@drawers drawers Nov 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to change this line because for some annotations usageInfo.referenced will return null.

I based the new interpretation on similar logic in IgnoreWithoutReasonDetector. One doubt would be what happens when there are multiple annotations. If I do context.uastFile.asRecursiveLogString() to look at the UAT nodes, these are still siblings within a single parent where the parent is the UMethod.

UMethod (name = test_foo2)
    UAnnotation (fqName = org.junit.Test)
    UAnnotation (fqName = kotlin.Deprecated)
    UBlockExpression


method.detectPrefix(context, usageInfo)
method.detectFormat(context, usageInfo)
Expand Down
26 changes: 19 additions & 7 deletions lint/src/test/kotlin/com/google/samples/apps/nowinandroid/lint/TestMethodNameDetectorTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ class TestMethodNameDetectorTest {
fun test_foo() = Unit
@Test
fun `test foo`() = Unit

@Test
fun test_foo2() {
Copy link
Contributor Author

@drawers drawers Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous version of the TestMethodNameDetector would not have reported on this method. In other words, a false negative.

// Blank body.
}
}
""",
).indented(),
Expand All @@ -53,19 +58,26 @@ class TestMethodNameDetectorTest {
src/Test.kt:8: Warning: Test method starts with test [TestMethodPrefix]
fun `test foo`() = Unit
~~~~~~~~~~
0 errors, 2 warnings
src/Test.kt:11: Warning: Test method starts with test [TestMethodPrefix]
fun test_foo2() {
~~~~~~~~~
0 errors, 3 warnings
""".trimIndent(),
)
.expectFixDiffs(
"""
Autofix for src/Test.kt line 6: Remove prefix:
@@ -6 +6
- fun test_foo() = Unit
+ fun foo() = Unit
@@ -6 +6 @@
- fun test_foo() = Unit
+ fun foo() = Unit
Autofix for src/Test.kt line 8: Remove prefix:
@@ -8 +8
- fun `test foo`() = Unit
+ fun `foo`() = Unit
@@ -8 +8 @@
- fun `test foo`() = Unit
+ fun `foo`() = Unit
Autofix for src/Test.kt line 11: Remove prefix:
@@ -11 +11 @@
- fun test_foo2() {
+ fun foo2() {
""".trimIndent(),
)
}
Expand Down
Loading