Merge pull request #889 from MarathonLabs/feature/imporove-max-filena…

…me-behaviour

fix(core): add maxFilename limit for outputConfiguration

configuration/src/main/kotlin/com/malinskiy/marathon/config/OutputConfiguration.kt

@@ -2,6 +2,12 @@ package com.malinskiy.marathon.config
 
 import com.fasterxml.jackson.annotation.JsonProperty
 
+// Value 0 is equivalent to unlimited path length
+const val OUTPUT_MAX_PATH = 0
+// Value 0 is equivalent to unlimited filename length
+const val OUTPUT_MAX_FILENAME = 255
+
 data class OutputConfiguration(
-    @JsonProperty("maxPath") val maxPath: Int = 255
+    @JsonProperty("maxPath") val maxPath: Int = OUTPUT_MAX_PATH,
+    @JsonProperty("maxFilename") val maxFilename: Int = OUTPUT_MAX_FILENAME,
 )

core/src/main/kotlin/com/malinskiy/marathon/di/Modules.kt

@@ -38,7 +38,7 @@ val analyticsModule = module {
 val coreModule = module {
     single {
         val configuration = get<Configuration>()
-        FileManager(configuration.outputConfiguration.maxPath, configuration.outputDir) 
+        FileManager(configuration.outputConfiguration.maxPath, configuration.outputConfiguration.maxFilename, configuration.outputDir)
     }
     single {
         GsonBuilder()

core/src/main/kotlin/com/malinskiy/marathon/io/FileManager.kt

@@ -12,97 +12,113 @@ import java.nio.file.Path
 import java.nio.file.Paths.get
 import java.util.UUID
 
+/**
+ * Validation logic should check filename first, then check if the resulting path is within max path len
+ */
 @Suppress("TooManyFunctions")
-class FileManager(private val maxPath: Int, private val output: File) {
+class FileManager(private val maxPath: Int, private val maxFilename: Int, private val output: File) {
     val log = MarathonLogging.logger("FileManager")
 
-    fun createFile(fileType: FileType, pool: DevicePoolId, device: DeviceInfo, test: Test, testBatchId: String? = null): File {
-        val directory = createDirectory(fileType, pool, device)
-        val filename = createFilename(test, fileType, maxPath - (directory.toAbsolutePath().toString().length + 1), testBatchId)
+    fun createFile(
+        fileType: FileType,
+        pool: DevicePoolId,
+        device: DeviceInfo,
+        test: Test? = null,
+        testBatchId: String? = null,
+        id: String? = null
+    ): File {
+        val directory = when {
+            test != null || testBatchId != null -> createDirectory(fileType, pool, device)
+            else -> createDirectory(fileType, pool)
+        }
+        val filename = when {
+            test != null -> createTestFilename(test, fileType, testBatchId, id = id)
+            testBatchId != null -> createBatchFilename(testBatchId, fileType, id = id)
+            else -> createDeviceFilename(device, fileType, id = id)
+        }
         return createFile(directory, filename)
     }
 
-    fun createFile(fileType: FileType, pool: DevicePoolId, device: DeviceInfo, testBatchId: String): File {
-        val directory = createDirectory(fileType, pool, device)
-        val filename = createFilename(fileType, testBatchId)
-        return createFile(directory, filename)
-    }
+    fun createFolder(folderType: FolderType, pool: DevicePoolId? = null, device: DeviceInfo? = null): File {
+        var path = get(output.absolutePath, folderType.dir)
+        if (pool != null) {
+            path = path.resolve(pool.name)
+        }
+        if (device != null) {
+            path = path.resolve(device.safeSerialNumber)
+        }
 
-    fun createFile(fileType: FileType, pool: DevicePoolId, device: DeviceInfo): File {
-        val directory = createDirectory(fileType, pool)
-        val filename = createFilename(device, fileType)
-        return createFile(directory, filename)
-    }
+        val maybeTooLongPath = path.toFile()
+        path = if (maxPath > 0 && maybeTooLongPath.absolutePath.length > maxPath) {
+            val trimmed = maybeTooLongPath.absolutePath.take(maxPath)
+            log.error {
+                "Directory path length cannot exceed $maxPath characters and has been trimmed from $maybeTooLongPath to $trimmed and can create a conflict. " +
+                    "This happened because the combination of file path, pool name and device serial is too long."
+            }
+            File(trimmed)
+        } else {
+            maybeTooLongPath
+        }.toPath()
 
-    fun createScreenshotFile(extension: String, pool: DevicePoolId, device: DeviceInfo, test: Test, testBatchId: String): File {
-        val directory = createDirectory(FileType.SCREENSHOT, pool, device)
-        val filename =
-            createFilename(
-                test,
-                FileType.SCREENSHOT,
-                maxPath - (directory.toAbsolutePath().toString().length + 1),
-                testBatchId = null,
-                extension,
-                UUID.randomUUID().toString()
-            )
-        return createFile(directory, filename)
+        return createDirectories(path).toFile()
     }
 
-    fun createFolder(folderType: FolderType): File = createDirectories(get(output.absolutePath, folderType.dir)).toFile()
-    fun createFolder(folderType: FolderType, pool: DevicePoolId, device: DeviceInfo): File =
-        createDirectories(get(output.absolutePath, folderType.dir, pool.name, device.safeSerialNumber)).toFile()
-
-    fun createFolder(folderType: FolderType, pool: DevicePoolId): File =
-        createDirectories(get(output.absolutePath, folderType.dir, pool.name)).toFile()
-
-    fun createFolder(folderType: FolderType, device: DeviceInfo): File =
-        createDirectories(get(output.absolutePath, folderType.dir, device.safeSerialNumber)).toFile()
-
     fun createTestResultFile(filename: String): File {
-        val resultsFolder = get(output.absolutePath, FileType.TEST_RESULT.dir).toFile()
-        resultsFolder.mkdirs()
-        return File(resultsFolder, filename)
+        val resultsFolder = get(output.absolutePath, FileType.TEST_RESULT.dir)
+        resultsFolder.toFile().mkdirs()
+        return createFile(resultsFolder, filename)
     }
 
-    private fun createDirectory(fileType: FileType, pool: DevicePoolId, device: DeviceInfo): Path =
-        createDirectories(getDirectory(fileType, pool, device))
-
-    private fun createDirectory(fileType: FileType, pool: DevicePoolId): Path =
-        createDirectories(getDirectory(fileType, pool))
-
-    private fun getDirectory(fileType: FileType, pool: DevicePoolId, device: DeviceInfo): Path =
-        getDirectory(fileType, pool, device.safeSerialNumber)
-
-    private fun getDirectory(fileType: FileType, pool: DevicePoolId, serial: String): Path =
-        get(output.absolutePath, fileType.dir, pool.name, serial)
+    private fun createDirectory(fileType: FileType, pool: DevicePoolId, device: DeviceInfo? = null): Path {
+        return createDirectories(getDirectory(fileType, pool, serial = device?.safeSerialNumber))
+    }
 
-    private fun getDirectory(fileType: FileType, pool: DevicePoolId): Path =
-        get(output.absolutePath, fileType.dir, pool.name)
+    private fun getDirectory(fileType: FileType, pool: DevicePoolId, serial: String? = null): Path {
+        val path = get(output.absolutePath, fileType.dir, pool.name)
+        return serial?.let {
+            path.resolve(serial)
+        } ?: path
+    }
 
     private fun createFile(directory: Path, filename: String): File {
-        val maybeTooLongPath = File(directory.toFile(), filename)
-        return if (maybeTooLongPath.absolutePath.length > maxPath) {
+        val trimmedFilename = if (maxFilename > 0 && filename.length > maxFilename) {
+            val safeFilename = filename.take(maxFilename)
+            log.error {
+                "File name length cannot exceed $maxFilename characters and has been trimmed to $safeFilename and can create a conflict." +
+                    "This usually happens because the test name is too long."
+            }
+            safeFilename
+        } else {
+            filename
+        }
+        val maybeTooLongPath = File(directory.toFile(), trimmedFilename)
+        return if (maxPath > 0 && maybeTooLongPath.absolutePath.length > maxPath) {
             val trimmed = maybeTooLongPath.absolutePath.substring(0 until maxPath)
-            log.error { "File path length cannot exceed $maxPath characters and has been trimmed to $trimmed and can create a conflict. This happened because the combination of file path, test class name, and test name is too long." }
+            log.error {
+                "File path length cannot exceed $maxPath characters and has been trimmed from $maybeTooLongPath to $trimmed and can create a conflict. " +
+                    "This happened because the combination of file path, test class name, and test name is too long."
+            }
             File(trimmed)
         } else {
             maybeTooLongPath
         }
     }
 
-    private fun createFilename(fileType: FileType, testBatchId: String): String {
+    private fun createBatchFilename(testBatchId: String, fileType: FileType, id: String? = null): String {
         return StringBuilder().apply {
             append(testBatchId)
+            if (id != null) {
+                append("-$id")
+            }
             if (fileType.suffix.isNotEmpty()) {
                 append(".$testBatchId")
             }
         }.toString()
     }
 
-    private fun createFilename(
+    private fun createTestFilename(
         test: Test,
         fileType: FileType,
-        limit: Int,
         testBatchId: String? = null,
         overrideExtension: String? = null,
         id: String? = null,
@@ -120,24 +136,16 @@ class FileManager(private val maxPath: Int, private val output: File) {
                 append(".${fileType.suffix}")
             }
         }.toString()
-        val rawTestName = test.toTestName().escape()
-        val testName = when {
-            limit - testSuffix.length >= 0 -> rawTestName.take(limit - testSuffix.length)
-            else -> ""
-        }
-        val fileName = "$testName$testSuffix"
-        if (rawTestName.length > testName.length) {
-            when {
-                limit >= 0 -> log.error { "File name length cannot exceed $limit characters and has been trimmed to $fileName and can create a conflict. This happened because the combination of file path, test class name, and test name is too long." }
-                else -> log.error { "Base path for writing a file ${rawTestName}$testSuffix is already maxed out and is ${-limit} characters more than the allowed limit of ${maxPath}." }
-            }
-        }
-        return fileName
+        val testName = test.toTestName().escape()
+        return "$testName$testSuffix"
     }
 
-    private fun createFilename(device: DeviceInfo, fileType: FileType): String {
+    private fun createDeviceFilename(device: DeviceInfo, fileType: FileType, id: String? = null): String {
         return StringBuilder().apply {
             append(device.safeSerialNumber)
+            if (id != null) {
+                append("-$id")
+            }
             if (fileType.suffix.isNotEmpty()) {
                 append(".${fileType.suffix}")
             }

core/src/main/kotlin/com/malinskiy/marathon/io/FileType.kt

@@ -9,6 +9,9 @@ enum class FileType(val dir: String, val suffix: String) {
     VIDEO("video", "mp4"),
     SCREENSHOT("screenshot", "gif"),
     SCREENSHOT_PNG("screenshot", "png"),
+    SCREENSHOT_JPG("screenshot", "jpg"),
+    SCREENSHOT_WEBP("screenshot", "jpg"),
+    SCREENSHOT_GIF("screenshot", "jpg"),
     XCTESTRUN("xctestrun", "xctestrun"),
     BILL("bill", "json"),
 }

core/src/main/kotlin/com/malinskiy/marathon/report/bill/BillingReporter.kt

@@ -57,7 +57,7 @@ internal class BillingReporter(
 
         bills.forEach {
             val json = gson.toJson(it)
-            fileManager.createFile(FileType.BILL, it.pool, it.device).writeText(json)
+            fileManager.createFile(FileType.BILL, it.pool, device = it.device).writeText(json)
         }
 
         usageTracker.trackEvent(Event.Devices(bills.size))

core/src/test/kotlin/com/malinskiy/marathon/io/FileManagerTest.kt

@@ -16,10 +16,8 @@ import kotlin.io.path.absolutePathString
 
 class FileManagerTest {
     private val output = Files.createTempDir()
-    private val fileManager = FileManager(MAX_PATH, output)
 
     private companion object {
-        val MAX_PATH = 255
         val poolId = DevicePoolId("testPoolId")
         val deviceInfo = DeviceInfo(
             operatingSystem = OperatingSystem("23"),
@@ -56,30 +54,32 @@ class FileManagerTest {
 
     @Test
     fun createFilenameNormalLengthTest() {
+        val fileManager = FileManager(0, 255, output)
         val file = fileManager.createFile(FileType.LOG, poolId, deviceInfo, shortNameTest, batchId)
         file.name shouldBeEqualTo "com.example.Clazz#method-batchId.log"
     }
 
     @Test
-    fun createFilenameLongLengthMethodTest() {
+    fun createFilenameLongLengthMethodLimitedPathTest() {
+        val fileManager = FileManager(255, 0, output)
         val file = fileManager.createFile(FileType.LOG, poolId, deviceInfo, longNameTest, batchId)
         file.absolutePath.length shouldBeEqualTo 255
         val filenameLimit = 255 - file.parentFile.absolutePath.length - File.separator.length
-        val fqtnLimit = filenameLimit - "-${batchId}.log".length
-        file.name shouldBeEqualTo "${longNameTest.toTestName().escape().take(fqtnLimit)}-${batchId}.log"
+        file.name shouldBeEqualTo "${longNameTest.toTestName()}-${batchId}.log".escape().take(filenameLimit)
     }
 
     @Test
     fun testCreateFilenameNamedParameterizedLong() {
+        val fileManager = FileManager(255, 0, output)
         val file = fileManager.createFile(FileType.LOG, poolId, deviceInfo, longNamedParameterizedTest, batchId)
         file.absolutePath.length shouldBeEqualTo 255
         val filenameLimit = 255 - file.parentFile.absolutePath.length - File.separator.length
-        val fqtnLimit = filenameLimit - "-${batchId}.log".length
-        file.name shouldBeEqualTo "${longNamedParameterizedTest.toTestName().escape().take(fqtnLimit)}-${batchId}.log"
+        file.name shouldBeEqualTo "${longNamedParameterizedTest.toTestName()}-${batchId}.log".escape().take(filenameLimit)
     }
 
     @Test
     fun testDeviceSerialEscaping() {
+        val fileManager = FileManager(0, 255, output)
         val file = fileManager.createFile(
             FileType.LOG, poolId, DeviceInfo(
                 operatingSystem = OperatingSystem("23"),
@@ -95,7 +95,26 @@ class FileManagerTest {
     }
 
     @Test
-    fun testTooLongOutputFolder() {
+    fun testScreenshotfile() {
+        val fileManager = FileManager(0, 255, output)
+        val file = fileManager.createFile(
+            FileType.SCREENSHOT_PNG, poolId, DeviceInfo(
+                operatingSystem = OperatingSystem("23"),
+                serialNumber = "127.0.0.1:5037:emulator-5554",
+                model = "Android SDK built for x86",
+                manufacturer = "unknown",
+                networkState = NetworkState.CONNECTED,
+                deviceFeatures = listOf(DeviceFeature.SCREENSHOT, DeviceFeature.VIDEO),
+                healthy = true
+            ),
+            test = shortNameTest,
+            id = "on-device-test",
+        )
+        file.name shouldBeEqualTo "com.example.Clazz#method-on-device-test.png"
+    }
+
+    @Test
+    fun testTooLongOutputPathUnlimitedFilename() {
         val test = com.malinskiy.marathon.test.Test(
             pkg = "com.xxyyzzxxyy.android.abcdefgh.abcdefghi",
             clazz = "PackageNameTest",
@@ -105,9 +124,12 @@ class FileManagerTest {
 
         val tempDir = Files.createTempDir()
         val proposedPath = Paths.get(tempDir.absolutePath, FileType.LOG.name, poolId.name, deviceInfo.safeSerialNumber)
-        val additionalPathCharacters = MAX_PATH - proposedPath.absolutePathString().length
+        val limitedMaxPath = 255
+        val additionalPathCharacters = limitedMaxPath - proposedPath.absolutePathString().length
         val limitedOutputDirectory = File(tempDir, "x".repeat(additionalPathCharacters))
-        val limitedFileManager = FileManager(MAX_PATH, limitedOutputDirectory)
+        val limitedFileManager = FileManager(limitedMaxPath, 0, limitedOutputDirectory)
         val file = limitedFileManager.createFile(FileType.LOG, poolId, deviceInfo, test, batchId)
-    } 
+
+        file.path.length shouldBeEqualTo 255
+    }
 }