-
-
Notifications
You must be signed in to change notification settings - Fork 628
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Python 3 - fixes to get green contrib #6340
Changes from 19 commits
f19f3a5
022d6c2
aeb72be
dfb5d0d
e6174d9
8bc11cf
8c79503
8b78fbe
2126adc
ec8f547
17137b2
6c16095
fd184f2
7fd5da4
4194f10
b89f16e
302a6df
c8bfdf2
2a64b85
503b0c6
71dd278
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,8 +12,8 @@ | |
from pants_test.backend.python.tasks.python_task_test_base import PythonTaskTestBase | ||
|
||
from pants.contrib.python.checks.tasks.checkstyle.checker import PythonCheckStyleTask | ||
from pants.contrib.python.checks.tasks.checkstyle.print_statements_subsystem import \ | ||
PrintStatementsSubsystem | ||
from pants.contrib.python.checks.tasks.checkstyle.variable_names_subsystem import \ | ||
VariableNamesSubsystem | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Print Statements tests don't work as expected on Py3. Because they raise a syntax error with Python 3, the tests weren't working as intended. So, I changed to using Variable Names. The specific error doesn't matter much - this file tests the |
||
|
||
|
||
class PythonCheckStyleTaskTest(PythonTaskTestBase): | ||
|
@@ -24,7 +24,7 @@ def task_type(cls): | |
def setUp(self): | ||
super(PythonCheckStyleTaskTest, self).setUp() | ||
PythonCheckStyleTask.clear_plugins() | ||
PythonCheckStyleTask.register_plugin('print-statements', PrintStatementsSubsystem) | ||
PythonCheckStyleTask.register_plugin('variable-names', VariableNamesSubsystem) | ||
|
||
def tearDown(self): | ||
super(PythonCheckStyleTaskTest, self).tearDown() | ||
|
@@ -36,7 +36,8 @@ def test_no_sources(self): | |
|
||
def test_pass(self): | ||
self.create_file('a/python/pass.py', contents=dedent(""" | ||
print('Print is a function') | ||
class UpperCase: | ||
pass | ||
""")) | ||
target = self.make_target('a/python:pass', PythonLibrary, sources=['pass.py']) | ||
context = self.context(target_roots=[target]) | ||
|
@@ -45,7 +46,8 @@ def test_pass(self): | |
|
||
def test_failure(self): | ||
self.create_file('a/python/fail.py', contents=dedent(""" | ||
print 'Print should not be used as a statement' | ||
class lower_case: | ||
pass | ||
""")) | ||
target = self.make_target('a/python:fail', PythonLibrary, sources=['fail.py']) | ||
context = self.context(target_roots=[target]) | ||
|
@@ -56,20 +58,21 @@ def test_failure(self): | |
|
||
def test_suppressed_file_passes(self): | ||
self.create_file('a/python/fail.py', contents=dedent(""" | ||
print 'Print should not be used as a statement' | ||
class lower_case: | ||
pass | ||
""")) | ||
suppression_file = self.create_file('suppress.txt', contents=dedent(""" | ||
a/python/fail\.py::print-statements""")) | ||
a/python/fail\.py::variable-names""")) | ||
target = self.make_target('a/python:fail', PythonLibrary, sources=['fail.py']) | ||
self.set_options(suppress=suppression_file) | ||
context = self.context(target_roots=[target], ) | ||
task = self.create_task(context) | ||
|
||
self.assertEqual(0, task.execute()) | ||
|
||
def test_failure_fail_false(self): | ||
self.create_file('a/python/fail.py', contents=dedent(""" | ||
print 'Print should not be used as a statement' | ||
class lower_case: | ||
pass | ||
""")) | ||
target = self.make_target('a/python:fail', PythonLibrary, sources=['fail.py']) | ||
self.set_options(fail=False) | ||
|
@@ -90,7 +93,8 @@ def test_syntax_error(self): | |
|
||
def test_failure_print_nit(self): | ||
self.create_file('a/python/fail.py', contents=dedent(""" | ||
print 'Print should not be used as a statement' | ||
class lower_case: | ||
pass | ||
""")) | ||
target = self.make_target('a/python:fail', PythonLibrary, sources=['fail.py']) | ||
context = self.context(target_roots=[target]) | ||
|
@@ -100,8 +104,8 @@ def test_failure_print_nit(self): | |
|
||
self.assertEqual(1, len(nits)) | ||
self.assertEqual( | ||
"""T607:ERROR a/python/fail.py:002 Print used as a statement.\n""" | ||
""" |print 'Print should not be used as a statement'""", | ||
"""T000:ERROR a/python/fail.py:002 Classes must be UpperCamelCased\n""" | ||
""" |class lower_case:""", | ||
str(nits[0])) | ||
|
||
def test_syntax_error_nit(self): | ||
|
@@ -121,21 +125,3 @@ def test_syntax_error_nit(self): | |
""" |invalid python\n""" | ||
""" |""", | ||
str(nits[0])) | ||
|
||
def test_multiline_nit_printed_only_once(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I couldn't think of an alternative test - it seems like the original was more related to print logic than checker logic? Let me know if you have ideas. |
||
self.create_file('a/python/fail.py', contents=dedent(""" | ||
print ('Multi' | ||
'line') + 'expression' | ||
""")) | ||
target = self.make_target('a/python:fail', PythonLibrary, sources=['fail.py']) | ||
context = self.context(target_roots=[target]) | ||
task = self.create_task(context) | ||
|
||
nits = list(task.get_nits('a/python/fail.py')) | ||
|
||
self.assertEqual(1, len(nits)) | ||
self.assertEqual( | ||
"""T607:ERROR a/python/fail.py:002-003 Print used as a statement.\n""" | ||
""" |print ('Multi'\n""" | ||
""" | 'line') + 'expression'""", | ||
str(nits[0])) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,15 +40,15 @@ def get_default_jvm_options(): | |
thrift_target = self.create_library('a', 'java_thrift_library', 'a', ['A.thrift']) | ||
task = self.create_task(self.context(target_roots=thrift_target)) | ||
self._prepare_mocks(task) | ||
expected_include_paths = {'src/thrift/tweet', 'src/thrift/users'} | ||
expected_paths = {'src/thrift/tweet/a.thrift', 'src/thrift/tweet/b.thrift'} | ||
expected_include_paths = ['src/thrift/users', 'src/thrift/tweet'] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Set isn't deterministic between Py2 vs Py3, so have to use a list. |
||
expected_paths = ['src/thrift/tweet/a.thrift', 'src/thrift/tweet/b.thrift'] | ||
mock_calculate_compile_sources.return_value = (expected_include_paths, expected_paths) | ||
task._lint(thrift_target, task.tool_classpath('scrooge-linter')) | ||
|
||
self._run_java_mock.assert_called_once_with( | ||
classpath='foo_classpath', | ||
main='com.twitter.scrooge.linter.Main', | ||
args=['--ignore-errors', '--include-path', 'src/thrift/users', '--include-path', | ||
'src/thrift/tweet', 'src/thrift/tweet/b.thrift', 'src/thrift/tweet/a.thrift'], | ||
'src/thrift/tweet', 'src/thrift/tweet/a.thrift', 'src/thrift/tweet/b.thrift'], | ||
jvm_options=get_default_jvm_options(), | ||
workunit_labels=[WorkUnitLabel.COMPILER, WorkUnitLabel.SUPPRESS_LABEL]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to lift these into constants.