Page MenuHomePhabricator

Provide a way to filter arc unit coverage by coverable paths
Open, Needs TriagePublic

Description

Context

Our code base includes many types of content including:

  • Source code.
  • Documentation (in the form of markdown).
  • Misc structural files.

When we run arc unit --everything, the coverage results include files that will never be covered by tests.

Repro steps

  1. Have an .arcunit config with at least one test engine.
  2. Run arc unit --everything

Expected output

A list filtered down to paths that can actually be covered by the unit test engines.

COVERAGE REPORT
      0%     src/private/SomePrivateSource.m
     62%     src/SourceFile1.m
    100%     src/SourceFile2.m

Actual output (specific names filtered)

COVERAGE REPORT
      0%     src/SomeSource.h
      0%     src/SomeSource.h
      0%     examples/apps/Catalog/Catalog/SomeSource.m
      0%     examples/apps/Catalog/Catalog/SomeFolder/SomeSource.h
      0%     examples/apps/Catalog/Catalog/SomeFolder/SomeSource.m
      0%     examples/apps/Catalog/Catalog/SomeSource.m
      0%     src/private/SomePrivateSource.m
      0%     src/private/SomePrivateSource.h
      0%     third_party/clang-format-linter
      0%     tests/unit/SomeTest.m
      0%     tests/apps/Catalog/Info.plist
      0%     tests/apps/Catalog/Catalog.xcodeproj/project.pbxproj
      0%     .travis.yml
      0%     .jazzy.yaml
      0%     AUTHORS
      0%     CONTRIBUTING.md
      0%     LICENSE
      0%     .gitmodules
      0%     .gitignore
      0%     .arcconfig
      0%     .arclint
      0%     .arcunit
      0%     .clang-format
      0%     SomePod.podspec
      0%     Podfile
      0%     Podfile.lock
      0%     examples/apps/Catalog/Catalog/Info.plist
      0%     README.md
     62%     src/SourceFile1.m
    100%     src/SourceFile2.m

My explorations of a fix

My first hack-fix involved filtering the $paths used to output the coverage table by the same include/exclude regex defined in .arcunit. This gave me a result that was close to but not exactly the desired behavior.

My .arcunit looks like so:

{
  "engines": {
    "xcode": {
      "type": "xcode-test-engine",
      "include": "(src/.+\\.(m|h|mm|swift)$)",
      "exclude": "(/Pods/)"
    }
  }
}

The output after my patch:

COVERAGE REPORT
      0%     src/SomeSource1.h
      0%     src/private/SomePrivateSource.h
      0%     src/private/SomePrivateSource.m
      0%     src/SomeSource2.h
     62%     src/SomeSource1.m
    100%     src/SomeSource2.m

The patch:

diff --git a/src/unit/engine/ArcanistConfigurationDrivenUnitTestEngine.php b/src/unit/engine/ArcanistConfigurationDrivenUnitTestEngine.php
index 6cc659a..9fe502a 100644
--- a/src/unit/engine/ArcanistConfigurationDrivenUnitTestEngine.php
+++ b/src/unit/engine/ArcanistConfigurationDrivenUnitTestEngine.php
@@ -58,6 +58,8 @@ final class ArcanistConfigurationDrivenUnitTestEngine
 
     $built_test_engines = array();
     $all_paths = $this->getPaths();
+    
+    $affectedCoveragePaths = array();
 
     foreach ($config['engines'] as $name => $spec) {
       $type = idx($spec, 'type');
@@ -106,10 +108,14 @@ final class ArcanistConfigurationDrivenUnitTestEngine
           $exclude);
 
         $test_engine->setPaths($paths);
+
+        $affectedCoveragePaths = array_merge($affectedCoveragePaths, $paths);
       }
 
       $built_test_engines[] = $test_engine;
     }
+    
+    $this->setAffectedCoveragePaths($affectedCoveragePaths);
 
     return $built_test_engines;
   }
diff --git a/src/unit/engine/ArcanistUnitTestEngine.php b/src/unit/engine/ArcanistUnitTestEngine.php
index b54bafe..674d993 100644
--- a/src/unit/engine/ArcanistUnitTestEngine.php
+++ b/src/unit/engine/ArcanistUnitTestEngine.php
@@ -7,6 +7,7 @@ abstract class ArcanistUnitTestEngine extends Phobject {
 
   private $workingCopy;
   private $paths = array();
+  private $affectedCoveragePaths = array();
   private $enableCoverage;
   private $runAllTests;
   private $configurationManager;
@@ -78,6 +79,15 @@ abstract class ArcanistUnitTestEngine extends Phobject {
     return $this->enableCoverage;
   }
 
+  final public function setAffectedCoveragePaths(array $paths) {
+    $this->affectedCoveragePaths = $paths;
+    return $this;
+  }
+
+  final public function getAffectedCoveragePaths() {
+    return $this->affectedCoveragePaths;
+  }
+
   final public function setRenderer(ArcanistUnitRenderer $renderer = null) {
     $this->renderer = $renderer;
     return $this;
diff --git a/src/workflow/ArcanistUnitWorkflow.php b/src/workflow/ArcanistUnitWorkflow.php
index a1aadd3..27a9b37 100644
--- a/src/workflow/ArcanistUnitWorkflow.php
+++ b/src/workflow/ArcanistUnitWorkflow.php
@@ -148,9 +148,8 @@ EOTEXT
     $this->engine = $this->newUnitTestEngine($this->getArgument('engine'));
     if ($everything) {
       $this->engine->setRunAllTests(true);
-    } else {
-      $this->engine->setPaths($paths);
     }
+    $this->engine->setPaths($paths);
 
     $renderer = new ArcanistUnitConsoleRenderer();
     $this->engine->setRenderer($renderer);
@@ -197,7 +196,7 @@ EOTEXT
 
     if ($coverage) {
       $file_coverage = array_fill_keys(
-        $paths,
+        $this->engine->getAffectedCoveragePaths(),
         0);
       $file_reports = array();
       foreach ($coverage as $file => $reports) {

The feature request

My faulty assumption was that the regex that starts a unit test is equal to the regex of coverable files. In practice I think they might be different regexes.

My follow-up idea is to add a new filter to .arcunit. This filter would allow you to provide a regex of coverable files. This filter would have to be read in some manner by ArcanistConfigurationDrivenUnitTestEngine and made available to ArcanistUnitWorkflow. This seems like a much bigger change (adding a new field to .arcunit for everyone), thus me posting the issue before pursuing any further changes.

An example of what this new field might look like:

{
  "engines": {
    "xcode": {
      "type": "xcode-test-engine",
      "include": "(src/.+\\.(m|h|mm|swift)$)",
      "exclude": "(/Pods/)",
      "coverable": "(src/.+\\.(m|mm|swift)$)"
    }
  }
}

And the desired output:

COVERAGE REPORT
      0%     src/private/SomePrivateSource.m
     62%     src/SourceFile1.m
    100%     src/SourceFile2.m

Event Timeline

This is reasonable, but should wait for T5568, which probably needs T5055.

My inclination is to implement something like this:

public function getCoverablePaths(array $paths) { ... }

...on the test engine itself, rather than configuring "coverable". The intersection of engine-coverable paths and configured-include paths would be the coverable set.

Would that method be impossible to write and/or not work correctly in any cases you've seen?

(My guess is that 95% of the time the engine can just match all ".js" or ".php" or ".c" or ".m/.mm/.swift" files and get the right result without requiring configuration.)

I can imagine an odd coverage reporter that chooses not to generate reports for files without any coverage. This might not be a real thing though.

In most cases I'd expect the coverage reporter to generate a list of all files that were coverable. That output would likely be a reasonable return value for getCoverablePaths.