Improve lint/unit limit, sort, view all, collapse behaviors

Summary:
Ref T8096. Various tweaks here:

  - Sort result lists by importance (even lint -- "errors first" seems better than "alphabetical by file", I think?).
  - Do sane stuff with display limits.
  - Add a "view all" view.
  - Don't show a huge table of passing tests in Differential.
  - Link to full results.

Test Plan: See screenshots.

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8096

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

resources/celerity/map.php

@@ -10,7 +10,7 @@ return array(
     'core.pkg.css' => 'eb51e6dc',
     'core.pkg.js' => '711e63c0',
     'darkconsole.pkg.js' => 'e7393ebb',
-    'differential.pkg.css' => '1ca3c116',
+    'differential.pkg.css' => '7b52b9be',
     'differential.pkg.js' => 'ebef29b1',
     'diffusion.pkg.css' => '591664fa',
     'diffusion.pkg.js' => '0115b37c',
@@ -64,7 +64,7 @@ return array(
     'rsrc/css/application/differential/revision-comment.css' => '14b8565a',
     'rsrc/css/application/differential/revision-history.css' => '0e8eb855',
     'rsrc/css/application/differential/revision-list.css' => 'f3c47d33',
-    'rsrc/css/application/differential/table-of-contents.css' => '63f3ef4a',
+    'rsrc/css/application/differential/table-of-contents.css' => 'ae4b7a55',
     'rsrc/css/application/diffusion/diffusion-icons.css' => '9c5828da',
     'rsrc/css/application/diffusion/diffusion-readme.css' => '2106ea08',
     'rsrc/css/application/diffusion/diffusion-source.css' => '66fdf661',
@@ -515,7 +515,7 @@ return array(
     'differential-revision-comment-css' => '14b8565a',
     'differential-revision-history-css' => '0e8eb855',
     'differential-revision-list-css' => 'f3c47d33',
-    'differential-table-of-contents-css' => '63f3ef4a',
+    'differential-table-of-contents-css' => 'ae4b7a55',
     'diffusion-icons-css' => '9c5828da',
     'diffusion-readme-css' => '2106ea08',
     'diffusion-source-css' => '66fdf661',

src/__phutil_library_map__.php

@@ -891,6 +891,7 @@ phutil_register_library_map(array(
     'HarbormasterDAO' => 'applications/harbormaster/storage/HarbormasterDAO.php',
     'HarbormasterHTTPRequestBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterHTTPRequestBuildStepImplementation.php',
     'HarbormasterLeaseHostBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterLeaseHostBuildStepImplementation.php',
+    'HarbormasterLintMessagesController' => 'applications/harbormaster/controller/HarbormasterLintMessagesController.php',
     'HarbormasterLintPropertyView' => 'applications/harbormaster/view/HarbormasterLintPropertyView.php',
     'HarbormasterManagePlansCapability' => 'applications/harbormaster/capability/HarbormasterManagePlansCapability.php',
     'HarbormasterManagementBuildWorkflow' => 'applications/harbormaster/management/HarbormasterManagementBuildWorkflow.php',
@@ -919,6 +920,7 @@ phutil_register_library_map(array(
     'HarbormasterTargetWorker' => 'applications/harbormaster/worker/HarbormasterTargetWorker.php',
     'HarbormasterThrowExceptionBuildStep' => 'applications/harbormaster/step/HarbormasterThrowExceptionBuildStep.php',
     'HarbormasterUIEventListener' => 'applications/harbormaster/event/HarbormasterUIEventListener.php',
+    'HarbormasterUnitMessagesController' => 'applications/harbormaster/controller/HarbormasterUnitMessagesController.php',
     'HarbormasterUnitPropertyView' => 'applications/harbormaster/view/HarbormasterUnitPropertyView.php',
     'HarbormasterUploadArtifactBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterUploadArtifactBuildStepImplementation.php',
     'HarbormasterWaitForPreviousBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterWaitForPreviousBuildStepImplementation.php',
@@ -4357,6 +4359,7 @@ phutil_register_library_map(array(
     'HarbormasterDAO' => 'PhabricatorLiskDAO',
     'HarbormasterHTTPRequestBuildStepImplementation' => 'HarbormasterBuildStepImplementation',
     'HarbormasterLeaseHostBuildStepImplementation' => 'HarbormasterBuildStepImplementation',
+    'HarbormasterLintMessagesController' => 'HarbormasterController',
     'HarbormasterLintPropertyView' => 'AphrontView',
     'HarbormasterManagePlansCapability' => 'PhabricatorPolicyCapability',
     'HarbormasterManagementBuildWorkflow' => 'HarbormasterManagementWorkflow',
@@ -4385,6 +4388,7 @@ phutil_register_library_map(array(
     'HarbormasterTargetWorker' => 'HarbormasterWorker',
     'HarbormasterThrowExceptionBuildStep' => 'HarbormasterBuildStepImplementation',
     'HarbormasterUIEventListener' => 'PhabricatorEventListener',
+    'HarbormasterUnitMessagesController' => 'HarbormasterController',
     'HarbormasterUnitPropertyView' => 'AphrontView',
     'HarbormasterUploadArtifactBuildStepImplementation' => 'HarbormasterBuildStepImplementation',
     'HarbormasterWaitForPreviousBuildStepImplementation' => 'HarbormasterBuildStepImplementation',

src/applications/differential/customfield/DifferentialHarbormasterField.php

@@ -81,12 +81,23 @@ abstract class DifferentialHarbormasterField
         $path_map[$path] = $href;
       }
 
-      $view = $this->newHarbormasterMessageView($messages)
+      $view = $this->newHarbormasterMessageView($messages);
-        ->setPathURIMap($path_map);
+      if ($view) {
+        $view->setPathURIMap($path_map);
+      }
     } else {
       $view = null;
     }
 
+    if ($view) {
+      $view = phutil_tag(
+        'div',
+        array(
+          'class' => 'differential-harbormaster-table-view',
+        ),
+        $view);
+    }
+
     return array(
       $status,
       $view,

src/applications/differential/customfield/DifferentialLintField.php

@@ -50,6 +50,7 @@ final class DifferentialLintField
 
   protected function newHarbormasterMessageView(array $messages) {
     return id(new HarbormasterLintPropertyView())
+      ->setLimit(25)
       ->setLintMessages($messages);
   }
 

src/applications/differential/customfield/DifferentialUnitField.php

@@ -44,7 +44,7 @@ final class DifferentialUnitField
 
   protected function loadHarbormasterTargetMessages(array $target_phids) {
     return id(new HarbormasterBuildUnitMessage())->loadAllWhere(
-      'buildTargetPHID IN (%Ls) LIMIT 25',
+      'buildTargetPHID IN (%Ls)',
       $target_phids);
   }
 
@@ -55,7 +55,19 @@ final class DifferentialUnitField
   }
 
   protected function newHarbormasterMessageView(array $messages) {
+    foreach ($messages as $key => $message) {
+      if ($message->getResult() == ArcanistUnitTestResult::RESULT_PASS) {
+        unset($messages[$key]);
+      }
+    }
+
+    if (!$messages) {
+      return null;
+    }
+
     return id(new HarbormasterUnitPropertyView())
+      ->setLimit(10)
+      ->setHidePassingTests(true)
       ->setUnitMessages($messages);
   }
 
@@ -97,6 +109,55 @@ final class DifferentialUnitField
 
     $message = DifferentialRevisionUpdateHistoryView::getDiffUnitMessage($diff);
 
+    $note = array();
+
+    $groups = mgroup($messages, 'getResult');
+
+    $groups = array_select_keys(
+      $groups,
+      array(
+        ArcanistUnitTestResult::RESULT_FAIL,
+        ArcanistUnitTestResult::RESULT_BROKEN,
+        ArcanistUnitTestResult::RESULT_UNSOUND,
+        ArcanistUnitTestResult::RESULT_SKIP,
+        ArcanistUnitTestResult::RESULT_PASS,
+      )) + $groups;
+
+    foreach ($groups as $result => $group) {
+      $count = new PhutilNumber(count($group));
+      switch ($result) {
+        case ArcanistUnitTestResult::RESULT_PASS:
+          $note[] = pht('%s Passed Test(s)', $count);
+          break;
+        case ArcanistUnitTestResult::RESULT_FAIL:
+          $note[] = pht('%s Failed Test(s)', $count);
+          break;
+        case ArcanistUnitTestResult::RESULT_SKIP:
+          $note[] = pht('%s Skipped Test(s)', $count);
+          break;
+        case ArcanistUnitTestResult::RESULT_BROKEN:
+          $note[] = pht('%s Broken Test(s)', $count);
+          break;
+        case ArcanistUnitTestResult::RESULT_UNSOUND:
+          $note[] = pht('%s Unsound Test(s)', $count);
+          break;
+        default:
+          $note[] = pht('%s Other Test(s)', $count);
+          break;
+      }
+    }
+
+    $buildable = $diff->getBuildable();
+    if ($buildable) {
+      $full_results = '/harbormaster/unit/'.$buildable->getID().'/';
+      $note[] = phutil_tag(
+        'a',
+        array(
+          'href' => $full_results,
+        ),
+        pht('View Full Results'));
+    }
+
     $excuse = $diff->getProperty('arc:unit-excuse');
     if (strlen($excuse)) {
       $excuse = array(
@@ -104,14 +165,17 @@ final class DifferentialUnitField
         ' ',
         phutil_escape_html_newlines($excuse),
       );
+      $note[] = $excuse;
     }
 
+    $note = phutil_implode_html(" \xC2\xB7 ", $note);
+
     $status = id(new PHUIStatusListView())
       ->addItem(
         id(new PHUIStatusItemView())
           ->setIcon(PHUIStatusItemView::ICON_STAR, $icon_color)
           ->setTarget($message)
-          ->setNote($excuse));
+          ->setNote($note));
 
     return $status;
   }

src/applications/harbormaster/application/PhabricatorHarbormasterApplication.php

@@ -77,6 +77,12 @@ final class PhabricatorHarbormasterApplication extends PhabricatorApplication {
           'run/(?P<id>\d+)/' => 'HarbormasterPlanRunController',
           '(?P<id>\d+)/' => 'HarbormasterPlanViewController',
         ),
+        'unit/' => array(
+          '(?P<id>\d+)/' => 'HarbormasterUnitMessagesController',
+        ),
+        'lint/' => array(
+          '(?P<id>\d+)/' => 'HarbormasterLintMessagesController',
+        ),
       ),
     );
   }

src/applications/harbormaster/controller/HarbormasterBuildableViewController.php

@@ -25,7 +25,7 @@ final class HarbormasterBuildableViewController
       ->needBuildTargets(true)
       ->execute();
 
-    list($lint, $unit) = $this->renderLintAndUnit($builds);
+    list($lint, $unit) = $this->renderLintAndUnit($buildable, $builds);
 
     $buildable->attachBuilds($builds);
     $object = $buildable->getBuildableObject();
@@ -255,7 +255,10 @@ final class HarbormasterBuildableViewController
     return $box;
   }
 
-  private function renderLintAndUnit(array $builds) {
+  private function renderLintAndUnit(
+    HarbormasterBuildable $buildable,
+    array $builds) {
+
     $viewer = $this->getViewer();
 
     $targets = array();
@@ -272,20 +275,32 @@ final class HarbormasterBuildableViewController
     $target_phids = mpull($targets, 'getPHID');
 
     $lint_data = id(new HarbormasterBuildLintMessage())->loadAllWhere(
-      'buildTargetPHID IN (%Ls) LIMIT 25',
+      'buildTargetPHID IN (%Ls)',
       $target_phids);
 
     $unit_data = id(new HarbormasterBuildUnitMessage())->loadAllWhere(
-      'buildTargetPHID IN (%Ls) LIMIT 25',
+      'buildTargetPHID IN (%Ls)',
       $target_phids);
 
     if ($lint_data) {
       $lint_table = id(new HarbormasterLintPropertyView())
         ->setUser($viewer)
+        ->setLimit(10)
         ->setLintMessages($lint_data);
 
+      $lint_href = $this->getApplicationURI('lint/'.$buildable->getID().'/');
+
+      $lint_header = id(new PHUIHeaderView())
+        ->setHeader(pht('Lint Messages'))
+        ->addActionLink(
+          id(new PHUIButtonView())
+            ->setTag('a')
+            ->setHref($lint_href)
+            ->setIconFont('fa-list-ul')
+            ->setText('View All'));
+
       $lint = id(new PHUIObjectBoxView())
-        ->setHeaderText(pht('Lint Messages'))
+        ->setHeader($lint_header)
         ->appendChild($lint_table);
     } else {
       $lint = null;
@@ -294,10 +309,22 @@ final class HarbormasterBuildableViewController
     if ($unit_data) {
       $unit_table = id(new HarbormasterUnitPropertyView())
         ->setUser($viewer)
+        ->setLimit(25)
         ->setUnitMessages($unit_data);
 
+      $unit_href = $this->getApplicationURI('unit/'.$buildable->getID().'/');
+
+      $unit_header = id(new PHUIHeaderView())
+        ->setHeader(pht('Unit Tests'))
+        ->addActionLink(
+          id(new PHUIButtonView())
+            ->setTag('a')
+            ->setHref($unit_href)
+            ->setIconFont('fa-list-ul')
+            ->setText('View All'));
+
       $unit = id(new PHUIObjectBoxView())
-        ->setHeaderText(pht('Unit Tests'))
+        ->setHeader($unit_header)
         ->appendChild($unit_table);
     } else {
       $unit = null;

src/applications/harbormaster/controller/HarbormasterLintMessagesController.php

@@ -0,0 +1,64 @@
+<?php
+
+final class HarbormasterLintMessagesController
+  extends HarbormasterController {
+
+  public function handleRequest(AphrontRequest $request) {
+    $viewer = $this->getViewer();
+
+    $buildable = id(new HarbormasterBuildableQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($request->getURIData('id')))
+      ->needBuilds(true)
+      ->needTargets(true)
+      ->executeOne();
+    if (!$buildable) {
+      return new Aphront404Response();
+    }
+
+    $id = $buildable->getID();
+
+    $target_phids = array();
+    foreach ($buildable->getBuilds() as $build) {
+      foreach ($build->getBuildTargets() as $target) {
+        $target_phids[] = $target->getPHID();
+      }
+    }
+
+    $lint_data = array();
+    if ($target_phids) {
+      $lint_data = id(new HarbormasterBuildLintMessage())->loadAllWhere(
+        'buildTargetPHID IN (%Ls)',
+        $target_phids);
+    } else {
+      $lint_data = array();
+    }
+
+    $lint_table = id(new HarbormasterLintPropertyView())
+      ->setUser($viewer)
+      ->setLintMessages($lint_data);
+
+    $lint = id(new PHUIObjectBoxView())
+      ->setHeaderText(pht('Lint Messages'))
+      ->appendChild($lint_table);
+
+    $crumbs = $this->buildApplicationCrumbs();
+    $this->addBuildableCrumb($crumbs, $buildable);
+    $crumbs->addTextCrumb(pht('Lint'));
+
+    $title = array(
+      $buildable->getMonogram(),
+      pht('Lint'),
+    );
+
+    return $this->buildApplicationPage(
+      array(
+        $crumbs,
+        $lint,
+      ),
+      array(
+        'title' => $title,
+      ));
+  }
+
+}

src/applications/harbormaster/controller/HarbormasterUnitMessagesController.php

@@ -0,0 +1,64 @@
+<?php
+
+final class HarbormasterUnitMessagesController
+  extends HarbormasterController {
+
+  public function handleRequest(AphrontRequest $request) {
+    $viewer = $this->getViewer();
+
+    $buildable = id(new HarbormasterBuildableQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($request->getURIData('id')))
+      ->needBuilds(true)
+      ->needTargets(true)
+      ->executeOne();
+    if (!$buildable) {
+      return new Aphront404Response();
+    }
+
+    $id = $buildable->getID();
+
+    $target_phids = array();
+    foreach ($buildable->getBuilds() as $build) {
+      foreach ($build->getBuildTargets() as $target) {
+        $target_phids[] = $target->getPHID();
+      }
+    }
+
+    $unit_data = array();
+    if ($target_phids) {
+      $unit_data = id(new HarbormasterBuildUnitMessage())->loadAllWhere(
+        'buildTargetPHID IN (%Ls)',
+        $target_phids);
+    } else {
+      $unit_data = array();
+    }
+
+    $unit_table = id(new HarbormasterUnitPropertyView())
+      ->setUser($viewer)
+      ->setUnitMessages($unit_data);
+
+    $unit = id(new PHUIObjectBoxView())
+      ->setHeaderText(pht('Unit Tests'))
+      ->appendChild($unit_table);
+
+    $crumbs = $this->buildApplicationCrumbs();
+    $this->addBuildableCrumb($crumbs, $buildable);
+    $crumbs->addTextCrumb(pht('Unit Tests'));
+
+    $title = array(
+      $buildable->getMonogram(),
+      pht('Unit Tests'),
+    );
+
+    return $this->buildApplicationPage(
+      array(
+        $crumbs,
+        $unit,
+      ),
+      array(
+        'title' => $title,
+      ));
+  }
+
+}

src/applications/harbormaster/storage/build/HarbormasterBuildLintMessage.php

@@ -95,4 +95,26 @@ final class HarbormasterBuildLintMessage
     return $this;
   }
 
+  public function getSortKey() {
+    // TODO: Maybe use more numeric values after T6861.
+    $map = array(
+      ArcanistLintSeverity::SEVERITY_ERROR => 'A',
+      ArcanistLintSeverity::SEVERITY_WARNING => 'B',
+      ArcanistLintSeverity::SEVERITY_AUTOFIX => 'C',
+      ArcanistLintSeverity::SEVERITY_ADVICE => 'Y',
+      ArcanistLintSeverity::SEVERITY_DISABLED => 'Z',
+    );
+
+    $severity = idx($map, $this->getSeverity(), 'N');
+
+    $parts = array(
+      $severity,
+      $this->getPath(),
+      sprintf('%08d', $this->getLine()),
+      $this->getCode(),
+    );
+
+    return implode("\0", $parts);
+  }
+
 }

src/applications/harbormaster/storage/build/HarbormasterBuildUnitMessage.php

@@ -97,4 +97,26 @@ final class HarbormasterBuildUnitMessage
     return $this;
   }
 
+  public function getSortKey() {
+    // TODO: Maybe use more numeric values after T6861.
+    $map = array(
+      ArcanistUnitTestResult::RESULT_FAIL => 'A',
+      ArcanistUnitTestResult::RESULT_BROKEN => 'B',
+      ArcanistUnitTestResult::RESULT_UNSOUND => 'C',
+      ArcanistUnitTestResult::RESULT_PASS => 'Z',
+    );
+
+    $result = idx($map, $this->getResult(), 'N');
+
+    $parts = array(
+      $result,
+      $this->getEngine(),
+      $this->getNamespace(),
+      $this->getName(),
+      $this->getID(),
+    );
+
+    return implode("\0", $parts);
+  }
+
 }

src/applications/harbormaster/view/HarbormasterLintPropertyView.php

@@ -4,6 +4,7 @@ final class HarbormasterLintPropertyView extends AphrontView {
 
   private $pathURIMap = array();
   private $lintMessages = array();
+  private $limit;
 
   public function setPathURIMap(array $map) {
     $this->pathURIMap = $map;
@@ -16,9 +17,21 @@ final class HarbormasterLintPropertyView extends AphrontView {
     return $this;
   }
 
+  public function setLimit($limit) {
+    $this->limit = $limit;
+    return $this;
+  }
+
   public function render() {
+    $messages = $this->lintMessages;
+    $messages = msort($messages, 'getSortKey');
+
+    if ($this->limit) {
+      $messages = array_slice($messages, 0, $this->limit);
+    }
+
     $rows = array();
-    foreach ($this->lintMessages as $message) {
+    foreach ($messages as $message) {
       $path = $message->getPath();
       $line = $message->getLine();
 
@@ -40,8 +53,8 @@ final class HarbormasterLintPropertyView extends AphrontView {
       }
 
       $rows[] = array(
-        $location,
         $severity,
+        $location,
         $message->getCode(),
         $message->getName(),
       );
@@ -50,15 +63,15 @@ final class HarbormasterLintPropertyView extends AphrontView {
     $table = id(new AphrontTableView($rows))
       ->setHeaders(
         array(
-          pht('Location'),
           pht('Severity'),
+          pht('Location'),
           pht('Code'),
           pht('Message'),
         ))
       ->setColumnClasses(
         array(
-          'pri',
           null,
+          'pri',
           null,
           'wide',
         ));

src/applications/harbormaster/view/HarbormasterUnitPropertyView.php

@@ -4,6 +4,7 @@ final class HarbormasterUnitPropertyView extends AphrontView {
 
   private $pathURIMap = array();
   private $unitMessages = array();
+  private $limit;
 
   public function setPathURIMap(array $map) {
     $this->pathURIMap = $map;
@@ -16,11 +17,22 @@ final class HarbormasterUnitPropertyView extends AphrontView {
     return $this;
   }
 
+  public function setLimit($limit) {
+    $this->limit = $limit;
+    return $this;
+  }
+
   public function render() {
+    $messages = $this->unitMessages;
+    $messages = msort($messages, 'getSortKey');
+
+    if ($this->limit) {
+      $messages = array_slice($messages, 0, $this->limit);
+    }
 
     $rows = array();
     $any_duration = false;
-    foreach ($this->unitMessages as $message) {
+    foreach ($messages as $message) {
       $result = $this->renderResult($message->getResult());
 
       $duration = $message->getDuration();
@@ -48,7 +60,6 @@ final class HarbormasterUnitPropertyView extends AphrontView {
       );
     }
 
-
     $table = id(new AphrontTableView($rows))
       ->setHeaders(
         array(

src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php

@@ -1170,6 +1170,13 @@ final class PhabricatorUSEnglishTranslation
           'This call takes %s parameters, but only %s are documented.',
         ),
       ),
+
+      '%s Passed Test(s)' => '%s Passed',
+      '%s Failed Test(s)' => '%s Failed',
+      '%s Skipped Test(s)' => '%s Skipped',
+      '%s Broken Test(s)' => '%s Broken',
+      '%s Unsound Test(s)' => '%s Unsound',
+      '%s Other Test(s)' => '%s Other',
     );
   }
 

webroot/rsrc/css/application/differential/table-of-contents.css

@@ -83,3 +83,8 @@ table.aphront-table-view td.differential-toc-ftype {
   border-top: 1px solid {$thinblueborder};
   padding: 8px;
 }
+
+.differential-harbormaster-table-view {
+  margin: 4px 0;
+  border: 1px solid {$thinblueborder};
+}