-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Pip should run Windows tests on Appveyor, as well as Linux on Travis #3948
Comments
Just a note, once Appveyor is setup to run tests and start reporting statuses we'll get feedback without requiring the tests to have run or to have run successfully prior to merging. At that point it will be entirely through cooperation (i.e. don't merge broken PRs on Appveyor) that keeps it working. Once that is working and we're/you're satisfied that the Appveyor tests are good, toggling them to mandatory to actually merge is a simple checkbox in Github. |
I will also say, I do remember one thing about the Appveyor tests... If I remember correctly they took a really long time when I tried to use them for virtualenv. Like > 1 hour for a single PR. |
OK, thanks for the info. If necessary, we can start very small, and just run some basic sanity checks on Windows - i.e., not the full test suite, maybe just the unit tests (or even less, if that's still too long). |
Although see https://ci.appveyor.com/project/pfmoore/pip/build/1.0.31 - 8 tests at a minute each (admittedly, a minute to fail) doesn't sound insane. |
This was awhile ago so I'm trying to go off memory, but I think the problem boiled down to Appveyor only allowing 1 test job at a time, and not even scheduling the other test jobs until the first one was finished. By the time all of the test jobs for all of the Pythons were completed it ended up taking a long time. It's entirely possible that they've made that better in the mean time! If they have great, if not and my memory is correct, it's possible we may just want to limit the test matrix to the latest 2.x and 3.x or something. |
Yea, looking at Travis, our tests there take ~15-20 minutes per Python version. Running them serially across all of our supported Pythons would take a minimum of 1.5 hours assuming Windows run time is anything like Linux. That goes up higher if we want to do 32 and 64 bit versions of our tests. Even their highest level plan only allows 2 concurrent jobs which is still something like a ~45 minute test run. Maybe they'd be willing to give us a higher concurrency limit for free if we ask them real nice? |
Yeah, it's probably because they don't run concurrently. But we currently just run Also, maybe we could just skip the matrix and use tox's built-in ability to run multiple Python versions. That way we'd just have one appveyor job. Probably only worth it if timing is difficult though. |
I've just created a pypa account on Appveyor, and a a "Appveyor Admins" team under the pypa account here. @dstufft I've added you to that team. I've also added pip as a project under that account, and it's just run a build. See https://ci.appveyor.com/project/pypa/pip - it took about 8 minutes, as expected. It looks like simply by enabling the integration, we have appveyor running against PRs now - but if I'm reading the status display right, appveyor success is not mandatory. I suggest we leave it like that at least for a while, and when we're confident it's not causing any problems, we can consider making it required. I don't know how to make a check mandatory, though, @dstufft can you advise? One final thing - at the moment, I am the only one with the password to the pypa account on Appveyor. Anyone in the Appveyor Admins group can manage the account, but I think the actual account credentials should be kept somewhere better than just with me. Do we have a central store of credentials, and if so how would I pass the details over? |
https://help.github.com/articles/enabling-required-status-checks/ there's the docs on how to make a status check mandatory. Alternatively, I can do it whenever you think it's good enough to make it mandatory. We don't really have a central store anywhere :/ |
Thanks. It's probably good enough now, but I'll leave it a little while and then flip the switch. I'll keep the account details, and if anyone else needs them give me a shout. |
FWIW, I use py.test with pytest-xdist and run two processes in // for my tests on Appveyor. This helps speed things up. |
This seems to be going well, time to make it required? |
Yeah, I think so. If you can flip the switch that'd be great, otherwise I'll do it probably tomorrow. |
Done. |
At the moment, we only run CI against Travis (Linux) for PRs to pip. We should add Appveyor to test against Windows as well.
The text was updated successfully, but these errors were encountered: