Skip to content
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

consider ignoreQualifier arguments when checking for duplicate bindings #1033

Merged
merged 2 commits into from
Jul 17, 2024

Conversation

RBusarow
Copy link
Member

fixes #986

@RBusarow RBusarow requested a review from JoelWilcox July 16, 2024 21:01
@RBusarow RBusarow marked this pull request as ready for review July 16, 2024 21:02
abstract class AnvilAnnotationsTest(
firstAnnotation: KClass<out Annotation>,
vararg fullTestRunAnnotations: KClass<out Annotation>,
) : KaseTestFactory<
Copy link
Member

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?

Copy link
Member Author

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.

Comment on lines -33 to -48
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)
}
}
}
}
Copy link
Member

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

Copy link
Member Author

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.

@RBusarow RBusarow force-pushed the rick/duplicate-binding-qualifier branch from 2f9061f to 4df2fb9 Compare July 17, 2024 17:16
import kotlin.reflect.KClass

/**
* Creates test factories to run the same test against multiple annotations.
Copy link
Member

Choose a reason for hiding this comment

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

Doc comment looks great!

@RBusarow RBusarow merged commit 132f198 into main Jul 17, 2024
17 checks passed
@RBusarow RBusarow deleted the rick/duplicate-binding-qualifier branch July 17, 2024 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate ContributesMultibinding check does not consider ignoreQualifier
2 participants