Convert BaseFlutterTask From Groovy to Kotlin
#163148
Conversation
6d5600d to
8eb0bbf
Compare
6c76b81 to
4902ed0
Compare
# Conflicts: # packages/flutter_tools/gradle/src/main/groovy/flutter.groovy
4902ed0 to
79b6933
Compare
BaseFlutterTask From Groovy to KotlinBaseFlutterTask From Groovy to Kotlin
| fun buildBundle() { | ||
| val helper = BaseFlutterTaskHelper(baseFlutterTask = this) | ||
| helper.checkPreConditions() | ||
| intermediateDir!!.mkdirs() |
There was a problem hiding this comment.
I dont like having a nullable intermediateDir then using !!. I thought maybe checkPreConditions would ensure that intermediateDir was not null but that does not look like the case.
I think the right thing to do here is to see if we can make intermediateDir non null and if we cant then handle null on this line. In BaseFlutterTaskHelper.getDependenciesFiles we treat the value as non null.
There was a problem hiding this comment.
I believe this was done because the original code behaved this way i.e.,
intermediateDirtook a JavaFile, and so was nullable and- we would throw a NPE on the old line 1642 of
flutter.groovy, ifintermediateDirwas null.
I know we had initially spoken about not making behavior changes in the conversion. Do you think we should amend that for the case of null pointer exceptions? I think that would be reasonable, but we should be explicit.
There was a problem hiding this comment.
@gmackall talked with some members of the flutter android team and the decision was made to file an issue to track fixing this issue and keeping the code as it was before.
There is a balance between fixing issues as we see them and introducing new behavior and the goal of this project (language conversion + unit testing) is to minimize (to the point of zero) the introduction of new behavior if possible.
Keeping an NPE we know about hurts but we are following our own principals in this project. After the conversion is done we do intend to fix many of the issues we have found.
| private val extraGenSnapshotOptionsTest = "--debugger" | ||
| private val splitDebugInfoTest = "/path/to/build/debug_info_directory" | ||
| private val codeSizeDirectoryTest = "/path/to/build/code_size_directory" | ||
| private val minSDKVersionTest = 21 |
There was a problem hiding this comment.
After @ash-google lands his min sdk check pr can this test use that value?
packages/flutter_tools/gradle/src/main/kotlin/BaseFlutterTaskHelper.kt
Outdated
Show resolved
Hide resolved
| baseFlutterTask.splitDebugInfo?.let { | ||
| args("-dSplitDebugInfo=${baseFlutterTask.splitDebugInfo}") | ||
| } | ||
| if (baseFlutterTask.treeShakeIcons == true) { | ||
| args("-dTreeShakeIcons=true") | ||
| } | ||
| if (baseFlutterTask.dartObfuscation == true) { | ||
| args("-dDartObfuscation=true") | ||
| } | ||
| baseFlutterTask.dartDefines?.let { | ||
| args("--DartDefines=${baseFlutterTask.dartDefines}") | ||
| } | ||
| baseFlutterTask.bundleSkSLPath?.let { | ||
| args("-dBundleSkSLPath=${baseFlutterTask.bundleSkSLPath}") | ||
| } | ||
| baseFlutterTask.codeSizeDirectory?.let { | ||
| args("-dCodeSizeDirectory=${baseFlutterTask.codeSizeDirectory}") | ||
| } | ||
| baseFlutterTask.flavor?.let { | ||
| args("-dFlavor=${baseFlutterTask.flavor}") | ||
| } | ||
| baseFlutterTask.extraGenSnapshotOptions?.let { | ||
| args("--ExtraGenSnapshotOptions=${baseFlutterTask.extraGenSnapshotOptions}") | ||
| } | ||
| baseFlutterTask.frontendServerStarterPath?.let { | ||
| args("-dFrontendServerStarterPath=${baseFlutterTask.frontendServerStarterPath}") | ||
| } | ||
| baseFlutterTask.extraFrontEndOptions?.let { | ||
| args("--ExtraFrontEndOptions=${baseFlutterTask.extraFrontEndOptions}") | ||
| } |
There was a problem hiding this comment.
Since we're not using the parameter from .let, let's just replace these with if null checks instead so it's a little more readable.
There was a problem hiding this comment.
Instead why not use the parameter from let?
baseFlutterTask.extraFrontEndOptions?.let {
args("--ExtraFrontEndOptions=$it")
}
vs
if (baseFlutterTask.extraFrontEndOptions != null) {
args("--ExtraFrontEndOptions=${baseFlutterTask.extraFrontEndOptions}")
}
There was a problem hiding this comment.
done. Used the parameter from let.
# Conflicts: # packages/flutter_tools/gradle/src/main/groovy/flutter.groovy
…' into i162111-convert-BaseFlutterTask
| import kotlin.test.assertTrue | ||
|
|
||
| class BaseFlutterTaskHelperTest { | ||
| object BaseFlutterTaskPropertiesTest { |
There was a problem hiding this comment.
Best practice for Kotlin when sharing constants, values, and/or variables across files.
| var flavor: String? = null | ||
|
|
||
| /** | ||
| * Gets the dependency file(s) based on the path from the intermediate directory. |
There was a problem hiding this comment.
If you know can you expand on which dependency files or what they are a dependency of.
This dart doc can also just point to the helper if tha tis where you want to keep the documentation.
| every { mockExecSpec.args("--local-engine-src-path", localEngineSrcPath) } returns mockExecSpec | ||
|
|
||
| val localEngineHost = baseFlutterTask.localEngineHost | ||
| every { mockExecSpec.args("--local-engine-host", localEngineHost) } returns mockExecSpec |
There was a problem hiding this comment.
I think you can collapse many of these args mock calls to a single
every { mockExecSpec.args(any<String>(), any()) } returns execSpec
|
This is looking good. Super close to being able to land. Something I had a comment for but removed was making a utility class for mocking that let you pass in values. I think because you have really a test for null values and a test for non null it is probably not worth creating a utility class. |
addressed comments changed back to list debugging windows integration test ktlint formatting matches CI delete commas and added platform-specific path separators update comment updated comment and removed debugging commands cut down the mocked function calls
a41a362 to
1b9b454
Compare
Converted
BaseFlutterTaskfrom Groovy to Kotlin.Note: Since it looks like Gradle prevents us from instantiating and using an object that extends abstract class BaseFlutterTask, we must use principles of Test Driven Development (TDD) in order to unit test
BaseFlutterTask.BaseFlutterTaskHelperwas created solely for unit testing to simulate the behavior of all functions in the originalBaseFlutterTask.Fixes #162111
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.