Let "arc unit --target" also update coverage

Root Problem

After 3 years off, I start using Phabricator again.
I’m building a product using CQRS-ES.

As the business side of the project is pretty complex, and there is less false positive (since CQRS is super decoupled, each test is mostly targeting one file and one file is mostly targeted by a single test suite).
So we value code coverage a lot on this project. In peculiar, we want to be sure that the business part of CQRS (aka command or query) is well tested. And be able to visually spot some branch of code not tested into those files.

I’m strugling to use the existing tools (either ArcanistUnitTestResult->setCoverage on custom UnitEngine or api/diffusion.updatecoverage).

I know there is plan for better coverage:

Contribution Intro

@epriestley,

I have already been in the process of contributing to Phabricator in the past.
I have also reread https://secure.phabricator.com/book/phabcontrib/article/contributing_code/

I know that Phabricator upstream is hesitant to contribution in code as it is hard to maintain over years on a giant codebase. I understand that we, as contributor,often misjudged the amount of work to then maintain the code.

I also know that the area touched is going to be rework and that my proposed contribution may, based on the interpretation fall under:

No Prototype Changes
With rare exceptions, we do not accept patches for prototype applications for the same reasons that we don’t accept feature requests or bug reports. To learn more about prototype applications, see User Guide: Prototype Applications.

I’m still willing to discuss a potential idea that I find pragmatic as:

  1. it uses existing infrastructure:
  • the --target option of arc unit,
  • the coverage of ArcanistUnitTestResult
  • a few existing conduit apis (mostly stable)
  1. the changes looks not to complex:
  • no data access
  • less that 50 lines of code,mostly conduit calls
  1. it should be relatively easy to maintains as it will be pretty much contained:
  • part of arc unit worflow
  • only accessible to a few users: the one that enable arc unit --everything --coverage --target {target.phid} ----conduit-token api
  • few reason of changes. I see three reasons of changes:
    • api/diffusion.updatecoverage is deleted/changed => ACTION: update or just delete the function in charge of updating the coverage
    • T13125 new infra will become reality => ACTION: update function in charge of updating the coverage to call new infra
    • --target option of arc unit is deprecated => ACTION: the function will be deleted in the same time of the unit result upload
  1. it brings a lot value quickly as it integrate into existing worklow:

Proposed changes

(Note that this is pseudo code: probably working but not tested. It is used to show the idea before really contributing).

public function run() {
    $working_copy = $this->getWorkingCopy();

    $paths = $this->getArgument('paths');
    $rev = $this->getArgument('rev');
    $everything = $this->getArgument('everything');


// ....


    $target_phid = $this->getArgument('target');
    if ($target_phid) {
      $this->uploadTestResults($target_phid, $overall_result, $results);
+      if (!empty($result->getCoverage())) {
+        $this->uploadTestCoverage($target_phid, $overall_result, $results);
+     }
    }

    return $overall_result;
  }

And then a uploadTestCoverage that does do roughly this:

  private function uploadTestCoverage(
    $target_phid,
    array $unit) {
    $log = new ArcanistLogEngine();

    // 1) Get buildPHID (API: stable)
    $build_targets_search_api_parameters = array('constraints' => array('phids' => array($target_phid)));
    $build_targets_search_results = $this->getConduit()->callMethodSynchronous('harbormaster.target.search', $build_targets_search_api_parameters);
    $build_phid = $build_targets_search_results['data'][0]['fields']['buildPHID'];

    // 2) Get buildablePHID (API: stable)
    $build_search_api_parameters = array('constraints' => array('phids' => array($build_phid)));
    $build_search_results = $this->getConduit()->callMethodSynchronous('harbormaster.build.search', $build_search_api_parameters);
    $buildable_phid = $build_search_results['data'][0]['fields']['buildablePHID'];

    // 3) Get objectPHID (API: stable)
    $buildable_api_parameters = array('constraints' => array('phids' => array($buildable_phid)));
    $buildable_search_results = $this->getConduit()->callMethodSynchronous('harbormaster.buildable.search', $buildable_api_parameters);
    $object_phid = $buildable_search_results['data'][0]['fields']['objectPHID'];

    if (strpos($object_phid, 'PHID-CMIT') !== 0) {
      $log->writeWarning("UNIT", "Skiping coverage as the object build is not a commit.");
      return;
    }

    // 4) Get commit identifier (API: stable)
    $commit_api_parameters = array('constraints' => array('phids' => array($object_phid)));
    $commit_search_results = $this->getConduit()->callMethodSynchronous('diffusion.commit.search', $commit_api_parameters);
    $commit_identifier = $commit_search_results['data'][0]['fields']['identifier'];
    $repository_phid = $commit_search_results['data'][0]['fields']['repositoryPHID'];

    // 5) Get the associated branch (API: stable)
    $branch_api_parameters = array('contains' => $commit_identifier,'repository' => $repository_phid);
    $branch_results = $this->getConduit()->callMethodSynchronous('diffusion.branchquery', $branch_api_parameters);
    $branch = $branch_results['0']['shortName'];

    // 6) Update coverage
    foreach ($unit as $result) {
      $update_coverage_api_parameters = array(
        'repositoryPHID' => $repository_phid,
        'branch' =>  $branch,
        'commit' => $commit_identifier,
        'coverage' => $result->getCoverage(),
      );
      $update_coverage_results = $this->getConduit()->callMethodSynchronous('diffusion.updatecoverage', $update_coverage_api_parameters);
    }
  }

Conclusion

@epriestley, would you accept such a contribution ?