Bug/#781 create results dir #785
Conversation
Codecov Report
@@ Coverage Diff @@
## master #785 +/- ##
=========================================
Coverage 78.50% 78.50%
Complexity 692 692
=========================================
Files 133 133
Lines 2857 2857
Branches 411 411
=========================================
Hits 2243 2243
Misses 355 355
Partials 259 259 |
| @@ -55,19 +55,22 @@ class AndroidArgs( | |||
| private val androidGcloud = androidGcloudYml.gcloud | |||
| var appApk = (cli?.app ?: androidGcloud.app ?: throw FlankFatalError("app is not set")).processFilePath("from app") | |||
| var testApk = (cli?.test ?: androidGcloud.test)?.processFilePath("from test") | |||
| val additionalApks = (cli?.additionalApks ?: androidGcloud.additionalApks).map { it.processFilePath("from additional-apks") } | |||
| val additionalApks = | |||
There was a problem hiding this comment.
Pls let's try to avoid autoformat changes
There was a problem hiding this comment.
Right! I set auto line breaking to 200 chars but this changes was made before. It shouldn't happen any more.
| val resultsDir = UUID.randomUUID().toString() | ||
| val directoryPath = Paths.get(resultsDir) | ||
| if (Files.exists(directoryPath)) { | ||
| Assert.fail("Test directory ($resultsDir) shouldn't exists! It's remote directory.") |
There was a problem hiding this comment.
It's a remote directory.
| """.trimIndent() | ||
| AndroidArgs.load(yaml) | ||
| if (Files.exists(directoryPath)) { | ||
| Assert.fail("Test directory ($resultsDir) shouldn't be created! It's remote directory on cloud!") |
There was a problem hiding this comment.
It's a remote directory on the cloud!
| } | ||
|
|
||
| private fun AndroidArgs.Companion.load(yamlData: String, cli: AndroidRunCommand? = null): AndroidArgs = load(StringReader(yamlData), cli) | ||
| private fun AndroidArgs.Companion.load(yamlData: String, cli: AndroidRunCommand? = null): AndroidArgs = | ||
| load(StringReader(yamlData), cli) |
There was a problem hiding this comment.
Sadly yes. I should check all files modified in this pr not only one. I revert changes. In future I will pay more attention for that.
release_notes.md
Outdated
| @@ -1,5 +1,6 @@ | |||
| ## next (unreleased) | |||
|
|
|||
| - [#781](https://github.com/Flank/flank/pull/781) Create results-dir. ([adamfilipow92](https://github.com/adamfilipow92)) | |||
There was a problem hiding this comment.
The bug is, we should not call assertFileExists on gcloud.resultsDir. I'd update the release notes to say something like:
#781 Remove local exists check on cloud results-dir. Fixes crash when results-dir is set by the user. (adamfilipow92)
| * it's should be enough | ||
| */ | ||
| @Test | ||
| fun `should not throw if directory not exists`() { |
There was a problem hiding this comment.
Comments should usually describe why we're doing something, not what we're doing.
Usually comments are useful when they explain why some code exists, and should not be explaining what some code is doing.
https://github.com/google/eng-practices/blob/master/review/reviewer/looking-for.md#comments
What do you think about this?
@Test
fun `results-dir (cloud directory) should not throw if it doesn't exist locally`There was a problem hiding this comment.
Thanks for this feedback! I change test name and remove my comment. :)
| @@ -0,0 +1,7 @@ | |||
| gcloud: | |||
| app: ./src/test/kotlin/ftl/fixtures/tmp/apk/app-debug.apk | |||
There was a problem hiding this comment.
Is this used anywhere? I think we inlined the example
There was a problem hiding this comment.
It's only used for my internal tests of this issue. I removed it from repository.
Fixes #781
Checklist