Fix a bunch of issues in the containers APIs #1116
Conversation
#1104 Signed-off-by: Paul Merlin <[email protected]>
#1042 Signed-off-by: Paul Merlin <[email protected]>
Signed-off-by: Paul Merlin <[email protected]>
Signed-off-by: Paul Merlin <[email protected]>
and collect incoherences Signed-off-by: Paul Merlin <[email protected]>
#1108 Signed-off-by: Paul Merlin <[email protected]>
Signed-off-by: Paul Merlin <[email protected]>
Signed-off-by: Paul Merlin <[email protected]>
#1108 Signed-off-by: Paul Merlin <[email protected]>
…n<T> Signed-off-by: Paul Merlin <[email protected]>
by allowing eval tests to opt-in for generated gradle api extensions Signed-off-by: Paul Merlin <[email protected]>
by fixing unused variable compiler warning Signed-off-by: Paul Merlin <[email protected]>
by simplifying jar file creation Signed-off-by: Paul Merlin <[email protected]>
| import org.junit.Test | ||
|
|
||
|
|
||
| class NamedContainersEvalTest : TestWithTempFiles() { |
| } | ||
|
|
||
| private | ||
| fun assertConfigurationsExtendsFrom(name: String, script: String, configuration: Project.() -> Unit = {}) { |
There was a problem hiding this comment.
❌ This should occur after its use on the test methods.
| @Test | ||
| fun `monomorphic named domain object container api`() { | ||
|
|
||
| assertConfigurationsExtendsFrom("api", """ |
There was a problem hiding this comment.
❌ The name of this function made me think that the tested configurations should all extend from a configuration named "api". How about renaming to something like testConfigurationContainerVia("api", """...""")?
| import kotlin.reflect.KClass | ||
|
|
||
|
|
||
| class TaskContainerEvalTest : TestWithTempFiles() { |
|
|
||
| class TaskContainerEvalTest : TestWithTempFiles() { | ||
|
|
||
| companion object { |
There was a problem hiding this comment.
❌ tests should always come first
| import java.io.File | ||
|
|
||
|
|
||
| fun File.newProjectBuilder(name: String = this.name): ProjectBuilder = |
There was a problem hiding this comment.
⭕ a File extension seems wrong here. I'd define this as a regular factory function like newProjectBuilderWith(projectDir: File, name: String = projectDir.name)
| */ | ||
| fun Project.eval( | ||
| script: String, | ||
| baseCacheDir: File = file(".gradle/kotlin-dsl-eval-cache"), |
There was a problem hiding this comment.
❌ this creates interference among the tests because:
- the simplified evaluator only hashes the content of the script
- it always overwrites the cache contents
I'd move this utility to a base test class where it can set the default baseCacheDir pointing to its own temp folder.
| val t3: Task = tasks.getByName<Task>("foo") | ||
|
|
||
| val t4: Task = tasks.getByName("bar") { | ||
| description += "!" |
There was a problem hiding this comment.
❌ it would be better to differentiate the strings added to the description so the proper ordering of execution is also tested.
Signed-off-by: Paul Merlin <[email protected]>
Signed-off-by: Paul Merlin <[email protected]>
|
Thanks for the careful review @bamboo, this is ready for another look |
| /** | ||
| * Provides a [TaskProvider] delegate for the task of the given type named after the property after configuring it with the given action. | ||
| */ | ||
| operator fun <U : Task> ExistingDomainObjectDelegateProviderWithTypeAndAction<out TaskContainer, U>.provideDelegate( |
There was a problem hiding this comment.
👀 That is quite a class name. 👀
| */ | ||
| @Suppress("extension_shadowed_by_member") | ||
| inline fun <reified T : Task> TaskContainer.register(name: String): TaskProvider<T> = | ||
| register(name, T::class.java) |
There was a problem hiding this comment.
Seeing a @Suppress("extension_shadowed_by_member") usually flags something isn't going to work correctly in my experience.
Do you have a test that ensures this can be called without an issue?
There was a problem hiding this comment.
Yes, look at other files changed in this PR
This PR covers #1104, #1042, #1108 and contains other related fixes to the containers API.