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

Make the tests work on Windows #213

Merged
merged 5 commits into from Mar 10, 2015
Merged

Conversation

@pfmoore
Copy link
Contributor

@pfmoore pfmoore commented Mar 5, 2015

Getting the tests to work on Windows wasn't as hard as expected. Mostly by skipping the ones that weren't obvious :-) I may have broken some of the tests on Unix, posting this PR for Travis to check...

@pfmoore
Copy link
Contributor Author

@pfmoore pfmoore commented Mar 5, 2015

Looks like there is a problem with me renaming (on Windows) the "err" script to "err.py". In doing so, I guess the execute bit got lost as Windows doesn't have one of them. A "simple" solution would be for someone to fix this on Unix and leave it at that, but doing so might mean that if a Windows user ever changed that file in future, it would get broken again. I'll try a more robust fix that runs the command using the Python interpreter directly.

Loading

@pfmoore
Copy link
Contributor Author

@pfmoore pfmoore commented Mar 5, 2015

@bitprophet That was a lot easier than I expected. This PR patches up the tests to make them work on Windows (mostly by skipping the pty-related ones, plus a few small tweaks to commands that don't work the same on Windows) and adds support for running on Appveyor.

To enable Appveyor testing all you need to do is sign up for the Appveyor free plan using your github account, then do "Add project", select github and pick this project. Once added, builds will run automatically.

Appveyor is phenomenally slow, because the free plan has pretty limited resources (mostly it's just that you wait for workers for a long time - when the tests finally run they run about as fast as Travis). I'm testing Python 2.7, 3.3 and 3.4 (32 and 64 bit) as those are the ones Appveyor has installed by default. You can add more just by downloading and installing the extra versions of Python yourself, but IMO it's not worth the effort.

Actually, Appveyor has found a bug for Windows - Python 3.3 chokes trying to read /etc/invoke which doesn't exist (looks like some issue in core python importlib, which has presumably been fixed in 3.4). At some point, invoke should get its config from a more appropriate location on Windows but I'll see if I can find a short-term patch in the meantime.

Loading

@pfmoore
Copy link
Contributor Author

@pfmoore pfmoore commented Mar 5, 2015

Actually, the "Python 3.3 bug" seems to be that you're using imp.load_source in invoke/config.py, which was removed in 3.3. I'm somewhat baffled as to why it still works on Travis :-(

I'll raise a separate issue for this - its not something that should be bundled in with this PR...

Loading

eq_(result.exited, 127)
if WINDOWS:
# Windows doesn't give a 127 error for "unknown command"
assert result.exited != 0
Copy link
Member

@bitprophet bitprophet Mar 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the actual numeric result vary or something?

Loading

Copy link
Contributor Author

@pfmoore pfmoore Mar 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's just 1, but I couldn't be sure it's the same everywhere (Windows doesn't have a tradition of well-defined "system-level" exit codes like this). So I just left it as non-zero as a safe option. I'm happy enough to say "equals 1" and fix it later if that's too specific.

Loading

Copy link
Member

@bitprophet bitprophet Mar 9, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't care super hard, was just curious, thanks.

Loading

@pfmoore pfmoore force-pushed the windows_tests branch 5 times, most recently from 50f78c7 to c21c06d Mar 5, 2015
@pfmoore
Copy link
Contributor Author

@pfmoore pfmoore commented Mar 5, 2015

OK, this is now ready to go. The tests fail on Windows without #215 but that's an independent issue hence the separate PR.

Loading

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Mar 10, 2015

Confirmed tests still run OK on my end. Thanks!

EDIT: Shoot, wrong ticket, that was 215. El oh el. However, the tests for this branch ALSO pass so yay for that.

Loading

bitprophet added a commit that referenced this issue Mar 10, 2015
Make the tests work on Windows
@bitprophet bitprophet merged commit 1bd901d into pyinvoke:master Mar 10, 2015
1 check passed
Loading
bitprophet added a commit that referenced this issue Mar 10, 2015
@bitprophet
Copy link
Member

@bitprophet bitprophet commented Mar 11, 2015

FTR I just set up AppVeyor under my own (Github) account. Seems to be running now, will see how it completes (I just did some big ol refactoring, so...).

Obviously, in some cases where the Windows breakage isn't obvious/trivial, I won't be able/willing to actually dig into stuff - but at least now I'll know, and ostensibly Windows users will be able to view the build status so you know you're not off your rocker if things are breaking on your end.

Loading

@pfmoore
Copy link
Contributor Author

@pfmoore pfmoore commented Mar 11, 2015

@bitprophet if there are windows issues, feel free to ping me and I'll see what I can do to assist.

Loading

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Mar 11, 2015

Of course, appreciated. Just saw aforementioned refactor did break things in one test, already applied fix. Fun times. EDIT: also, you weren't kidding about slow :D good thing this is all semi-optional for me, I can just ignore it and check again later.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants