-
Notifications
You must be signed in to change notification settings - Fork 91
consider ignoreQualifier arguments when checking for duplicate bindings
#1033
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
Conversation
| abstract class AnvilAnnotationsTest( | ||
| firstAnnotation: KClass<out Annotation>, | ||
| vararg fullTestRunAnnotations: KClass<out Annotation>, | ||
| ) : KaseTestFactory< |
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.
Is there a way to use KaseTestFactory via composition or a test rule instead of introducing a base class?
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.
No to the Rule, technically yes to the base class. But the intent is for it to be used for the other tests with annotation parameters as well (there are five Binding___Test in total). The setup for each is identical except for those parameters.
| private val annotation = "@${annotationClass.simpleName}" | ||
| private val import = "import ${annotationClass.java.canonicalName}" | ||
|
|
||
| companion object { | ||
| @Parameters(name = "{0}") | ||
| @JvmStatic | ||
| fun annotationClasses(): Collection<Any> { | ||
| return buildList { | ||
| add(MergeComponent::class) | ||
| if (isFullTestRun()) { | ||
| add(MergeSubcomponent::class) | ||
| add(MergeModules::class) | ||
| } | ||
| } | ||
| } | ||
| } |
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.
Is the move to kase necessary here? I feel like the further these details move away from the test, the harder it is to reason about and maintain vs keeping it simple and using the Junit apis directly. E.g. it's no longer clear that this test suite is impacted by the isFullTestRun() check
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.
It's not strictly necessary. Most of these tests were failing at some point during or after ignoreQualifier change, and it's much easier to debug them when you have the consistent working directories from Kase. Any time I run into a similar situation with JUnit 4 tests, I've been migrating them over to Kase.
I would also argue that the mechanism for parameterized tests in JUnit 4 is more magical than simple -- at least for me.
compiler/src/test/java/com/squareup/anvil/compiler/testing/AnvilAnnotationsTest.kt
Outdated
Show resolved
Hide resolved
compiler/src/test/java/com/squareup/anvil/compiler/testing/AnvilAnnotationsTest.kt
Show resolved
Hide resolved
2f9061f to
4df2fb9
Compare
| import kotlin.reflect.KClass | ||
|
|
||
| /** | ||
| * Creates test factories to run the same test against multiple annotations. |
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.
Doc comment looks great!
fixes #986