From eba76109c494caf27986fbb40bdd62f735ce5acd Mon Sep 17 00:00:00 2001 From: Rebecca Graber Date: Tue, 8 Mar 2022 14:18:45 -0500 Subject: [PATCH] feat: remove xss-commitlint test (#30025) --- pavelib/paver_tests/test_paver_quality.py | 48 +----------- pavelib/paver_tests/test_xsscommitlint.py | 46 ----------- pavelib/quality.py | 83 -------------------- scripts/generic-ci-tests.sh | 6 -- scripts/xss-commit-linter.sh | 93 ----------------------- 5 files changed, 1 insertion(+), 275 deletions(-) delete mode 100644 pavelib/paver_tests/test_xsscommitlint.py delete mode 100755 scripts/xss-commit-linter.sh diff --git a/pavelib/paver_tests/test_paver_quality.py b/pavelib/paver_tests/test_paver_quality.py index 1b740d07b3..b2a91a3475 100644 --- a/pavelib/paver_tests/test_paver_quality.py +++ b/pavelib/paver_tests/test_paver_quality.py @@ -67,7 +67,7 @@ class TestPaverQualityViolations(unittest.TestCase): class TestPaverReportViolationsCounts(unittest.TestCase): """ For testing utility functions for getting counts from reports for - run_eslint, run_xsslint, and run_xsscommitlint. + run_eslint and run_xsslint. """ def setUp(self): @@ -158,52 +158,6 @@ class TestPaverReportViolationsCounts(unittest.TestCase): 'total': None, }) - def test_get_xsscommitlint_count_happy(self): - """ - Test happy path getting violation count from xsscommitlint report. - """ - 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_xsscommitlint_count(self.f.name) # pylint: disable=protected-access - - assert count == 5 - - def test_get_xsscommitlint_count_bad_counts(self): - """ - Test getting violation count from truncated xsscommitlint report. - """ - report = textwrap.dedent(""" - Linting lms/templates/navigation.html: - """) - with open(self.f.name, 'w') as f: - f.write(report) - count = pavelib.quality._get_xsscommitlint_count(self.f.name) # pylint: disable=protected-access - - assert count is None - - def test_get_xsscommitlint_count_no_files(self): - """ - Test getting violation count from xsscommitlint report where no files were - linted. - """ - report = textwrap.dedent(""" - No files linted. - """) - with open(self.f.name, 'w') as f: - f.write(report) - count = pavelib.quality._get_xsscommitlint_count(self.f.name) # pylint: disable=protected-access - - assert count == 0 - class TestPrepareReportDir(unittest.TestCase): """ diff --git a/pavelib/paver_tests/test_xsscommitlint.py b/pavelib/paver_tests/test_xsscommitlint.py deleted file mode 100644 index 47697ee740..0000000000 --- a/pavelib/paver_tests/test_xsscommitlint.py +++ /dev/null @@ -1,46 +0,0 @@ -""" -Tests for paver xsscommitlint quality tasks -""" -from unittest.mock import patch - -import pytest -from paver.easy import call_task - -import pavelib.quality - -from .utils import PaverTestCase - - -class PaverXSSCommitLintTest(PaverTestCase): - """ - Test run_xsscommitlint with a mocked environment in order to pass in - opts. - - """ - - def setUp(self): - super().setUp() - self.reset_task_messages() - - @patch.object(pavelib.quality, '_write_metric') - @patch.object(pavelib.quality, '_prepare_report_dir') - @patch.object(pavelib.quality, '_get_xsscommitlint_count') - def test_xsscommitlint_violation_number_not_found(self, _mock_count, _mock_report_dir, _mock_write_metric): - """ - run_xsscommitlint encounters an error parsing the xsscommitlint output - log. - - """ - _mock_count.return_value = None - with pytest.raises(SystemExit): - call_task('pavelib.quality.run_xsscommitlint') - - @patch.object(pavelib.quality, '_write_metric') - @patch.object(pavelib.quality, '_prepare_report_dir') - @patch.object(pavelib.quality, '_get_xsscommitlint_count') - def test_xsscommitlint_vanilla(self, _mock_count, _mock_report_dir, _mock_write_metric): - """ - run_xsscommitlint finds violations. - """ - _mock_count.return_value = 0 - call_task('pavelib.quality.run_xsscommitlint') diff --git a/pavelib/quality.py b/pavelib/quality.py index 00c5ab76cc..1a2d83a4fa 100644 --- a/pavelib/quality.py +++ b/pavelib/quality.py @@ -505,61 +505,6 @@ def run_xsslint(options): write_junit_xml('xsslint') -@task -@needs('pavelib.prereqs.install_python_prereqs') -@timed -def run_xsscommitlint(): - """ - Runs xss-commit-linter.sh on the current branch. - """ - xsscommitlint_script = "xss-commit-linter.sh" - xsscommitlint_report_dir = (Env.REPORT_DIR / "xsscommitlint") - xsscommitlint_report = xsscommitlint_report_dir / "xsscommitlint.report" - _prepare_report_dir(xsscommitlint_report_dir) - - sh( - "{repo_root}/scripts/{xsscommitlint_script} -v | tee {xsscommitlint_report}".format( - repo_root=Env.REPO_ROOT, - xsscommitlint_script=xsscommitlint_script, - xsscommitlint_report=xsscommitlint_report, - ), - ignore_error=True - ) - - xsscommitlint_count = _get_xsscommitlint_count(xsscommitlint_report) - - try: - num_violations = int(xsscommitlint_count) - except TypeError: - fail_quality( - 'xsscommitlint', - "FAILURE: Number of {xsscommitlint_script} violations could not be found in {xsscommitlint_report}".format( - xsscommitlint_script=xsscommitlint_script, xsscommitlint_report=xsscommitlint_report - ) - ) - - # Print number of violations to log. - violations_count_str = "Number of {xsscommitlint_script} violations: {num_violations}\n".format( - xsscommitlint_script=xsscommitlint_script, num_violations=num_violations - ) - - # Record the metric - metrics_report = (Env.METRICS_DIR / "xsscommitlint") - _write_metric(violations_count_str, metrics_report) - # Output report to console. - sh(f"cat {metrics_report}", ignore_error=True) - if num_violations: - fail_quality( - 'xsscommitlint', - "FAILURE: XSSCommitLinter Failed.\n{error_message}\n" - "See {xsscommitlint_report} or run the following command to hone in on the problem:\n" - " ./scripts/xss-commit-linter.sh -h".format( - error_message=violations_count_str, xsscommitlint_report=xsscommitlint_report - ) - ) - write_junit_xml("xsscommitlint") - - def _write_metric(metric, filename): """ Write a given metric to a given file @@ -663,34 +608,6 @@ def _get_xsslint_counts(filename): return violations -def _get_xsscommitlint_count(filename): - """ - Returns the violation count from the xsscommitlint report. - - Arguments: - filename: The name of the xsscommitlint report. - - Returns: - The count of xsscommitlint violations, or None if there is a problem. - - """ - report_contents = _get_report_contents(filename, 'xsscommitlint') - - if 'No files linted' in report_contents: - return 0 - - file_count_regex = re.compile(r"^(?P\d+) violations total", re.MULTILINE) - try: - validation_count = None - 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')) - return validation_count - except ValueError: - return None - - def _extract_missing_pii_annotations(filename): """ Returns the number of uncovered models from the stdout report of django_find_annotations. diff --git a/scripts/generic-ci-tests.sh b/scripts/generic-ci-tests.sh index 0643c93ac3..c850ec96b5 100755 --- a/scripts/generic-ci-tests.sh +++ b/scripts/generic-ci-tests.sh @@ -133,12 +133,6 @@ case "$TEST_SUITE" in run_paver_quality run_stylelint || { EXIT=1; } echo "Running xss linter report." run_paver_quality run_xsslint -t $XSSLINT_THRESHOLDS || { EXIT=1; } - if [[ ! -z "$TARGET_BRANCH" ]]; then - echo "Running safe commit linter report." - run_paver_quality run_xsscommitlint || { EXIT=1; } - else - echo "Skipping safe commit linter report." - fi echo "Running PII checker on all Django models..." run_paver_quality run_pii_check || { EXIT=1; } echo "Running reserved keyword checker on all Django models..." diff --git a/scripts/xss-commit-linter.sh b/scripts/xss-commit-linter.sh deleted file mode 100755 index d6d144497c..0000000000 --- a/scripts/xss-commit-linter.sh +++ /dev/null @@ -1,93 +0,0 @@ -#!/usr/bin/env bash -set -e - -############################################################################### -# -# xss-commit-linter.sh -# -# Executes xsslint/xss_linter.py on the set of files in a particular git commit. -# -############################################################################### - -show_help() { - echo "Usage: xss-commit-linter.sh [OPTION]" - echo "Runs the XSS Linter against all files in a git commit." - echo "" - echo "Mandatory arguments to long options are mandatory for short options too." - echo " -h, --help Output this help." - echo " -m, --main-branch=COMMIT Run against files changed between the" - echo " current branch and this commit." - echo " Defaults to origin/master." - echo " -v, --verbose Output details of git commands run." - echo "" - echo "This scripts does not give a grand total. Be sure to check for" - echo "0 violations on each file." - echo "" - echo "For more help using the xss linter, including details on how to" - echo "understand and fix any violations, read the docs here:" - echo "" - echo " https://edx.readthedocs.io/projects/edx-developer-guide/en/latest/preventing_xss/index.html" - -} - -show_verbose() { - echo "Files linted is based on the following:" - echo "- Current commit: ${current_branch_hash}" - echo "- Main commit: ${MAIN_COMMIT}" - echo "- Target branch: ${TARGET_BRANCH}" - echo "- Merge base command: ${merge_base_command}" - echo "- Merge base: ${merge_base}" - echo "- Diff command: ${diff_command}" - -} - -for i in "$@"; do - case $i in - -m=*|--main-branch=*) - MAIN_COMMIT="${i#*=}" - shift # past argument=value - ;; - -v|--verbose) - show_verbose - ;; - -h|--help|*) - # help or unknown option - show_help - exit 0 - ;; - esac -done - -current_branch_hash=`git rev-parse HEAD` - -if [ -z "${MAIN_COMMIT+x}" ]; then - if [ -z ${TARGET_BRANCH+x} ]; then - # if commit is not set and no target branch, get hash of current branch - MAIN_COMMIT="origin/master" - else - if [[ $TARGET_BRANCH == origin/* ]]; then - MAIN_COMMIT=$TARGET_BRANCH - else - MAIN_COMMIT=origin/$TARGET_BRANCH - fi - fi -fi - -merge_base_command="git merge-base $current_branch_hash $MAIN_COMMIT" -merge_base=$(${merge_base_command}) -diff_command="git diff --name-only --diff-filter=ACM $merge_base $current_branch_hash" -diff_files=$(${diff_command}) - -if [ "$diff_files" = "" ]; then - # When no files are found, automatically display verbose details to help - # understand why. - show_verbose - echo "" - echo "No files linted." -else - for f in $diff_files; do - echo "" - echo "Linting $f:" - ./scripts/xsslint/xss_linter.py --config=scripts.xsslint_config $f - done -fi