From 9baebd06f233323452512a126f06ba1941a6f480 Mon Sep 17 00:00:00 2001 From: Piotr Adamczyk Date: Thu, 10 Sep 2020 19:05:03 +0200 Subject: [PATCH] feat: check if gcs path exist before run --- .../src/main/kotlin/ftl/args/ArgsHelper.kt | 26 +++++---- .../src/main/kotlin/ftl/gc/GcStorage.kt | 12 +++- .../test/kotlin/ftl/args/AndroidArgsTest.kt | 4 +- .../test/kotlin/ftl/args/ArgsHelperTest.kt | 25 +++++++- .../src/test/kotlin/ftl/args/IosArgsTest.kt | 4 +- .../src/test/kotlin/ftl/gc/GcStorageTest.kt | 58 +++++++++---------- 6 files changed, 80 insertions(+), 49 deletions(-) diff --git a/test_runner/src/main/kotlin/ftl/args/ArgsHelper.kt b/test_runner/src/main/kotlin/ftl/args/ArgsHelper.kt index acebcc5410..d7e440f5a1 100644 --- a/test_runner/src/main/kotlin/ftl/args/ArgsHelper.kt +++ b/test_runner/src/main/kotlin/ftl/args/ArgsHelper.kt @@ -42,20 +42,18 @@ object ArgsHelper { } fun assertFileExists(file: String, name: String) { - // flank can't detect (for now) if files exists on GCS, therefore we want to skip validation - // todo https://github.com/Flank/flank/issues/1099 - if (file.startsWith(GCS_PREFIX)) return - - if (!File(file).exists()) { + if (!file.exist()) throw FlankGeneralError("'$file' $name doesn't exist") - } } + private fun String.exist() = + if (startsWith(GCS_PREFIX)) GcStorage.exist(this) else File(this).exists() + fun assertCommonProps(args: IArgs) { assertNotEmpty( args.project, "The project is not set. Define GOOGLE_CLOUD_PROJECT, set project in flank.yml\n" + - "or save service account credential to ${defaultCredentialPath}\n" + - " See https://github.com/GoogleCloudPlatform/google-cloud-java#specifying-a-project-id" + "or save service account credential to ${defaultCredentialPath}\n" + + " See https://github.com/GoogleCloudPlatform/google-cloud-java#specifying-a-project-id" ) if (args.maxTestShards !in AVAILABLE_PHYSICAL_SHARD_COUNT_RANGE && args.maxTestShards != -1) @@ -226,14 +224,22 @@ object ArgsHelper { } val (ignoredTests, testsToExecute) = filteredTests.partition { it.ignored } val shards = if (args.disableSharding) { - listOf(Chunk(testsToExecute.map { TestMethod(name = it.testName, isParameterized = it.isParameterizedClass, time = 0.0) })) + listOf(Chunk(testsToExecute.map { + TestMethod( + name = it.testName, + isParameterized = it.isParameterizedClass, + time = 0.0 + ) + })) } else { val oldTestResult = GcStorage.downloadJunitXml(args) ?: JUnitTestResult(mutableListOf()) val shardCount = forcedShardCount ?: shardCountByTime(testsToExecute, oldTestResult, args) createShardsByShardCount(testsToExecute, oldTestResult, args, shardCount).map { Chunk(it.testMethods) } } - return CalculateShardsResult(testMethodsAlwaysRun(shards, args), ignoredTestCases = ignoredTests.map { it.testName }) + return CalculateShardsResult( + testMethodsAlwaysRun(shards, args), + ignoredTestCases = ignoredTests.map { it.testName }) } private fun testMethodsAlwaysRun(shards: List, args: IArgs): List { diff --git a/test_runner/src/main/kotlin/ftl/gc/GcStorage.kt b/test_runner/src/main/kotlin/ftl/gc/GcStorage.kt index 5fcea2fbf7..5a34a9852e 100644 --- a/test_runner/src/main/kotlin/ftl/gc/GcStorage.kt +++ b/test_runner/src/main/kotlin/ftl/gc/GcStorage.kt @@ -12,6 +12,7 @@ import ftl.args.IArgs import ftl.args.IosArgs import ftl.config.FtlConstants import ftl.config.FtlConstants.GCS_PREFIX +import ftl.gc.GcStorage.dropLeadingSlash import ftl.reports.xml.model.JUnitTestResult import ftl.reports.xml.parseAllSuitesXml import ftl.reports.xml.xmlToString @@ -155,7 +156,7 @@ object GcStorage { fun download(gcsUriString: String, ignoreError: Boolean = false): String { val gcsURI = URI.create(gcsUriString) val bucket = gcsURI.authority - val path = gcsURI.path.drop(1) // Drop leading slash + val path = gcsURI.path.dropLeadingSlash() return downloadCache[path] ?: downloadCache.computeIfAbsent(path) { val outputFile = File.createTempFile("tmp", null) outputFile.deleteOnExit() @@ -177,7 +178,12 @@ object GcStorage { fun exist( rootGcsBucket: String, - runGcsPath: String, - storage: Storage = GcStorage.storage + runGcsPath: String ) = storage.list(rootGcsBucket, pageSize(1), prefix("$runGcsPath/")).values.count() > 0 + + fun exist(gcsUriString: String) = with(URI.create(gcsUriString)) { + storage.get(authority, path.dropLeadingSlash())?.exists() ?: false + } + + private fun String.dropLeadingSlash() = drop(1) } diff --git a/test_runner/src/test/kotlin/ftl/args/AndroidArgsTest.kt b/test_runner/src/test/kotlin/ftl/args/AndroidArgsTest.kt index c70d8a6e10..0419137f38 100644 --- a/test_runner/src/test/kotlin/ftl/args/AndroidArgsTest.kt +++ b/test_runner/src/test/kotlin/ftl/args/AndroidArgsTest.kt @@ -1794,7 +1794,7 @@ AndroidArgs legacy-junit-result: true """.trimIndent() mockkObject(GcStorage) { - every { exist(any(), any(), any()) } returns true + every { exist(any(), any()) } returns true // when AndroidArgs.load(yaml).validate() @@ -1835,7 +1835,7 @@ AndroidArgs legacy-junit-result: true """.trimIndent() mockkObject(GcStorage) { - every { exist(any(), any(), any()) } returns false + every { exist(any(), any()) } returns false // when AndroidArgs.load(yaml).validate() diff --git a/test_runner/src/test/kotlin/ftl/args/ArgsHelperTest.kt b/test_runner/src/test/kotlin/ftl/args/ArgsHelperTest.kt index 445511993e..0c5e688bee 100644 --- a/test_runner/src/test/kotlin/ftl/args/ArgsHelperTest.kt +++ b/test_runner/src/test/kotlin/ftl/args/ArgsHelperTest.kt @@ -10,6 +10,8 @@ import ftl.args.yml.mergeYmlKeys import ftl.config.FtlConstants import ftl.config.common.CommonGcloudConfig import ftl.config.ios.IosGcloudConfig +import ftl.gc.GcStorage +import ftl.gc.GcStorage.exist import ftl.shard.TestMethod import ftl.shard.TestShard import ftl.shard.stringShards @@ -19,6 +21,7 @@ import ftl.test.util.assertThrowsWithMessage import ftl.run.exception.FlankGeneralError import ftl.run.exception.FlankConfigurationError import io.mockk.every +import io.mockk.mockkObject import io.mockk.spyk import io.mockk.unmockkAll import org.junit.After @@ -193,7 +196,10 @@ class ArgsHelperTest { val args = spyk(AndroidArgs.default()) every { args.maxTestShards } returns maxTestShards - assertThrowsWithMessage(Throwable::class, "max-test-shards must be >= ${IArgs.AVAILABLE_PHYSICAL_SHARD_COUNT_RANGE.first} and <= ${IArgs.AVAILABLE_PHYSICAL_SHARD_COUNT_RANGE.last}. But current is $maxTestShards") { + assertThrowsWithMessage( + Throwable::class, + "max-test-shards must be >= ${IArgs.AVAILABLE_PHYSICAL_SHARD_COUNT_RANGE.first} and <= ${IArgs.AVAILABLE_PHYSICAL_SHARD_COUNT_RANGE.last}. But current is $maxTestShards" + ) { assertCommonProps(args) } } @@ -217,7 +223,20 @@ class ArgsHelperTest { } @Test - fun `should not throw an error if apk exists on gcs instead of local fs`() { - assertFileExists("gs://any-bucket/any-file.apk", "from apk") + fun `should throw an error if apk file does not exists on gcs`() { + mockkObject(GcStorage) { + every { exist("gs://any-bucket/any-file.apk") } returns false + assertThrowsWithMessage(FlankGeneralError::class, "'gs://any-bucket/any-file.apk' from apk doesn't exist") { + assertFileExists("gs://any-bucket/any-file.apk", "from apk") + } + } + } + + @Test + fun `should not throw and error if apk file exist on gcs`() { + mockkObject(GcStorage) { + every { exist("gs://any-bucket/any-file.apk") } returns true + assertFileExists("gs://any-bucket/any-file.apk", "from apk") + } } } diff --git a/test_runner/src/test/kotlin/ftl/args/IosArgsTest.kt b/test_runner/src/test/kotlin/ftl/args/IosArgsTest.kt index a67a2e30ba..4a4cdde5da 100644 --- a/test_runner/src/test/kotlin/ftl/args/IosArgsTest.kt +++ b/test_runner/src/test/kotlin/ftl/args/IosArgsTest.kt @@ -1066,7 +1066,7 @@ IosArgs max-test-shards: -1 """.trimIndent() mockkObject(GcStorage) { - every { GcStorage.exist(any(), any(), any()) } returns true + every { GcStorage.exist(any(), any()) } returns true // when IosArgs.load(yaml).validate() @@ -1087,7 +1087,7 @@ IosArgs max-test-shards: -1 """.trimIndent() mockkObject(GcStorage) { - every { GcStorage.exist(any(), any(), any()) } returns false + every { GcStorage.exist(any(), any()) } returns false // when IosArgs.load(yaml).validate() diff --git a/test_runner/src/test/kotlin/ftl/gc/GcStorageTest.kt b/test_runner/src/test/kotlin/ftl/gc/GcStorageTest.kt index 82a756f9b5..ff259356e1 100644 --- a/test_runner/src/test/kotlin/ftl/gc/GcStorageTest.kt +++ b/test_runner/src/test/kotlin/ftl/gc/GcStorageTest.kt @@ -1,6 +1,5 @@ package ftl.gc -import com.google.cloud.storage.Storage import ftl.args.AndroidArgs import ftl.test.util.FlankTestRunner import io.mockk.every @@ -12,6 +11,7 @@ import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue import org.junit.Test import org.junit.runner.RunWith +import java.io.File @RunWith(FlankTestRunner::class) class GcStorageTest { @@ -62,23 +62,12 @@ class GcStorageTest { @Test fun `should return that file exist`() { // given - val mockedStorage: Storage = mockk { - every { - list( - "bucket", - Storage.BlobListOption.pageSize(1), - Storage.BlobListOption.prefix("path/") - ) - } returns mockk { - every { values } returns listOf(mockk()) - } - } + GcStorage.upload(createTempFile("testFile", ".txt").path, "bucket", "path") // when val actual = GcStorage.exist( "bucket", - "path", - mockedStorage + "path" ) // then @@ -87,27 +76,38 @@ class GcStorageTest { @Test fun `should return that file does not exist`() { - // given - val mockedStorage: Storage = mockk { - every { - list( - "bucket", - Storage.BlobListOption.pageSize(1), - Storage.BlobListOption.prefix("path/") - ) - } returns mockk { - every { values } returns listOf() - } - } - // when val actual = GcStorage.exist( "bucket", - "path", - mockedStorage + "path_not_existed" ) // then assertFalse(actual) } + + @Test + fun `should return false when file does not exits`() { + // given + val actual = GcStorage.exist("gs://not/existed/file.txt") + + // then + assertFalse(actual) + } + + @Test + fun `should return true when file exist`() { + // given + val tempFile = File.createTempFile("test", ".txt") + GcStorage.upload(tempFile.path, "bucket", "path") + + // given + val actual = GcStorage.exist("gs://bucket/path/${tempFile.name}") + + // then + assertTrue(actual) + + // clean + tempFile.delete() + } }