From 4722a187e6c222d21d96a25f49e13893b111f4b1 Mon Sep 17 00:00:00 2001
From: Natthu Bharambe <natthu@fb.com>
Date: Wed, 22 Feb 2012 15:57:55 -0800
Subject: [PATCH] Require explanation for failing lint/tests in arc.

Summary:
Modified the arc diff workflow, introduced new diff properties -
arc:lint-excuse and arc:unit-excuse for respective errors/warnings.
The diff author inputs the explanation in an editor (similar to commit
message).

Task ID: #

Blame Rev:

Test Plan:
Ran sandbox arc on itself with some lint errors and verified that the
Diff property (== user explanation) is being set.

Revert Plan:

Tags:

Reviewers: epriestley, nh

CC: akramer, blair, aran, andreygoder, Girish, epriestley

Differential Revision: https://secure.phabricator.com/D1676
---
 src/workflow/diff/ArcanistDiffWorkflow.php | 66 +++++++++++++++++++++-
 src/workflow/diff/__init__.php             |  1 +
 2 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/src/workflow/diff/ArcanistDiffWorkflow.php b/src/workflow/diff/ArcanistDiffWorkflow.php
index 78dfc09d..4b780cf4 100644
--- a/src/workflow/diff/ArcanistDiffWorkflow.php
+++ b/src/workflow/diff/ArcanistDiffWorkflow.php
@@ -30,6 +30,8 @@ final class ArcanistDiffWorkflow extends ArcanistBaseWorkflow {
 
   private $hasWarnedExternals = false;
   private $unresolvedLint;
+  private $lintExcuse;
+  private $unitExcuse;
   private $testResults;
   private $diffID;
   private $unitWorkflow;
@@ -1010,6 +1012,7 @@ EOTEXT
 
       $lint_result = $lint_workflow->run();
 
+      $continue = false;
       switch ($lint_result) {
         case ArcanistLintWorkflow::RESULT_OKAY:
           echo phutil_console_format(
@@ -1017,7 +1020,8 @@ EOTEXT
           break;
         case ArcanistLintWorkflow::RESULT_WARNINGS:
           $continue = phutil_console_confirm(
-            "Lint issued unresolved warnings. Ignore them?");
+            "Lint issued unresolved warnings.".
+            "Provide explanation and continue?");
           if (!$continue) {
             throw new ArcanistUserAbortException();
           }
@@ -1026,7 +1030,7 @@ EOTEXT
           echo phutil_console_format(
             "<bg:red>** LINT ERRORS **</bg> Lint raised errors!\n");
           $continue = phutil_console_confirm(
-            "Lint issued unresolved errors! Ignore lint errors?");
+            "Lint issued unresolved errors! Provide explanation and continue?");
           if (!$continue) {
             throw new ArcanistUserAbortException();
           }
@@ -1034,6 +1038,17 @@ EOTEXT
       }
 
       $this->unresolvedLint = $lint_workflow->getUnresolvedMessages();
+      if ($continue) {
+        $template = "\n\n# Provide an explanation for these lint failures:\n";
+        foreach ($this->unresolvedLint as $message) {
+          $template = $template."# ".
+            $message->getPath().":".
+            $message->getLine()." ".
+            $message->getCode()." :: ".
+            $message->getDescription()."\n";
+        }
+        $this->lintExcuse = $this->getErrorExcuse($template);
+      }
 
       return $lint_result;
     } catch (ArcanistNoEngineException $ex) {
@@ -1067,6 +1082,7 @@ EOTEXT
       }
       $this->unitWorkflow = $this->buildChildWorkflow('unit', $argv);
       $unit_result = $this->unitWorkflow->run();
+      $explain = false;
       switch ($unit_result) {
         case ArcanistUnitWorkflow::RESULT_OKAY:
           echo phutil_console_format(
@@ -1084,14 +1100,34 @@ EOTEXT
           echo phutil_console_format(
             "<bg:red>** UNIT ERRORS **</bg> Unit testing raised errors!\n");
           $continue = phutil_console_confirm(
-            "Unit test results include failures! Ignore test failures?");
+            "Unit test results include failures!".
+            " Explain test failures and continue?");
           if (!$continue) {
             throw new ArcanistUserAbortException();
           }
+          $explain = true;
           break;
       }
 
       $this->testResults = $this->unitWorkflow->getTestResults();
+      if ($explain) {
+        $template = "\n\n".
+          "# Provide an explanation for these unit test failures:\n";
+        foreach ($this->testResults as $test) {
+          $testResult = $test->getResult();
+          switch ($testResult) {
+            case ArcanistUnitTestResult::RESULT_FAIL:
+            case ArcanistUnitTestResult::RESULT_BROKEN:
+              $template = $template."# ".
+                $test->getName()." :: ".
+                $test->getResult()."\n";
+              break;
+            default:
+              break;
+          }
+        }
+        $this->unitExcuse = $this->getErrorExcuse($template);
+      }
 
       return $unit_result;
     } catch (ArcanistNoEngineException $ex) {
@@ -1103,6 +1139,22 @@ EOTEXT
     return null;
   }
 
+  private function getErrorExcuse($template) {
+    $new_template = id(new PhutilInteractiveEditor($template))
+      ->setName('error-excuse')
+      ->editInteractively();
+
+    if ($new_template == $template) {
+      throw new ArcanistUsageException(
+        "No explanation provided.");
+    }
+
+    $template = preg_replace('/^\s*#.*$/m', '', $new_template);
+    $template = rtrim($template)."\n";
+
+    return $template;
+  }
+
 
 /* -(  Commit and Update Messages  )----------------------------------------- */
 
@@ -1679,6 +1731,10 @@ EOTEXT
     }
 
     $this->updateDiffProperty('arc:lint', json_encode($data));
+    if (strlen($this->lintExcuse)) {
+      $this->updateDiffProperty('arc:lint-excuse',
+        json_encode($this->lintExcuse));
+    }
   }
 
 
@@ -1705,6 +1761,10 @@ EOTEXT
     }
 
     $this->updateDiffProperty('arc:unit', json_encode($data));
+    if (strlen($this->unitExcuse)) {
+      $this->updateDiffProperty('arc:unit-excuse',
+        json_encode($this->unitExcuse));
+    }
   }
 
 
diff --git a/src/workflow/diff/__init__.php b/src/workflow/diff/__init__.php
index 64177eb6..9956cab5 100644
--- a/src/workflow/diff/__init__.php
+++ b/src/workflow/diff/__init__.php
@@ -13,6 +13,7 @@ phutil_require_module('arcanist', 'exception/usage/userabort');
 phutil_require_module('arcanist', 'parser/diff');
 phutil_require_module('arcanist', 'parser/diff/changetype');
 phutil_require_module('arcanist', 'repository/api/base');
+phutil_require_module('arcanist', 'unit/result');
 phutil_require_module('arcanist', 'workflow/base');
 phutil_require_module('arcanist', 'workflow/lint');
 phutil_require_module('arcanist', 'workflow/unit');