From b9a51eb250fbc247fdbf2d0dd0983d1e98a81c06 Mon Sep 17 00:00:00 2001
From: Robert Raposa <rraposa@edx.org>
Date: Tue, 5 May 2020 15:55:37 -0400
Subject: [PATCH] update diff_quality message and tests (#23896)

- remove unused assertions
- add tests for exception handling
- update error message
---
 pavelib/paver_tests/test_paver_quality.py | 36 +++++++++++++++++++++--
 pavelib/quality.py                        |  5 +++-
 2 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/pavelib/paver_tests/test_paver_quality.py b/pavelib/paver_tests/test_paver_quality.py
index 9615990ccd..7423f20856 100644
--- a/pavelib/paver_tests/test_paver_quality.py
+++ b/pavelib/paver_tests/test_paver_quality.py
@@ -325,7 +325,6 @@ class TestPaverRunQuality(PaverTestCase):
         with patch('pavelib.quality._get_pylint_violations', _mock_pylint_violations):
             with self.assertRaises(SystemExit):
                 pavelib.quality.run_quality("")
-                self.assertRaises(BuildFailure)
         print(self._mock_paver_sh.mock_calls)
 
         # Test that pylint is called
@@ -344,7 +343,6 @@ class TestPaverRunQuality(PaverTestCase):
         self._mock_paver_sh.side_effect = [Exception('unrecognized failure!'), 0]
         with self.assertRaises(SystemExit):
             pavelib.quality.run_quality("")
-            self.assertRaises(Exception)
         # Test that pylint is NOT called by counting calls
         self.assertEqual(self._mock_paver_sh.call_count, 1)
 
@@ -357,3 +355,37 @@ class TestPaverRunQuality(PaverTestCase):
         # 1 for diff_quality for pylint
         # 1 for diff_quality for eslint
         self.assertEqual(self._mock_paver_sh.call_count, 8)
+
+
+class TestPaverRunDiffQuality(PaverTestCase):
+    """
+    For testing the paver run_diff_quality task
+
+    Note: Although diff_quality is tested as part of quality, some
+    cases weren't tested properly.
+    """
+    def setUp(self):
+        super(TestPaverRunDiffQuality, self).setUp()
+
+        # mock the @needs decorator to skip it
+        patcher = patch('pavelib.quality.sh')
+        self._mock_paver_sh = patcher.start()
+        self.addCleanup(patcher.stop)
+
+    @patch(OPEN_BUILTIN, mock_open())
+    def test_percentage_failure(self):
+        """
+        When diff_quality is run with a threshold percentage, it ends with an exit code of 1.
+        This bubbles up to paver with a subprocess return code error and should return False.
+        """
+        self._mock_paver_sh.side_effect = [BuildFailure('Subprocess return code: 1')]
+        self.assertEqual(pavelib.quality.run_diff_quality(""), False)
+
+    @patch(OPEN_BUILTIN, mock_open())
+    def test_other_failures(self):
+        """
+        Run diff_quality with an exception that is not a percentage failure.
+        """
+        self._mock_paver_sh.side_effect = [BuildFailure('Some failure.')]
+        with self.assertRaisesRegex(BuildFailure, '.*Diff Quality Report.*Some failure.'):
+            pavelib.quality.run_diff_quality("")
diff --git a/pavelib/quality.py b/pavelib/quality.py
index 375061a8f2..40fe7bb092 100644
--- a/pavelib/quality.py
+++ b/pavelib/quality.py
@@ -1035,7 +1035,10 @@ def run_diff_quality(
         if is_percentage_failure(failure.args):
             return False
         else:
-            fail_quality('diff_quality', 'FAILURE: {}'.format(failure))
+            fail_quality(
+                'diff_quality',
+                'FAILURE: See "Diff Quality Report" in Jenkins left-sidebar for details. {}'.format(failure)
+            )
 
 
 def is_percentage_failure(error_message):