`ArcanistUnitTestResult->setCoverage` looks broken where `diffusion.updatecoverage` (conduit) looks OK

Intro

It has been a long time I didn’t use / contribute Phabricator so I’m maybe not doing my bug report correctly. If so let me know and I will fix it.

Kind of Related:

Bug Summary

ArcanistUnitTestResult->setCoverage looks broken where diffusion.updatecoverage (conduit) looks OK.

Note that I have read https://secure.phabricator.com/book/phabcontrib/article/adding_new_classes/ and I don’t ask for support building my unit engine.

Reproduction Instructions

  1. Write a ArcanistTestEngine that use ArcanistUnitTestResult->setCoverage

eg

final class ArcanistElixirTestEngine extends ArcanistUnitTestEngine
{
  private $log;

  private function setLogger(ArcanistLogEngine $logger)
  {
    $this->log = $logger;
  }

  private function getLogger(): ArcanistLogEngine
  {
    return $this->log;
  }

  public function getEngineConfigurationName()
  {
    return 'elixir';
  }

  protected function supportsRunAllTests()
  {
    return true;
  }

  public function run()
  {
    $this->setLogger(new ArcanistLogEngine());

    $cmd_line = csprintf("ENV=test mix test");
    $future = new ExecFuture('%C', $cmd_line);
    $future->setCWD($this->getWorkingCopy()->getProjectRoot());
    $future->resolvex();

    $parser = new ArcanistXUnitTestResultParser();
    $results = $parser->parseTestResults(
      Filesystem::readFile($this->getWorkingCopy()->getProjectPath("test-junit-report.xml"))
    );

    if ($this->getEnableCoverage() !== false) {
      $coverage_report = $this->readCoverage($this->getWorkingCopy()->getProjectPath("coverage.xml"));
      foreach ($results as $result) {
        $result->setCoverage($coverage_report);
      }
    }
    var_dump($results);
    return $results;
  }

  public function readCoverage($path)
  {
    $coverage_data = Filesystem::readFile($path);
    if (empty($coverage_data)) {
      return array();
    }

    $coverage_dom = new DOMDocument();
    $coverage_dom->loadXML($coverage_data);

    $reports = array();
    $classes = $coverage_dom->getElementsByTagName('class');

    foreach ($classes as $class) {
      // filename is actually python module path with ".py" at the end,
      // e.g.: tornado.web.py
      $relative_path = explode('.', $class->getAttribute('filename'));
      array_pop($relative_path);
      $relative_path = implode('/', $relative_path);

      $absolute_path = Filesystem::resolvePath($relative_path);

      // then we check if the path with ".ex" at the end is file (a Python
      // submodule), if it is - set relative and absolute paths to have
      // ".ex" at the end.
      if (is_file($absolute_path . '.ex')) {
        $relative_path .= '.ex';
        $absolute_path .= '.ex';
      }

      // then we check if the path with ".exs" at the end is file (a Python
      // submodule), if it is - set relative and absolute paths to have
      // ".exs" at the end.
      if (is_file($absolute_path . '.exs')) {
        $relative_path .= '.exs';
        $absolute_path .= '.exs';
      }

      if (!file_exists($absolute_path)) {
        continue;
      }

      // get total line count in file
      $line_count = count(file($absolute_path));

      $coverage = '';
      $start_line = 1;
      $lines = $class->getElementsByTagName('line');
      for ($ii = 0; $ii < $lines->length; $ii++) {
        $line = $lines->item($ii);

        $next_line = (int) $line->getAttribute('number');
        for ($start_line; $start_line < $next_line; $start_line++) {
          $coverage .= 'N';
        }

        if ((int) $line->getAttribute('hits') == 0) {
          $coverage .= 'U';
        } else if ((int) $line->getAttribute('hits') > 0) {
          $coverage .= 'C';
        }

        $start_line++;
      }

      if ($start_line < $line_count) {
        foreach (range($start_line, $line_count) as $line_num) {
          $coverage .= 'N';
        }
      }

      $reports[$relative_path] = $coverage;
    }

    return $reports;
  }
}
  1. Run arc unit to test that everything is OK locally (including a var dump)

Image 1

  1. do arc diff
expected reality OK/ KO
lint are reported yes yes OK
unit are reported yes yes OK
coverage is reported yes no KO

See

I did reach image upload limit as a new user -> See Image 2 in comment

  1. Compare to doing a call to diffusion.updatecoverage
expected reality OK/ KO
lint are reported - - NA
unit are reported - - NA
coverage is reported yes yes OK

I did reach image upload limit as a new user -> See Image 3 in comment

I did reach image upload limit as a new user -> See Image 4 in comment

Phabricator/Arcanist Version

package git_hash date
phabricator 62f5bdbbd2c5 Mon, Mar 9
arcanist 66a6128239e2 Fri, Mar 6

Image 2

Image 3

Image 4

I’m stupid:

  • Harbormaster api is for diffusion
  • the arc unit is for differential

I expected that:

  • outputting a coverage in a Diff, would affect also the one in diffusion

But I guess I will duplicate the code so that Harbomaster can arc unit with a new Conduit reporter and thus actualise coverage overall.

Closing this.