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

Feature request: add configuration option for excluding some source sets from ABI analysis. #1026

Closed
ianbrandt opened this issue Nov 7, 2023 · 9 comments · Fixed by #1058
Labels
enhancement New feature or request toolchain:kotlin
Milestone

Comments

@ianbrandt
Copy link

ianbrandt commented Nov 7, 2023

Build scan link
https://scans.gradle.com/s/6srk2bfreceje

Plugin version
1.23.0 and 1.25.0

Gradle version
8.4

JDK version
17

(Optional) Android Gradle Plugin (AGP) version
N/A

(Optional) reason output for bugs relating to incorrect advice

----------------------------------------
You asked about the dependency 'org.junit.jupiter:junit-jupiter-api:5.10.0'.
You have been advised to change this dependency to 'integrationTestApi' from 'integrationTestImplementation'.
----------------------------------------

...

Source: integrationTest
-----------------------
* Exposes 2 classes: org.junit.jupiter.api.Test, org.junit.jupiter.api.extension.RegisterExtension (implies integrationTestApi).

Describe the bug
It appears the Kotlin Gradle Plugin automatically adds a testApi configuration to projects. In combination with the Gradle JVM Test Suite Plugin, this results in the addition of <testSuiteName>Api configurations as well.

I'm not sure if this is a regression or intentional, but starting with 1.23.0 I'm getting recommendations to change <testSuiteName>Implementation dependencies to <testSuiteName>Api if they're exposed publicly at all. I do not get any recommendations to change testImplementation dependencies to testApi, so this appears to only impact my custom JVM Test Suite Plugin suites.

If I revert to 1.22.0, I no longer receive these recommendations.

To Reproduce

  1. Run gradlew buildHealth on https://github.com/sdkotlin/sd-kotlin-talks/tree/410287b13db359a16a57bbd99ba9ebe9a5f99a1f.

Expected behavior
No suggestions to change integrationTestImplementation dependencies to integrationTestApi are reported, or if this change is intentional, I'd like to request a configuration flag for ABI filtering test classes for all JVM Test Suite Plugin added test suites.

Additional context
I have my build set to fail on incorrect configurations, so this is blocking me from upgrading past 1.22.0. (I confess that I haven't yet tried to use ABI Filtering as a workaround.)

My Kotlin JUnit 5 test classes are internal, though I understand that compiles to public on the JVM. Without a @JvmPackagePrivate interop annotation, or a native package-private access modifier in Kotlin, I don't believe there's anything I can do to keep test dependencies out of test class public ABI. Even with either of those features, JUnit 4 requires test classes and @Test-annotated methods to be public. I have many legacy tests still on JUnit 4 via the JUnit 5 Vintage Engine.

I also intend to file a request for the Kotlin Gradle Plugin to stop adding the testApi configuration. I don't believe it makes sense, as project test code generally shouldn't be depended on. Gradle offers Test Fixtures for sharing test code. That said, its removal would probably require a deprecation period, and I imagine this plugin could be adapted to once again ignore <testSuiteName>Api configurations on a much shorter timescale.

@ianbrandt
Copy link
Author

Upstream issue filed (though I believe this issue is still relevant in the meantime): https://youtrack.jetbrains.com/issue/KT-63285/KGP-Deprecate-and-remove-testApi-configuration.

@autonomousapps
Copy link
Owner

Thank you for the issue. I've been aware of it for a while, and it is the result of KGP adding ...Api configurations for custom source sets. I believe this is fixed for KGP 1.9.20. Which version of KGP are you using?

@ianbrandt
Copy link
Author

Ah, yes, I forgot to include that detail in the post. The linked reproducer is on 1.9.20. I get these when I run :di-with-koin:dependencies:

...

integrationTestApi - API dependencies for null/integrationTest (n)
No dependencies

integrationTestApiDependenciesMetadata
\--- org.jetbrains.kotlin:kotlin-stdlib:1.9.20

...

testApi - API dependencies for null/test (n)
No dependencies

testApiDependenciesMetadata
No dependencies

@autonomousapps
Copy link
Owner

That's pretty frustrating. I guess JB didn't release this yet, or I'm misunderstanding the change.

I'm contemplating a feature for the plugin to let users indicate that certain sourcesets do not have an API, even if an ...Api configuration exists. Would that resolve your issue?

@ianbrandt
Copy link
Author

ianbrandt commented Nov 8, 2023

I believe it would. I'd foresee adding the configuration to the precompiled script plugins I use to configure each test suite, e.g.:

https://github.com/sdkotlin/sd-kotlin-talks/blob/b9ded1befbea60858a4c6427e10ecff505ae0945/build-logic/src/main/kotlin/org.sdkotlin.buildlogic.test.integration-test-suite.gradle.kts.

I just have the one in that toy project, but in my work project I have several very similar ones for each test suite (legacy, gui, load, etc.).

@autonomousapps autonomousapps added enhancement New feature or request and removed bug Something isn't working more information needed labels Nov 8, 2023
@autonomousapps autonomousapps changed the title Custom <testSuiteName>Api configurations no longer ignored as of 1.23.0 Feature request: add configuration option for excluding some source sets from ABI analysis. Nov 8, 2023
@autonomousapps autonomousapps added this to the next milestone Nov 8, 2023
ianbrandt added a commit to sdkotlin/sd-kotlin-talks that referenced this issue Dec 4, 2023
@ianbrandt
Copy link
Author

I upgraded to 1.27.0, and added an ABI analysis exclusion for my "integrationTest" source set:

sdkotlin/sd-kotlin-talks@22226ea#diff-fd6471cf24707cb9fd6dd3d7c3d244b31b9fbed49d3f870b761423ad2cef8146R75

Full tree:

https://github.com/sdkotlin/sd-kotlin-talks/tree/22226eabc979674260fe156de540fb3c3313ebca.

Now I'm getting:

Advice for :di-with-koin
Existing dependencies which should be modified to be as indicated:
  integrationTestImplementation("org.jetbrains.kotlin:kotlin-stdlib:1.9.21") (was integrationTestApi)

My guess is it's the Kotlin Gradle Plugin that's automatically adding the stdlib to the integrationTestApi configuration in given the custom JVM Test Suite.

Is this user error on my part, or would this be a new issue/feature request?

@autonomousapps
Copy link
Owner

Is this user error on my part, or would this be a new issue/feature request?

KGP whhhyyyy

Could you file a new issue? I'm not sure if this is a bug in my new implementation, an old bug you're somehow surfacing, or what 😅

@ianbrandt
Copy link
Author

Done: #1059.

On the upside, https://youtrack.jetbrains.com/issue/KT-63285/KGP-Deprecate-and-remove-testApi-configuration was recently updated to a target of 2.0.0-Beta3.

@ianbrandt
Copy link
Author

Possible partial regression with Kotlin 2.0.20-RC issue filed: #1239.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request toolchain:kotlin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants