Windows test fixes and CI with AppVeyor #1504

jtatum · 2016-05-07T07:28:36Z

I noticed in #1353 that testing for the new parser is needed on Windows. I thought I'd get that started with some CI from AppVeyor. It turned out to be a bit more complicated than that.

util.try_find_python2_interpreter() doesn't work well on Windows. I cheat in appveyor.yml by renaming Python 2.7's python.exe to python2.exe. I'm not sure how you could find multiple interpreters in the path on Windows since those handy version symlinks (python3, python35, etc) don't exist.
waiter, the parallel execution task runner, used some posix tricks to run multiple processes at once and wait for the first one to return. I replaced all that clever code with some much uglier and stupider polling that works on both Windows and saner OSes.
waiter also tried to open a file that was already open. On Windows, this is impossible. It's okay, though, because NamedTemporaryFile (and TemporaryFile) give you a file-like object with a file already open in binary mode. It looks like the stub generation test cases are failing with a similar issue.

This is what the tests look like on Windows: https://ci.appveyor.com/project/jtatum/mypy/build/1.0.16

Still to-do:

Fix failing tests on Windows. Most look like path differences (forward vs. backslash) and NamedTempFile. A couple seem a bit more mysterious.
Add os.replace() to typeshed

Waiter depended on a couple of posix tricks. The first was a clever use of os.waitpid(-1, 0) to wait for any spawned processes to return. On Windows, this raises an exception - Windows has no concept of a process group. The second was the use of NamedTempFile(). On anything but Windows, a file can be open for writing and for reading at the same time. This trick actually isn't necessary the way NamedTempFile is used here. It exposes a file-like object pointing to a file already opened in binary mode. All we have to do is seek and read from it.

The only tricky bit of this is renaming python.exe to python2.exe. This is due to util.try_find_python2_interpreter(), which may well need work for Windows since the version symlinks don't exist on Windows.

gvanrossum · 2016-05-07T15:23:01Z

Currently the Travis CI tests (on Linux) fail, that needs to be addressed.

I'll leave this to @ddfisher to review.

jtatum · 2016-05-07T17:49:26Z

Getting a traceback in the incremental builder:

File ".\mypy\build.py", line 767, in write_cache
    os.rename(data_json_tmp, data_json)
FileExistsError: [WinError 183] Cannot create a file when that file already exists: '.mypy_cache\\3.5\\__main__.data.json'

os.rename cannot replace a file on Windows. Python 3.3 adds os.replace(), a cross-platform function, but it looks like mypy's minimum version is 3.2. There's a TODO in build.py that mentions it may not be atomic, but that's incorrect - it actually fails. Not sure how this should be fixed.

gvanrossum · 2016-05-07T17:53:35Z

Support for PY 3.2 will be dropped soon anyway, so you can do os.replace if it exists else os.rename.

Most of these fixes revolve around the path separator and the way Windows handles files and locking. There is one bug fix in here - build.write_cache() was using os.rename to replace a file, which fails on Windows. I was only able to fix that for Python 3.3 and up.

jtatum · 2016-05-07T20:53:20Z

I'm not sure why the type checking works on Python 3.2 after adding os.replace() to typeshed (I put it on a conditional for Python 3.3 or newer) but everything looks good now. Latest Windows build: https://ci.appveyor.com/project/jtatum/mypy/build/1.0.21

There are probably too many Python versions in the matrix - the tests are much slower and AppVeyor doesn't offer concurrency for FOSS projects.

gvanrossum · 2016-05-07T21:00:30Z

Conditions aren't implemented yet in mypy. No worries.

…

--Guido (mobile)

gvanrossum · 2016-05-09T16:58:30Z

mypy/test/data.py

@@ -65,6 +65,8 @@ def parse_test_cases(
            tcout = []  # type: List[str]
            if i < len(p) and p[i].id == 'out':
                tcout = p[i].data
+                if p[i].arg == 'pathfix':
+                    tcout = [s.replace('/', os.path.sep) for s in tcout]


Rather than having to remember to add 'pathfix' to tests that happen to have pathnames with slashes in the expected output, maybe you could always fix the path in the text up to the first colon, skipping this completely if the line starts with whitespace or there's whitespace in the stuff before that first colon (or there is no colon)?

I considered this but was hesitant to add more magic to this code. If that's the preference, I certainly could, though. Do you suppose there are other [out] sections that might be affected by that change? Should that behavior be documented somewhere, maybe in comments in test/data.py?

The only way to be sure is to survey all [out] sections of all test files. Or at least sample a bunch of different ones (some have a different format, the test prints the repr() of some Node).

I just worry that your current approach will mean that every time a new test requires [out pathfix] it will first be committed without the pathfix and then after some time someone will report that it's broken on Windows and someone has to remember how to fix it.

gvanrossum · 2016-05-09T17:15:23Z

FWIW I need to spend more time reviewing your changes to the code.

gvanrossum · 2016-05-09T18:39:13Z

(I meant to say that I don't have that time right now but I will get back to this later in the week.)

gvanrossum · 2016-05-20T03:46:47Z

So this looks fine except I still don't like the idea of having to add "pathfix" to all those tests.

Did you find a better way?

Would be nice if we could release this next week (just before PyCon).

gvanrossum · 2016-05-29T00:50:00Z

Hey, I'm taking over this PR here: #1593. I'm closing this version. Thanks for getting it this far! I'm building on your work and acknowledging it in the commit message.

jtatum force-pushed the appveyor branch 2 times, most recently from a57a81e to df5c18e Compare May 7, 2016 07:55

James Tatum and others added 2 commits May 7, 2016 00:58

Add appveyor config for Windows CI

addd9f4

The only tricky bit of this is renaming python.exe to python2.exe. This is due to util.try_find_python2_interpreter(), which may well need work for Windows since the version symlinks don't exist on Windows.

jtatum force-pushed the appveyor branch from df5c18e to addd9f4 Compare May 7, 2016 07:59

jtatum force-pushed the appveyor branch from 360be0d to f83683e Compare May 7, 2016 17:34

jtatum force-pushed the appveyor branch from f83683e to 42a7b48 Compare May 7, 2016 17:57

Fixing Windows tests

81511fc

Most of these fixes revolve around the path separator and the way Windows handles files and locking. There is one bug fix in here - build.write_cache() was using os.rename to replace a file, which fails on Windows. I was only able to fix that for Python 3.3 and up.

jtatum force-pushed the appveyor branch from 42a7b48 to 81511fc Compare May 7, 2016 18:06

Update typeshed to latest

e843a34

gvanrossum reviewed May 9, 2016
View reviewed changes

gvanrossum mentioned this pull request May 29, 2016

Make tests runnable on Windows and add appveyor.yaml #1593

Merged

gvanrossum closed this May 29, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows test fixes and CI with AppVeyor #1504

Windows test fixes and CI with AppVeyor #1504

jtatum commented May 7, 2016 •

edited

Loading

gvanrossum commented May 7, 2016

jtatum commented May 7, 2016

gvanrossum commented May 7, 2016 via email

jtatum commented May 7, 2016 •

edited

Loading

gvanrossum commented May 7, 2016 via email

gvanrossum May 9, 2016

jtatum May 10, 2016

gvanrossum May 10, 2016

gvanrossum commented May 9, 2016 via email

gvanrossum commented May 9, 2016

gvanrossum commented May 20, 2016

gvanrossum commented May 29, 2016

Windows test fixes and CI with AppVeyor #1504

Windows test fixes and CI with AppVeyor #1504

Conversation

jtatum commented May 7, 2016 • edited Loading

gvanrossum commented May 7, 2016

jtatum commented May 7, 2016

gvanrossum commented May 7, 2016 via email

jtatum commented May 7, 2016 • edited Loading

gvanrossum commented May 7, 2016 via email

gvanrossum May 9, 2016

Choose a reason for hiding this comment

jtatum May 10, 2016

Choose a reason for hiding this comment

gvanrossum May 10, 2016

Choose a reason for hiding this comment

gvanrossum commented May 9, 2016 via email

gvanrossum commented May 9, 2016

gvanrossum commented May 20, 2016

gvanrossum commented May 29, 2016

jtatum commented May 7, 2016 •

edited

Loading

jtatum commented May 7, 2016 •

edited

Loading