Add new test utils to skip test if using a particular python interpreter #7420
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| from __future__ import absolute_import, division, print_function, unicode_literals | ||
|
|
||
| import os | ||
| import sys | ||
| from unittest import skipIf | ||
|
|
||
| from future.utils import PY2 | ||
|
|
@@ -61,7 +62,7 @@ def python_interpreter_path(version): | |
| def skip_unless_any_pythons_present(*versions): | ||
| """A decorator that only runs the decorated test method if any of the specified pythons are present. | ||
|
|
||
| :param string *versions: Python version strings, such as "2.7", "3". | ||
| """ | ||
| if any(v for v in versions if has_python_version(v)): | ||
| return skipIf(False, 'At least one of the expected python versions found.') | ||
|
|
@@ -71,7 +72,7 @@ def skip_unless_any_pythons_present(*versions): | |
| def skip_unless_all_pythons_present(*versions): | ||
| """A decorator that only runs the decorated test method if all of the specified pythons are present. | ||
|
|
||
| :param string *versions: Python version strings, such as "2.7", "3". | ||
| """ | ||
| missing_versions = [v for v in versions if not has_python_version(v)] | ||
| if len(missing_versions) == 1: | ||
|
|
@@ -107,3 +108,41 @@ 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): | ||
| """A decorator that skips if the current interpreter version is any of the of the specified versions. | ||
|
|
||
| :param string *versions: Python version strings, such as "2.7", "3". | ||
| """ | ||
| interpreter_major, interpreter_minor = sys.version_info[0:2] | ||
| parsed_versions = [version.split(".") for version in versions] | ||
Eric-Arellano marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| def version_matches_current_interpreter(major, minor=None): | ||
| if int(major) == interpreter_major and minor is None: | ||
| return True | ||
| return int(major) == interpreter_major and int(minor) == interpreter_minor | ||
|
|
||
| if any(version_matches_current_interpreter(*parsed_version) for parsed_version in parsed_versions): | ||
| return skipIf(True, "Current interpreter is one of the specified Python versions.") | ||
| return skipIf(False, "Current interpreter") | ||
|
|
||
|
|
||
| def skip_if_python27(func): | ||
| """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) | ||
|
|
||
|
|
||
|
There was a problem hiding this comment. 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? There was a problem hiding this comment.
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 (Speaking of which, I think this file actually belongs in There was a problem hiding this comment. Commented there... I think that just using the one-liner for now is fine. There was a problem hiding this comment. Regarding integration vs unit: I was referring more specifically to |
||
| def skip_if_python3(func): | ||
| """A test skip decorator that skips if the current interpreter is Python 3.""" | ||
| return skip_if_interpreter_is_any_python_version(PY_3)(func) | ||
|
|
||
|
|
||
| def skip_if_python36(func): | ||
| """A test skip decorator that skips if the current interpreter is Python 3.6.""" | ||
| return skip_if_interpreter_is_any_python_version(PY_36)(func) | ||
|
|
||
|
|
||
| def skip_if_python37(func): | ||
| """A test skip decorator that skips if the current interpreter is Python 3.7.""" | ||
| return skip_if_interpreter_is_any_python_version(PY_37)(func) | ||
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.
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.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.
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 likeskip 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_presentfunctions to usePY_27,PY_3, etc.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.
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 theasynckeyword". 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.