Add new test utils to skip test if using a particular python interpreter #7420

Eric-Arellano · 2019-03-22T18:09:47Z

Problem

A few times in the past, tests have not worked for particular Python interpreters, such as our TensorFlow test not working with Python 3.7 currently #7417.

Historically, we've used the idiom @skipIf(sys.version_info[0:2] == (3,7)), which works, but is difficult to understand what's going on and makes it harder to grep for the tests that we're blacklisting against certain interpreters. Instead, we should provide utility functions for this.

This is especially helpful to add with us soon removing the Python 3 blacklist we have at https://github.com/pantsbuild/pants/blob/master/build-support/known_py3_pex_failures.txt. In case we ever somehow need a temporary blacklist, we will have the utilities to do this.

Solution

In the same file where we provide skip_unless_pythonx_is_present decorators, provide a new series of decorators called skip_if_pythonx.

tests/python/pants_test/backend/python/interpreter_selection_utils.py

Don't reinvent the wheel

cosmicexplorer · 2019-03-25T19:25:16Z

tests/python/pants_test/backend/python/interpreter_selection_utils.py

@@ -107,3 +110,48 @@ def skip_unless_python27_and_python3_present(func):
 def skip_unless_python27_and_python36_present(func):
  """A test skip decorator that only runs a test method if python2.7 and python3.6 are present."""
  return skip_unless_all_pythons_present(PY_27, PY_36)(func)
+
+
+def skip_if_interpreter_is_any_python_version(*versions):


I might change this one to just accept interpreter constraints directly and avoid any custom parsing. Since we're just converting them to interpreter constraints anyway, it seems like a good idea to use the same format people are already familiar with for interpreter constraints, and it seems to reduce the chance that something unexpected will happen and we accidentally skip more python versions than the user intended. We can do the '=={}.*'.format(PY_2) in any calls to this method instead of trying to figure it out here.

I disagree with this because the API is meant to check if that major version is being used, and the API should not be anything more flexible or complex than that.

Rather than wanting us to allow arbitrary interpreter constraints like @skip_if_interpreter_is_any_python_version(">=3.4,<3.7"), I think it's better to be more explicit with @skip_if_interpreter_is_any_python_version("3.4", "3.5", "3.6").

We should start with the simplest API possible, and only make it more flexible if there's a compelling reason we need it. Right now, we're trying to only solve the problem of skip if python37, not a problem like skip if python34 up to python37.

Further, it would be confusing to change what we expect the parameter for these functions to be, because it would diverge from the expectation in the file's skip_unless_pythonx_present functions to use PY_27, PY_3, etc.

The last paragraph seems to be resolved by '=={}.*'.format(PY_2). I was under the impression that typical restrictions that cause us to want to use interpreters or not aren't "this single interpreter version has a weird bug" but rather "these set of interpreters don't have the async keyword". I think I disagree with your statement that @skip_if_interpreter_is_any_python_version(">=3.4,<3.7") is less explicit, or at least, I consider it to be much more explicitly stating the intent of the interpreter restriction than repeating single python versions and forcing the reader to somehow understand what is being restricted here. I also consider it to be a much "simpler" API than having repeated decorators for different python version restrictions, all of which don't do anything you couldn't type yourself in fewer characters -- using repeated decorators for slight variations seems to be an overengineering of sorts here.

stuhood

Thanks.

stuhood · 2019-03-25T21:50:42Z

tests/python/pants_test/backend/python/interpreter_selection_utils.py

+  """A test skip decorator that skips if the current interpreter is Python 2.7."""
+  return skip_if_interpreter_is_any_python_version(PY_27)(func)
+
+


So, I think that the existing decorators in this file attempt to locate interpreters on the system: that is, they determine whether an interpreter "is present".

The new ones you've added skip if pytest is running inside of a particular interpreter (which you call "current" in the comments).

The two implementations have different usages: the former is for integration tests (usually?), while the latter are very much for unit tests (since a subprocessed pants will not necessarily use the same interpreter as the parent).

Because of this difference, it might be good to put the two sets of decorators in different places?

The two implementations have different usages: the former is for integration tests (usually?), while the latter are very much for unit tests (since a subprocessed pants will not necessarily use the same interpreter as the parent).

Not necessarily. The intended use is to not run the integration test to begin with. See https://github.com/pantsbuild/pants/pull/7261/files#diff-c624152df01281abd123b6e962f5f5f9R18 for the original motivation. We never get to the point of having to inspect which interpreter the subprocess will run with - we simply skip the test if using Python 3.7. This is useful for both unit and integration tests.

Thus, I would advocate keeping these in the same file, though I can still move it if you'd like? If so, which file do you recommend using? Create a new file in testutils?

(Speaking of which, I think this file actually belongs in testutils and not backend/python, regardless of what we decide to do with this PR).

Commented there... I think that just using the one-liner for now is fine.

Regarding integration vs unit: I was referring more specifically to PantsRunIntegrationTest (which subprocesses pants) vs TestBase (which doesn't subprocess)... the case on #7261 is different, but shaped more like the latter case.

stuhood · 2019-03-25T21:54:38Z

tests/python/pants_test/backend/python/interpreter_selection_utils.py

+  :param string *versions: Python version strings, such as "2.7", "3".
+  """
+  current_interpreter = ".".join(map(str, sys.version_info[0:2]))
+  # NB: if only the major Python version is specified, e.g. `2` or `3`, we must append `.*` to the string


Would it be a useful validation to attempt to parse these as floating point, and declare them invalid otherwise? I could imagine someone trying to pass in something like CPython>=3. ...but maybe that would fail with a good error in SpecifierSet?

SpecifierSet performs great validation for us already. Running pex --python=python3 packaging==16.8:

>>> from packaging.specifiers import SpecifierSet >>> SpecifierSet("CPython>3.6") Traceback (most recent call last): File "/private/var/folders/sx/pdpbqz4x5cscn9hhfpbsbqvm0000gn/T/tmpvkf49eb1/.deps/packaging-16.8-py2.py3-none-any.whl/packaging/specifiers.py", line 601, in __init__ parsed.add(Specifier(specifier)) File "/private/var/folders/sx/pdpbqz4x5cscn9hhfpbsbqvm0000gn/T/tmpvkf49eb1/.deps/packaging-16.8-py2.py3-none-any.whl/packaging/specifiers.py", line 85, in __init__ raise InvalidSpecifier("Invalid specifier: '{0}'".format(spec)) packaging.specifiers.InvalidSpecifier: Invalid specifier: 'CPython>3.6'

Eric-Arellano · 2019-03-26T22:41:11Z

Closing per the discussion in #7261 (comment).

Eric-Arellano added 2 commits March 22, 2019 11:04

Add new test utils to skip test if certain python interpreter

40555f2

Add docstring

0c9e11a

Eric-Arellano requested review from stuhood and cosmicexplorer March 22, 2019 18:09

Docstring should use full sentences with periods

b577d5e

Eric-Arellano mentioned this pull request Mar 22, 2019

Add Python 3.7 tests to nightly cron CI run #7261

Merged

cosmicexplorer reviewed Mar 22, 2019

View reviewed changes

tests/python/pants_test/backend/python/interpreter_selection_utils.py Outdated Show resolved Hide resolved

Refactor to use packaging.specifiers.SpecifierSet

0bfaaed

Don't reinvent the wheel

cosmicexplorer reviewed Mar 25, 2019

View reviewed changes

stuhood approved these changes Mar 25, 2019

View reviewed changes

Eric-Arellano closed this Mar 26, 2019

Eric-Arellano deleted the skip-if-pythonx branch March 26, 2019 22:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new test utils to skip test if using a particular python interpreter #7420

Add new test utils to skip test if using a particular python interpreter #7420

Eric-Arellano commented Mar 22, 2019 •

edited

Loading

cosmicexplorer Mar 25, 2019

Eric-Arellano Mar 25, 2019 •

edited

Loading

cosmicexplorer Mar 26, 2019

stuhood left a comment

stuhood Mar 25, 2019

Eric-Arellano Mar 25, 2019 •

edited

Loading

stuhood Mar 26, 2019

stuhood Mar 26, 2019

stuhood Mar 25, 2019

Eric-Arellano Mar 25, 2019

Eric-Arellano commented Mar 26, 2019

		"""A test skip decorator that skips if the current interpreter is Python 2.7."""
		return skip_if_interpreter_is_any_python_version(PY_27)(func)

Add new test utils to skip test if using a particular python interpreter #7420

Add new test utils to skip test if using a particular python interpreter #7420

Conversation

Eric-Arellano commented Mar 22, 2019 • edited Loading

Problem

Solution

cosmicexplorer Mar 25, 2019

Choose a reason for hiding this comment

Eric-Arellano Mar 25, 2019 • edited Loading

Choose a reason for hiding this comment

cosmicexplorer Mar 26, 2019

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

stuhood Mar 25, 2019

Choose a reason for hiding this comment

Eric-Arellano Mar 25, 2019 • edited Loading

Choose a reason for hiding this comment

stuhood Mar 26, 2019

Choose a reason for hiding this comment

stuhood Mar 26, 2019

Choose a reason for hiding this comment

stuhood Mar 25, 2019

Choose a reason for hiding this comment

Eric-Arellano Mar 25, 2019

Choose a reason for hiding this comment

Eric-Arellano commented Mar 26, 2019

Eric-Arellano commented Mar 22, 2019 •

edited

Loading

Eric-Arellano Mar 25, 2019 •

edited

Loading

Eric-Arellano Mar 25, 2019 •

edited

Loading