Fixing null check (#59)

* Fixing null check

* linting

* Updating method names to call artifacts instead of screenshots

* addressing feedback

Author: rnakade

AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/testRunner/TestRunnerService.kt

@@ -98,7 +98,7 @@ interface TestRunnerService {
      * Returns null when the testMatrix is not complete,
      * and emptyMap if there are no screenshots associated with the test (non-screenshot tests)
      */
-    suspend fun getTestMatrixResultsScreenshots(
+    suspend fun getTestMatrixArtifacts(
         testMatrix: TestMatrix,
         testIdentifiers: List<TestIdentifier>
     ): Map<TestIdentifier, List<TestCaseArtifact>>?

AndroidXCI/lib/src/main/kotlin/dev/androidx/ci/testRunner/TestRunnerServiceImpl.kt

@@ -207,12 +207,12 @@ internal class TestRunnerServiceImpl internal constructor(
         }
     }
 
-    private suspend fun findScreenshotFiles(
+    private suspend fun findArtifacts(
         resultPath: GcsPath,
         testIdentifiers: List<TestRunnerService.TestIdentifier>
-    ): Map< TestRunnerService.TestIdentifier, List<TestRunnerService.TestCaseArtifact>> {
-        val screenshotArtifactsBlobs = mutableMapOf<TestRunnerService.TestIdentifier, MutableList<TestRunnerService.TestCaseArtifact>>()
-        val screenshotArtifacts: Map<TestRunnerService.TestIdentifier, List<TestRunnerService.TestCaseArtifact>>
+    ): Map< TestRunnerService.TestIdentifier, List<TestRunnerService.TestCaseArtifact>>? {
+        if (testIdentifiers.isEmpty()) return null
+        val testArtifactsBlobs = mutableMapOf<TestRunnerService.TestIdentifier, MutableList<TestRunnerService.TestCaseArtifact>>()
         val testNames = testIdentifiers.associateBy { testIdentifier ->
             "${testIdentifier.className}_${testIdentifier.name}"
         }
@@ -227,8 +227,11 @@ internal class TestRunnerServiceImpl internal constructor(
                     visitor.fileName.startsWith(testName)
                 }
                 val testIdentifier = testNames[testName]
+                println("filename =  ${visitor.fileName}")
+                println("testName = $testName")
+                println("testIdentifier = $testIdentifier")
                 if (testIdentifier != null) {
-                    screenshotArtifactsBlobs.getOrPut(testIdentifier) {
+                    testArtifactsBlobs.getOrPut(testIdentifier) {
                         mutableListOf()
                     }.add(
                         TestRunnerService.TestCaseArtifact(
@@ -239,8 +242,7 @@ internal class TestRunnerServiceImpl internal constructor(
                 }
             }
         }
-        screenshotArtifacts = screenshotArtifactsBlobs
-        return screenshotArtifacts
+        return testArtifactsBlobs
     }
 
     suspend fun getTestMatrixResults(
@@ -250,12 +252,12 @@ internal class TestRunnerServiceImpl internal constructor(
         return getTestMatrixResults(testMatrix)
     }
 
-    suspend fun getTestMatrixResultsScreenshots(
+    suspend fun getTestMatrixArtifacts(
         testMatrixId: String,
         testIdentifiers: List<TestRunnerService.TestIdentifier>
     ): Map<TestRunnerService.TestIdentifier, List<TestRunnerService.TestCaseArtifact>>? {
         val testMatrix = testLabController.getTestMatrix(testMatrixId) ?: return null
-        return getTestMatrixResultsScreenshots(testMatrix, testIdentifiers)
+        return getTestMatrixArtifacts(testMatrix, testIdentifiers)
     }
 
     override suspend fun getTestMatrixResults(
@@ -266,13 +268,13 @@ internal class TestRunnerServiceImpl internal constructor(
         return findResultFiles(resultPath, testMatrix)
     }
 
-    override suspend fun getTestMatrixResultsScreenshots(
+    override suspend fun getTestMatrixArtifacts(
         testMatrix: TestMatrix,
         testIdentifiers: List<TestRunnerService.TestIdentifier>
     ): Map<TestRunnerService.TestIdentifier, List<TestRunnerService.TestCaseArtifact>>? {
         if (!testMatrix.isComplete()) return null
         val resultPath = GcsPath(testMatrix.resultStorage.googleCloudStorage.gcsPath)
-        return findScreenshotFiles(resultPath, testIdentifiers)
+        return findArtifacts(resultPath, testIdentifiers)
     }
 
     companion object {

AndroidXCI/lib/src/test/kotlin/dev/androidx/ci/testRunner/TestRunnerServiceImplTest.kt

@@ -332,6 +332,11 @@ class TestRunnerServiceImplTest {
             "$resultRelativePath/redfin-30-en-portrait/artifacts/sdcard/Android/data/test/cache/androidx_screenshots/class1_name1_emulator_goldResult.textproto",
             "class1 name1 emulator textproto".toByteArray(Charsets.UTF_8)
         )
+        // No test is associated with this artifact. findArtifacts should not throw errors, even when unexpected files are encountered
+        fakeBackend.fakeGoogleCloudApi.upload(
+            "$resultRelativePath/redfin-30-en-portrait/artifacts/sdcard/Android/data/test/cache/androidx_screenshots/class5_name5_emulator_goldResult.textproto",
+            "class5 name5 emulator textproto".toByteArray(Charsets.UTF_8)
+        )
 
         fakeToolsResultApi.addStep(
             projectId = fakeBackend.firebaseProjectId,
@@ -380,14 +385,14 @@ class TestRunnerServiceImplTest {
 
         assertThat(result.testRuns).hasSize(1)
         result.testRuns.first().let { testRun ->
-            val testIdentifier = TestRunnerService.TestIdentifier(
+            val testIdentifier1 = TestRunnerService.TestIdentifier(
                 className = "class1",
                 name = "name1",
                 runNumber = testRun.deviceRun.runNumber
             )
-            val screenshots = subject.getTestMatrixResultsScreenshots(
+            val screenshots1 = subject.getTestMatrixArtifacts(
                 testMatrixId,
-                listOf(testIdentifier)
+                listOf(testIdentifier1)
             )
             assertThat(
                 testRun.deviceRun.deviceId
@@ -415,15 +420,15 @@ class TestRunnerServiceImplTest {
             )
             assertThat(
                 testRun.testCaseArtifacts[
-                    testIdentifier
+                    testIdentifier1
                 ]?.size
             ).isEqualTo(
                 1
             )
             // step and logcat both have valid values for test1
             assertThat(
                 testRun.testCaseArtifacts[
-                    testIdentifier
+                    testIdentifier1
                 ]?.first {
                     it.resourceType == TestRunnerService.TestCaseArtifact.ResourceType.LOGCAT
                 }?.resultFileResource?.gcsPath.toString()
@@ -432,29 +437,29 @@ class TestRunnerServiceImplTest {
             )
             assertThat(
                 testRun.testCaseArtifacts[
-                    testIdentifier
+                    testIdentifier1
                 ]?.first {
                     it.resourceType == TestRunnerService.TestCaseArtifact.ResourceType.LOGCAT
                 }?.resultFileResource?.readFully()?.toString(Charsets.UTF_8)
             ).isEqualTo(
                 "test1 logcat"
             )
             assertThat(
-                screenshots?.get(testIdentifier)?.count {
+                screenshots1?.get(testIdentifier1)?.count {
                     it.resourceType == TestRunnerService.TestCaseArtifact.ResourceType.PNG
                 }
             ).isEqualTo(
                 3
             )
             assertThat(
-                screenshots?.get(testIdentifier)?.count {
+                screenshots1?.get(testIdentifier1)?.count {
                     it.resourceType == TestRunnerService.TestCaseArtifact.ResourceType.TEXTPROTO
                 }
             ).isEqualTo(
                 1
             )
             assertThat(
-                screenshots?.get(testIdentifier)?.first {
+                screenshots1?.get(testIdentifier1)?.first {
                     it.resourceType == TestRunnerService.TestCaseArtifact.ResourceType.TEXTPROTO
                 }?.resultFileResource?.gcsPath.toString()
             ).isEqualTo(
@@ -471,6 +476,19 @@ class TestRunnerServiceImplTest {
                     )
                 ]
             ).isNull()
+            // No screenshots for test2
+            val testIdentifier2 = TestRunnerService.TestIdentifier(
+                className = "class2",
+                name = "name2",
+                runNumber = testRun.deviceRun.runNumber
+            )
+            val screenshots2 = subject.getTestMatrixArtifacts(
+                testMatrixId,
+                listOf(testIdentifier2)
+            )
+            assertThat(
+                screenshots2
+            ).isEmpty()
             // logcat for test3 is missing from gcloud folder
             assertThat(
                 testRun.testCaseArtifacts[
@@ -481,7 +499,28 @@ class TestRunnerServiceImplTest {
                     )
                 ]
             ).isNull()
+            // No screenshots for test3 either
+            val testIdentifier3 = TestRunnerService.TestIdentifier(
+                className = "class2",
+                name = "name2",
+                runNumber = testRun.deviceRun.runNumber
+            )
+            val screenshots3 = subject.getTestMatrixArtifacts(
+                testMatrixId,
+                listOf(testIdentifier3)
+            )
+            assertThat(
+                screenshots3
+            ).isEmpty()
         }
+        // check screenshots is null when list of testIdentifiers is empty
+        val screenshots = subject.getTestMatrixArtifacts(
+            testMatrixId,
+            emptyList()
+        )
+        assertThat(
+            screenshots
+        ).isNull()
     }
 
     @Test
@@ -773,7 +812,7 @@ class TestRunnerServiceImplTest {
             "name1",
             0
         )
-        val screenshots = subject.getTestMatrixResultsScreenshots(
+        val screenshots = subject.getTestMatrixArtifacts(
             testMatrixId,
             listOf(testIdentifier)
         )