-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Move pythoneval to mypy.api and increase timeout #4047
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
Conversation
mypy/test/testpythoneval.py
Outdated
# This uses the same PYTHONPATH as the current process. | ||
returncode, out = run(mypy_cmdline) | ||
out, err, returncode = api.run(mypy_cmdline) | ||
output.extend([s.rstrip('\r\n').lstrip(test_temp_dir + os.sep) |
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.
Looks like you're misunderstanding how lstrip()
works. It removes characters from the left as long as they are in the argument, which is surely not what you meant. (Ditto for rstrip()
but in this case it's harmless.)
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.
Ah, you are right. Apologies. The rstrip should be fine (removing trailing newlines is what I want), but the lstrip I can replace with a slice. Thanks!
The AppVeyor failure is a flake. |
Also, I have a change set for #3895 but it modifies testpythoneval (to allow for filtering of test names), so it is blocked by this. |
OK I've restarted the failing build. |
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 am a little surprised that this still flaked the same way. Is that because it was run without the benefit of #4048?
mypy/test/testpythoneval.py
Outdated
returncode, out = run(mypy_cmdline) | ||
out, err, returncode = api.run(mypy_cmdline) | ||
# split lines, remove newlines, and remove directory of test case | ||
output.extend([s.rstrip('\r\n')[len(test_temp_dir + os.sep):] |
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.
Hm. I don't like blindly clipping. Please use a regular for-loop that inspects the start of each line before stripping the temp directory and separator.
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.
Okay I will do that.
This is because the change still uses subprocesses to evaluate the Python files. I have a change set to disable the python2eval tests that will make it go away, it is just waiting on this. |
I am not 100% sure I believe the story about the AppVeyor tests (this would imply that the problem was always about the subprocess running the Python code rather than the subprocess running mypy) but at this point I am willing to try anything to stop the bleeding, so I'm merging now. |
This is a re-do of #4025.
This change moves the mypy checking of the eval files away from subprocesses, instead using
mypy.api
.It also increases the timeout of the
run
function in an attempt to fix #3543.