-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Make freeze functional tests depend on less external services #2509
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
os.path.dirname(__file__), | ||
'..', 'data', 'repos', 'bzr', 'django-wikiapp', 'release-0.1' | ||
) | ||
|
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've not looked at this in context, but should this not use the test data directory fixture data
?
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.
Oh probably. I'm having trouble grokking how the tests fit together.
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.
OK, updated.
By the way, I notice that even without hitting the network, these tests are pretty slow and it seems like a good chunk of time is spent on creating the virtualenvs -- any ideas for how to speed that up? |
c9e0f73
to
7754f36
Compare
Nope, on Windows creating virtualenvs seems to fail with no explanation (at least sometimes, it hit me earlier today). Someone with way more time than I have needs to get the tests working on Windows (or lots of people need to fix them one little bit at a time, as per your posting on distutils-sig - do you have access to an army of open-source enthusiasts who are Windows experts? ;-)) |
141cdab
to
cef7910
Compare
But tests are passing so this could be merged as-is. Either way. |
2c99b6f
to
0a3be72
Compare
Do we really need to include the whole django-wikiapp ? We could certainly make a simplebzr module and put it in a bzr repo. Maybe even via a fixture ? Edit: something similar to https://github.com/pypa/pip/blob/develop/tests/lib/__init__.py#L461-L485 maybe ? |
We don't need too. The pip tests have a bad habit of using whatever package the person writing the test happened to be using at the time. We should absolutely try to get rid of these things and use dummy packages. Ideally local test fixtures where possible.
|
Yeah, I just pulled that in because that's what it had, but I could certainly change it to use something a little smaller. The fixtures -- hmmm -- I guess I could see using fixtures for dynamically creating git repos (since you had to have git to clone the pip tree) and maybe mercurial because most folks probably have that, but I don't know if I would use that for bzr and then force people to have bzr on their system. I feel like for bzr I would rather commit a small package -- you guys okay with that? |
Surely if you don't have bzr the bzr tests won't work anyway, so needing bzr as a prerequisite to setting up a bzr test isn't any extra issue? Just don't try to create the bzr repo at the start of the test run and fail everything if you can't ;-) |
@pfmoore: Good point. I hadn't had my coffee yet. I could probably just skip the test if bzr is not available. Crap - now I need to look up how to use bzr again :) |
Sure. Anything to make the tests less fragile :-) |
Cool, I'm on it |
I just made a bunch of changes to I still need to do something similar for the |
BTW, I wouldn't tackle it here, but has there ever been discussion of moving all the VCS code out into some kind of plugins? That seems like it would remove a good bit of code from pip, though of course we'd have to come up with an API. |
d210efc
to
5cc6fa8
Compare
So we don't have to check in the entirety of django-wikiapp
I changed None of the tests in Travis CI tests are passing. I hope that this can be reviewed and merged soon so that the tests are less fragile. |
"""Test freezing a svn checkout""" | ||
|
||
checkout_path = local_checkout( | ||
'svn+http://svn.colorstudy.com/INITools/trunk', | ||
tmpdir.join("cache"), | ||
tmpdir.join("cache") |
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.
Spurious "tidy up" change, presumably?
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.
Oops. Fixed.
(Rats, got distracted by adding line comments and lost my larger comment-in-progress :-() Wow, that's a big change! A lot of it looks like a more or less opaque change to Any chance you could summarise what the various bits of the PR are about? You've got 8 separate commits here. Maybe you could summarise what each one does? (Maybe it would have been easier to review if the PR was split into separate PRs for each commit). I'll try to get a chance to review properly, but it could be some time before I'm able to. |
If bzr is present, we can't test.
I could split it into separate PRs, if that makes it easier to review. Little atomic chunks should be easier to understand and review quickly I think. |
Thanks, @msabramo - I'll go through the individual PRs. |
This can be closed as it was replaced by multiple small PRs:
|
This change:
from 90 seconds to 76 seconds. I think that a lot of the remaining time is spent on creating virtualenvs rather than downloading things.