From 60a529b9d1926a3e8198eeffa3466c8b17ca7861 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 10 Aug 2015 14:16:25 -0700 Subject: [PATCH] Remove some obsolete lint and unit support Summary: Ref T8096. Long ago, support for "postponed" lint and unit tests got hacked in. `arc` would publish a bunch of ghost results, and then something else would fill the results in later. This was always a hack. It is not nearly as powerful or flexible as having a real build system, and is obsolete with Harbormaster, which supports these operations in a more reasonable and straightforward way. This was used (only? almost only?) at Facebook. - Remove `differential.finishpostponedlinters`. This only served to update postponed linters. - Remove lint magic in `differential.setdiffproperty`. This magic only made sense in the context of postponed linters. - Remove `differential.updateunitresults`. The only made sense for postponed unit tests. And one minor change: when a diff contains >100 affected files, we hide the content by default, but show content for files with inline comments. Previously, we'd do this for lint inlines, too. I don't tink this is too useful, and it's much simpler to just remove it. We could add it back at some point, but I think large changes often trigger a lot of lint and no one actually cares. The behavior for actual human inlines is retained. Test Plan: `grep` Reviewers: chad Reviewed By: chad Maniphest Tasks: T8096 Differential Revision: https://secure.phabricator.com/D13848 --- src/__phutil_library_map__.php | 4 - ...FinishPostponedLintersConduitAPIMethod.php | 118 -------------- ...rentialSetDiffPropertyConduitAPIMethod.php | 60 ------- ...ntialUpdateUnitResultsConduitAPIMethod.php | 154 ------------------ .../DifferentialRevisionViewController.php | 10 -- 5 files changed, 346 deletions(-) delete mode 100644 src/applications/differential/conduit/DifferentialFinishPostponedLintersConduitAPIMethod.php delete mode 100644 src/applications/differential/conduit/DifferentialUpdateUnitResultsConduitAPIMethod.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index acda1020a9..633efef26c 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -378,7 +378,6 @@ phutil_register_library_map(array( 'DifferentialFieldParseException' => 'applications/differential/exception/DifferentialFieldParseException.php', 'DifferentialFieldValidationException' => 'applications/differential/exception/DifferentialFieldValidationException.php', 'DifferentialFindConduitAPIMethod' => 'applications/differential/conduit/DifferentialFindConduitAPIMethod.php', - 'DifferentialFinishPostponedLintersConduitAPIMethod' => 'applications/differential/conduit/DifferentialFinishPostponedLintersConduitAPIMethod.php', 'DifferentialGetAllDiffsConduitAPIMethod' => 'applications/differential/conduit/DifferentialGetAllDiffsConduitAPIMethod.php', 'DifferentialGetCommitMessageConduitAPIMethod' => 'applications/differential/conduit/DifferentialGetCommitMessageConduitAPIMethod.php', 'DifferentialGetCommitPathsConduitAPIMethod' => 'applications/differential/conduit/DifferentialGetCommitPathsConduitAPIMethod.php', @@ -495,7 +494,6 @@ phutil_register_library_map(array( 'DifferentialUnitStatus' => 'applications/differential/constants/DifferentialUnitStatus.php', 'DifferentialUnitTestResult' => 'applications/differential/constants/DifferentialUnitTestResult.php', 'DifferentialUpdateRevisionConduitAPIMethod' => 'applications/differential/conduit/DifferentialUpdateRevisionConduitAPIMethod.php', - 'DifferentialUpdateUnitResultsConduitAPIMethod' => 'applications/differential/conduit/DifferentialUpdateUnitResultsConduitAPIMethod.php', 'DifferentialViewPolicyField' => 'applications/differential/customfield/DifferentialViewPolicyField.php', 'DiffusionAuditorDatasource' => 'applications/diffusion/typeahead/DiffusionAuditorDatasource.php', 'DiffusionAuditorsAddAuditorsHeraldAction' => 'applications/diffusion/herald/DiffusionAuditorsAddAuditorsHeraldAction.php', @@ -3999,7 +3997,6 @@ phutil_register_library_map(array( 'DifferentialFieldParseException' => 'Exception', 'DifferentialFieldValidationException' => 'Exception', 'DifferentialFindConduitAPIMethod' => 'DifferentialConduitAPIMethod', - 'DifferentialFinishPostponedLintersConduitAPIMethod' => 'DifferentialConduitAPIMethod', 'DifferentialGetAllDiffsConduitAPIMethod' => 'DifferentialConduitAPIMethod', 'DifferentialGetCommitMessageConduitAPIMethod' => 'DifferentialConduitAPIMethod', 'DifferentialGetCommitPathsConduitAPIMethod' => 'DifferentialConduitAPIMethod', @@ -4136,7 +4133,6 @@ phutil_register_library_map(array( 'DifferentialUnitStatus' => 'Phobject', 'DifferentialUnitTestResult' => 'Phobject', 'DifferentialUpdateRevisionConduitAPIMethod' => 'DifferentialConduitAPIMethod', - 'DifferentialUpdateUnitResultsConduitAPIMethod' => 'DifferentialConduitAPIMethod', 'DifferentialViewPolicyField' => 'DifferentialCoreCustomField', 'DiffusionAuditorDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'DiffusionAuditorsAddAuditorsHeraldAction' => 'DiffusionAuditorsHeraldAction', diff --git a/src/applications/differential/conduit/DifferentialFinishPostponedLintersConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialFinishPostponedLintersConduitAPIMethod.php deleted file mode 100644 index f53340f06b..0000000000 --- a/src/applications/differential/conduit/DifferentialFinishPostponedLintersConduitAPIMethod.php +++ /dev/null @@ -1,118 +0,0 @@ - 'required diffID', - 'linters' => 'required dict', - ); - } - - protected function defineReturnType() { - return 'void'; - } - - protected function defineErrorTypes() { - return array( - 'ERR-BAD-DIFF' => pht('Bad diff ID.'), - 'ERR-BAD-LINTER' => pht('No postponed linter by the given name.'), - 'ERR-NO-LINT' => pht('No postponed lint field available in diff.'), - ); - } - - protected function execute(ConduitAPIRequest $request) { - $diff_id = $request->getValue('diffID'); - $linter_map = $request->getValue('linters'); - - $diff = id(new DifferentialDiffQuery()) - ->setViewer($request->getUser()) - ->withIDs(array($diff_id)) - ->executeOne(); - if (!$diff) { - throw new ConduitException('ERR-BAD-DIFF'); - } - - // Extract the finished linters and messages from the linter map. - $finished_linters = array_keys($linter_map); - $new_messages = array(); - foreach ($linter_map as $linter => $messages) { - $new_messages = array_merge($new_messages, $messages); - } - - // Load the postponed linters attached to this diff. - $postponed_linters_property = id( - new DifferentialDiffProperty())->loadOneWhere( - 'diffID = %d AND name = %s', - $diff_id, - 'arc:lint-postponed'); - if ($postponed_linters_property) { - $postponed_linters = $postponed_linters_property->getData(); - } else { - $postponed_linters = array(); - } - - foreach ($finished_linters as $linter) { - if (!in_array($linter, $postponed_linters)) { - throw new ConduitException('ERR-BAD-LINTER'); - } - } - - foreach ($postponed_linters as $idx => $linter) { - if (in_array($linter, $finished_linters)) { - unset($postponed_linters[$idx]); - } - } - - // Load the lint messages currenty attached to the diff. If this - // diff property doesn't exist, create it. - $messages_property = id(new DifferentialDiffProperty())->loadOneWhere( - 'diffID = %d AND name = %s', - $diff_id, - 'arc:lint'); - if ($messages_property) { - $messages = $messages_property->getData(); - } else { - $messages = array(); - } - - // Add new lint messages, removing duplicates. - foreach ($new_messages as $new_message) { - if (!in_array($new_message, $messages)) { - $messages[] = $new_message; - } - } - - // Use setdiffproperty to update the postponed linters and messages, - // as these will also update the lint status correctly. - $call = new ConduitCall( - 'differential.setdiffproperty', - array( - 'diff_id' => $diff_id, - 'name' => 'arc:lint', - 'data' => json_encode($messages), - )); - $call->setUser($request->getUser()); - $call->execute(); - $call = new ConduitCall( - 'differential.setdiffproperty', - array( - 'diff_id' => $diff_id, - 'name' => 'arc:lint-postponed', - 'data' => json_encode($postponed_linters), - )); - $call->setUser($request->getUser()); - $call->execute(); - } -} diff --git a/src/applications/differential/conduit/DifferentialSetDiffPropertyConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialSetDiffPropertyConduitAPIMethod.php index f9db11e3fb..12264ca598 100644 --- a/src/applications/differential/conduit/DifferentialSetDiffPropertyConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialSetDiffPropertyConduitAPIMethod.php @@ -29,72 +29,12 @@ final class DifferentialSetDiffPropertyConduitAPIMethod ); } - private static function updateLintStatus($diff_id) { - - $diff = id(new DifferentialDiff())->load($diff_id); - if (!$diff) { - throw new ConduitException('ERR_NOT_FOUND'); - } - - // Load the postponed linters attached to this diff. - $postponed_linters_property = id( - new DifferentialDiffProperty())->loadOneWhere( - 'diffID = %d AND name = %s', - $diff_id, - 'arc:lint-postponed'); - if ($postponed_linters_property) { - $postponed_linters = $postponed_linters_property->getData(); - } else { - $postponed_linters = array(); - } - - // Load the lint messages currenty attached to the diff - $messages_property = id(new DifferentialDiffProperty())->loadOneWhere( - 'diffID = %d AND name = %s', - $diff_id, - 'arc:lint'); - if ($messages_property) { - $results = $messages_property->getData(); - } else { - $results = array(); - } - - $has_error = false; - $has_warning = false; - foreach ($results as $result) { - if ($result['severity'] === ArcanistLintSeverity::SEVERITY_ERROR) { - $has_error = true; - break; - } else if ($result['severity'] === - ArcanistLintSeverity::SEVERITY_WARNING) { - $has_warning = true; - } - } - - if ($has_error) { - $diff->setLintStatus(DifferentialLintStatus::LINT_FAIL); - } else if ($has_warning) { - $diff->setLintStatus(DifferentialLintStatus::LINT_WARN); - } else if (!empty($postponed_linters)) { - $diff->setLintStatus(DifferentialLintStatus::LINT_POSTPONED); - } else if ($diff->getLintStatus() != DifferentialLintStatus::LINT_SKIP) { - $diff->setLintStatus(DifferentialLintStatus::LINT_OKAY); - } - $diff->save(); - } - protected function execute(ConduitAPIRequest $request) { $diff_id = $request->getValue('diff_id'); $name = $request->getValue('name'); $data = json_decode($request->getValue('data'), true); self::updateDiffProperty($diff_id, $name, $data); - - if ($name === 'arc:lint' || $name == 'arc:lint-postponed') { - self::updateLintStatus($diff_id); - } - - return; } private static function updateDiffProperty($diff_id, $name, $data) { diff --git a/src/applications/differential/conduit/DifferentialUpdateUnitResultsConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialUpdateUnitResultsConduitAPIMethod.php deleted file mode 100644 index 7b1a250666..0000000000 --- a/src/applications/differential/conduit/DifferentialUpdateUnitResultsConduitAPIMethod.php +++ /dev/null @@ -1,154 +0,0 @@ - 'required diff_id', - 'file' => 'required string', - 'name' => 'required string', - 'link' => 'optional string', - 'result' => 'required string', - 'message' => 'required string', - 'coverage' => 'optional map', - ); - } - - protected function defineReturnType() { - return 'void'; - } - - protected function defineErrorTypes() { - return array( - 'ERR_BAD_DIFF' => pht('Bad diff ID.'), - 'ERR_NO_RESULTS' => pht('Could not find the postponed test'), - ); - } - - protected function execute(ConduitAPIRequest $request) { - $diff_id = $request->getValue('diff_id'); - if (!$diff_id) { - throw new ConduitException('ERR_BAD_DIFF'); - } - - $file = $request->getValue('file'); - $name = $request->getValue('name'); - $link = $request->getValue('link'); - $message = $request->getValue('message'); - $result = $request->getValue('result'); - $coverage = $request->getValue('coverage', array()); - - $diff_property = id(new DifferentialDiffProperty())->loadOneWhere( - 'diffID = %d AND name = %s', - $diff_id, - 'arc:unit'); - - if (!$diff_property) { - throw new ConduitException('ERR_NO_RESULTS'); - } - - $diff = id(new DifferentialDiffQuery()) - ->setViewer($request->getUser()) - ->withIDs(array($diff_id)) - ->executeOne(); - - $unit_results = $diff_property->getData(); - $postponed_count = 0; - $unit_status = null; - - // If the test result already exists, then update it with - // the new info. - foreach ($unit_results as &$unit_result) { - if ($unit_result['name'] === $name || - $unit_result['name'] === $file || - $unit_result['name'] === $diff->getSourcePath().$file) { - $unit_result['name'] = $name; - $unit_result['link'] = $link; - $unit_result['file'] = $file; - $unit_result['result'] = $result; - $unit_result['userdata'] = $message; - $unit_result['coverage'] = $coverage; - $unit_status = $result; - break; - } - } - unset($unit_result); - - // If the test result doesn't exist, just add it. - if (!$unit_status) { - $unit_result = array(); - $unit_result['file'] = $file; - $unit_result['name'] = $name; - $unit_result['link'] = $link; - $unit_result['result'] = $result; - $unit_result['userdata'] = $message; - $unit_result['coverage'] = $coverage; - $unit_status = $result; - $unit_results[] = $unit_result; - } - unset($unit_result); - - $diff_property->setData($unit_results); - $diff_property->save(); - - // Map external unit test status to internal overall diff status - $status_codes = - array( - DifferentialUnitTestResult::RESULT_PASS => - DifferentialUnitStatus::UNIT_OKAY, - DifferentialUnitTestResult::RESULT_UNSOUND => - DifferentialUnitStatus::UNIT_WARN, - DifferentialUnitTestResult::RESULT_FAIL => - DifferentialUnitStatus::UNIT_FAIL, - DifferentialUnitTestResult::RESULT_BROKEN => - DifferentialUnitStatus::UNIT_FAIL, - DifferentialUnitTestResult::RESULT_SKIP => - DifferentialUnitStatus::UNIT_OKAY, - DifferentialUnitTestResult::RESULT_POSTPONED => - DifferentialUnitStatus::UNIT_POSTPONED, - ); - - // These are the relative priorities for the unit test results - $status_codes_priority = - array( - DifferentialUnitStatus::UNIT_OKAY => 1, - DifferentialUnitStatus::UNIT_WARN => 2, - DifferentialUnitStatus::UNIT_POSTPONED => 3, - DifferentialUnitStatus::UNIT_FAIL => 4, - ); - - // Walk the now-current list of status codes to find the overall diff - // status - $final_diff_status = DifferentialUnitStatus::UNIT_NONE; - foreach ($unit_results as $unit_result) { - // Convert the text result into a diff unit status value - $status_code = idx($status_codes, - $unit_result['result'], - DifferentialUnitStatus::UNIT_NONE); - - // Convert the unit status into a relative value - $diff_status_priority = idx($status_codes_priority, $status_code, 0); - - // If the relative value of this result is "more bad" than previous - // results, use it as the new final diff status - if ($diff_status_priority > idx($status_codes_priority, - $final_diff_status, 0)) { - $final_diff_status = $status_code; - } - } - - // Update our unit test result status with the final value - $diff->setUnitStatus($final_diff_status); - $diff->save(); - } - -} diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 94d9db0716..6c695b8ce4 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -191,16 +191,6 @@ final class DifferentialRevisionViewController extends DifferentialController { $visible_changesets[$changeset_id] = $changesets[$changeset_id]; } } - - if (!empty($props['arc:lint'])) { - $changeset_paths = mpull($changesets, null, 'getFilename'); - foreach ($props['arc:lint'] as $lint) { - $changeset = idx($changeset_paths, $lint['path']); - if ($changeset) { - $visible_changesets[$changeset->getID()] = $changeset; - } - } - } } else { $warning = null; $visible_changesets = $changesets;