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
Allow additional types in @KotlinCoreEnvironmentTest annotation #5188
Conversation
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.
Thanks for this :)
But I'm wondering... Shouldn't we add "by default" all the classes that we have inside the classpath
? I mean, in my case my code compiled correctly. I mean, the compiler knew all the types correctly. But the env
didn't know abot them. That's realy strange as a test writer and that could create odd issues difficult to spot. We use a lot of safe calls and maybe we are thinking that our tests are working because of X but they are just working because the BindingContext
is not the correct one.
import org.junit.jupiter.api.Nested | ||
import org.junit.jupiter.api.Test | ||
|
||
internal class KotlinCoreEnvironmentTestSpec { |
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.
Shouldn't these tests be inside detekt-test-utils
instead of detekt-test
?
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.
Technically yes. I wanted to test exactly your case from #5182 and detekt-test-utils
is completely unaware of detekt rules and I wanted to keep it that way. Should I create an additional test in detekt-test-utils
?
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 reason why the KotlinCoreEnvironmentExtensions
are in detekt-test
? This makes it really hard to test anything in detekt-test-utils
.
That is acutally not the case. For regular test execution, the compilation is effectively deactivated (
I do not think so. The BindingContext should (in general) only need to resolve kotlin language types. For library specific rule sets this is may be different. There it might be reasonable to add spring or compose or ... to the test compiler via this annotation. |
I had one of the tests that was invalid but I just fixed it (a |
(I'm ok merging this but I think that we should take a look on that because it could create some noise for us and/or third party authors) |
You are right. The Maybe @3flex can shed some light on this. I only have a very vague understanding of what is going on here. |
I think I can also shed some light on the |
No, the issue that we have is that in #5182 the test snippets compile perfectly fine BUT the We can fix it with this PR but the behaviour is counter intuitive and third party rule authors are more likely to find this error because they want to check integration with libraries (that's the case of #5182 where I'm checking against detekt itself). |
} | ||
|
||
@Nested | ||
@KotlinCoreEnvironmentTest(additionalTypes = [Rule::class, CharRange::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.
Commenting here but sort of valid for the whole PR. I really like this new test feature, but at the same time I'm afraid it might reduce readability.
In this line, it's not immediately clear why you need to rely on CharRange
.
Plus this reduces the test isolation as you're effectively exposing those tests to failures if the Rule
class change (which might be considered a 'good' end result, but it's not what this test is trying to accomplish). Having a Rule
class declared locally in a snippet should serve the same purpose no?
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.
@BraisGabin and you are right. I will move the test to detekt-test-utils
and check against some other class being available
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.
In this case, I would use just lint()
rather than compileAndLint()
or merge this PR with the workaround. I don’t it’s so critical.
Let me try to summarize the discussion here as there are multiple threads.
|
This change allows #5182 to be tested.
When the test compiler is initialized, only the kotlin standard lib and kotlin coroutines are available to the binding context. This change allows additional jars to be added to the compiler environment via the
@KotlinCoreEnvironmentTest
annotation. The jars are determined by defining a type from the jar that should be added.