From 5ea529ae3915415933051185aee750604dc9368d Mon Sep 17 00:00:00 2001 From: Robert Raposa <rraposa@edx.org> Date: Wed, 11 May 2016 09:56:23 -0400 Subject: [PATCH] Revert: Enhance Jenkins integration of safe template linting --- pavelib/paver_tests/test_paver_quality.py | 59 ----------------------------------------------------------- pavelib/paver_tests/test_safecommit.py | 44 -------------------------------------------- pavelib/paver_tests/test_safelint.py | 86 +++++++++++++++----------------------------------------------------------------------- pavelib/quality.py | 216 +++++++++++++++++++++--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- scripts/all-tests.sh | 2 +- scripts/generic-ci-tests.sh | 4 +--- scripts/safelint_thresholds.sh | 47 ----------------------------------------------- 7 files changed, 38 insertions(+), 420 deletions(-) delete mode 100644 pavelib/paver_tests/test_safecommit.py delete mode 100755 scripts/safelint_thresholds.sh diff --git a/pavelib/paver_tests/test_paver_quality.py b/pavelib/paver_tests/test_paver_quality.py index ea2b9d3..db08061 100644 --- a/pavelib/paver_tests/test_paver_quality.py +++ b/pavelib/paver_tests/test_paver_quality.py @@ -4,7 +4,6 @@ Tests for paver quality tasks import os from path import Path as path import tempfile -import textwrap import unittest from mock import patch, MagicMock, mock_open from ddt import ddt, file_data @@ -133,64 +132,6 @@ class TestPaverReportViolationsCounts(unittest.TestCase): actual_count = pavelib.quality._get_count_from_last_line(self.f.name, "foo") # pylint: disable=protected-access self.assertEqual(actual_count, None) - def test_get_safelint_counts_happy(self): - report = textwrap.dedent(""" - test.html: 30:53: javascript-jquery-append: $('#test').append(print_tos); - - javascript-concat-html: 310 violations - javascript-escape: 7 violations - - 2608 violations total - """) - with open(self.f.name, 'w') as f: - f.write(report) - counts = pavelib.quality._get_safelint_counts(self.f.name) # pylint: disable=protected-access - self.assertDictEqual(counts, { - 'rules': { - 'javascript-concat-html': 310, - 'javascript-escape': 7, - }, - 'total': 2608, - }) - - def test_get_safelint_counts_bad_counts(self): - report = textwrap.dedent(""" - javascript-concat-html: violations - """) - with open(self.f.name, 'w') as f: - f.write(report) - counts = pavelib.quality._get_safelint_counts(self.f.name) # pylint: disable=protected-access - self.assertDictEqual(counts, { - 'rules': {}, - 'total': None, - }) - - def test_get_safecommit_count_happy(self): - report = textwrap.dedent(""" - Linting lms/templates/navigation.html: - - 2 violations total - - Linting scripts/tests/templates/test.underscore: - - 3 violations total - """) - with open(self.f.name, 'w') as f: - f.write(report) - count = pavelib.quality._get_safecommit_count(self.f.name) # pylint: disable=protected-access - - self.assertEqual(count, 5) - - def test_get_safecommit_count_bad_counts(self): - report = textwrap.dedent(""" - Linting lms/templates/navigation.html: - """) - with open(self.f.name, 'w') as f: - f.write(report) - count = pavelib.quality._get_safecommit_count(self.f.name) # pylint: disable=protected-access - - self.assertIsNone(count) - class TestPrepareReportDir(unittest.TestCase): """ diff --git a/pavelib/paver_tests/test_safecommit.py b/pavelib/paver_tests/test_safecommit.py deleted file mode 100644 index 75055a3..0000000 --- a/pavelib/paver_tests/test_safecommit.py +++ /dev/null @@ -1,44 +0,0 @@ -""" -Tests for paver safecommit quality tasks -""" -from mock import patch - -import pavelib.quality -from paver.easy import call_task - -from .utils import PaverTestCase - - -class PaverSafeCommitTest(PaverTestCase): - """ - Test run_safecommit_report with a mocked environment in order to pass in - opts. - - """ - - def setUp(self): - super(PaverSafeCommitTest, self).setUp() - self.reset_task_messages() - - @patch.object(pavelib.quality, '_write_metric') - @patch.object(pavelib.quality, '_prepare_report_dir') - @patch.object(pavelib.quality, '_get_safecommit_count') - def test_safecommit_violation_number_not_found(self, _mock_count, _mock_report_dir, _mock_write_metric): - """ - run_safecommit_report encounters an error parsing the safecommit output - log. - - """ - _mock_count.return_value = None - with self.assertRaises(SystemExit): - call_task('pavelib.quality.run_safecommit_report') - - @patch.object(pavelib.quality, '_write_metric') - @patch.object(pavelib.quality, '_prepare_report_dir') - @patch.object(pavelib.quality, '_get_safecommit_count') - def test_safecommit_vanilla(self, _mock_count, _mock_report_dir, _mock_write_metric): - """ - run_safecommit_report finds violations. - """ - _mock_count.return_value = 0 - call_task('pavelib.quality.run_safecommit_report') diff --git a/pavelib/paver_tests/test_safelint.py b/pavelib/paver_tests/test_safelint.py index f653ae5..2b39ca1 100644 --- a/pavelib/paver_tests/test_safelint.py +++ b/pavelib/paver_tests/test_safelint.py @@ -1,5 +1,5 @@ """ -Tests for paver safelint quality tasks +Tests for paver quality tasks """ from mock import patch @@ -20,99 +20,43 @@ class PaverSafeLintTest(PaverTestCase): @patch.object(pavelib.quality, '_write_metric') @patch.object(pavelib.quality, '_prepare_report_dir') - @patch.object(pavelib.quality, '_get_safelint_counts') - def test_safelint_violation_number_not_found(self, _mock_counts, _mock_report_dir, _mock_write_metric): + @patch.object(pavelib.quality, '_get_count_from_last_line') + def test_safelint_violation_number_not_found(self, _mock_count, _mock_report_dir, _mock_write_metric): """ run_safelint encounters an error parsing the safelint output log """ - _mock_counts.return_value = {} + _mock_count.return_value = None with self.assertRaises(SystemExit): call_task('pavelib.quality.run_safelint') @patch.object(pavelib.quality, '_write_metric') @patch.object(pavelib.quality, '_prepare_report_dir') - @patch.object(pavelib.quality, '_get_safelint_counts') - def test_safelint_vanilla(self, _mock_counts, _mock_report_dir, _mock_write_metric): + @patch.object(pavelib.quality, '_get_count_from_last_line') + def test_safelint_vanilla(self, _mock_count, _mock_report_dir, _mock_write_metric): """ run_safelint finds violations, but a limit was not set """ - _mock_counts.return_value = {'total': 0} + _mock_count.return_value = 1 call_task('pavelib.quality.run_safelint') @patch.object(pavelib.quality, '_write_metric') @patch.object(pavelib.quality, '_prepare_report_dir') - @patch.object(pavelib.quality, '_get_safelint_counts') - def test_safelint_invalid_thresholds_option(self, _mock_counts, _mock_report_dir, _mock_write_metric): - """ - run_safelint fails when thresholds option is poorly formatted - """ - _mock_counts.return_value = {'total': 0} - with self.assertRaises(SystemExit): - call_task('pavelib.quality.run_safelint', options={"thresholds": "invalid"}) - - @patch.object(pavelib.quality, '_write_metric') - @patch.object(pavelib.quality, '_prepare_report_dir') - @patch.object(pavelib.quality, '_get_safelint_counts') - def test_safelint_invalid_thresholds_option_key(self, _mock_counts, _mock_report_dir, _mock_write_metric): - """ - run_safelint fails when thresholds option is poorly formatted - """ - _mock_counts.return_value = {'total': 0} - with self.assertRaises(SystemExit): - call_task('pavelib.quality.run_safelint', options={"thresholds": '{"invalid": 3}'}) - - @patch.object(pavelib.quality, '_write_metric') - @patch.object(pavelib.quality, '_prepare_report_dir') - @patch.object(pavelib.quality, '_get_safelint_counts') - def test_safelint_too_many_violations(self, _mock_counts, _mock_report_dir, _mock_write_metric): + @patch.object(pavelib.quality, '_get_count_from_last_line') + def test_safelint_too_many_violations(self, _mock_count, _mock_report_dir, _mock_write_metric): """ run_safelint finds more violations than are allowed """ - _mock_counts.return_value = {'total': 4} + _mock_count.return_value = 4 with self.assertRaises(SystemExit): - call_task('pavelib.quality.run_safelint', options={"thresholds": '{"total": 3}'}) + call_task('pavelib.quality.run_safelint', options={"limit": "3"}) @patch.object(pavelib.quality, '_write_metric') @patch.object(pavelib.quality, '_prepare_report_dir') - @patch.object(pavelib.quality, '_get_safelint_counts') - def test_safelint_under_limit(self, _mock_counts, _mock_report_dir, _mock_write_metric): + @patch.object(pavelib.quality, '_get_count_from_last_line') + def test_safelint_under_limit(self, _mock_count, _mock_report_dir, _mock_write_metric): """ run_safelint finds fewer violations than are allowed """ - _mock_counts.return_value = {'total': 4} - # No System Exit is expected - call_task('pavelib.quality.run_safelint', options={"thresholds": '{"total": 5}'}) - - @patch.object(pavelib.quality, '_write_metric') - @patch.object(pavelib.quality, '_prepare_report_dir') - @patch.object(pavelib.quality, '_get_safelint_counts') - def test_safelint_rule_violation_number_not_found(self, _mock_counts, _mock_report_dir, _mock_write_metric): - """ - run_safelint encounters an error parsing the safelint output log for a - given rule threshold that was set. - """ - _mock_counts.return_value = {'total': 4} - with self.assertRaises(SystemExit): - call_task('pavelib.quality.run_safelint', options={"thresholds": '{"rules": {"javascript-escape": 3}}'}) - - @patch.object(pavelib.quality, '_write_metric') - @patch.object(pavelib.quality, '_prepare_report_dir') - @patch.object(pavelib.quality, '_get_safelint_counts') - def test_safelint_too_many_rule_violations(self, _mock_counts, _mock_report_dir, _mock_write_metric): - """ - run_safelint finds more rule violations than are allowed - """ - _mock_counts.return_value = {'total': 4, 'rules': {'javascript-escape': 4}} - with self.assertRaises(SystemExit): - call_task('pavelib.quality.run_safelint', options={"thresholds": '{"rules": {"javascript-escape": 3}}'}) - - @patch.object(pavelib.quality, '_write_metric') - @patch.object(pavelib.quality, '_prepare_report_dir') - @patch.object(pavelib.quality, '_get_safelint_counts') - def test_safelint_under_rule_limit(self, _mock_counts, _mock_report_dir, _mock_write_metric): - """ - run_safelint finds fewer rule violations than are allowed - """ - _mock_counts.return_value = {'total': 4, 'rules': {'javascript-escape': 4}} + _mock_count.return_value = 4 # No System Exit is expected - call_task('pavelib.quality.run_safelint', options={"thresholds": '{"rules": {"javascript-escape": 5}}'}) + call_task('pavelib.quality.run_safelint', options={"limit": "5"}) diff --git a/pavelib/quality.py b/pavelib/quality.py index be773ed..e8fc55d 100644 --- a/pavelib/quality.py +++ b/pavelib/quality.py @@ -2,7 +2,6 @@ Check code quality using pep8, pylint, and diff_quality. """ from paver.easy import sh, task, cmdopts, needs, BuildFailure -import json import os import re @@ -136,8 +135,8 @@ def run_pylint(options): # Fail number of violations is greater than the limit if num_violations > violations_limit > -1: - raise BuildFailure("Failed. Too many pylint violations. " - "The limit is {violations_limit}.".format(violations_limit=violations_limit)) + raise Exception("Failed. Too many pylint violations. " + "The limit is {violations_limit}.".format(violations_limit=violations_limit)) def _count_pylint_violations(report_file): @@ -219,7 +218,7 @@ def run_pep8(options): # pylint: disable=unused-argument if count: failure_string = "Too many pep8 violations. " + violations_count_str failure_string += "\n\nViolations:\n{violations_list}".format(violations_list=violations_list) - raise BuildFailure(failure_string) + raise Exception(failure_string) @task @@ -294,7 +293,7 @@ def run_jshint(options): # Fail if number of violations is greater than the limit if num_violations > violations_limit > -1: - raise BuildFailure( + raise Exception( "JSHint Failed. Too many violations ({count}).\nThe limit is {violations_limit}.".format( count=num_violations, violations_limit=violations_limit ) @@ -304,151 +303,47 @@ def run_jshint(options): @task @needs('pavelib.prereqs.install_python_prereqs') @cmdopts([ - ("thresholds=", "t", "json containing limit for number of acceptable violations per rule"), + ("limit=", "l", "limit for number of acceptable violations"), ]) def run_safelint(options): """ Runs safe_template_linter.py on the codebase """ - thresholds_option = getattr(options, 'thresholds', '{}') - try: - violation_thresholds = json.loads(thresholds_option) - except ValueError: - violation_thresholds = None - if isinstance(violation_thresholds, dict) is False or \ - any(key not in ("total", "rules") for key in violation_thresholds.keys()): - - raise BuildFailure( - """Error. Thresholds option "{thresholds_option}" was not supplied using proper format.\n""" - """Here is a properly formatted example, '{{"total":100,"rules":{{"javascript-escape":0}}}}' """ - """with property names in double-quotes.""".format( - thresholds_option=thresholds_option - ) - ) + violations_limit = int(getattr(options, 'limit', -1)) - safelint_script = "safe_template_linter.py" safelint_report_dir = (Env.REPORT_DIR / "safelint") safelint_report = safelint_report_dir / "safelint.report" _prepare_report_dir(safelint_report_dir) sh( - "{repo_root}/scripts/{safelint_script} --rule-totals >> {safelint_report}".format( + "{repo_root}/scripts/safe_template_linter.py >> {safelint_report}".format( repo_root=Env.REPO_ROOT, - safelint_script=safelint_script, safelint_report=safelint_report, ), ignore_error=True ) - safelint_counts = _get_safelint_counts(safelint_report) - try: - metrics_str = "Number of {safelint_script} violations: {num_violations}\n".format( - safelint_script=safelint_script, num_violations=int(safelint_counts['total']) - ) - if 'rules' in safelint_counts and any(safelint_counts['rules']): - metrics_str += "\n" - rule_keys = sorted(safelint_counts['rules'].keys()) - for rule in rule_keys: - metrics_str += "{rule} violations: {count}\n".format( - rule=rule, - count=int(safelint_counts['rules'][rule]) - ) + num_violations = int(_get_count_from_last_line(safelint_report, "safelint")) except TypeError: raise BuildFailure( - "Error. Number of {safelint_script} violations could not be found in {safelint_report}".format( - safelint_script=safelint_script, safelint_report=safelint_report + "Error. Number of safelint violations could not be found in {safelint_report}".format( + safelint_report=safelint_report ) ) - metrics_report = (Env.METRICS_DIR / "safelint") # Record the metric - _write_metric(metrics_str, metrics_report) - # Print number of violations to log. - sh("cat {metrics_report}".format(metrics_report=metrics_report), ignore_error=True) - - error_message = "" - - # Test total violations against threshold. - if 'total' in violation_thresholds.keys(): - if violation_thresholds['total'] < safelint_counts['total']: - error_message = "Too many violations total ({count}).\nThe limit is {violations_limit}.".format( - count=safelint_counts['total'], violations_limit=violation_thresholds['total'] - ) + _write_metric(num_violations, (Env.METRICS_DIR / "safelint")) - # Test rule violations against thresholds. - if 'rules' in violation_thresholds: - threshold_keys = sorted(violation_thresholds['rules'].keys()) - for threshold_key in threshold_keys: - if threshold_key not in safelint_counts['rules']: - error_message += ( - "\nNumber of {safelint_script} violations for {rule} could not be found in " - "{safelint_report}." - ).format( - safelint_script=safelint_script, rule=threshold_key, safelint_report=safelint_report - ) - elif violation_thresholds['rules'][threshold_key] < safelint_counts['rules'][threshold_key]: - error_message += \ - "\nToo many {rule} violations ({count}).\nThe {rule} limit is {violations_limit}.".format( - rule=threshold_key, count=safelint_counts['rules'][threshold_key], - violations_limit=violation_thresholds['rules'][threshold_key], - ) - - if error_message is not "": - raise BuildFailure( - "SafeTemplateLinter Failed.\n{error_message}\n" - "See {safelint_report} or run the following command to hone in on the problem:\n" - " ./scripts/safe-commit-linter.sh -h".format( - error_message=error_message, safelint_report=safelint_report - ) - ) - - -@task -@needs('pavelib.prereqs.install_python_prereqs') -def run_safecommit_report(): - """ - Runs safe-commit-linter.sh on the current branch. - """ - safecommit_script = "safe-commit-linter.sh" - safecommit_report_dir = (Env.REPORT_DIR / "safecommit") - safecommit_report = safecommit_report_dir / "safecommit.report" - _prepare_report_dir(safecommit_report_dir) - - sh( - "{repo_root}/scripts/{safecommit_script} >> {safecommit_report}".format( - repo_root=Env.REPO_ROOT, - safecommit_script=safecommit_script, - safecommit_report=safecommit_report, - ), - ignore_error=True - ) - # Output report to console. - sh("cat {safecommit_report}".format(safecommit_report=safecommit_report), ignore_error=True) - - safecommit_count = _get_safecommit_count(safecommit_report) - - try: - num_violations = int(safecommit_count) - except TypeError: - raise BuildFailure( - "Error. Number of {safecommit_script} violations could not be found in {safecommit_report}".format( - safecommit_script=safecommit_script, safecommit_report=safecommit_report + # Fail if number of violations is greater than the limit + if num_violations > violations_limit > -1: + raise Exception( + "SafeTemplateLinter Failed. Too many violations ({count}).\nThe limit is {violations_limit}.".format( + count=num_violations, violations_limit=violations_limit ) ) - # Print number of violations to log. - violations_count_str = "Number of {safecommit_script} violations: {num_violations}\n".format( - safecommit_script=safecommit_script, num_violations=num_violations - ) - - # Record the metric - metrics_report = (Env.METRICS_DIR / "safecommit") - _write_metric(violations_count_str, metrics_report) - # Output report to console. - sh("cat {metrics_report}".format(metrics_report=metrics_report), ignore_error=True) - def _write_metric(metric, filename): """ @@ -468,28 +363,15 @@ def _prepare_report_dir(dir_name): dir_name.mkdir_p() -def _get_report_contents(filename, last_line_only=False): +def _get_last_report_line(filename): """ - Returns the contents of the given file. Use last_line_only to only return - the last line, which can be used for getting output from quality output - files. - - Arguments: - last_line_only: True to return the last line only, False to return a - string with full contents. - - Returns: - String containing full contents of the report, or the last line. - + Returns the last line of a given file. Used for getting output from quality output files. """ file_not_found_message = "The following log file could not be found: {file}".format(file=filename) if os.path.isfile(filename): with open(filename, 'r') as report_file: - if last_line_only: - lines = report_file.readlines() - return lines[len(lines) - 1] - else: - return report_file.read() + lines = report_file.readlines() + return lines[len(lines) - 1] else: # Raise a build error if the file is not found raise BuildFailure(file_not_found_message) @@ -500,7 +382,7 @@ def _get_count_from_last_line(filename, file_type): This will return the number in the last line of a file. It is returning only the value (as a floating number). """ - last_line = _get_report_contents(filename, last_line_only=True) + last_line = _get_last_report_line(filename) if file_type is "python_complexity": # Example of the last line of a complexity report: "Average complexity: A (1.93953443446)" regex = r'\d+.\d+' @@ -516,62 +398,6 @@ def _get_count_from_last_line(filename, file_type): return None -def _get_safelint_counts(filename): - """ - This returns a dict of violations from the safelint report. - - Arguments: - filename: The name of the safelint report. - - Returns: - A dict containing the following: - rules: A dict containing the count for each rule as follows: - violation-rule-id: N, where N is the number of violations - total: M, where M is the number of total violations - - """ - report_contents = _get_report_contents(filename) - rule_count_regex = re.compile(r"^(?P<rule_id>[a-z-]+):\s+(?P<count>\d+) violations", re.MULTILINE) - total_count_regex = re.compile(r"^(?P<count>\d+) violations total", re.MULTILINE) - violations = {'rules': {}} - for violation_match in rule_count_regex.finditer(report_contents): - try: - violations['rules'][violation_match.group('rule_id')] = int(violation_match.group('count')) - except ValueError: - violations['rules'][violation_match.group('rule_id')] = None - try: - violations['total'] = int(total_count_regex.search(report_contents).group('count')) - # An AttributeError will occur if the regex finds no matches. - # A ValueError will occur if the returned regex cannot be cast as a float. - except (AttributeError, ValueError): - violations['total'] = None - return violations - - -def _get_safecommit_count(filename): - """ - Returns the violation count from the safecommit report. - - Arguments: - filename: The name of the safecommit report. - - Returns: - The count of safecommit violations, or None if there is a problem. - - """ - report_contents = _get_report_contents(filename) - validation_count = None - file_count_regex = re.compile(r"^(?P<count>\d+) violations total", re.MULTILINE) - try: - for count_match in file_count_regex.finditer(report_contents): - if validation_count is None: - validation_count = 0 - validation_count += int(count_match.group('count')) - except ValueError: - validation_count = None - return validation_count - - @task @needs('pavelib.prereqs.install_python_prereqs') @cmdopts([ diff --git a/scripts/all-tests.sh b/scripts/all-tests.sh index 5d15696..4edb99b 100755 --- a/scripts/all-tests.sh +++ b/scripts/all-tests.sh @@ -13,7 +13,7 @@ set -e # Violations thresholds for failing the build export PYLINT_THRESHOLD=4175 export JSHINT_THRESHOLD=7550 -source scripts/safelint_thresholds.sh +export SAFELINT_THRESHOLD=2700 doCheckVars() { if [ -n "$CIRCLECI" ] ; then diff --git a/scripts/generic-ci-tests.sh b/scripts/generic-ci-tests.sh index 085fb16..64bb98f 100755 --- a/scripts/generic-ci-tests.sh +++ b/scripts/generic-ci-tests.sh @@ -85,9 +85,7 @@ case "$TEST_SUITE" in echo "Running code complexity report (python)." paver run_complexity > reports/code_complexity.log || echo "Unable to calculate code complexity. Ignoring error." echo "Running safe template linter report." - paver run_safelint -t $SAFELINT_THRESHOLDS > safelint.log || { cat safelint.log; EXIT=1; } - echo "Running safe commit linter report." - paver run_safecommit_report > safecommit.log || { cat safecommit.log; EXIT=1; } + paver run_safelint -l $SAFELINT_THRESHOLD > safelint.log || { cat safelint.log; EXIT=1; } # Run quality task. Pass in the 'fail-under' percentage to diff-quality echo "Running diff quality." paver run_quality -p 100 || EXIT=1 diff --git a/scripts/safelint_thresholds.sh b/scripts/safelint_thresholds.sh deleted file mode 100755 index 3662e6c..0000000 --- a/scripts/safelint_thresholds.sh +++ /dev/null @@ -1,47 +0,0 @@ -#!/usr/bin/env bash -set -e - -############################################################################### -# -# safelint_thresholds.sh -# -# The thresholds used for paver run_safelint when used with various CI -# systems. -# -############################################################################### - -# Violations thresholds for failing the build -export SAFELINT_THRESHOLDS=' - { - "rules": { - "javascript-concat-html": 313, - "javascript-escape": 7, - "javascript-interpolate": 71, - "javascript-jquery-append": 120, - "javascript-jquery-html": 313, - "javascript-jquery-insert-into-target": 26, - "javascript-jquery-insertion": 30, - "javascript-jquery-prepend": 12, - "mako-html-entities": 0, - "mako-invalid-html-filter": 33, - "mako-invalid-js-filter": 249, - "mako-js-html-string": 0, - "mako-js-missing-quotes": 0, - "mako-missing-default": 248, - "mako-multiple-page-tags": 0, - "mako-unknown-context": 0, - "mako-unparseable-expression": 0, - "mako-unwanted-html-filter": 0, - "python-close-before-format": 0, - "python-concat-html": 28, - "python-custom-escape": 13, - "python-deprecated-display-name": 53, - "python-interpolate-html": 68, - "python-parse-error": 0, - "python-requires-html-or-text": 0, - "python-wrap-html": 289, - "underscore-not-escaped": 709 - }, - "total": 2565 - }' -export SAFELINT_THRESHOLDS=${SAFELINT_THRESHOLDS//[[:space:]]/} -- libgit2 0.26.0