diff --git a/.jshintignore b/.jshintignore new file mode 100644 index 0000000000..91523ae0d2 --- /dev/null +++ b/.jshintignore @@ -0,0 +1,2 @@ +common/static/js/vendor +lms/static/js/vendor diff --git a/docs/en_us/internal/testing.rst b/docs/en_us/internal/testing.rst index 1b644b62e8..6518c33717 100644 --- a/docs/en_us/internal/testing.rst +++ b/docs/en_us/internal/testing.rst @@ -494,10 +494,10 @@ To view test coverage: 3. Reports are located in the ``reports`` folder. The command generates HTML and XML (Cobertura format) reports. -Code Style Quality +Python Code Style Quality ------------------ -To view code style quality (including pep8 and pylint violations):: +To view Python code style quality (including pep8 and pylint violations):: paver run_quality @@ -530,6 +530,20 @@ More specific options are below. ``system`` is an optional argument here. It defaults to ``cms,lms,common``. + + +JavaScript Code Style Quality +------------------ + +To view JavaScript code style quality:: + + paver run_jshint + +- This command also comes with a ``--limit`` switch, for example:: + + paver run_jshint --limit=700 + + Testing using queue servers --------------------------- diff --git a/pavelib/paver_tests/test_jshint.py b/pavelib/paver_tests/test_jshint.py new file mode 100644 index 0000000000..1b683eeb01 --- /dev/null +++ b/pavelib/paver_tests/test_jshint.py @@ -0,0 +1,50 @@ +""" +Tests for paver quality tasks +""" +import unittest +from mock import patch + +import pavelib.quality +from paver.easy import BuildFailure + + +class TestPaverJsHint(unittest.TestCase): + """ + For testing run_jshint + """ + + def setUp(self): + super(TestPaverJsHint, self).setUp() + + # Mock the paver @needs decorator + self._mock_paver_needs = patch.object(pavelib.quality.run_jshint, 'needs').start() + self._mock_paver_needs.return_value = 0 + + # Mock shell commands + patcher = patch('pavelib.quality.sh') + self._mock_paver_sh = patcher.start() + + # Cleanup mocks + self.addCleanup(patcher.stop) + self.addCleanup(self._mock_paver_needs.stop) + + @patch.object(pavelib.quality, '_write_metric') + @patch.object(pavelib.quality, '_prepare_report_dir') + @patch.object(pavelib.quality, '_get_count_from_last_line') + def test_jshint_violation_number_not_found(self, mock_count, mock_report_dir, mock_write_metric): # pylint: disable=unused-argument + """ + run_jshint encounters an error parsing the jshint output log + """ + mock_count.return_value = None + with self.assertRaises(BuildFailure): + pavelib.quality.run_jshint("") + + @patch.object(pavelib.quality, '_write_metric') + @patch.object(pavelib.quality, '_prepare_report_dir') + @patch.object(pavelib.quality, '_get_count_from_last_line') + def test_jshint_vanilla(self, mock_count, mock_report_dir, mock_write_metric): # pylint: disable=unused-argument + """ + jshint finds violations, but a limit was not set + """ + mock_count.return_value = 1 + pavelib.quality.run_jshint("") diff --git a/pavelib/paver_tests/test_paver_quality.py b/pavelib/paver_tests/test_paver_quality.py index 30836c4cf7..a0d15706cf 100644 --- a/pavelib/paver_tests/test_paver_quality.py +++ b/pavelib/paver_tests/test_paver_quality.py @@ -2,6 +2,7 @@ Tests for paver quality tasks """ import os +from path import path # pylint: disable=no-name-in-module import tempfile import unittest from mock import patch, MagicMock, mock_open @@ -56,6 +57,70 @@ class TestPaverQualityViolations(unittest.TestCase): self.assertEqual(num, 2) +class TestPaverJsHintViolationsCounts(unittest.TestCase): + """ + For testing run_jshint + """ + + def setUp(self): + super(TestPaverJsHintViolationsCounts, 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_violations_count(self): + with open(self.f.name, 'w') as f: + f.write("3000 violations found") + actual_count = pavelib.quality._get_count_from_last_line(self.f.name) # pylint: disable=protected-access + self.assertEqual(actual_count, 3000) + + def test_get_violations_no_number_found(self): + with open(self.f.name, 'w') as f: + f.write("Not expected string regex") + actual_count = pavelib.quality._get_count_from_last_line(self.f.name) # pylint: disable=protected-access + self.assertEqual(actual_count, None) + + def test_get_violations_count_truncated_report(self): + """ + A truncated report (i.e. last line is just a violation) + """ + with open(self.f.name, 'w') as f: + f.write("foo/bar/js/fizzbuzz.js: line 45, col 59, Missing semicolon.") + actual_count = pavelib.quality._get_count_from_last_line(self.f.name) # pylint: disable=protected-access + self.assertEqual(actual_count, None) + + +class TestPrepareReportDir(unittest.TestCase): + """ + Tests the report directory preparation + """ + + def setUp(self): + super(TestPrepareReportDir, self).setUp() + self.test_dir = tempfile.mkdtemp() + self.test_file = tempfile.NamedTemporaryFile(delete=False, dir=self.test_dir) + self.addCleanup(os.removedirs, self.test_dir) + + def test_report_dir_with_files(self): + self.assertTrue(os.path.exists(self.test_file.name)) + pavelib.quality._prepare_report_dir(path(self.test_dir)) # pylint: disable=protected-access + self.assertFalse(os.path.exists(self.test_file.name)) + + def test_report_dir_without_files(self): + os.remove(self.test_file.name) + pavelib.quality._prepare_report_dir(path(self.test_dir)) # pylint: disable=protected-access + self.assertEqual(os.listdir(path(self.test_dir)), []) + + class TestPaverRunQuality(unittest.TestCase): """ For testing the paver run_quality task diff --git a/pavelib/quality.py b/pavelib/quality.py index 03352f2dec..a6e860aeb3 100644 --- a/pavelib/quality.py +++ b/pavelib/quality.py @@ -240,6 +240,86 @@ def run_complexity(): print "ERROR: Unable to calculate python-only code-complexity." +@task +@needs('pavelib.prereqs.install_node_prereqs') +@cmdopts([ + ("limit=", "l", "limit for number of acceptable violations"), +]) +def run_jshint(options): + """ + Runs jshint on static asset directories + """ + + violations_limit = int(getattr(options, 'limit', -1)) + + jshint_report_dir = (Env.REPORT_DIR / "jshint") + jshint_report = jshint_report_dir / "jshint.report" + _prepare_report_dir(jshint_report_dir) + + jshint_directories = ["common/static/js", "cms/static/js", "lms/static/js"] + + sh( + "jshint {list} --config .jshintrc >> {jshint_report}".format( + list=(" ".join(jshint_directories)), jshint_report=jshint_report + ), + ignore_error=True + ) + num_violations = _get_count_from_last_line(jshint_report) + + if not num_violations: + raise BuildFailure("Error in calculating total number of violations.") + + # Record the metric + _write_metric(str(num_violations), (Env.METRICS_DIR / "jshint")) + + # Fail if number of violations is greater than the limit + if num_violations > violations_limit > -1: + raise Exception( + "JSHint Failed. Too many violations ({count}).\nThe limit is {violations_limit}.".format( + count=num_violations, violations_limit=violations_limit + ) + ) + + +def _write_metric(metric, filename): + """ + Write a given metric to a given file + Used for things like reports/metrics/jshint, which will simply tell you the number of + jshint violations found + """ + with open(filename, "w") as metric_file: + metric_file.write(metric) + + +def _prepare_report_dir(dir_name): + """ + Sets a given directory to a created, but empty state + """ + dir_name.rmtree_p() + dir_name.mkdir_p() + + +def _get_last_report_line(filename): + """ + Returns the last line of a given file. Used for getting output from quality output files. + """ + with open(filename, 'r') as report_file: + lines = report_file.readlines() + return lines[len(lines) - 1] + + +def _get_count_from_last_line(filename): + """ + This will return the number in a line that looks something like "3000 errors found". It is returning + the digits only (as an integer). + """ + last_line = _get_last_report_line(filename) + try: + return int(re.search(r'^\d+', last_line).group(0)) + except AttributeError: + return None + + @task @needs('pavelib.prereqs.install_python_prereqs') @cmdopts([ diff --git a/scripts/all-tests.sh b/scripts/all-tests.sh index 68f73e3fc3..68cd052560 100755 --- a/scripts/all-tests.sh +++ b/scripts/all-tests.sh @@ -57,6 +57,7 @@ source scripts/jenkins-common.sh # Violations thresholds for failing the build PYLINT_THRESHOLD=7350 +JSHINT_THRESHOLD=3700 # If the environment variable 'SHARD' is not set, default to 'all'. # This could happen if you are trying to use this script from @@ -78,6 +79,10 @@ case "$TEST_SUITE" in paver run_quality -p 100 mkdir -p reports + echo "Finding jshint violations and storing report..." + PATH=$PATH:node_modules/.bin + paver run_jshint -l $JSHINT_THRESHOLD > jshint.log || { cat jshint.log; EXIT=1; } + echo "Running code complexity report (python)." paver run_complexity > reports/code_complexity.log || echo "Unable to calculate code complexity. Ignoring error." # Need to create an empty test result so the post-build # action doesn't fail the build.