From a9f375aa93242f20afacbe7fdb60bcfd2732c6db Mon Sep 17 00:00:00 2001 From: Christian Long Date: Mon, 23 Feb 2015 16:52:11 -0600 Subject: [PATCH 1/4] Fix tests on Windows The tests were failing on Windows. On Windows, the jobs argument never gets converted to an int (engine.get_style_guide line 133) This resulted in AssertionError '2' != 2. So, do the int conversion in the test. Also, on Winddows, the call count is always 0, no matter the jobs argument. --- flake8/tests/test_integration.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/flake8/tests/test_integration.py b/flake8/tests/test_integration.py index 55ace48..d1417c6 100644 --- a/flake8/tests/test_integration.py +++ b/flake8/tests/test_integration.py @@ -8,6 +8,7 @@ except ImportError: import mock # < PY33 from flake8 import engine +from flake8.util import is_windows class IntegrationTestCase(unittest.TestCase): @@ -41,8 +42,15 @@ class IntegrationTestCase(unittest.TestCase): # mock stdout.flush so we can count the number of jobs created with mock.patch('sys.stdout.flush') as mocked: guide, report = self.check_files(arglist=['--jobs=%s' % jobs]) - self.assertEqual(guide.options.jobs, jobs) - self.assertEqual(mocked.call_count, jobs) + if is_windows(): + # The code path where guide.options.jobs gets converted to an + # int is not run on windows. So, do the int conversion here. + self.assertEqual(int(guide.options.jobs), jobs) + # On windows, call count is always zero. + self.assertEqual(mocked.call_count, 0) + else: + self.assertEqual(guide.options.jobs, jobs) + self.assertEqual(mocked.call_count, jobs) def test_jobs(self): self._job_tester(2) From c3f5d144bca0f16c946d64b7909a35ef66c3273d Mon Sep 17 00:00:00 2001 From: Christian Long Date: Mon, 23 Feb 2015 17:08:04 -0600 Subject: [PATCH 2/4] Add warnings when --jobs is invalid Some platforms and options are not compatible with the --jobs option. For example, it is not valid on Windows. Nor is it compatible with the --diff option. This adds warnings when the --jobs option is supplied but is not allowed. --- flake8/engine.py | 16 +++ flake8/tests/toast_warnings.py | 184 +++++++++++++++++++++++++++++++++ tox.ini | 3 + 3 files changed, 203 insertions(+) create mode 100644 flake8/tests/toast_warnings.py diff --git a/flake8/engine.py b/flake8/engine.py index 1ed06f2..e29b764 100644 --- a/flake8/engine.py +++ b/flake8/engine.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- import re import platform +import warnings import pep8 @@ -123,6 +124,21 @@ def get_style_guide(**kwargs): for options_hook in options_hooks: options_hook(options) + if options.jobs and options.jobs.isdigit() and int(options.jobs) > 1: + if not multiprocessing: + warnings.warn("The multiprocessing module is not available. " + "Ignoring --jobs arguments.") + if is_windows(): + warnings.warn("The --jobs option is not available on Windows. " + "Ignoring --jobs arguments.") + if is_using_stdin(styleguide.paths): + warnings.warn("The --jobs option is not compatible with supplying " + "input using - . Ignoring --jobs arguments.") + if options.diff: + warnings.warn("The --diff option was specified with --jobs but " + "they are not compatible. Ignoring --jobs arguments." + ) + if options.diff: options.jobs = None diff --git a/flake8/tests/toast_warnings.py b/flake8/tests/toast_warnings.py new file mode 100644 index 0000000..2313a81 --- /dev/null +++ b/flake8/tests/toast_warnings.py @@ -0,0 +1,184 @@ +""" + toast_warnings.py + + Tests for the warnings that are emitted by flake8. + + This module is named toast_warnings instead of test_warnings so that a + normal nosetests run does not collect it. The tests in this module pass + when they are run alone, but they fail when they are run along with other + tests (nosetests --with-isolation doesn't help). + + In tox.ini, these tests are run separately. +""" + +from __future__ import with_statement + +import os +import warnings +import unittest +try: + from unittest import mock +except ImportError: + import mock # < PY33 + +from flake8 import engine +from flake8.util import is_windows + + +class IntegrationTestCaseWarnings(unittest.TestCase): + """Integration style tests to check that warnings are issued properly for + different command line options.""" + + def this_file(self): + """Return the real path of this file.""" + this_file = os.path.realpath(__file__) + if this_file.endswith("pyc"): + this_file = this_file[:-1] + return this_file + + @staticmethod + def get_style_guide_with_warnings(engine, *args, **kwargs): + """ + Return a style guide object (obtained by calling + engine.get_style_guide) and a list of the warnings that were raised in + the process. + + Note: not threadsafe + """ + + # Note + # https://docs.python.org/2/library/warnings.html + # + # The catch_warnings manager works by replacing and then later + # restoring the module's showwarning() function and internal list of + # filter specifications. This means the context manager is modifying + # global state and therefore is not thread-safe + + with warnings.catch_warnings(record=True) as collected_warnings: + # Cause all warnings to always be triggered. + warnings.simplefilter("always") + + # Get the style guide + style_guide = engine.get_style_guide(*args, **kwargs) + + # Now that the warnings have been collected, return the style guide and + # the + # warnings. + return (style_guide, collected_warnings) + + def verify_warnings(self, collected_warnings, expected_warnings=None): + """ + Verifies that collected_warnings is a sequence that contains user + warnings that match the sequence of string values passed in as + expected_warnings. + """ + if expected_warnings is None: + expected_warnings = [] + + collected_user_warnings = [w for w in collected_warnings + if issubclass(w.category, UserWarning)] + + self.assertEqual(len(collected_user_warnings), + len(expected_warnings)) + + collected_warnings_set = set(str(warning.message) + for warning + in collected_user_warnings) + expected_warnings_set = set(expected_warnings) + self.assertEqual(collected_warnings_set, expected_warnings_set) + + def check_files_collect_warnings(self, + arglist=[], + explicit_stdin=False, + count=0): + """Call check_files and collect any warnings that are issued.""" + if explicit_stdin: + target_file = "-" + else: + target_file = self.this_file() + argv = ['flake8'] + arglist + [target_file] + with mock.patch("sys.argv", argv): + (style_guide, + collected_warnings, + ) = self.get_style_guide_with_warnings(engine, + parse_argv=True) + report = style_guide.check_files() + self.assertEqual(report.total_errors, count) + return style_guide, report, collected_warnings + + def check_files_no_warnings_allowed(self, + arglist=[], + explicit_stdin=False, + count=0): + """Call check_files, and assert that there were no warnings issued.""" + (style_guide, + report, + collected_warnings, + ) = self.check_files_collect_warnings(arglist=arglist, + explicit_stdin=explicit_stdin, + count=count) + self.verify_warnings(collected_warnings) + return style_guide, report + + def test_no_args_no_warnings(self): + # assert there are no reported errors or warnings + self.check_files_no_warnings_allowed() + + def _job_tester(self, jobs): + # mock stdout.flush so we can count the number of jobs created + with mock.patch('sys.stdout.flush') as mocked: + (guide, + report, + collected_warnings, + ) = self.check_files_collect_warnings(arglist=['--jobs=%s' + % jobs]) + + expected_warings = [] + if is_windows(): + expected_warings.append("The --jobs option is not " + "available on Windows. Ignoring " + "--jobs arguments.") + + # The code path where guide.options.jobs gets converted to an + # int is not run on windows. So, do the int conversion here. + self.assertEqual(int(guide.options.jobs), jobs) + # On windows, call count is always zero. + self.assertEqual(mocked.call_count, 0) + else: + self.assertEqual(guide.options.jobs, jobs) + self.assertEqual(mocked.call_count, jobs) + self.verify_warnings(collected_warnings, expected_warings) + + def test_jobs(self): + self._job_tester(2) + self._job_tester(10) + + def test_stdin_jobs_warning(self): + self.count = 0 + + def fake_stdin(): + self.count += 1 + with open(self.this_file(), "r") as f: + return f.read() + + with mock.patch("pep8.stdin_get_value", fake_stdin): + (style_guide, + report, + collected_warnings, + ) = self.check_files_collect_warnings( + arglist=['--jobs=4'], + explicit_stdin=True) + + expected_warings = ["The --jobs option is not compatible with " + "supplying input using - . Ignoring --jobs " + "arguments."] + if is_windows(): + expected_warings.append("The --jobs option is not " + "available on Windows. Ignoring " + "--jobs arguments.") + self.verify_warnings(collected_warnings, expected_warings) + self.assertEqual(self.count, 1) + + +if __name__ == '__main__': + unittest.main() diff --git a/tox.ini b/tox.ini index 2170c62..4c77305 100644 --- a/tox.ini +++ b/tox.ini @@ -6,9 +6,12 @@ envlist = [testenv] usedevelop = True deps = + nose + mock commands = python setup.py test -q python setup.py flake8 + nosetests flake8.tests.toast_warnings [testenv:py27-flake8] basepython = python2.7 From 59ae3dfec704d47fc9686f979825b4b9da2e45e3 Mon Sep 17 00:00:00 2001 From: Christian Long Date: Thu, 26 Feb 2015 16:25:27 -0600 Subject: [PATCH 3/4] Add --verbose flag, and tests for it. The new warnings associated with the --jobs argument should only appear when the --verbose flag is passed. --- flake8/engine.py | 5 +- .../{toast_warnings.py => _test_warnings.py} | 84 +++++++++++-------- tox.ini | 2 +- 3 files changed, 55 insertions(+), 36 deletions(-) rename flake8/tests/{toast_warnings.py => _test_warnings.py} (73%) diff --git a/flake8/engine.py b/flake8/engine.py index e29b764..be539db 100644 --- a/flake8/engine.py +++ b/flake8/engine.py @@ -124,7 +124,10 @@ def get_style_guide(**kwargs): for options_hook in options_hooks: options_hook(options) - if options.jobs and options.jobs.isdigit() and int(options.jobs) > 1: + if (options.verbose + and options.jobs + and options.jobs.isdigit() + and int(options.jobs) > 1): if not multiprocessing: warnings.warn("The multiprocessing module is not available. " "Ignoring --jobs arguments.") diff --git a/flake8/tests/toast_warnings.py b/flake8/tests/_test_warnings.py similarity index 73% rename from flake8/tests/toast_warnings.py rename to flake8/tests/_test_warnings.py index 2313a81..9dca44b 100644 --- a/flake8/tests/toast_warnings.py +++ b/flake8/tests/_test_warnings.py @@ -1,9 +1,9 @@ """ - toast_warnings.py + _test_warnings.py Tests for the warnings that are emitted by flake8. - This module is named toast_warnings instead of test_warnings so that a + This module is named _test_warnings instead of test_warnings so that a normal nosetests run does not collect it. The tests in this module pass when they are run alone, but they fail when they are run along with other tests (nosetests --with-isolation doesn't help). @@ -29,6 +29,12 @@ class IntegrationTestCaseWarnings(unittest.TestCase): """Integration style tests to check that warnings are issued properly for different command line options.""" + windows_warning_text = ("The --jobs option is not available on Windows." + " Ignoring --jobs arguments.") + stdin_warning_text = ("The --jobs option is not compatible with" + " supplying input using - . Ignoring --jobs" + " arguments.") + def this_file(self): """Return the real path of this file.""" this_file = os.path.realpath(__file__) @@ -66,7 +72,7 @@ class IntegrationTestCaseWarnings(unittest.TestCase): # warnings. return (style_guide, collected_warnings) - def verify_warnings(self, collected_warnings, expected_warnings=None): + def verify_warnings(self, collected_warnings, expected_warnings): """ Verifies that collected_warnings is a sequence that contains user warnings that match the sequence of string values passed in as @@ -90,8 +96,11 @@ class IntegrationTestCaseWarnings(unittest.TestCase): def check_files_collect_warnings(self, arglist=[], explicit_stdin=False, - count=0): + count=0, + verbose=False): """Call check_files and collect any warnings that are issued.""" + if verbose: + arglist.append('--verbose') if explicit_stdin: target_file = "-" else: @@ -109,36 +118,30 @@ class IntegrationTestCaseWarnings(unittest.TestCase): def check_files_no_warnings_allowed(self, arglist=[], explicit_stdin=False, - count=0): + count=0, + verbose=False): """Call check_files, and assert that there were no warnings issued.""" (style_guide, report, collected_warnings, ) = self.check_files_collect_warnings(arglist=arglist, explicit_stdin=explicit_stdin, - count=count) - self.verify_warnings(collected_warnings) + count=count, + verbose=verbose) + self.verify_warnings(collected_warnings, expected_warnings=None) return style_guide, report - def test_no_args_no_warnings(self): - # assert there are no reported errors or warnings - self.check_files_no_warnings_allowed() - - def _job_tester(self, jobs): + def _job_tester(self, jobs, verbose=False): # mock stdout.flush so we can count the number of jobs created with mock.patch('sys.stdout.flush') as mocked: (guide, report, collected_warnings, - ) = self.check_files_collect_warnings(arglist=['--jobs=%s' - % jobs]) + ) = self.check_files_collect_warnings( + arglist=['--jobs=%s' % jobs], + verbose=verbose) - expected_warings = [] if is_windows(): - expected_warings.append("The --jobs option is not " - "available on Windows. Ignoring " - "--jobs arguments.") - # The code path where guide.options.jobs gets converted to an # int is not run on windows. So, do the int conversion here. self.assertEqual(int(guide.options.jobs), jobs) @@ -147,13 +150,20 @@ class IntegrationTestCaseWarnings(unittest.TestCase): else: self.assertEqual(guide.options.jobs, jobs) self.assertEqual(mocked.call_count, jobs) + + expected_warings = [] + if verbose and is_windows(): + expected_warings.append(self.windows_warning_text) self.verify_warnings(collected_warnings, expected_warings) - def test_jobs(self): - self._job_tester(2) - self._job_tester(10) + def test_jobs(self, verbose=False): + self._job_tester(2, verbose=verbose) + self._job_tester(10, verbose=verbose) - def test_stdin_jobs_warning(self): + def test_no_args_no_warnings(self, verbose=False): + self.check_files_no_warnings_allowed(verbose=verbose) + + def test_stdin_jobs_warning(self, verbose=False): self.count = 0 def fake_stdin(): @@ -165,20 +175,26 @@ class IntegrationTestCaseWarnings(unittest.TestCase): (style_guide, report, collected_warnings, - ) = self.check_files_collect_warnings( - arglist=['--jobs=4'], - explicit_stdin=True) - - expected_warings = ["The --jobs option is not compatible with " - "supplying input using - . Ignoring --jobs " - "arguments."] - if is_windows(): - expected_warings.append("The --jobs option is not " - "available on Windows. Ignoring " - "--jobs arguments.") + ) = self.check_files_collect_warnings(arglist=['--jobs=4'], + explicit_stdin=True, + verbose=verbose) + expected_warings = [] + if verbose: + expected_warings.append(self.stdin_warning_text) + if is_windows(): + expected_warings.append(self.windows_warning_text) self.verify_warnings(collected_warnings, expected_warings) self.assertEqual(self.count, 1) + def test_jobs_verbose(self): + self.test_jobs(verbose=True) + + def test_no_args_no_warnings_verbose(self): + self.test_no_args_no_warnings(verbose=True) + + def test_stdin_jobs_warning_verbose(self): + self.test_stdin_jobs_warning(verbose=True) + if __name__ == '__main__': unittest.main() diff --git a/tox.ini b/tox.ini index 4c77305..5ff17ed 100644 --- a/tox.ini +++ b/tox.ini @@ -11,7 +11,7 @@ deps = commands = python setup.py test -q python setup.py flake8 - nosetests flake8.tests.toast_warnings + nosetests flake8.tests._test_warnings [testenv:py27-flake8] basepython = python2.7 From a6fc242c5ef89571f1550e7fce33ac8b806a3451 Mon Sep 17 00:00:00 2001 From: Ian Cordasco Date: Sat, 7 Mar 2015 20:21:58 -0600 Subject: [PATCH 4/4] Slightly simplify our conditionals Test warnings by default --- flake8/engine.py | 15 ++++++--------- flake8/tests/test_engine.py | 5 +++-- .../tests/{_test_warnings.py => test_warnings.py} | 0 flake8/util.py | 9 +++++++++ tox.ini | 2 -- 5 files changed, 18 insertions(+), 13 deletions(-) rename flake8/tests/{_test_warnings.py => test_warnings.py} (100%) diff --git a/flake8/engine.py b/flake8/engine.py index be539db..0711f8d 100644 --- a/flake8/engine.py +++ b/flake8/engine.py @@ -9,7 +9,7 @@ from flake8 import __version__ from flake8 import callbacks from flake8.reporter import (multiprocessing, BaseQReport, FileQReport, QueueReport) -from flake8.util import OrderedSet, is_windows, is_using_stdin +from flake8 import util _flake8_noqa = re.compile(r'\s*# flake8[:=]\s*noqa', re.I).search @@ -18,7 +18,7 @@ EXTRA_EXCLUDE = ['.tox', '.eggs', '*.egg'] def _register_extensions(): """Register all the extensions.""" - extensions = OrderedSet() + extensions = util.OrderedSet() extensions.add(('pep8', pep8.__version__)) parser_hooks = [] options_hooks = [] @@ -124,17 +124,14 @@ def get_style_guide(**kwargs): for options_hook in options_hooks: options_hook(options) - if (options.verbose - and options.jobs - and options.jobs.isdigit() - and int(options.jobs) > 1): + if util.warn_when_using_jobs(options): if not multiprocessing: warnings.warn("The multiprocessing module is not available. " "Ignoring --jobs arguments.") - if is_windows(): + if util.is_windows(): warnings.warn("The --jobs option is not available on Windows. " "Ignoring --jobs arguments.") - if is_using_stdin(styleguide.paths): + if util.is_using_stdin(styleguide.paths): warnings.warn("The --jobs option is not compatible with supplying " "input using - . Ignoring --jobs arguments.") if options.diff: @@ -145,7 +142,7 @@ def get_style_guide(**kwargs): if options.diff: options.jobs = None - force_disable_jobs = is_windows() or is_using_stdin(styleguide.paths) + force_disable_jobs = util.force_disable_jobs(styleguide) if multiprocessing and options.jobs and not force_disable_jobs: if options.jobs.isdigit(): diff --git a/flake8/tests/test_engine.py b/flake8/tests/test_engine.py index b7f7f5f..06fe62a 100644 --- a/flake8/tests/test_engine.py +++ b/flake8/tests/test_engine.py @@ -39,6 +39,7 @@ class TestEngine(unittest.TestCase): with mock.patch('flake8.engine.get_parser') as get_parser: m.ignored_extensions = [] StyleGuide.return_value.options.jobs = '42' + StyleGuide.return_value.options.diff = False get_parser.return_value = (m, []) engine.get_style_guide(foo='bar') get_parser.assert_called_once_with() @@ -85,13 +86,13 @@ class TestEngine(unittest.TestCase): # ourselves) what system we may be testing on. def test_windows_disables_jobs(self): - with mock.patch('flake8.engine.is_windows') as is_windows: + with mock.patch('flake8.util.is_windows') as is_windows: is_windows.return_value = True guide = engine.get_style_guide() assert isinstance(guide, reporter.BaseQReport) is False def test_stdin_disables_jobs(self): - with mock.patch('flake8.engine.is_using_stdin') as is_using_stdin: + with mock.patch('flake8.util.is_using_stdin') as is_using_stdin: is_using_stdin.return_value = True guide = engine.get_style_guide() assert isinstance(guide, reporter.BaseQReport) is False diff --git a/flake8/tests/_test_warnings.py b/flake8/tests/test_warnings.py similarity index 100% rename from flake8/tests/_test_warnings.py rename to flake8/tests/test_warnings.py diff --git a/flake8/util.py b/flake8/util.py index 5a954f8..fda6331 100644 --- a/flake8/util.py +++ b/flake8/util.py @@ -54,6 +54,15 @@ def is_using_stdin(paths): return '-' in paths +def warn_when_using_jobs(options): + return (options.verbose and options.jobs and options.jobs.isdigit() and + int(options.jobs) > 1) + + +def force_disable_jobs(styleguide): + return is_windows() or is_using_stdin(styleguide.paths) + + def flag_on(val): """Return true if flag is on""" return str(val).upper() in ('1', 'T', 'TRUE', 'ON') diff --git a/tox.ini b/tox.ini index 5ff17ed..b21e44c 100644 --- a/tox.ini +++ b/tox.ini @@ -6,12 +6,10 @@ envlist = [testenv] usedevelop = True deps = - nose mock commands = python setup.py test -q python setup.py flake8 - nosetests flake8.tests._test_warnings [testenv:py27-flake8] basepython = python2.7