fix: Fix NPE (#1387)

Fixes #1382 

TLDR;
* fix NPE when `<testsuites/>` was deserialized (a workaround was needed though)
* prevent XML results from being uploaded (for smart flank) if contain no results

When an infrastructure error occurs result matrix is empty which leads to empty `JUnitTestResult`being created. Flank should not upload empty results to prevent old data corruption. 

Flank should also handle XML results with a self-closed root element (XML results for smart flank can be also uploaded manually). That could be easily fixed by changing `EMPTY_ELEMENT_AS_NULL` to false but flank uses this configuration quite extensively (ex skipped tag), so it's not an option. A small workaround was implemented: when the XML file is parsed flank checks if the string contains `<testsuites/>`. If does, an empty `JUnitTestResult` object is returned.

## Test Plan
> How do we know the code works?

Unfortunately, I did not come up with any way of testing it (no idea how to simulate infrastructure error). I covered it in unit test (with mocks)

## Checklist

- [x] Documented
- [x] Unit tested

docs/index.md

@@ -237,6 +237,7 @@ flank:
 
   ### Smart Flank GCS Paths
   ## Google cloud storage path to store the JUnit XML results from the last run.
+  ## NOTE: Empty results will not be uploaded
   # smart-flank-gcs-path: gs://tmp_flank/flank/test_app_ios.xml
 
   ### Smart Flank Disable Upload flag
@@ -574,6 +575,7 @@ flank:
 
   ### Smart Flank GCS Path
   ## Google cloud storage path where the JUnit XML results from the last run is stored.
+  ## NOTE: Empty results will not be uploaded
   # smart-flank-gcs-path: gs://tmp_flank/tmp/JUnitReport.xml
 
   ### Smart Flank Upload Disable flag

docs/smart_flank.md

@@ -8,7 +8,7 @@ At the start of a run, Flank checks to see if there's a JUnit XML with timing in
 
 After each test run, the aggregated JUnit XML from the new run is merged with the old run. Any tests not in the new run are discarded. Tests that were skipped, errored, failed, or empty are discarded. The test time value of successful tests is set using the time from the latest run. If a test failed in the new run and passed in the old run, the timing info from the old run is carried over.
 
-The merged XML is uploaded to the user defined `smartFlankGcsPath`. If `smartFlankGcsPath` is not defined, then the smart flank feature will not activate.
+The merged XML is uploaded to the user defined `smartFlankGcsPath`. If `smartFlankGcsPath` is not defined, then the smart flank feature will not activate. Empty results are not uploaded.
 
 Example:
 

test_runner/.gitignore

@@ -6,6 +6,7 @@ EarlGreyExample/
 xctestrun/
 src/test/kotlin/ftl/fixtures/error_result/*.xml
 src/test/kotlin/ftl/fixtures/success_result/*.xml
+src/test/kotlin/ftl/fixtures/empty_result/*.xml
 *.json
 *-describe.adoc
 should_exists.txt

test_runner/src/main/kotlin/ftl/reports/util/ReportManager.kt

@@ -226,7 +226,7 @@ object ReportManager {
         args: IArgs,
         testShardChunks: ShardChunks
     ) {
-        if (newTestResult == null) return
+        if (newTestResult == null || newTestResult.testsuites.isNullOrEmpty()) return
 
         val oldTestResult = GcStorage.downloadJunitXml(args)
 

test_runner/src/main/kotlin/ftl/reports/xml/JUnitXml.kt

@@ -55,6 +55,11 @@ fun parseAllSuitesXml(path: File): JUnitTestResult {
     return parseAllSuitesXml(path.toPath())
 }
 
-fun parseAllSuitesXml(data: String): JUnitTestResult {
-    return xmlMapper.readValue(fixHtmlCodes(data), JUnitTestResult::class.java)
-}
+fun parseAllSuitesXml(data: String): JUnitTestResult =
+    // This is workaround for flank being unable to parse <testsuites/> into JUnitTesResults
+    // We need to preserve configure(EMPTY_ELEMENT_AS_NULL, true) to skip empty elements
+    // Once better solution is found, this should be fixed
+    if (data.contains("<testsuites/>"))
+        JUnitTestResult(null)
+    else
+        xmlMapper.readValue(fixHtmlCodes(data), JUnitTestResult::class.java)

test_runner/src/main/kotlin/ftl/reports/xml/model/JUnitTestResult.kt

@@ -1,5 +1,6 @@
 package ftl.reports.xml.model
 
+import com.fasterxml.jackson.annotation.JsonInclude
 import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlProperty
 import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlRootElement
 
@@ -8,8 +9,9 @@ import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlRootElement
  * .xctestrun file may contain multiple test bundles (each one is a testsuite) */
 @JacksonXmlRootElement(localName = "testsuites")
 data class JUnitTestResult(
+    @JsonInclude(JsonInclude.Include.NON_EMPTY)
     @JacksonXmlProperty(localName = "testsuite")
-    var testsuites: MutableList<JUnitTestSuite>?
+    var testsuites: MutableList<JUnitTestSuite>? = null
 ) {
     fun successful(): Boolean {
         var successful = true

test_runner/src/test/kotlin/ftl/fixtures/empty_result/.gitignore

@@ -0,0 +1,3 @@
+*.txt
+*.html
+*.csv

test_runner/src/test/kotlin/ftl/fixtures/empty_result/JUnitReport.xml

@@ -0,0 +1,2 @@
+<?xml version='1.0' encoding='UTF-8' ?>
+<testsuites/>

test_runner/src/test/kotlin/ftl/fixtures/empty_result/matrix_ids.json

@@ -0,0 +1,12 @@
+{
+  "matrix-xo22hroienrza": {
+    "matrixId": "matrix-xo22hroienrza",
+    "state": "FINISHED",
+    "gcsPath": "test-lab-jwwvtzdh5d20w-yyju9fnudnpts/2018-09-09_00:38:10.110000_UQjK/shard_0",
+    "webLink": "https://console.firebase.google.com/project/delta-essence-114723/testlab/histories/bh.58317d9cd7ab9ba2/matrices/6385582924491679444/executions/bs.bb41d8a3f63489be",
+    "downloaded": false,
+    "billableVirtualMinutes": 1,
+    "billablePhysicalMinutes": 0,
+    "outcome": "inconclusive"
+  }
+}

test_runner/src/test/kotlin/ftl/fixtures/success_result/matrix_ids.json

@@ -7,6 +7,13 @@
     "downloaded": false,
     "billableVirtualMinutes": 1,
     "billablePhysicalMinutes": 0,
-    "outcome": "success"
+    "outcome": "success",
+    "testAxises": [
+      {
+        "device": "NexusLowRes-28-en-portrait",
+        "outcome": "success",
+        "details": "2 test cases passed"
+      }
+    ]
   }
-}
+}

test_runner/src/test/kotlin/ftl/reports/utils/ReportManagerTest.kt

@@ -1,6 +1,7 @@
 package ftl.reports.utils
 
 import com.google.common.truth.Truth.assertThat
+import com.google.testing.model.TestExecution
 import ftl.args.AndroidArgs
 import ftl.config.FtlConstants
 import ftl.gc.GcStorage
@@ -9,6 +10,7 @@ import ftl.reports.CostReport
 import ftl.reports.FullJUnitReport
 import ftl.reports.JUnitReport
 import ftl.reports.MatrixResultsReport
+import ftl.reports.api.createJUnitTestResult
 import ftl.reports.api.refreshMatricesAndGetExecutions
 import ftl.reports.util.ReportManager
 import ftl.reports.util.getMatrixPath
@@ -89,6 +91,27 @@ class ReportManagerTest {
         return mockArgs
     }
 
+    @Test
+    fun `JUnit results should not be uploaded if junit result is null`() {
+        val matrix = matrixPathToObj("./src/test/kotlin/ftl/fixtures/empty_result", AndroidArgs.default())
+        val mockArgs = prepareMockAndroidArgs()
+        every { mockArgs.localResultDir } returns "./src/test/kotlin/ftl/fixtures/empty_result"
+        mockkObject(GcStorage)
+        ReportManager.generate(matrix, mockArgs, emptyList())
+        verify(exactly = 0) { GcStorage.uploadJunitXml(any(), any()) }
+    }
+
+    @Test
+    fun `JUnit results should not be uploaded if junit result is empty`() {
+        val matrix = matrixPathToObj("./src/test/kotlin/ftl/fixtures/empty_result", AndroidArgs.default())
+        val mockArgs = prepareMockAndroidArgs()
+        every { mockArgs.localResultDir } returns "./src/test/kotlin/ftl/fixtures/empty_result"
+        every { mockArgs.useLegacyJUnitResult } returns false
+        mockkObject(GcStorage)
+        ReportManager.generate(matrix, mockArgs, emptyList())
+        verify(exactly = 0) { GcStorage.uploadJunitXml(any(), any()) }
+    }
+
     @Test
     fun `uploadJunitXml should be called`() {
         val matrix = matrixPathToObj("./src/test/kotlin/ftl/fixtures/success_result", AndroidArgs.default())
@@ -105,6 +128,13 @@ class ReportManagerTest {
         every { mockArgs.useLegacyJUnitResult } returns false
         every { mockArgs.project } returns "projecId"
 
+        val executions = emptyList<TestExecution>()
+        mockkStatic("ftl.reports.api.ProcessFromApiKt")
+        mockkStatic("ftl.reports.api.CreateJUnitTestResultKt")
+        every { refreshMatricesAndGetExecutions(any(), any()) } returns executions
+        every { executions.createJUnitTestResult(any()) } returns JUnitTestResult(mutableListOf(suite))
+
+
         val junitTestResult = ReportManager.processXmlFromFile(matrix, mockArgs, ::parseOneSuiteXml)
         ReportManager.generate(matrix, mockArgs, emptyList())
         verify { GcStorage.uploadJunitXml(junitTestResult!!, mockArgs) }
@@ -118,6 +148,12 @@ class ReportManagerTest {
         every { mockArgs.fullJUnitResult } returns true
         every { mockArgs.project } returns "projecId"
 
+        val executions = emptyList<TestExecution>()
+        mockkStatic("ftl.reports.api.ProcessFromApiKt")
+        mockkStatic("ftl.reports.api.CreateJUnitTestResultKt")
+        every { refreshMatricesAndGetExecutions(any(), any()) } returns executions
+        every { executions.createJUnitTestResult(any()) } returns JUnitTestResult(mutableListOf(suite))
+
         val junitTestResult = ReportManager.processXmlFromFile(matrix, mockArgs, ::parseOneSuiteXml)
         ReportManager.generate(matrix, mockArgs, emptyList())
         verify { GcStorage.uploadJunitXml(junitTestResult!!, mockArgs) }
@@ -250,4 +286,16 @@ class ReportManagerTest {
         val path = File("results/test_dir/test_dir/shard_0/iphone8-12.0-en-portrait/test_result_0.xml")
         assertEquals("test_dir/shard_0", path.getMatrixPath("test_dir"))
     }
+
+    private val suite: JUnitTestSuite
+        get() = JUnitTestSuite(
+            name = "any",
+            tests = "2",
+            failures = "0",
+            errors = "0",
+            skipped = null,
+            time = "0.12",
+            timestamp = "123:456",
+            testcases = null
+        )
 }

test_runner/src/test/kotlin/ftl/reports/xml/JUnitXmlTest.kt

@@ -41,6 +41,15 @@ class JUnitXmlTest {
         parseAllSuitesXml(xml)
     }
 
+    @Test
+    fun `empty testcase -- infrastructure error result`() {
+        val xml = "<testsuites/>"
+
+        parseAllSuitesXml(xml).run {
+            assertThat(testsuites).isNull()
+        }
+    }
+
     @Test
     fun `merge android`() {
         val mergedXml = parseOneSuiteXml(androidPassXml).merge(parseOneSuiteXml(androidFailXml))