Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: check if gcs path exist before run #1111

Merged
merged 1 commit into from
Sep 11, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 16 additions & 10 deletions test_runner/src/main/kotlin/ftl/args/ArgsHelper.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, totaly cosmetic change, IMO can improve readability a bit. How about exists().not() or notExists() ? I mean, this negation mark can be skipped by mistake. It's not a change request :)

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)
Expand Down Expand Up @@ -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<Chunk>, args: IArgs): List<Chunk> {
Expand Down
12 changes: 9 additions & 3 deletions test_runner/src/main/kotlin/ftl/gc/GcStorage.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand All @@ -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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's totally cosmetic, just small wording change exist -> exists

storage.get(authority, path.dropLeadingSlash())?.exists() ?: false
}

private fun String.dropLeadingSlash() = drop(1)
}
4 changes: 2 additions & 2 deletions test_runner/src/test/kotlin/ftl/args/AndroidArgsTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down
25 changes: 22 additions & 3 deletions test_runner/src/test/kotlin/ftl/args/ArgsHelperTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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)
}
}
Expand All @@ -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")
}
}
}
4 changes: 2 additions & 2 deletions test_runner/src/test/kotlin/ftl/args/IosArgsTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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()
Expand Down
58 changes: 29 additions & 29 deletions test_runner/src/test/kotlin/ftl/gc/GcStorageTest.kt
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -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
Expand All @@ -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()
}
}