Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

BOM-2543: Remove the diff-quality step from quality checks (#27619)

Browse Source 
* build: Removed the diff-quality step
Applied lint-amnesty on all the warnings
Removed pylint thresholds comparison code and related tests

Co-authored-by: Usama Sadiq <usama.sadiq@arbisoft.com>
This commit is contained in:
Aarif
2021-06-07 18:37:06 +05:00
committed by GitHub
parent f497f0d103
commit 58a54f8d0b
8 changed files with 44 additions and 357 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
openedx/core/djangoapps/credentials/management/commands/tests/test_notify_credentials.py
View File

@@ -1,4 +1,4 @@
"""
""" # lint-amnesty, pylint: disable=django-not-configured
Tests the ``notify_credentials`` management command.
"""

10
openedx/core/djangoapps/util/tests/test_update_fixtures.py
View File

@@ -1,4 +1,4 @@
"""
""" # lint-amnesty, pylint: disable=django-not-configured
Tests of the update_fixtures management command for bok-choy test database
initialization.
"""
@@ -12,19 +12,19 @@ from django.core.management import call_command
@pytest.fixture(scope='function')
def sites(db):
def sites(db): # lint-amnesty, pylint: disable=unused-argument
Site.objects.create(name='cms', domain='localhost:8031')
Site.objects.create(name='lms', domain='localhost:8003')
def test_localhost(db, monkeypatch, sites):
def test_localhost(db, monkeypatch, sites): # lint-amnesty, pylint: disable=redefined-outer-name, unused-argument
monkeypatch.delitem(os.environ, 'BOK_CHOY_HOSTNAME', raising=False)
call_command('update_fixtures')
assert Site.objects.get(name='cms').domain == 'localhost:8031'
assert Site.objects.get(name='lms').domain == 'localhost:8003'
def test_devstack_cms(db, monkeypatch, sites):
def test_devstack_cms(db, monkeypatch, sites): # lint-amnesty, pylint: disable=redefined-outer-name, unused-argument
monkeypatch.setitem(os.environ, 'BOK_CHOY_HOSTNAME', 'edx.devstack.cms')
monkeypatch.setitem(os.environ, 'BOK_CHOY_CMS_PORT', '18031')
monkeypatch.setitem(os.environ, 'BOK_CHOY_LMS_PORT', '18003')
@@ -33,7 +33,7 @@ def test_devstack_cms(db, monkeypatch, sites):
assert Site.objects.get(name='lms').domain == 'edx.devstack.cms:18003'
def test_devstack_lms(db, monkeypatch, sites):
def test_devstack_lms(db, monkeypatch, sites): # lint-amnesty, pylint: disable=redefined-outer-name, unused-argument
monkeypatch.setitem(os.environ, 'BOK_CHOY_HOSTNAME', 'edx.devstack.lms')
monkeypatch.setitem(os.environ, 'BOK_CHOY_CMS_PORT', '18031')
monkeypatch.setitem(os.environ, 'BOK_CHOY_LMS_PORT', '18003')

159
pavelib/paver_tests/test_paver_quality.py
View File

@@ -1,22 +1,22 @@
"""
""" # lint-amnesty, pylint: disable=django-not-configured
Tests for paver quality tasks
"""
import os
import shutil
import shutil # lint-amnesty, pylint: disable=unused-import
import tempfile
import textwrap
import unittest
from unittest.mock import MagicMock, mock_open, patch
from unittest.mock import MagicMock, mock_open, patch # lint-amnesty, pylint: disable=unused-import
import pytest
from ddt import data, ddt, file_data, unpack
import pytest # lint-amnesty, pylint: disable=unused-import
from ddt import data, ddt, file_data, unpack # lint-amnesty, pylint: disable=unused-import
from path import Path as path
from paver.easy import BuildFailure
from paver.easy import BuildFailure # lint-amnesty, pylint: disable=unused-import
import pavelib.quality
from pavelib.paver_tests.utils import PaverTestCase, fail_on_eslint
from pavelib.paver_tests.utils import PaverTestCase, fail_on_eslint # lint-amnesty, pylint: disable=unused-import
OPEN_BUILTIN = 'builtins.open'
@@ -64,34 +64,6 @@ class TestPaverQualityViolations(unittest.TestCase):
assert num == 2
@ddt
class TestPaverQualityOptions(unittest.TestCase):
"""
Tests the paver pylint command-line options parsing.
"""
@data(
({'limit': '5500'}, (-1, 5500, False, pavelib.quality.ALL_SYSTEMS.split(','))),
({'limit': '1000:5500'}, (1000, 5500, False, pavelib.quality.ALL_SYSTEMS.split(','))),
({'limit': '1:2:3:4:5'}, (1, 2, False, pavelib.quality.ALL_SYSTEMS.split(','))),
({'system': 'lms,cms'}, (-1, -1, False, ['lms', 'cms'])),
(
{'limit': '2000:5000', 'errors': True, 'system': 'lms,cms,openedx'},
(2000, 5000, True, ['lms', 'cms', 'openedx'])
),
)
@unpack
def test_pylint_parser_other_string(self, options, expected_values):
class PaverOptions:
"""
Simple options class to mimick paver's Namespace object.
"""
def __init__(self, d):
self.__dict__ = d
paver_options = PaverOptions(options)
returned_values = pavelib.quality._parse_pylint_options(paver_options) # pylint: disable=protected-access
assert returned_values == expected_values
class TestPaverReportViolationsCounts(unittest.TestCase):
"""
For testing utility functions for getting counts from reports for
@@ -253,120 +225,3 @@ class TestPrepareReportDir(unittest.TestCase):
os.remove(self.test_file.name)
pavelib.quality._prepare_report_dir(path(self.test_dir)) # pylint: disable=protected-access
assert os.listdir(path(self.test_dir)) == []
class TestPaverRunQuality(PaverTestCase):
"""
For testing the paver run_quality task
"""
def setUp(self):
super().setUp()
# mock the @needs decorator to skip it
patcher = patch('pavelib.quality.sh')
self._mock_paver_sh = patcher.start()
self.addCleanup(patcher.stop)
self.report_dir = tempfile.mkdtemp()
report_dir_patcher = patch('pavelib.utils.envs.Env.REPORT_DIR', path(self.report_dir))
report_dir_patcher.start()
self.addCleanup(shutil.rmtree, self.report_dir)
self.addCleanup(report_dir_patcher.stop)
@patch(OPEN_BUILTIN, mock_open())
def test_failure_on_diffquality_pylint(self):
"""
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
_mock_pylint_violations = MagicMock(return_value=(10000, ['some error']))
with patch('pavelib.quality._get_pylint_violations', _mock_pylint_violations):
with patch('pavelib.quality._parse_pylint_options', return_value=(0, 1000, 0, 0)):
with pytest.raises(SystemExit):
pavelib.quality.run_quality("")
# Assert that _get_pylint_violations (which calls "pylint") is called once
assert _mock_pylint_violations.call_count == 1
# 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.
assert self._mock_paver_sh.call_count == 2
@patch(OPEN_BUILTIN, mock_open())
def test_failure_on_diffquality_eslint(self):
"""
If diff-quality fails on eslint, the paver task should also fail
"""
# Underlying sh call must fail when it is running the eslint diff-quality task
self._mock_paver_sh.side_effect = fail_on_eslint
_mock_pylint_violations = MagicMock(return_value=(0, []))
with patch('pavelib.quality._get_pylint_violations', _mock_pylint_violations):
with pytest.raises(SystemExit):
pavelib.quality.run_quality("")
print(self._mock_paver_sh.mock_calls)
# Test that pylint is called
_mock_pylint_violations.assert_called_once_with(clean=False)
# Assert that sh was called four times - once to get the comparison commit hash,
# once to get the current commit hash, once for diff quality with pylint,
# and once for diff quality with eslint
assert self._mock_paver_sh.call_count == 4
@patch(OPEN_BUILTIN, mock_open())
def test_other_exception(self):
"""
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]
with pytest.raises(SystemExit):
pavelib.quality.run_quality("")
# Test that pylint is NOT called by counting calls
assert self._mock_paver_sh.call_count == 1
@patch(OPEN_BUILTIN, mock_open())
def test_no_diff_quality_failures(self):
# Assert nothing is raised
pavelib.quality.run_quality("")
# And assert that sh was called 8 times:
# 6 for pylint on each of the system directories
# 1 for diff_quality for pylint
# 1 for diff_quality for eslint
assert 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().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')]
assert pavelib.quality.run_diff_quality('') is 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("")

183
pavelib/quality.py
View File

@@ -1,4 +1,4 @@
"""
""" # lint-amnesty, pylint: disable=django-not-configured
Check code quality using pycodestyle, pylint, and diff_quality.
"""
@@ -71,6 +71,14 @@ def top_python_dirs(dirname):
dirs = os.listdir(subdir)
top_dirs.extend(d for d in dirs if os.path.isdir(os.path.join(subdir, d)))
# sandbox-packages module causes F0001: module-not-found error when running pylint
# this will exclude sandbox-packages module from pylint execution
# TODO: upgrade the functionality to run pylint tests on sandbox-packages module too.
modules_to_remove = ['sandbox-packages', '__pycache__']
for module in modules_to_remove:
if module in top_dirs:
top_dirs.remove(module)
return top_dirs
@@ -161,7 +169,6 @@ def _get_pylint_violations(systems=ALL_SYSTEMS.split(','), errors_only=False, cl
@cmdopts([
("system=", "s", "System to act on"),
("errors", "e", "Check for errors only"),
("limit=", "l", "Limits for number of acceptable violations - either <upper> or <lower>:<upper>"),
])
@timed
def run_pylint(options):
@@ -169,12 +176,11 @@ 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(',')
result_name = 'pylint_{}'.format('_'.join(systems))
num_violations, _ = _get_pylint_violations(systems, errors)
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)
@@ -184,51 +190,16 @@ def run_pylint(options):
with open(Env.METRICS_DIR / "pylint", "w") as f:
f.write(violations_count_str)
# Fail when number of violations is less than the lower limit,
# which likely means that pylint did not run successfully.
# If pylint *did* run successfully, then great! Modify the lower limit.
if num_violations < lower_violations_limit > -1:
fail_quality(
result_name,
"FAILURE: Too few pylint violations. "
"Expected to see at least {lower_limit} pylint violations. "
"Either pylint is not running correctly -or- "
"the limits should be lowered and/or the lower limit should be removed.".format(
lower_limit=lower_violations_limit
)
)
# Fail when number of violations is greater than the upper limit.
if num_violations > upper_violations_limit > -1:
fail_quality(
result_name,
"FAILURE: Too many pylint violations. "
"The limit is {upper_limit}.".format(upper_limit=upper_violations_limit)
)
# Fail if there are violations found in pylint report.
if num_violations > 0:
failure_message = "FAILURE: Pylint violations found.\n"
for violation in violations_list:
failure_message += violation # lint-amnesty, pylint: disable=consider-using-join
fail_quality(result_name, failure_message)
else:
write_junit_xml(result_name)
def _parse_pylint_options(options):
"""
Parse the options passed to run_pylint.
"""
lower_violations_limit = upper_violations_limit = -1
violations_limit = getattr(options, 'limit', '').split(':')
if violations_limit[0]:
# Limit was specified.
if len(violations_limit) == 1:
# Only upper limit was specified.
upper_violations_limit = int(violations_limit[0])
else:
# Upper and lower limits were both specified.
lower_violations_limit = int(violations_limit[0])
upper_violations_limit = int(violations_limit[1])
errors = getattr(options, 'errors', False)
systems = getattr(options, 'system', ALL_SYSTEMS).split(',')
return lower_violations_limit, upper_violations_limit, errors, systems
def _count_pylint_violations(report_file):
"""
Parses a pylint report line-by-line and determines the number of violations reported
@@ -859,30 +830,15 @@ def check_keywords():
@task
@needs('pavelib.prereqs.install_python_prereqs')
@cmdopts([
("compare-branch=", "b", "Branch to compare against, defaults to origin/master"),
("percentage=", "p", "fail if diff-quality is below this percentage"),
("limit=", "l", "Limits for number of acceptable violations - either <upper> or <lower>:<upper>"),
])
@timed
# pylint: disable=too-many-statements
def run_quality(options):
def run_quality():
"""
Build the html diff quality reports, and print the reports to the console.
:param: b, the branch to compare against, defaults to origin/master
: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.
Build the html quality reports, and print the reports to the console.
"""
# 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_pass = True
failure_reasons = []
def _lint_output(linter, count, violations_list, is_html=False, limit=0):
def _lint_output(linter, count, violations_list, is_html=False):
"""
Given a count & list of pylint violations, pretty-print the output.
If `is_html`, will print out with HTML markup.
@@ -903,13 +859,13 @@ def run_quality(options):
title = f"Quality Report: {linter}\n"
violations_str = ''.join(violations_list)
violations_count_str = "Violations: {count}\n"
fail_line = f"FAILURE: {linter} count should be {limit}\n"
fail_line = f"FAILURE: {linter} count should be 0\n"
violations_count_str = violations_count_str.format(count=count)
lines.extend([sep, title, sep, violations_str, sep, violations_count_str])
if count > limit > -1:
if count > 0:
lines.append(fail_line)
lines.append(sep + '\n')
if is_html:
@@ -917,103 +873,14 @@ def run_quality(options):
return ''.join(lines)
# 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 total number of violations to log
print(_lint_output('pylint', count, violations_list, limit=upper_violations_limit))
if count > upper_violations_limit > -1:
diff_quality_pass = False
failure_reasons.append('Too many total violations.')
# ----- Set up for diff-quality pylint call -----
# Set the string to be used for the diff-quality --compare-branch switch.
compare_branch = getattr(options, 'compare_branch', 'origin/master')
compare_commit = sh(f'git merge-base HEAD {compare_branch}', capture=True).strip()
if sh('git rev-parse HEAD', capture=True).strip() != compare_commit:
compare_branch_string = f'--compare-branch={compare_commit}'
# Set the string, if needed, to be used for the diff-quality --fail-under switch.
diff_threshold = int(getattr(options, 'percentage', -1))
percentage_string = ''
if diff_threshold > -1:
percentage_string = f'--fail-under={diff_threshold}'
pylint_files = get_violations_reports("pylint")
pylint_reports = ' '.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 = ' '.join(eslint_files)
if not run_diff_quality(
violations_type="eslint",
reports=eslint_reports,
percentage_string=percentage_string,
branch_string=compare_branch_string,
dquality_dir=dquality_dir
):
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_pass:
print(_lint_output('pylint', count, violations_list))
if count > 0:
failure_reasons.append('Too many total pylint violations.')
msg = "FAILURE: " + " ".join(failure_reasons)
fail_quality('diff_quality', msg)
else:
write_junit_xml('diff_quality')
def run_diff_quality(
violations_type=None, reports=None, percentage_string=None, branch_string=None, dquality_dir=None
):
"""
This executes the diff-quality commandline tool for the given violation type (e.g., pylint, eslint).
If diff-quality fails due to quality issues, this method returns False.
"""
try:
sh(
"diff-quality --violations={type} "
"{reports} {percentage_string} {compare_branch_string} "
"--html-report {dquality_dir}/diff_quality_{type}.html ".format(
type=violations_type,
reports=reports,
percentage_string=percentage_string,
compare_branch_string=branch_string,
dquality_dir=dquality_dir,
)
)
return True
except BuildFailure as failure:
if is_percentage_failure(failure.args):
return False
else:
fail_quality(
'diff_quality',
f'FAILURE: See "Diff Quality Report" in Jenkins left-sidebar for details. {failure}'
)
def is_percentage_failure(error_message):
"""
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. If the subprocess exits with anything other than 1, raise
a paver exception.
"""
if "Subprocess return code: 1" not in error_message:
return False
else:
return True
fail_quality('Quality Failure', msg)
def get_violations_reports(violations_type):

5
pylintrc
View File

@@ -67,7 +67,7 @@
# SERIOUSLY.
#
# ------------------------------
# Generated by edx-lint version: 4.0.1
# Generated by edx-lint version: 5.0.0
# ------------------------------
[MASTER]
ignore = ,.git,.tox,migrations,node_modules,.pycharm_helpers
@@ -248,6 +248,7 @@ enable =
global-at-module-level,
global-variable-not-assigned,
literal-used-as-attribute,
logging-format-interpolation,
logging-not-lazy,
metaclass-assignment,
model-has-unicode,
@@ -492,4 +493,4 @@ int-import-graph =
[EXCEPTIONS]
overgeneral-exceptions = Exception
# 63eee6a406604bde1c7e8e5cbdddcd7f173ccdef
# c5312bad86f927500a51e6aba50677c3c19598d4

33
scripts/Jenkinsfiles/quality
View File

@@ -73,7 +73,7 @@ pipeline {
}
stage('Run Tests') {
parallel {
stage("commonlib pylint") {
stage("common pylint") {
// "pylint common" requires 5.5 GB of RAM, so use js-worker (8 GB) instead of jenkins-worker (4 GB)
agent { label "js-worker" }
environment {
@@ -153,37 +153,6 @@ pipeline {
}
}
}
stage('Diff quality') {
when {
// Only run diff quality on PR builds
expression { env.ghprbTargetBranch != null }
}
environment {
TARGET_BRANCH = "origin/${ghprbTargetBranch}"
}
steps {
sshagent(credentials: ['jenkins-worker'], ignoreMissing: true) {
checkout changelog: false, poll: false, scm: [$class: 'GitSCM', branches: [[name: "${ghprbActualCommit}"]],
doGenerateSubmoduleConfigurations: false, extensions: [[$class: 'CloneOption',
honorRefspec: true, noTags: true, shallow: false], [$class: 'WipeWorkspace']], submoduleCfg: [],
userRemoteConfigs: [[credentialsId: 'jenkins-worker',
refspec: "+refs/heads/${ghprbTargetBranch}:refs/remotes/origin/${ghprbTargetBranch} +refs/pull/${ghprbPullId}/*:refs/remotes/origin/pr/${ghprbPullId}/*",
url: "git@github.com:edx/${REPO_NAME}.git"]]]
unstash 'quality-1-reports'
unstash 'quality-2-reports'
unstash 'quality-3-reports'
unstash 'quality-4-reports'
sh "./scripts/jenkins-quality-diff.sh"
}
}
post {
always {
qualityTestCleanup()
publishHTML([allowMissing: true, alwaysLinkToLastBuild: false, keepAll: true, reportDir: 'reports/diff_quality',
reportFiles: 'diff_quality_pylint.html, diff_quality_eslint.html', reportName: 'Diff Quality Report', reportTitles: ''])
}
}
}
}
post {
always {

7
scripts/jenkins-quality-diff.sh → scripts/jenkins-quality-report.sh
View File

@@ -6,11 +6,8 @@ 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."
echo "Generating quality report."
mkdir -p test_root/log/
LOG_PREFIX=test_root/log/run_quality
if [[ $TARGET_BRANCH != origin/* ]]; then
TARGET_BRANCH=origin/$TARGET_BRANCH
fi
paver run_quality -b $TARGET_BRANCH -p 100 -l $LOWER_PYLINT_THRESHOLD:$UPPER_PYLINT_THRESHOLD 2> $LOG_PREFIX.err.log > $LOG_PREFIX.out.log
paver run_quality 2> $LOG_PREFIX.err.log > $LOG_PREFIX.out.log

2
scripts/thresholds.sh
View File

@@ -1,7 +1,5 @@
#!/usr/bin/env bash
set -e
export LOWER_PYLINT_THRESHOLD=0
export UPPER_PYLINT_THRESHOLD=1
export ESLINT_THRESHOLD=5300
export STYLELINT_THRESHOLD=880