From de58fc809e378995a19019f5842dad89d348dd60 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 10 Aug 2015 15:35:15 -0700
Subject: [PATCH] Preserve more information when merging coverage

Summary:
Ref T8096. Each test reports coverage information, which we sometimes merge into a combined coverage report.

Usually, each test will report results for every line in the file, so if the file is 30 lines long, coverage is usually 30 characters long.

However, for whatever reason, tests might report results for only the first part of the file. This is allowed and we handle it properly.

Right now, if one test reports 10 lines of results and another reports 30 lines of results, we only use the first 10 lines of results. Instead, extend the merged coverage to include the extra 20 lines of results.

(This is an uncommon case which I only hit because I was manually banging on my keyboard to generate test data, but there's no reason not to handle it better.)

Test Plan: Used web UI, added + executed unit tests.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8096

Differential Revision: https://secure.phabricator.com/D13854
---
 src/__phutil_library_map__.php                |  2 +
 src/unit/ArcanistUnitTestResult.php           | 11 ++++-
 .../ArcanistUnitTestResultTestCase.php        | 43 +++++++++++++++++++
 3 files changed, 55 insertions(+), 1 deletion(-)
 create mode 100644 src/unit/__tests__/ArcanistUnitTestResultTestCase.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 422c792f..aa38462b 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -247,6 +247,7 @@ phutil_register_library_map(array(
     'ArcanistUnitRenderer' => 'unit/renderer/ArcanistUnitRenderer.php',
     'ArcanistUnitTestEngine' => 'unit/engine/ArcanistUnitTestEngine.php',
     'ArcanistUnitTestResult' => 'unit/ArcanistUnitTestResult.php',
+    'ArcanistUnitTestResultTestCase' => 'unit/__tests__/ArcanistUnitTestResultTestCase.php',
     'ArcanistUnitTestableLintEngine' => 'lint/engine/ArcanistUnitTestableLintEngine.php',
     'ArcanistUnitWorkflow' => 'workflow/ArcanistUnitWorkflow.php',
     'ArcanistUnnecessaryFinalModifierXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistUnnecessaryFinalModifierXHPASTLinterRule.php',
@@ -525,6 +526,7 @@ phutil_register_library_map(array(
     'ArcanistUnitRenderer' => 'Phobject',
     'ArcanistUnitTestEngine' => 'Phobject',
     'ArcanistUnitTestResult' => 'Phobject',
+    'ArcanistUnitTestResultTestCase' => 'PhutilTestCase',
     'ArcanistUnitTestableLintEngine' => 'ArcanistLintEngine',
     'ArcanistUnitWorkflow' => 'ArcanistWorkflow',
     'ArcanistUnnecessaryFinalModifierXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
diff --git a/src/unit/ArcanistUnitTestResult.php b/src/unit/ArcanistUnitTestResult.php
index 039393f8..a0dc2f72 100644
--- a/src/unit/ArcanistUnitTestResult.php
+++ b/src/unit/ArcanistUnitTestResult.php
@@ -129,13 +129,22 @@ final class ArcanistUnitTestResult extends Phobject {
 
     $base = reset($coverage);
     foreach ($coverage as $more_coverage) {
-      $len = min(strlen($base), strlen($more_coverage));
+      $base_len = strlen($base);
+      $more_len = strlen($more_coverage);
+
+      $len = min($base_len, $more_len);
       for ($ii = 0; $ii < $len; $ii++) {
         if ($more_coverage[$ii] == 'C') {
           $base[$ii] = 'C';
         }
       }
+
+      // If a secondary report has more data, copy all of it over.
+      if ($more_len > $base_len) {
+        $base .= substr($more_coverage, $base_len);
+      }
     }
+
     return $base;
   }
 
diff --git a/src/unit/__tests__/ArcanistUnitTestResultTestCase.php b/src/unit/__tests__/ArcanistUnitTestResultTestCase.php
new file mode 100644
index 00000000..7c4d43dd
--- /dev/null
+++ b/src/unit/__tests__/ArcanistUnitTestResultTestCase.php
@@ -0,0 +1,43 @@
+<?php
+
+final class ArcanistUnitTestResultTestCase extends PhutilTestCase {
+
+  public function testCoverageMerges() {
+    $cases = array(
+      array(
+        'coverage' => array(),
+        'expect' => null,
+      ),
+      array(
+        'coverage' => array(
+          'UUUNCNC',
+        ),
+        'expect' => 'UUUNCNC',
+      ),
+      array(
+        'coverage' => array(
+          'UUCUUU',
+          'UUUUCU',
+        ),
+        'expect' => 'UUCUCU',
+      ),
+      array(
+        'coverage' => array(
+          'UUCCCU',
+          'UUUCCCNNNC',
+        ),
+        'expect' => 'UUCCCCNNNC',
+      ),
+    );
+
+    foreach ($cases as $case) {
+      $input = $case['coverage'];
+      $expect = $case['expect'];
+
+      $actual = ArcanistUnitTestResult::mergeCoverage($input);
+
+      $this->assertEqual($expect, $actual);
+    }
+  }
+
+}