Check excluding stdlib entries in KotlinPluginsTest#1371
Conversation
There was a problem hiding this comment.
Code Review
The pull request introduces a new test parameter excludeStdlib to check the behavior of the shadow plugin when the Kotlin standard library is excluded. This is a valuable addition to ensure the plugin functions correctly under different configurations. The changes look good overall, but there are a few areas that could be improved for clarity and maintainability.
Summary of Findings
- Test clarity with
containsOnly: The use ofcontainsOnlywhenexcludeStdlibis true provides a clear and strict assertion about the contents of the shadow JAR. This is a good approach for verifying the exclusion of the standard library. - Duplication in test setup: The
stdlibvariable andwriteGradleProperties()call are duplicated in both test methods. Consider extracting this logic into a shared function to reduce redundancy.
Merge Readiness
The changes are well-structured and the addition of the excludeStdlib parameter enhances the test coverage. However, addressing the duplication in test setup would improve the code's maintainability. I recommend addressing the duplication issue before merging. I am unable to approve this pull request, and recommend that others review and approve this code before merging.
src/functionalTest/kotlin/com/github/jengelman/gradle/plugins/shadow/KotlinPluginsTest.kt
Outdated
Show resolved
Hide resolved
src/functionalTest/kotlin/com/github/jengelman/gradle/plugins/shadow/KotlinPluginsTest.kt
Outdated
Show resolved
Hide resolved
cf6339d to
3423db8
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request introduces a new test parameter excludeStdlib to check the behavior of the shadow plugin when the Kotlin standard library is excluded. This is a valuable addition to ensure the plugin correctly handles different dependency configurations. The implementation is generally well-structured and the tests cover both cases (with and without stdlib).
Merge Readiness
The code changes are well-structured and the tests cover both cases (with and without stdlib). I would recommend merging this pull request.
Follow up #1339.