Allow PhutilUnitTestEngine to execute tests from a single path

Summary: Fixes T8042. Changes the way that `PhutilUnitTestEngine` discovers unit tests. In particular, if you only modify a single test case then there is no reason to run all other test cases within the same directory (which is the current behavior).

Test Plan: Added some unit tests.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Maniphest Tasks: T8042

Differential Revision: https://secure.phabricator.com/D12689

src/unit/engine/ArcanistUnitTestEngine.php

@@ -8,14 +8,14 @@ abstract class ArcanistUnitTestEngine {
   private $workingCopy;
   private $paths;
   private $arguments = array();
-  protected $diffID;
   private $enableAsyncTests;
   private $enableCoverage;
   private $runAllTests;
   protected $renderer;
 
+  final public function __construct() {}
 
-  public function setRunAllTests($run_all_tests) {
+  final public function setRunAllTests($run_all_tests) {
     if (!$this->supportsRunAllTests() && $run_all_tests) {
       throw new Exception(
         pht(
@@ -28,7 +28,7 @@ abstract class ArcanistUnitTestEngine {
     return $this;
   }
 
-  public function getRunAllTests() {
+  final public function getRunAllTests() {
     return $this->runAllTests;
   }
 
@@ -36,15 +36,13 @@ abstract class ArcanistUnitTestEngine {
     return false;
   }
 
-  final public function __construct() {}
+  final public function setConfigurationManager(
-
-  public function setConfigurationManager(
     ArcanistConfigurationManager $configuration_manager) {
     $this->configurationManager = $configuration_manager;
     return $this;
   }
 
-  public function getConfigurationManager() {
+  final public function getConfigurationManager() {
     return $this->configurationManager;
   }
 
@@ -95,7 +93,7 @@ abstract class ArcanistUnitTestEngine {
     return $this->enableCoverage;
   }
 
-  public function setRenderer(ArcanistUnitRenderer $renderer) {
+  final public function setRenderer(ArcanistUnitRenderer $renderer) {
     $this->renderer = $renderer;
     return $this;
   }

src/unit/engine/PhutilUnitTestEngine.php

@@ -21,14 +21,16 @@ final class PhutilUnitTestEngine extends ArcanistUnitTestEngine {
     }
 
     $enable_coverage = $this->getEnableCoverage();
+
     if ($enable_coverage !== false) {
       if (!function_exists('xdebug_start_code_coverage')) {
         if ($enable_coverage === true) {
           throw new ArcanistUsageException(
             pht(
-              'You specified %s but xdebug is not available, so '.
+              'You specified %s but %s is not available, so '.
               'coverage can not be enabled for %s.',
               '--coverage',
+              'XDebug',
               __CLASS__));
         }
       } else {
@@ -36,20 +38,21 @@ final class PhutilUnitTestEngine extends ArcanistUnitTestEngine {
       }
     }
 
-    $project_root = $this->getWorkingCopy()->getProjectRoot();
-
     $test_cases = array();
+
     foreach ($run_tests as $test_class) {
-      $test_case = newv($test_class, array());
+      $test_case = newv($test_class, array())
-      $test_case->setEnableCoverage($enable_coverage);
+        ->setEnableCoverage($enable_coverage)
-      $test_case->setWorkingCopy($this->getWorkingCopy());
+        ->setWorkingCopy($this->getWorkingCopy());
 
       if ($this->getPaths()) {
         $test_case->setPaths($this->getPaths());
       }
+
       if ($this->renderer) {
         $test_case->setRenderer($this->renderer);
       }
+
       $test_cases[] = $test_case;
     }
 
@@ -104,75 +107,24 @@ final class PhutilUnitTestEngine extends ArcanistUnitTestEngine {
     return $run_tests;
   }
 
+  /**
+   * Retrieve all relevant test cases.
+   *
+   * Looks for any class that extends @{class:PhutilTestCase} inside a
+   * `__tests__` directory in any parent directory of every affected file.
+   *
+   * The idea is that "infrastructure/__tests__/" tests defines general tests
+   * for all of "infrastructure/", and those tests run for any change in
+   * "infrastructure/". However, "infrastructure/concrete/rebar/__tests__/"
+   * defines more specific tests that run only when "rebar/" (or some
+   * subdirectory) changes.
+   *
+   * @return list<string>  The names of the test case classes to be executed.
+   */
   private function getTestsForPaths() {
-    $project_root = $this->getWorkingCopy()->getProjectRoot();
+    $look_here = $this->getTestPaths();
-
-    $look_here = array();
-
-    foreach ($this->getPaths() as $path) {
-      $library_root = phutil_get_library_root_for_path($path);
-      if (!$library_root) {
-        continue;
-      }
-      $library_name = phutil_get_library_name_for_root($library_root);
-
-      if (!$library_name) {
-        throw new Exception(
-          sprintf(
-            "%s\n\n    %s\n\n%s\n\n    - %s\n    - %s\n",
-            pht(
-              'Attempting to run unit tests on a libphutil library '.
-              'which has not been loaded, at:'),
-            $library_root,
-            pht('This probably means one of two things:'),
-            pht(
-              'You may need to add this library to %s.',
-              '.arcconfig.'),
-            pht(
-              'You may be running tests on a copy of libphutil or '.
-              'arcanist using a different copy of libphutil or arcanist. '.
-              'This operation is not supported.')));
-      }
-
-      $path = Filesystem::resolvePath($path, $project_root);
-
-      if (!is_dir($path)) {
-        $path = dirname($path);
-      }
-
-      if ($path == $library_root) {
-        $look_here[$library_name.':.'] = array(
-          'library' => $library_name,
-          'path'    => '',
-        );
-      } else if (!Filesystem::isDescendant($path, $library_root)) {
-        // We have encountered some kind of symlink maze -- for instance, $path
-        // is some symlink living outside the library that links into some file
-        // inside the library. Just ignore these cases, since the affected file
-        // does not actually lie within the library.
-        continue;
-      } else {
-        $library_path = Filesystem::readablePath($path, $library_root);
-        do {
-          $look_here[$library_name.':'.$library_path] = array(
-            'library' => $library_name,
-            'path'    => $library_path,
-          );
-          $library_path = dirname($library_path);
-        } while ($library_path != '.');
-      }
-    }
-
-    // Look for any class that extends PhutilTestCase inside a `__tests__`
-    // directory in any parent directory of every affected file.
-    //
-    // The idea is that "infrastructure/__tests__/" tests defines general tests
-    // for all of "infrastructure/", and those tests run for any change in
-    // "infrastructure/". However, "infrastructure/concrete/rebar/__tests__/"
-    // defines more specific tests that run only when rebar/ (or some
-    // subdirectory) changes.
-
     $run_tests = array();
+
     foreach ($look_here as $path_info) {
       $library = $path_info['library'];
       $path    = $path_info['path'];
@@ -180,7 +132,7 @@ final class PhutilUnitTestEngine extends ArcanistUnitTestEngine {
       $symbols = id(new PhutilSymbolLoader())
         ->setType('class')
         ->setLibrary($library)
-        ->setPathPrefix(($path ? $path.'/' : '').'__tests__/')
+        ->setPathPrefix($path)
         ->setAncestorClass('PhutilTestCase')
         ->setConcreteOnly(true)
         ->selectAndLoadSymbols();
@@ -189,9 +141,71 @@ final class PhutilUnitTestEngine extends ArcanistUnitTestEngine {
         $run_tests[$symbol['name']] = true;
       }
     }
-    $run_tests = array_keys($run_tests);
 
-    return $run_tests;
+    return array_keys($run_tests);
+  }
+
+  /**
+   * Returns the paths in which we should look for tests to execute.
+   *
+   * @return list<string>  A list of paths in which to search for test cases.
+   */
+  public function getTestPaths() {
+    $root  = $this->getWorkingCopy()->getProjectRoot();
+    $paths = array();
+
+    foreach ($this->getPaths() as $path) {
+      $library_root = phutil_get_library_root_for_path($path);
+
+      if (!$library_root) {
+        continue;
+      }
+
+      $library_name = phutil_get_library_name_for_root($library_root);
+
+      if (!$library_name) {
+        throw new Exception(
+          pht(
+            "Attempting to run unit tests on a libphutil library which has ".
+            "not been loaded, at:\n\n".
+            "    %s\n\n".
+            "This probably means one of two things:\n\n".
+            "    - You may need to add this library to %s.\n".
+            "    - You may be running tests on a copy of libphutil or ".
+            "arcanist using a different copy of libphutil or arcanist. ".
+            "This operation is not supported.\n",
+            $library_root,
+            '.arcconfig.'));
+      }
+
+      $path = Filesystem::resolvePath($path, $root);
+      $library_path = Filesystem::readablePath($path, $library_root);
+
+      if (!Filesystem::isDescendant($path, $library_root)) {
+        // We have encountered some kind of symlink maze -- for instance, $path
+        // is some symlink living outside the library that links into some file
+        // inside the library. Just ignore these cases, since the affected file
+        // does not actually lie within the library.
+        continue;
+      }
+
+      if (is_file($path) && preg_match('@(?:^|/)__tests__/@', $path)) {
+        $paths[$library_name.':'.$library_path] = array(
+          'library' => $library_name,
+          'path'    => $library_path,
+        );
+        continue;
+      }
+
+      while (($library_path = dirname($library_path)) != '.') {
+        $paths[$library_name.':'.$library_path] = array(
+          'library' => $library_name,
+          'path'    => $library_path.'/__tests__/',
+        );
+      }
+    }
+
+    return $paths;
   }
 
   public function shouldEchoTestResults() {

src/unit/engine/__tests__/PhutilUnitTestEngineTestCase.php

@@ -24,7 +24,7 @@ final class PhutilUnitTestEngineTestCase extends PhutilTestCase {
 
     self::$allTestsCounter--;
 
-    $actual_test_count = 4;
+    $actual_test_count = 5;
 
     $this->assertEqual(
       $actual_test_count,
@@ -120,4 +120,47 @@ final class PhutilUnitTestEngineTestCase extends PhutilTestCase {
     }
   }
 
+  public function testGetTestPaths() {
+    $tests = array(
+      array(
+        array(),
+        array(),
+      ),
+
+      array(
+        array(__FILE__),
+        array(__FILE__),
+      ),
+
+      array(
+        array(dirname(__FILE__)),
+        array(
+          dirname(dirname(__FILE__)).'/__tests__/',
+          dirname(dirname(dirname(__FILE__))).'/__tests__/',
+        ),
+      ),
+    );
+
+    $test_engine = id(new PhutilUnitTestEngine())
+      ->setWorkingCopy($this->getWorkingCopy());
+
+    foreach ($tests as $test) {
+      list($paths, $tests) = $test;
+      $expected = array();
+
+      foreach ($tests as $path) {
+        $library_root = phutil_get_library_root_for_path($path);
+        $library = phutil_get_library_name_for_root($library_root);
+
+        $expected[] = array(
+          'library' => $library,
+          'path' => Filesystem::readablePath($path, $library_root),
+        );
+      }
+
+      $test_engine->setPaths($paths);
+      $this->assertEqual($expected, array_values($test_engine->getTestPaths()));
+    }
+  }
+
 }

src/unit/engine/phutil/PhutilTestCase.php

@@ -166,8 +166,10 @@ abstract class PhutilTestCase {
    * @return void
    * @task exceptions
    */
-  final protected function assertException($expected_exception_class,
+  final protected function assertException(
-                                           $callable) {
+    $expected_exception_class,
+    $callable) {
+
     $this->tryTestCases(
       array('assertException' => array()),
       array(false),
@@ -300,6 +302,7 @@ abstract class PhutilTestCase {
     array $map,
     $callable,
     $exception_class = 'Exception') {
+
     return $this->tryTestCases(
       array_fuse(array_keys($map)),
       array_values($map),
@@ -655,7 +658,7 @@ abstract class PhutilTestCase {
     return (string)$uri;
   }
 
-  public function setRenderer(ArcanistUnitRenderer $renderer) {
+  final public function setRenderer(ArcanistUnitRenderer $renderer) {
     $this->renderer = $renderer;
     return $this;
   }