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

support for PyCharm test debugging, and add tox environment setup #5189

Merged
merged 8 commits into from Jun 14, 2018

Conversation

Projects
None yet
4 participants
@gaborbernat
Copy link
Contributor

gaborbernat commented Jun 10, 2018

Most open source projects use tox as a way of automating project setup. It means the user can just invoke tox and that will setup automatically environments in what to develop and run tests. Additionally, PyCharm is a highly popular IDE, that allows users not that friendly to the command line to quickly become productive.

This PR makes some changes to ensure that out of box clone of mypy allows running the tests from within PyCharm (and that they pass). Plus, it exposes targets that also run inside the CI locally (e.g. running tests for Python 3.4, 3.5, 3.6, 3.7).

Once you have tox installed on a system or user level (pip install tox):

cd mypy
tox -av
default environments:
py34 -> run the test driver with python3.4
py35 -> run the test driver with python3.5
py36 -> run the test driver with python3.6
py37 -> run the test driver with python3.7
lint -> check the code style
type -> type check ourselves
docs -> invoke sphinx-build to build the HTML docs

additional environments:
dev  -> generate a DEV environment, that has all project libraries

E.g. to generate the documentation locally all one needs to do is tox -e docs, and can then view it by opening the HTML up from a browser inside .tox/docs_out folder. Tox is kinda like make, but cross-platform (https://tox.readthedocs.org), written in Python and built-in isolation and virtualenv creation.

These are most of the changes, I've did to help me to develop the feature #5139. Ideally this would be merged before that, leaving the PR to contain the feature only.

@gaborbernat gaborbernat changed the title adopt support for PyCharm test debugging, and add tox environment setup Jun 10, 2018

@gvanrossum
Copy link
Member

gvanrossum left a comment

I am unhappy with the changes to support tox. I don't want us to have to maintain another config file that we don't use in our own workflow.

pytest.ini Outdated
@@ -19,5 +19,3 @@ python_files = test*.py
python_classes =
python_functions =

# always run in parallel (requires pytest-xdist, see test-requirements.txt)
addopts = -nauto --cov-append --cov-report=

This comment has been minimized.

@gvanrossum

gvanrossum Jun 10, 2018

Member

Why are you deleting this? It serves a real purpose for our typical workflow (running pytest from the command line). We're trying to get rid of runtests.py, so moving it there is not a good development IMO.

This comment has been minimized.

@gaborbernat

gaborbernat Jun 10, 2018

Author Contributor

@gvanrossum this is my proposal; feedback is welcome. I feel tox fills in a void that is not solved yet, so I'm not sure what type of configs are you talking about here yet; feel free to enlighten me!

This config line contains two things set, which I disabled:

  • run tests in parallel. Generally, I think tests should only be run in parallel when you want to run the entire test suite. E.g. becomes cumbersome to fix/check a single test with this. Shouldn't this be the other way around? That is single threaded by default, multi-threaded on demand?
  • automatically enable coverage report. Again this seems unwarranted when running a single test (e.g. developing a new feature, writing a new test, debugging a test). Also once this is set PyCharm debugger cannot connect anymore, as both use the same mechanism to intercept calls.

I don't want to break your workflow. As pointed out in my other PR, mainly I did not make this PR earlier because I did not know what that workflow is, and did not want to break it.

This comment has been minimized.

@ethanhs

ethanhs Jun 11, 2018

Collaborator

I think it would be better to document that the tests run in parallel, and how to disable that (I think setting $PYTEST_ADDOPTS to -n0 does it). Though I do agree that you probably do not want to run coverage in the general case.

@gvanrossum

This comment has been minimized.

Copy link
Member

gvanrossum commented Jun 10, 2018

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Jun 10, 2018

Then again this argument also takes time so if one of the other devs speaks
up I'll gladly back off. :-)

I am using PyCharm to develop mypy approximately half of time since beginning of this year. But TBH I don't like these changes. IMO our top level directory has already too many config-related things, adding tox.ini will make it only worse. Also I don'y see much value in testing on many Python versions, the situations where something doesn't work on a given version are quite rare, and Travis will catch them (I have Travis enabled on my fork, so I see failures before even opening a PR).

@gaborbernat

This comment has been minimized.

Copy link
Contributor Author

gaborbernat commented Jun 10, 2018

@gaborbernat

This comment has been minimized.

Copy link
Contributor Author

gaborbernat commented Jun 10, 2018

Oh and both setup.cfg and pytest.ini can be merged into tox.ini. And soon moved into pyproject.toml file. To help with clutter of files at root level.

@ethanhs
Copy link
Collaborator

ethanhs left a comment

I left a few minor points of feedback, I'll have more time tomorrow to do a more thorough review.

tox.ini Outdated
[testenv]
description = run the test driver with {basepython}
deps = -r ./test-requirements.txt
commands = python runtests.py -x lint -x package {posargs}

This comment has been minimized.

@ethanhs

ethanhs Jun 11, 2018

Collaborator

package has been replaced with self-check.

tox.ini Outdated
[testenv:type]
description = type check ourselves
basepython = python3.6
commands = python runtests.py testselfcheck -p '-n0' -p '-v'

This comment has been minimized.

@ethanhs

ethanhs Jun 11, 2018

Collaborator

self-check would be more consistent here.

@@ -58,6 +58,14 @@ def test_python_cmdline(testcase: DataDrivenTestCase) -> None:
outb = process.stdout.read()
# Split output into lines.
out = [s.rstrip('\n\r') for s in str(outb, 'utf8').splitlines()]

is_running_in_py_charm = "PYCHARM_HOSTED" in os.environ
if is_running_in_py_charm:

This comment has been minimized.

@ethanhs

ethanhs Jun 11, 2018

Collaborator

I don't think you need to create a variable here. I'd just do if "PYCHARM_HOSTED" in os.environ: ...

@gaborbernat gaborbernat force-pushed the gaborbernat:pycharm branch from 681c2e6 to 1c928bf Jun 11, 2018

@gaborbernat

This comment has been minimized.

Copy link
Contributor Author

gaborbernat commented Jun 11, 2018

@ethanhs addressed all requests

@gaborbernat

This comment has been minimized.

Copy link
Contributor Author

gaborbernat commented Jun 11, 2018

@ilevkivskyi

Also I don'y see much value in testing on many Python versions, the situations where something doesn't work on a given version are quite rare, and Travis will catch them (I have Travis enabled on my fork, so I see failures before even opening a PR).

I don't run all tests either locally. However, when Travis catches something often (especially when you're new to the code base) it helps a lot to be able to reproduce locally quickly, debug it, understand why the problem arises and being able to fix/test it locally. I suppose at the end of the day it flows into the category of different workflow between us. I had a few such instances when creating #5139.

That being said, I don't want to propose anything that breaks the current core developers workflow. I'm just suggesting to extend things that potentially help new people interested in contributing.

@gaborbernat

This comment has been minimized.

Copy link
Contributor Author

gaborbernat commented Jun 12, 2018

I've restored the parallel execution of the test suite via pytest.ini and added a section on the wiki about options on how to make PyCharm play nicely with mypy (https://github.com/python/mypy/wiki/Getting-Started-Developing#pycharm). Is this more agreeable for the core developers?

I still propose keeping the tox.ini file around. That could replace the long make files from within docs. E.g. all users need to generate documentation is pip install --user tox && tox -e docs (which then will go ahead setup a valid environment and generate the documentation).

@gaborbernat gaborbernat force-pushed the gaborbernat:pycharm branch 2 times, most recently from 8022070 to 5f68d9e Jun 12, 2018

@gvanrossum
Copy link
Member

gvanrossum left a comment

OK, I am fine with this. I don't expect I'll be switching to tox myself but I don't want to stand in the way of progress for others' workflow.

@ethanhs

This comment has been minimized.

Copy link
Collaborator

ethanhs commented Jun 13, 2018

Taking another look at this now.

@gaborbernat gaborbernat force-pushed the gaborbernat:pycharm branch from 5f68d9e to 5f1c3cf Jun 13, 2018

@gaborbernat

This comment has been minimized.

Copy link
Contributor Author

gaborbernat commented Jun 13, 2018

cool 😎 is anyone responsible with merging it?

gaborbernat added some commits Jun 7, 2018

@gaborbernat gaborbernat force-pushed the gaborbernat:pycharm branch from 5f1c3cf to e5567f4 Jun 13, 2018

@ethanhs

This comment has been minimized.

Copy link
Collaborator

ethanhs commented Jun 13, 2018

I think I will defer to @ilevkivskyi since he also uses PyCharm on occasion, and this could affect his workflow.

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

Just one comment, otherwise looks good to me.

pos = next((p for p, i in enumerate(out) if i.startswith('pydev debugger: ')), None)
if pos is not None:
del out[pos] # the attaching debugger message itself
del out[pos] # plus the extra new line added

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 14, 2018

Collaborator

This code looks quite fragile (what if the debugger output format changes?). Is it possible to configure the debugger to just not print these lines?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 14, 2018

Collaborator

Also please don't rebase your PR, merge when needed, otherwise it is hard to follow.

This comment has been minimized.

@gaborbernat

gaborbernat Jun 14, 2018

Author Contributor

we'll need to follow up then and amend it; I haven't found a way to disable it sadly 😢

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 14, 2018

Collaborator

OK.

@ilevkivskyi ilevkivskyi merged commit 5959e1a into python:master Jun 14, 2018

2 checks passed

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

@gaborbernat gaborbernat deleted the gaborbernat:pycharm branch Jun 14, 2018

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.