diff --git a/resources/sql/autopatches/20160822.buildable.details.1.sql b/resources/sql/autopatches/20160822.buildable.details.1.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20160822.buildable.details.1.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_harbormaster.harbormaster_buildable +ADD details LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20160822.buildable.details.2.sql b/resources/sql/autopatches/20160822.buildable.details.2.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20160822.buildable.details.2.sql @@ -0,0 +1,2 @@ +UPDATE {$NAMESPACE}_harbormaster.harbormaster_buildable +SET details = '{}' WHERE details = ''; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1172,6 +1172,7 @@ 'HarbormasterCircleCIBuildableInterface' => 'applications/harbormaster/interface/HarbormasterCircleCIBuildableInterface.php', 'HarbormasterCircleCIHookController' => 'applications/harbormaster/controller/HarbormasterCircleCIHookController.php', 'HarbormasterConduitAPIMethod' => 'applications/harbormaster/conduit/HarbormasterConduitAPIMethod.php', + 'HarbormasterConstantsTestCase' => 'applications/harbormaster/__tests__/HarbormasterConstantsTestCase.php', 'HarbormasterController' => 'applications/harbormaster/controller/HarbormasterController.php', 'HarbormasterCreateArtifactConduitAPIMethod' => 'applications/harbormaster/conduit/HarbormasterCreateArtifactConduitAPIMethod.php', 'HarbormasterCreatePlansCapability' => 'applications/harbormaster/capability/HarbormasterCreatePlansCapability.php', @@ -1187,6 +1188,7 @@ 'HarbormasterLeaseWorkingCopyBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php', 'HarbormasterLintMessagesController' => 'applications/harbormaster/controller/HarbormasterLintMessagesController.php', 'HarbormasterLintPropertyView' => 'applications/harbormaster/view/HarbormasterLintPropertyView.php', + 'HarbormasterLintStatus' => 'applications/harbormaster/constants/HarbormasterLintStatus.php', 'HarbormasterManagementArchiveLogsWorkflow' => 'applications/harbormaster/management/HarbormasterManagementArchiveLogsWorkflow.php', 'HarbormasterManagementBuildWorkflow' => 'applications/harbormaster/management/HarbormasterManagementBuildWorkflow.php', 'HarbormasterManagementUpdateWorkflow' => 'applications/harbormaster/management/HarbormasterManagementUpdateWorkflow.php', @@ -5761,6 +5763,7 @@ 'HarbormasterCircleCIBuildStepImplementation' => 'HarbormasterBuildStepImplementation', 'HarbormasterCircleCIHookController' => 'HarbormasterController', 'HarbormasterConduitAPIMethod' => 'ConduitAPIMethod', + 'HarbormasterConstantsTestCase' => 'PhabricatorTestCase', 'HarbormasterController' => 'PhabricatorController', 'HarbormasterCreateArtifactConduitAPIMethod' => 'HarbormasterConduitAPIMethod', 'HarbormasterCreatePlansCapability' => 'PhabricatorPolicyCapability', @@ -5776,6 +5779,7 @@ 'HarbormasterLeaseWorkingCopyBuildStepImplementation' => 'HarbormasterBuildStepImplementation', 'HarbormasterLintMessagesController' => 'HarbormasterController', 'HarbormasterLintPropertyView' => 'AphrontView', + 'HarbormasterLintStatus' => 'Phobject', 'HarbormasterManagementArchiveLogsWorkflow' => 'HarbormasterManagementWorkflow', 'HarbormasterManagementBuildWorkflow' => 'HarbormasterManagementWorkflow', 'HarbormasterManagementUpdateWorkflow' => 'HarbormasterManagementWorkflow', diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php --- a/src/applications/differential/controller/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/DifferentialChangesetViewController.php @@ -403,37 +403,8 @@ } private function loadCoverage(DifferentialChangeset $changeset) { - $target_phids = $changeset->getDiff()->getBuildTargetPHIDs(); - if (!$target_phids) { - return null; - } - - $unit = id(new HarbormasterBuildUnitMessage())->loadAllWhere( - 'buildTargetPHID IN (%Ls)', - $target_phids); - - if (!$unit) { - return null; - } - - $coverage = array(); - foreach ($unit as $message) { - $test_coverage = $message->getProperty('coverage'); - if ($test_coverage === null) { - continue; - } - $coverage_data = idx($test_coverage, $changeset->getFileName()); - if (!strlen($coverage_data)) { - continue; - } - $coverage[] = $coverage_data; - } - - if (!$coverage) { - return null; - } - - return ArcanistUnitTestResult::mergeCoverage($coverage); + $coverage_map = $changeset->getDiff()->loadCoverageMap(); + return idx($coverage_map, $changeset->getFileName()); } } diff --git a/src/applications/differential/controller/DifferentialController.php b/src/applications/differential/controller/DifferentialController.php --- a/src/applications/differential/controller/DifferentialController.php +++ b/src/applications/differential/controller/DifferentialController.php @@ -108,8 +108,7 @@ } } - - protected function loadHarbormasterData(array $diffs) { + protected function loadHarbormasterBuilds(array $diffs) { $viewer = $this->getViewer(); $diffs = mpull($diffs, null, 'getPHID'); @@ -127,12 +126,18 @@ $diff->attachBuildable(idx($buildables, $phid)); } + } + + protected function loadHarbormasterData(array $diffs) { + $diffs = mpull($diffs, null, 'getPHID'); + $target_map = array(); foreach ($diffs as $phid => $diff) { $target_map[$phid] = $diff->getBuildTargetPHIDs(); } $all_target_phids = array_mergev($target_map); + // T10635: most of this information is already aggregated in BuildTarget. if ($all_target_phids) { $unit_messages = id(new HarbormasterBuildUnitMessage())->loadAllWhere( 'buildTargetPHID IN (%Ls)', diff --git a/src/applications/differential/controller/DifferentialDiffViewController.php b/src/applications/differential/controller/DifferentialDiffViewController.php --- a/src/applications/differential/controller/DifferentialDiffViewController.php +++ b/src/applications/differential/controller/DifferentialDiffViewController.php @@ -113,7 +113,7 @@ $table_of_contents = $this->buildTableOfContents( $changesets, $changesets, - $diff->loadCoverageMap($viewer)); + $diff->loadCoverageMap()); $refs = array(); foreach ($changesets as $changeset) { diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -239,6 +239,8 @@ ->setErrors($revision_warnings); } + $this->loadHarbormasterBuilds($diffs); + $detail_diffs = array_select_keys( $diffs, array($diff_vs, $target->getID())); @@ -322,7 +324,7 @@ $toc_view = $this->buildTableOfContents( $changesets, $visible_changesets, - $target->loadCoverageMap($viewer)); + $target->loadCoverageMap()); $tab_group = id(new PHUITabGroupView()) ->addTab( diff --git a/src/applications/differential/customfield/DifferentialLintField.php b/src/applications/differential/customfield/DifferentialLintField.php --- a/src/applications/differential/customfield/DifferentialLintField.php +++ b/src/applications/differential/customfield/DifferentialLintField.php @@ -84,6 +84,7 @@ DifferentialDiff $diff, array $messages) { + $status = $diff->getSummaryLintStatus(); $colors = array( DifferentialLintStatus::LINT_NONE => 'grey', DifferentialLintStatus::LINT_OKAY => 'green', @@ -92,9 +93,10 @@ DifferentialLintStatus::LINT_SKIP => 'blue', DifferentialLintStatus::LINT_AUTO_SKIP => 'blue', ); - $icon_color = idx($colors, $diff->getLintStatus(), 'grey'); + $icon_color = idx($colors, $status, 'grey'); - $message = DifferentialRevisionUpdateHistoryView::getDiffLintMessage($diff); + $message = DifferentialRevisionUpdateHistoryView::getDiffLintMessage( + $status); $excuse = $diff->getProperty('arc:lint-excuse'); if (strlen($excuse)) { diff --git a/src/applications/differential/customfield/DifferentialUnitField.php b/src/applications/differential/customfield/DifferentialUnitField.php --- a/src/applications/differential/customfield/DifferentialUnitField.php +++ b/src/applications/differential/customfield/DifferentialUnitField.php @@ -32,7 +32,8 @@ } public function getWarningsForDetailView() { - $status = $this->getObject()->getActiveDiff()->getUnitStatus(); + $active_diff = $this->getObject()->getActiveDiff(); + $status = $active_diff->getSummaryUnitStatus(); $warnings = array(); if ($status < DifferentialUnitStatus::UNIT_WARN) { @@ -50,6 +51,7 @@ } public function renderDiffPropertyViewValue(DifferentialDiff $diff) { + $buildable = $diff->getBuildable(); $colors = array( DifferentialUnitStatus::UNIT_NONE => 'grey', @@ -59,19 +61,66 @@ DifferentialUnitStatus::UNIT_SKIP => 'blue', DifferentialUnitStatus::UNIT_AUTO_SKIP => 'blue', ); - $icon_color = idx($colors, $diff->getUnitStatus(), 'grey'); - - $message = DifferentialRevisionUpdateHistoryView::getDiffUnitMessage($diff); + $view = id(new PHUIStatusListView()); + + $builds_unit_status = null; + if ($buildable) { + $builds_unit_status = $buildable->getDetail( + HarbormasterBuildable::DETAIL_UNIT_STATUS); + $builds_unit_status = HarbormasterUnitStatus::getDifferentialUnitStatus( + $builds_unit_status); + } - $status = id(new PHUIStatusListView()) - ->addItem( + if ($builds_unit_status) { + $icon_color = idx($colors, $builds_unit_status, 'grey'); + $message = DifferentialRevisionUpdateHistoryView::getDiffUnitMessage( + $builds_unit_status); + + $unit_count = $buildable->getDetail( + HarbormasterBuildable::DETAIL_UNIT_COUNTS); + $counts = array(); + foreach ($unit_count as $status => $count) { + $counts[] = HarbormasterUnitStatus::getUnitStatusCountLabel( + $status, + $count); + } + $counts = implode(" \xC2\xB7 ", $counts); + + $view->addItem( id(new PHUIStatusItemView()) ->setIcon(PHUIStatusItemView::ICON_STAR, $icon_color) - ->setTarget($message)); - - return $status; - } + ->setTarget($message) + ->setNote($counts)); + } + $arcanist_tests_status = $diff->getUnitStatus(); + if ($arcanist_tests_status == DifferentialUnitStatus::UNIT_SKIP) { + $show_manual = true; + } else { + $show_manual = !$builds_unit_status; + } + if ($show_manual) { + $icon_color = idx($colors, $arcanist_tests_status, 'grey'); + $message = DifferentialRevisionUpdateHistoryView::getDiffUnitMessage( + $arcanist_tests_status); + + $excuse = null; + if ($diff->hasDiffProperty('arc:unit-excuse')) { + $excuse = array( + phutil_tag('strong', array(), pht('Excuse:')), + ' ', + phutil_escape_html_newlines($diff->getProperty('arc:unit-excuse')), + ); + } + + $view->addItem( + id(new PHUIStatusItemView()) + ->setIcon(PHUIStatusItemView::ICON_STAR, $icon_color) + ->setTarget($message) + ->setNote($excuse)); + } + return $view; + } } diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php --- a/src/applications/differential/storage/DifferentialDiff.php +++ b/src/applications/differential/storage/DifferentialDiff.php @@ -360,28 +360,27 @@ } public function getBuildTargetPHIDs() { + $targets = $this->getBuildTargets(); + return mpull($targets, 'getPHID'); + } + + public function getBuildTargets() { $buildable = $this->getBuildable(); if (!$buildable) { return array(); } - $target_phids = array(); - foreach ($buildable->getBuilds() as $build) { - foreach ($build->getBuildTargets() as $target) { - $target_phids[] = $target->getPHID(); - } - } - - return $target_phids; + $targets = mpull($buildable->getBuilds(), 'getBuildTargets'); + return array_mergev($targets); } - public function loadCoverageMap(PhabricatorUser $viewer) { + public function loadCoverageMap() { + // T10635: most of this information is already aggregated in BuildTarget. $target_phids = $this->getBuildTargetPHIDs(); if (!$target_phids) { return array(); } - $unit = id(new HarbormasterBuildUnitMessage())->loadAllWhere( 'buildTargetPHID IN (%Ls)', $target_phids); @@ -412,11 +411,59 @@ return $this; } - public function getUnitMessages() { + // T10635: practically no callsite of this method should want ALL messages. return $this->assertAttached($this->unitMessages); } + public function getSummaryLintStatus() { + // For now, I'll hack this with a clever summation of + // $builds_lint_status and $diff->getLintStatus. + // Next, actually add a "meta" message about lint total status. + // That will also give us a path twords having a message of "10,000 units + // passed" (For T10635). + + // With modern arc (post Harbormaster), any lint event (message) that + // contributes to $this->lintStatus will also exist in the Buildable. + // So only these cases have information in the diff that isn't reflected + // in the Buildable: LINT_NONE, LINT_OKAY, LINT_SKIP, LINT_AUTOSKIP. + // Of these, NONE, OKAY and AUTOSKIP are not interesting if there are no + // lint messages in the Buildable. + + $buildable = $this->getBuildable(); + $buildable_lint = $buildable ? + $buildable->getDetail(HarbormasterBuildable::DETAIL_LINT_STATUS): + null; + + $buildable_lint = HarbormasterLintStatus::getDifferentialLintStatus( + $buildable_lint); + + $arc_status = $this->getLintStatus(); + + if ($arc_status == DifferentialLintStatus::LINT_SKIP || !$buildable_lint) { + return $arc_status; + } + + return $buildable_lint; + } + + public function getSummaryUnitStatus() { + $buildable = $this->getBuildable(); + $buildable_unit = $buildable ? + $buildable->getDetail(HarbormasterBuildable::DETAIL_UNIT_STATUS) : + null; + + $buildable_unit = HarbormasterUnitStatus::getDifferentialUnitStatus( + $buildable_unit); + + $arc_status = $this->getUnitStatus(); + if ($arc_status == DifferentialUnitStatus::UNIT_SKIP || !$buildable_unit) { + return $arc_status; + } + + return $buildable_unit; + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php b/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php --- a/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php +++ b/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php @@ -139,21 +139,24 @@ } if ($diff) { - $lint = self::renderDiffLintStar($row['obj']); + $lint_status = $diff->getSummaryLintStatus(); + $unit_status = $diff->getSummaryUnitStatus(); + + $lint = self::renderDiffLintStar($lint_status); $lint = phutil_tag( 'div', array( 'class' => 'lintunit-star', - 'title' => self::getDiffLintMessage($diff), + 'title' => self::getDiffLintMessage($lint_status), ), $lint); - $unit = self::renderDiffUnitStar($row['obj']); + $unit = self::renderDiffUnitStar($unit_status); $unit = phutil_tag( 'div', array( 'class' => 'lintunit-star', - 'title' => self::getDiffUnitMessage($diff), + 'title' => self::getDiffUnitMessage($unit_status), ), $unit); @@ -312,7 +315,7 @@ const STAR_FAIL = 'fail'; const STAR_SKIP = 'skip'; - public static function renderDiffLintStar(DifferentialDiff $diff) { + private static function renderDiffLintStar($status) { static $map = array( DifferentialLintStatus::LINT_NONE => self::STAR_NONE, DifferentialLintStatus::LINT_OKAY => self::STAR_OKAY, @@ -322,12 +325,12 @@ DifferentialLintStatus::LINT_AUTO_SKIP => self::STAR_SKIP, ); - $star = idx($map, $diff->getLintStatus(), self::STAR_FAIL); + $star = idx($map, $status, self::STAR_FAIL); return self::renderDiffStar($star); } - public static function renderDiffUnitStar(DifferentialDiff $diff) { + private static function renderDiffUnitStar($unit_status) { static $map = array( DifferentialUnitStatus::UNIT_NONE => self::STAR_NONE, DifferentialUnitStatus::UNIT_OKAY => self::STAR_OKAY, @@ -337,13 +340,14 @@ DifferentialUnitStatus::UNIT_AUTO_SKIP => self::STAR_SKIP, ); - $star = idx($map, $diff->getUnitStatus(), self::STAR_FAIL); + + $star = idx($map, $unit_status, self::STAR_FAIL); return self::renderDiffStar($star); } - public static function getDiffLintMessage(DifferentialDiff $diff) { - switch ($diff->getLintStatus()) { + public static function getDiffLintMessage($status) { + switch ($status) { case DifferentialLintStatus::LINT_NONE: return pht('No Linters Available'); case DifferentialLintStatus::LINT_OKAY: @@ -360,8 +364,8 @@ return pht('Unknown'); } - public static function getDiffUnitMessage(DifferentialDiff $diff) { - switch ($diff->getUnitStatus()) { + public static function getDiffUnitMessage($status) { + switch ($status) { case DifferentialUnitStatus::UNIT_NONE: return pht('No Unit Test Coverage'); case DifferentialUnitStatus::UNIT_OKAY: diff --git a/src/applications/harbormaster/__tests__/HarbormasterConstantsTestCase.php b/src/applications/harbormaster/__tests__/HarbormasterConstantsTestCase.php new file mode 100644 --- /dev/null +++ b/src/applications/harbormaster/__tests__/HarbormasterConstantsTestCase.php @@ -0,0 +1,95 @@ + array(), + 'expected' => null, + ); + + $test_sets['simple'] = array( + 'input' => array( + ArcanistLintSeverity::SEVERITY_ADVICE, + ArcanistLintSeverity::SEVERITY_WARNING, + ArcanistLintSeverity::SEVERITY_ERROR, + ), + 'expected' => ArcanistLintSeverity::SEVERITY_ERROR, + ); + + $test_sets['reversed simple'] = array( + 'input' => array( + ArcanistLintSeverity::SEVERITY_ERROR, + ArcanistLintSeverity::SEVERITY_WARNING, + ArcanistLintSeverity::SEVERITY_ADVICE, + ), + 'expected' => ArcanistLintSeverity::SEVERITY_ERROR, + ); + + $test_sets['all advice'] = array( + 'input' => array( + ArcanistLintSeverity::SEVERITY_ADVICE, + ArcanistLintSeverity::SEVERITY_ADVICE, + ArcanistLintSeverity::SEVERITY_ADVICE, + ), + 'expected' => ArcanistLintSeverity::SEVERITY_ADVICE, + ); + + foreach ($test_sets as $key => $set) { + $input = $set['input']; + $expected = $set['expected']; + + $result = HarbormasterLintStatus::summarizeStatuses($input); + + $this->assertEqual($expected, $result, 'test case: '.$key); + } + } + + public function testSummarizeUnitStatuses() { + $test_sets = array(); + + $test_sets['empty'] = array( + 'input' => array(), + 'expected' => null, + ); + + $test_sets['simple'] = array( + 'input' => array( + ArcanistUnitTestResult::RESULT_PASS, + ArcanistUnitTestResult::RESULT_SKIP, + ArcanistUnitTestResult::RESULT_FAIL, + ), + 'expected' => ArcanistUnitTestResult::RESULT_FAIL, + ); + + $test_sets['reversed simple'] = array( + 'input' => array( + ArcanistUnitTestResult::RESULT_FAIL, + ArcanistUnitTestResult::RESULT_SKIP, + ArcanistUnitTestResult::RESULT_PASS, + ), + 'expected' => ArcanistUnitTestResult::RESULT_FAIL, + ); + + $test_sets['all pass'] = array( + 'input' => array( + ArcanistUnitTestResult::RESULT_PASS, + ArcanistUnitTestResult::RESULT_PASS, + ArcanistUnitTestResult::RESULT_PASS, + ), + 'expected' => ArcanistUnitTestResult::RESULT_PASS, + ); + + foreach ($test_sets as $key => $set) { + $input = $set['input']; + $expected = $set['expected']; + + $result = HarbormasterUnitStatus::summarizeStatuses($input); + + $this->assertEqual($expected, $result, 'test case: '.$key); + } + } + +} diff --git a/src/applications/harbormaster/constants/HarbormasterLintStatus.php b/src/applications/harbormaster/constants/HarbormasterLintStatus.php new file mode 100644 --- /dev/null +++ b/src/applications/harbormaster/constants/HarbormasterLintStatus.php @@ -0,0 +1,68 @@ + array( + 'differential_value' => DifferentialLintStatus::LINT_FAIL, + 'label' => pht('Error'), + 'sort' => 'A', + ), + ArcanistLintSeverity::SEVERITY_WARNING => array( + 'differential_value' => DifferentialLintStatus::LINT_WARN, + 'label' => pht('Warning'), + 'sort' => 'B', + ), + ArcanistLintSeverity::SEVERITY_ADVICE => array( + 'differential_value' => DifferentialLintStatus::LINT_OKAY, + 'label' => pht('Advice'), + 'sort' => 'Y', + ), + ); + } + +} diff --git a/src/applications/harbormaster/constants/HarbormasterUnitStatus.php b/src/applications/harbormaster/constants/HarbormasterUnitStatus.php --- a/src/applications/harbormaster/constants/HarbormasterUnitStatus.php +++ b/src/applications/harbormaster/constants/HarbormasterUnitStatus.php @@ -27,6 +27,30 @@ return idx($map, 'sort', $default); } + public static function summarizeStatuses(array $statuses) { + if (!$statuses) { + return null; + } + $map = self::getUnitStatusMap(); + $map = ipull($map, 'sort'); + $default = 'Z'; + $summary = head($statuses); + $w_index = idx($map, $summary, $default); + foreach ($statuses as $status) { + $r = idx($map, $status, $default); + if ($r < $w_index) { + $summary = $status; + $w_index = $r; + } + } + return $summary; + } + + public static function getDifferentialUnitStatus($status) { + $map = self::getUnitStatusDictionary($status); + return idx($map, 'differantial_result', $status); + } + private static function getUnitStatusDictionary($status) { $map = self::getUnitStatusMap(); $default = array(); @@ -55,30 +79,35 @@ private static function getUnitStatusMap() { return array( ArcanistUnitTestResult::RESULT_FAIL => array( + 'differantial_result' => DifferentialUnitStatus::UNIT_FAIL, 'label' => pht('Failed'), 'icon' => 'fa-times', 'color' => 'red', 'sort' => 'A', ), ArcanistUnitTestResult::RESULT_BROKEN => array( + 'differantial_result' => DifferentialUnitStatus::UNIT_WARN, 'label' => pht('Broken'), 'icon' => 'fa-bomb', 'color' => 'indigo', 'sort' => 'B', ), ArcanistUnitTestResult::RESULT_UNSOUND => array( + 'differantial_result' => DifferentialUnitStatus::UNIT_WARN, 'label' => pht('Unsound'), 'icon' => 'fa-exclamation-triangle', 'color' => 'yellow', 'sort' => 'C', ), ArcanistUnitTestResult::RESULT_PASS => array( + 'differantial_result' => DifferentialUnitStatus::UNIT_OKAY, 'label' => pht('Passed'), 'icon' => 'fa-check', 'color' => 'green', 'sort' => 'D', ), ArcanistUnitTestResult::RESULT_SKIP => array( + 'differantial_result' => DifferentialUnitStatus::UNIT_SKIP, 'label' => pht('Skipped'), 'icon' => 'fa-fast-forward', 'color' => 'blue', diff --git a/src/applications/harbormaster/engine/HarbormasterBuildEngine.php b/src/applications/harbormaster/engine/HarbormasterBuildEngine.php --- a/src/applications/harbormaster/engine/HarbormasterBuildEngine.php +++ b/src/applications/harbormaster/engine/HarbormasterBuildEngine.php @@ -12,6 +12,8 @@ private $artifactReleaseQueue = array(); private $forceBuildableUpdate; + private $consumedNewMessages = false; + public function setForceBuildableUpdate($force_buildable_update) { $this->forceBuildableUpdate = $force_buildable_update; return $this; @@ -91,7 +93,10 @@ // If the build changed status, we might need to update the overall status // on the buildable. $new_status = $build->getBuildStatus(); - if ($new_status != $old_status || $this->shouldForceBuildableUpdate()) { + if ($new_status != $old_status || + $this->consumedNewMessages || + $this->shouldForceBuildableUpdate()) { + $this->updateBuildable($build->getBuildable()); } @@ -375,7 +380,6 @@ $waiting_targets[$target->getPHID()] = $target; } } - if (!$waiting_targets) { return; } @@ -386,6 +390,7 @@ ->withConsumed(false) ->execute(); + $updated_targets = array(); foreach ($messages as $message) { $target = $waiting_targets[$message->getBuildTargetPHID()]; @@ -403,21 +408,22 @@ } if ($new_status !== null) { - $message->setIsConsumed(true); - $message->save(); - $target->setTargetStatus($new_status); + if ($target->isComplete()) { $target->setDateCompleted(PhabricatorTime::getNow()); } $target->save(); } + + $this->consumedNewMessages = true; + $message->setIsConsumed(true); + $message->save(); } } - /** * Update the overall status of the buildable this build is attached to. * @@ -438,8 +444,12 @@ ->setViewer($viewer) ->withIDs(array($buildable->getID())) ->needBuilds(true) + ->needTargets(true) ->executeOne(); + // This doesn't effect the `should_publish` state. + $this->updateLintAndUnitStatus($buildable); + $all_pass = true; $any_fail = false; foreach ($buildable->getBuilds() as $build) { @@ -535,6 +545,75 @@ array($template)); } + /** + * Update lint and unit aggregated information. + * Invoked in the context of the Buildable lock. + */ + private function updateLintAndUnitStatus(HarbormasterBuildable $buildable) { + $targets = array_mergev(mpull($buildable->getBuilds(), 'getBuildTargets')); + $target_phids = mpull($targets, 'getPHID'); + + $buildable_lints = id(new HarbormasterBuildLintMessage())->loadAllWhere( + 'buildTargetPHID IN (%Ls)', + $target_phids); + + $buildable_units = id(new HarbormasterBuildUnitMessage())->loadAllWhere( + 'buildTargetPHID IN (%Ls)', + $target_phids); + + if ($buildable_lints) { + $severities = mpull($buildable_lints, 'getSeverity'); + $lint_severity = HarbormasterLintStatus::summarizeStatuses($severities); + } else { + $lint_severity = null; + } + + if ($buildable_units) { + $coverage_map = array(); + foreach ($buildable_units as $unit) { + $coverage = $unit->getProperty('coverage', array()); + foreach ($coverage as $path => $coverage_data) { + $coverage_map[$path][] = $coverage_data; + } + } + + $groups = mgroup($buildable_units, 'getResult'); + $unit_counts = array(); + foreach ($groups as $status => $group) { + $unit_counts[$status] = count($group); + } + + $statuses = array_keys($unit_counts); + $unit_result = HarbormasterUnitStatus::summarizeStatuses($statuses); + + foreach ($coverage_map as $path => $items) { + $coverage_map[$path] = ArcanistUnitTestResult::mergeCoverage($items); + } + } else { + $unit_result = null; + $coverage_map = null; + $unit_counts = null; + } + + $buildable->setDetail( + HarbormasterBuildable::DETAIL_LINT_STATUS, + $lint_severity); + + $buildable->setDetail( + HarbormasterBuildable::DETAIL_UNIT_STATUS, + $unit_result); + + $buildable->setDetail( + HarbormasterBuildable::DETAIL_UNIT_COUNTS, + $unit_counts); + + $buildable->setDetail( + HarbormasterBuildable::DETAIL_COVERAGE_MAP, + $coverage_map); + + $buildable->save(); + } + private function releaseAllArtifacts(HarbormasterBuild $build) { $targets = id(new HarbormasterBuildTargetQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) diff --git a/src/applications/harbormaster/storage/HarbormasterBuildable.php b/src/applications/harbormaster/storage/HarbormasterBuildable.php --- a/src/applications/harbormaster/storage/HarbormasterBuildable.php +++ b/src/applications/harbormaster/storage/HarbormasterBuildable.php @@ -9,6 +9,7 @@ protected $buildablePHID; protected $containerPHID; protected $buildableStatus; + protected $details; protected $isManualBuildable; private $buildableObject = self::ATTACHABLE; @@ -19,6 +20,11 @@ const STATUS_PASSED = 'passed'; const STATUS_FAILED = 'failed'; + const DETAIL_LINT_STATUS = 'lint:status'; + const DETAIL_UNIT_STATUS = 'unit:status'; + const DETAIL_UNIT_COUNTS = 'unit:counts'; + const DETAIL_COVERAGE_MAP = 'unit:coverage'; + public static function getBuildableStatusName($status) { $map = self::getBuildStatusMap(); return idx($map, $status, pht('Unknown ("%s")', $status)); @@ -197,6 +203,9 @@ protected function getConfiguration() { return array( self::CONFIG_AUX_PHID => true, + self::CONFIG_SERIALIZATION => array( + 'details' => self::SERIALIZATION_JSON, + ), self::CONFIG_COLUMN_SCHEMA => array( 'containerPHID' => 'phid?', 'buildableStatus' => 'text32', @@ -249,6 +258,14 @@ return $this->assertAttached($this->builds); } + public function getDetail($key, $default = null) { + return idx($this->details, $key, $default); + } + + public function setDetail($key, $value) { + $this->details[$key] = $value; + return $this; + } /* -( PhabricatorApplicationTransactionInterface )------------------------- */ diff --git a/src/applications/harbormaster/view/HarbormasterUnitPropertyView.php b/src/applications/harbormaster/view/HarbormasterUnitPropertyView.php --- a/src/applications/harbormaster/view/HarbormasterUnitPropertyView.php +++ b/src/applications/harbormaster/view/HarbormasterUnitPropertyView.php @@ -103,6 +103,7 @@ if ($full_uri && (count($messages) > $limit)) { $counts = array(); + // T10635: most of this information is already aggregated in BuildTarget. $groups = mgroup($messages, 'getResult'); foreach ($groups as $status => $group) { $counts[] = HarbormasterUnitStatus::getUnitStatusCountLabel( diff --git a/src/applications/harbormaster/view/HarbormasterUnitSummaryView.php b/src/applications/harbormaster/view/HarbormasterUnitSummaryView.php --- a/src/applications/harbormaster/view/HarbormasterUnitSummaryView.php +++ b/src/applications/harbormaster/view/HarbormasterUnitSummaryView.php @@ -40,6 +40,7 @@ $id = $buildable->getID(); $full_uri = "/harbormaster/unit/{$id}/"; + // T10635: most of this information is already aggregated in BuildTarget. $messages = msort($messages, 'getSortKey'); $head_unit = head($messages); if ($head_unit) {