From 31eeb16081290f5eed3453168841967f60efc6e1 Mon Sep 17 00:00:00 2001 From: Jesse Zoldak Date: Tue, 16 Jan 2018 15:52:59 -0500 Subject: [PATCH] Shard the quality tasks on jenkins --- pavelib/paver_tests/test_paver_quality.py | 53 ++++++++--------------- pavelib/paver_tests/utils.py | 10 +++-- pavelib/quality.py | 53 ++++++++++------------- scripts/all-tests.sh | 5 +-- scripts/generic-ci-tests.sh | 53 +++++++++++++++-------- scripts/jenkins-quality-diff.sh | 13 ++++++ scripts/thresholds.sh | 7 +++ 7 files changed, 101 insertions(+), 93 deletions(-) create mode 100755 scripts/jenkins-quality-diff.sh create mode 100755 scripts/thresholds.sh diff --git a/pavelib/paver_tests/test_paver_quality.py b/pavelib/paver_tests/test_paver_quality.py index 7b2f33e81a..8024ca8255 100644 --- a/pavelib/paver_tests/test_paver_quality.py +++ b/pavelib/paver_tests/test_paver_quality.py @@ -300,28 +300,11 @@ class TestPaverRunQuality(unittest.TestCase): self.addCleanup(shutil.rmtree, self.report_dir) self.addCleanup(report_dir_patcher.stop) - @patch('__builtin__.open', mock_open()) - def test_failure_on_diffquality_pep8(self): - """ - If pep8 finds errors, pylint and eslint should still be run - """ - # Mock _get_pep8_violations to return a violation - _mock_pep8_violations = MagicMock( - return_value=(1, ['lms/envs/common.py:32:2: E225 missing whitespace around operator']) - ) - with patch('pavelib.quality._get_pep8_violations', _mock_pep8_violations): - with self.assertRaises(SystemExit): - pavelib.quality.run_quality("") - - # Test that pep8, pylint and eslint were called by counting the calls to - # _get_pep8_violations (for pep8) and sh (5 for pylint & 1 for eslint) - self.assertEqual(_mock_pep8_violations.call_count, 1) - self.assertEqual(self._mock_paver_sh.call_count, 6) - @patch('__builtin__.open', mock_open()) def test_failure_on_diffquality_pylint(self): """ - If diff-quality fails on pylint, the paver task should also fail + If diff-quality fails on pylint, the paver task should also fail, but + only after runnning diff-quality with eslint """ # Underlying sh call must fail when it is running the pylint diff-quality task @@ -331,11 +314,11 @@ class TestPaverRunQuality(unittest.TestCase): with self.assertRaises(SystemExit): pavelib.quality.run_quality("") - # Test that both pep8 and pylint were called by counting the calls # Assert that _get_pylint_violations (which calls "pylint") is called once self.assertEqual(_mock_pylint_violations.call_count, 1) - # And assert that sh was called 6 times (1 for pep8 & 1 for eslint). - # This means that even in the event of a diff-quality pylint failure, eslint and pep8 are still called. + # Assert that sh was called twice- once for diff quality with pylint + # and once for diff quality with eslint. This means that in the event + # of a diff-quality pylint failure, eslint is still called. self.assertEqual(self._mock_paver_sh.call_count, 2) @patch('__builtin__.open', mock_open()) @@ -346,26 +329,23 @@ class TestPaverRunQuality(unittest.TestCase): # Underlying sh call must fail when it is running the eslint diff-quality task self._mock_paver_sh.side_effect = fail_on_eslint - _mock_pep8_violations = MagicMock(return_value=(0, [])) _mock_pylint_violations = MagicMock(return_value=(0, [])) - with patch('pavelib.quality._get_pep8_violations', _mock_pep8_violations): - with patch('pavelib.quality._get_pylint_violations', _mock_pylint_violations): - with self.assertRaises(SystemExit): - pavelib.quality.run_quality("") - self.assertRaises(BuildFailure) + 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 both pep8 and pylint were called by counting the calls - # Assert that _get_pep8_violations (which calls "pep8") is called once - _mock_pep8_violations.assert_called_once_with(clean=False) + # Test that pylint is called _mock_pylint_violations.assert_called_once_with(clean=False) - # And assert that sh was called once (the call to eslint) - self.assertEqual(self._mock_paver_sh.call_count, 1) + # Assert that sh was called twice- once for diff quality with pylint + # and once for diff quality with eslint + self.assertEqual(self._mock_paver_sh.call_count, 2) @patch('__builtin__.open', mock_open()) def test_other_exception(self): """ - If diff-quality fails for an unknown reason on the first run (pep8), then + If diff-quality fails for an unknown reason on the first run, then pylint should not be run """ self._mock_paver_sh.side_effect = [Exception('unrecognized failure!'), 0] @@ -379,5 +359,8 @@ class TestPaverRunQuality(unittest.TestCase): def test_no_diff_quality_failures(self): # Assert nothing is raised pavelib.quality.run_quality("") - # And assert that sh was called 7 times (1 for pep8, 5 for pylint, and 1 for eslint) + # And assert that sh was called 7 times: + # 5 for pylint on each of the system directories + # 1 for diff_quality for pylint + # 1 for diff_quality for eslint self.assertEqual(self._mock_paver_sh.call_count, 7) diff --git a/pavelib/paver_tests/utils.py b/pavelib/paver_tests/utils.py index ba5a837edf..0d0e271aed 100644 --- a/pavelib/paver_tests/utils.py +++ b/pavelib/paver_tests/utils.py @@ -65,8 +65,9 @@ class MockEnvironment(tasks.Environment): def fail_on_eslint(*args): """ - For our tests, we need the call for diff-quality running pep8 reports to fail, since that is what - is going to fail when we pass in a percentage ("p") requirement. + For our tests, we need the call for diff-quality running eslint reports + to fail, since that is what is going to fail when we pass in a + percentage ("p") requirement. """ if "eslint" in args[0]: # Essentially mock diff-quality exiting with 1 @@ -77,8 +78,9 @@ def fail_on_eslint(*args): def fail_on_pylint(*args): """ - For our tests, we need the call for diff-quality running pep8 reports to fail, since that is what - is going to fail when we pass in a percentage ("p") requirement. + For our tests, we need the call for diff-quality running pylint reports + to fail, since that is what is going to fail when we pass in a + percentage ("p") requirement. """ if "pylint" in args[0]: # Essentially mock diff-quality exiting with 1 diff --git a/pavelib/quality.py b/pavelib/quality.py index 5f5be1bfc7..1fe36f40fa 100644 --- a/pavelib/quality.py +++ b/pavelib/quality.py @@ -696,18 +696,18 @@ def run_quality(options): :param: p, diff-quality will fail if the quality percentage calculated is below this percentage. For example, if p is set to 80, and diff-quality finds quality of the branch vs the compare branch is less than 80%, then this task will fail. - This threshold would be applied to both pep8 and pylint. """ # Directory to put the diff reports in. # This makes the folder if it doesn't already exist. dquality_dir = (Env.REPORT_DIR / "diff_quality").makedirs_p() # Save the pass variable. It will be set to false later if failures are detected. - diff_quality_percentage_pass = True + diff_quality_pass = True + failure_reasons = [] def _lint_output(linter, count, violations_list, is_html=False, limit=0): """ - Given a count & list of pep8 violations, pretty-print the pep8 output. + Given a count & list of pylint violations, pretty-print the output. If `is_html`, will print out with HTML markup. """ if is_html: @@ -740,36 +740,16 @@ def run_quality(options): return ''.join(lines) - # Run pep8 directly since we have 0 violations on master - (count, violations_list) = _get_pep8_violations(clean=False) - - # Print number of violations to log - print _lint_output('pep8', count, violations_list) - - # Also write the number of violations to a file - with open(dquality_dir / "diff_quality_pep8.html", "w") as f: - f.write(_lint_output('pep8', count, violations_list, is_html=True)) - - if count > 0: - diff_quality_percentage_pass = False - - # Generate diff-quality html report for pylint, and print to console # If pylint reports exist, use those # Otherwise, `diff-quality` will call pylint itself - (count, violations_list) = _get_pylint_violations(clean=False) - _, upper_violations_limit, _, _ = _parse_pylint_options(options) - # Print number of violations to log + # Print total number of violations to log print _lint_output('pylint', count, violations_list, limit=upper_violations_limit) - - # Also write the number of violations to a file - with open(dquality_dir / "diff_quality_pylint.html", "w") as f: - f.write(_lint_output('pylint', count, violations_list, is_html=True, limit=upper_violations_limit)) - if count > upper_violations_limit > -1: - diff_quality_percentage_pass = False + diff_quality_pass = False + failure_reasons.append('Too many total violations.') # ----- Set up for diff-quality pylint call ----- # Set the string, if needed, to be used for the diff-quality --compare-branch switch. @@ -784,10 +764,20 @@ def run_quality(options): if diff_threshold > -1: percentage_string = u'--fail-under={0}'.format(diff_threshold) + pylint_files = get_violations_reports("pylint") + pylint_reports = u' '.join(pylint_files) + if not run_diff_quality( + violations_type="pylint", + reports=pylint_reports, + percentage_string=percentage_string, + branch_string=compare_branch_string, + dquality_dir=dquality_dir + ): + diff_quality_pass = False + failure_reasons.append('Pylint violation(s) were found in the lines of code that were added or changed.') + eslint_files = get_violations_reports("eslint") eslint_reports = u' '.join(eslint_files) - - # run diff-quality for eslint. if not run_diff_quality( violations_type="eslint", reports=eslint_reports, @@ -795,11 +785,12 @@ def run_quality(options): branch_string=compare_branch_string, dquality_dir=dquality_dir ): - diff_quality_percentage_pass = False + diff_quality_pass = False + failure_reasons.append('Eslint violation(s) were found in the lines of code that were added or changed.') # If one of the quality runs fails, then paver exits with an error when it is finished - if not diff_quality_percentage_pass: - msg = "FAILURE: Diff-quality failure(s). This means that violations were found in the current changeset." + if not diff_quality_pass: + msg = "FAILURE: " + " ".join(failure_reasons) raise BuildFailure(msg) diff --git a/scripts/all-tests.sh b/scripts/all-tests.sh index 33c023959e..4d4e0622eb 100755 --- a/scripts/all-tests.sh +++ b/scripts/all-tests.sh @@ -11,10 +11,7 @@ set -e ############################################################################### # Violations thresholds for failing the build -export LOWER_PYLINT_THRESHOLD=1000 -export UPPER_PYLINT_THRESHOLD=5900 -export ESLINT_THRESHOLD=5700 -export STYLELINT_THRESHOLD=973 +source scripts/thresholds.sh XSSLINT_THRESHOLDS=`cat scripts/xsslint_thresholds.json` export XSSLINT_THRESHOLDS=${XSSLINT_THRESHOLDS//[[:space:]]/} diff --git a/scripts/generic-ci-tests.sh b/scripts/generic-ci-tests.sh index 0594019ee4..55565e7989 100755 --- a/scripts/generic-ci-tests.sh +++ b/scripts/generic-ci-tests.sh @@ -100,28 +100,43 @@ function run_paver_quality { case "$TEST_SUITE" in "quality") - echo "Finding fixme's and storing report..." - run_paver_quality find_fixme || EXIT=1 - echo "Finding pep8 violations and storing report..." - run_paver_quality run_pep8 || EXIT=1 - echo "Finding pylint violations and storing in report..." - run_paver_quality run_pylint -l $LOWER_PYLINT_THRESHOLD:$UPPER_PYLINT_THRESHOLD || EXIT=1 mkdir -p reports - echo "Finding ESLint violations and storing report..." - run_paver_quality run_eslint -l $ESLINT_THRESHOLD || EXIT=1 - echo "Finding Stylelint violations and storing report..." - run_paver_quality run_stylelint -l $STYLELINT_THRESHOLD || EXIT=1 - echo "Running code complexity report (python)." - run_paver_quality run_complexity || echo "Unable to calculate code complexity. Ignoring error." - echo "Running xss linter report." - run_paver_quality run_xsslint -t $XSSLINT_THRESHOLDS || EXIT=1 - echo "Running safe commit linter report." - run_paver_quality run_xsscommitlint || EXIT=1 - # Run quality task. Pass in the 'fail-under' percentage to diff-quality - echo "Running diff quality." - run_paver_quality run_quality -p 100 -l $LOWER_PYLINT_THRESHOLD:$UPPER_PYLINT_THRESHOLD || EXIT=1 + case "$SHARD" in + 1) + echo "Finding pylint violations and storing in report..." + run_paver_quality run_pylint --system=common || { EXIT=1; } + ;; + + 2) + echo "Finding pylint violations and storing in report..." + run_paver_quality run_pylint --system=lms || { EXIT=1; } + ;; + + 3) + echo "Finding pylint violations and storing in report..." + run_paver_quality run_pylint --system="cms,openedx,pavelib" || { EXIT=1; } + ;; + + 4) + echo "Finding fixme's and storing report..." + run_paver_quality find_fixme || { EXIT=1; } + echo "Finding pep8 violations and storing report..." + run_paver_quality run_pep8 || { EXIT=1; } + echo "Finding ESLint violations and storing report..." + run_paver_quality run_eslint -l $ESLINT_THRESHOLD || { EXIT=1; } + echo "Finding Stylelint violations and storing report..." + run_paver_quality run_stylelint -l $STYLELINT_THRESHOLD || { EXIT=1; } + echo "Running code complexity report (python)." + run_paver_quality run_complexity || echo "Unable to calculate code complexity. Ignoring error." + echo "Running xss linter report." + run_paver_quality run_xsslint -t $XSSLINT_THRESHOLDS || { EXIT=1; } + echo "Running safe commit linter report." + run_paver_quality run_xsscommitlint || { EXIT=1; } + ;; + + esac # Need to create an empty test result so the post-build # action doesn't fail the build. diff --git a/scripts/jenkins-quality-diff.sh b/scripts/jenkins-quality-diff.sh new file mode 100755 index 0000000000..83da85c285 --- /dev/null +++ b/scripts/jenkins-quality-diff.sh @@ -0,0 +1,13 @@ +#!/usr/bin/env bash +set -e + +# This script is used by the edx-platform-quality-check jenkins job. +source scripts/jenkins-common.sh +source scripts/thresholds.sh + +# Run quality task. Pass in the 'fail-under' percentage to diff-quality +echo "Running diff quality." +mkdir -p test_root/log/ +LOG_PREFIX=test_root/log/run_quality + +paver run_quality -p 100 -l $LOWER_PYLINT_THRESHOLD:$UPPER_PYLINT_THRESHOLD 2> $LOG_PREFIX.err.log > $LOG_PREFIX.out.log diff --git a/scripts/thresholds.sh b/scripts/thresholds.sh new file mode 100755 index 0000000000..e5f49a7de3 --- /dev/null +++ b/scripts/thresholds.sh @@ -0,0 +1,7 @@ +#!/usr/bin/env bash +set -e + +export LOWER_PYLINT_THRESHOLD=1000 +export UPPER_PYLINT_THRESHOLD=5900 +export ESLINT_THRESHOLD=5700 +export STYLELINT_THRESHOLD=973