diff --git a/pavelib/paver_tests/test_paver_quality.py b/pavelib/paver_tests/test_paver_quality.py index 8e35861ac5..b9bfccd25c 100644 --- a/pavelib/paver_tests/test_paver_quality.py +++ b/pavelib/paver_tests/test_paver_quality.py @@ -56,7 +56,7 @@ class TestPaverQualityViolations(unittest.TestCase): def test_pep8_parser(self): with open(self.f.name, 'w') as f: f.write("hello\nhithere") - num, _violations = pavelib.quality._pep8_violations(f.name) # pylint: disable=protected-access + num = len(pavelib.quality._pep8_violations(f.name)) # pylint: disable=protected-access self.assertEqual(num, 2) @@ -97,16 +97,11 @@ class TestPaverReportViolationsCounts(unittest.TestCase): def setUp(self): super(TestPaverReportViolationsCounts, self).setUp() - # Mock the paver @needs decorator - self._mock_paver_needs = patch.object(pavelib.quality.run_quality, 'needs').start() - self._mock_paver_needs.return_value = 0 - # Temporary file infrastructure self.f = tempfile.NamedTemporaryFile(delete=False) self.f.close() # Cleanup various mocks and tempfiles - self.addCleanup(self._mock_paver_needs.stop) self.addCleanup(os.remove, self.f.name) def test_get_eslint_violations_count(self): @@ -294,12 +289,9 @@ class TestPaverRunQuality(unittest.TestCase): paver.tasks.environment = paver.tasks.Environment() # mock the @needs decorator to skip it - self._mock_paver_needs = patch.object(pavelib.quality.run_quality, 'needs').start() - self._mock_paver_needs.return_value = 0 patcher = patch('pavelib.quality.sh') self._mock_paver_sh = patcher.start() self.addCleanup(patcher.stop) - self.addCleanup(self._mock_paver_needs.stop) @patch('__builtin__.open', mock_open()) def test_failure_on_diffquality_pep8(self): @@ -326,9 +318,8 @@ class TestPaverRunQuality(unittest.TestCase): """ # Underlying sh call must fail when it is running the pylint diff-quality task - self._mock_paver_sh.side_effect = fail_on_pylint - _mock_pep8_violations = MagicMock(return_value=(0, [])) - with patch('pavelib.quality._get_pep8_violations', _mock_pep8_violations): + _mock_pylint_violations = MagicMock(return_value=(10000, ['some error'])) + with patch('pavelib.quality._get_pylint_violations', _mock_pylint_violations): with self.assertRaises(SystemExit): pavelib.quality.run_quality("") diff --git a/pavelib/quality.py b/pavelib/quality.py index 1c5de5cfca..e8b7bc64c8 100644 --- a/pavelib/quality.py +++ b/pavelib/quality.py @@ -74,32 +74,25 @@ def find_fixme(options): print "Number of pylint fixmes: " + str(num_fixme) -@task -@needs('pavelib.prereqs.install_python_prereqs') -@cmdopts([ - ("system=", "s", "System to act on"), - ("errors", "e", "Check for errors only"), - ("limit=", "l", "Limits for number of acceptable violations - either or :"), -]) -@timed -def run_pylint(options): +def _get_pylint_violations(systems=ALL_SYSTEMS.split(','), errors_only=False): """ - Run pylint on system code. When violations limit is passed in, - fail the task if too many violations are found. + Runs pylint. Returns a tuple of (number_of_violations, list_of_violations) + where list_of_violations is a list of all pylint violations found, separated + by new lines. """ - lower_violations_limit, upper_violations_limit, errors, systems = _parse_pylint_options(options) - # Make sure the metrics subdirectory exists Env.METRICS_DIR.makedirs_p() num_violations = 0 + violations_list = [] + for system in systems: # Directory to put the pylint report in. # This makes the folder if it doesn't already exist. report_dir = (Env.REPORT_DIR / system).makedirs_p() flags = [] - if errors: + if errors_only: flags.append("--errors-only") apps_list = ' '.join(top_python_dirs(system)) @@ -114,8 +107,33 @@ def run_pylint(options): ignore_error=True, ) - num_violations += _count_pylint_violations( - "{report_dir}/pylint.report".format(report_dir=report_dir)) + report = "{report_dir}/pylint.report".format(report_dir=report_dir) + num_violations += _count_pylint_violations(report) + with open(report) as report_contents: + violations_list.extend(report_contents) + + # Print number of violations to log + return num_violations, violations_list + + +@task +@needs('pavelib.prereqs.install_python_prereqs') +@cmdopts([ + ("system=", "s", "System to act on"), + ("errors", "e", "Check for errors only"), + ("limit=", "l", "Limits for number of acceptable violations - either or :"), +]) +@timed +def run_pylint(options): + """ + Run pylint on system code. When violations limit is passed in, + fail the task if too many violations are found. + """ + lower_violations_limit, upper_violations_limit, errors, systems = _parse_pylint_options(options) + errors = getattr(options, 'errors', False) + systems = getattr(options, 'system', ALL_SYSTEMS).split(',') + + num_violations, violations_list = _get_pylint_violations(systems, errors) # Print number of violations to log violations_count_str = "Number of pylint violations: " + str(num_violations) @@ -200,22 +218,19 @@ def _get_pep8_violations(): sh('pep8 . | tee {report_dir}/pep8.report -a'.format(report_dir=report_dir)) - count, violations_list = _pep8_violations( + violations_list = _pep8_violations( "{report_dir}/pep8.report".format(report_dir=report_dir) ) - return count, violations_list + return (len(violations_list), violations_list) def _pep8_violations(report_file): """ - Returns a tuple of (num_violations, violations_list) for all - pep8 violations in the given report_file. + Returns the list of all pep8 violations in the given report_file. """ with open(report_file) as f: - violations_list = f.readlines() - num_lines = len(violations_list) - return num_lines, violations_list + return f.readlines() @task @@ -686,7 +701,7 @@ def run_quality(options): # Save the pass variable. It will be set to false later if failures are detected. diff_quality_percentage_pass = True - def _pep8_output(count, violations_list, is_html=False): + def _lint_output(linter, count, violations_list, is_html=False, limit=0): """ Given a count & list of pep8 violations, pretty-print the pep8 output. If `is_html`, will print out with HTML markup. @@ -694,26 +709,26 @@ def run_quality(options): if is_html: lines = ['\n'] sep = '-------------
\n' - title = "

Quality Report: pep8

\n" + title = HTML("

Quality Report: {}

\n").format(linter) violations_bullets = ''.join( [HTML('
  • {violation}

    • \n').format(violation=violation) for violation in violations_list] ) violations_str = HTML('
      \n{bullets}
    \n').format(bullets=HTML(violations_bullets)) - violations_count_str = "Violations: {count}
    \n" - fail_line = "FAILURE: pep8 count should be 0
    \n" + violations_count_str = HTML("Violations: {count}
    \n") + fail_line = HTML("FAILURE: {} count should be 0
    \n").format(linter) else: lines = [] sep = '-------------\n' - title = "Quality Report: pep8\n" + title = "Quality Report: {}\n".format(linter) violations_str = ''.join(violations_list) violations_count_str = "Violations: {count}\n" - fail_line = "FAILURE: pep8 count should be 0\n" + fail_line = "FAILURE: {} count should be {}\n".format(linter, limit) violations_count_str = violations_count_str.format(count=count) lines.extend([sep, title, sep, violations_str, sep, violations_count_str]) - if count > 0: + if count > limit: lines.append(fail_line) lines.append(sep + '\n') if is_html: @@ -725,15 +740,31 @@ def run_quality(options): (count, violations_list) = _get_pep8_violations() # Print number of violations to log - print _pep8_output(count, violations_list) + 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(_pep8_output(count, violations_list, is_html=True)) + 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() + + # Print number of violations to log + print _lint_output('pylint', count, violations_list, limit=6100) + + # 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=6100)) + + if count > 6100: + diff_quality_percentage_pass = False + # ----- Set up for diff-quality pylint call ----- # Set the string, if needed, to be used for the diff-quality --compare-branch switch. compare_branch = getattr(options, 'compare_branch', None) @@ -747,26 +778,9 @@ def run_quality(options): if diff_threshold > -1: percentage_string = u'--fail-under={0}'.format(diff_threshold) - # Generate diff-quality html report for pylint, and print to console - # If pylint reports exist, use those - # Otherwise, `diff-quality` will call pylint itself - - pylint_files = get_violations_reports("pylint") - pylint_reports = u' '.join(pylint_files) - eslint_files = get_violations_reports("eslint") eslint_reports = u' '.join(eslint_files) - # run diff-quality for pylint. - 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_percentage_pass = False - # run diff-quality for eslint. if not run_diff_quality( violations_type="eslint", diff --git a/scripts/all-tests.sh b/scripts/all-tests.sh index 8de5bdf801..88c0b7df2a 100755 --- a/scripts/all-tests.sh +++ b/scripts/all-tests.sh @@ -12,7 +12,7 @@ set -e # Violations thresholds for failing the build export LOWER_PYLINT_THRESHOLD=1000 -export UPPER_PYLINT_THRESHOLD=5335 +export UPPER_PYLINT_THRESHOLD=6200 export ESLINT_THRESHOLD=9134 export STYLELINT_THRESHOLD=973