-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
|
||
| // Restrict this detector to `AnnotationUsageType.DEFINITION` following | ||
| // similar examples in AOSP like `IgnoreWithoutReasonDetector`. | ||
| override fun isApplicableAnnotationUsage(type: AnnotationUsageType): Boolean = |
There was a problem hiding this comment.
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
| usageInfo: AnnotationUsageInfo, | ||
| ) { | ||
| val method = usageInfo.referenced as? PsiMethod ?: return | ||
| val method = annotationInfo.annotation.uastParent as? UMethod ?: return |
There was a problem hiding this comment.
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
| fun `test foo`() = Unit | ||
| @Test | ||
| fun test_foo2() { |
There was a problem hiding this comment.
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.
What I have done and why
Fix #1984 where
TestMethodNameDetectordoes not scan some tests.The reason this is happening is because the annotations in the unit tests are of usage type
AnnotationUsageType.DEFINTIONbut the detector does not opt in to scan these.I have based my solution on
IgnoreWithoutReasonDetectorwhich is conceptually similar.