Skip to content
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

Refactor test code's interpreter_selection_utils.py for better naming of util functions #7366

Conversation

Projects
None yet
2 participants
@Eric-Arellano
Copy link
Contributor

commented Mar 12, 2019

Problem

We currently have multiple functions named skip_unless_pythonX which skip unless the Python version is present on the system. The docstring explains this.

However, the name suggests it will skip if the current interpreter itself is not python27, rather than the version being available to begin with. This could be made clearer by updating the name.

Solution

Rename skip_unless_pythonX to skip_unless_pythonX_present. This is safe to do without a deprecation because this is internal test util code, and not exposed as an API.

Also refactor below changes:

  • Define PY_X for more Python versions for better consistency.
  • Add skip_unless_all_pythons_present().
    • We have a function to check all, and it's helpful to check any.
    • For example, we might want to run the test if either Py36 or Py37 are available, and we don't care that both are.
  • Add find_all_pythons_present().
    • This is useful when paired with skip_unless_all_pythons_present(). If the test is not skipped because at least one requested version is available, this function may be used to identify which are valid.
Allow user to predefine which versions they want in find_all_pythons_…
…present()

If they pass none, then use a default of 2.6-2.7 and 3.4-3.8.

This is a much better API, as it makes it easy for a test to do something like:

@skip_unless_any_pythons_present(PY_36, PY_37)
def test():
  target_version = find_all_pythons_present(PY_36, PY_37)[0]

@Eric-Arellano Eric-Arellano merged commit 3d00d1f into pantsbuild:master Mar 14, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:refactor-interpreter-selection-utils branch Mar 14, 2019

Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Mar 14, 2019

Eric-Arellano added a commit that referenced this pull request Mar 14, 2019

Hotfix Python 2 regression from #7366 using FileNotFoundError (#7381)
### Problem
The function `python_interpreter_path()` from `interpreter_selection_utils.py` was originally failing to execute on Py3 because it would encounter `FileNotFoundError`, which we weren't catching, so we added this to the except statement in #7366.

However, the exception apparently does not exist in Python 2, so the nightly cron job failed: https://travis-ci.org/pantsbuild/pants/jobs/506186166#L952.

### Solution
Alias the error `if PY2`. We use this idiom, rather than a try except, so that we can easily automatically delete this code once we drop Py2.

### Result
`PY=python2.7 ./build-support/bin/pre-commit.sh` now works again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.