-
Notifications
You must be signed in to change notification settings - Fork 83
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
[RFR] Added openshift tests + Minor fixes in image info parsing #166
Conversation
gshefer
commented
Aug 10, 2017
•
edited
Loading
edited
- Added openshift tests (tests/test_openshift.py)
- Option to run tests with both mock provider and real provider, default is mock.
- Fix image info parsing which was problematic in case we had more than one ':' in the image name. (happens when we have tag, in this case we have 2, one for the ID and one for the tag)
3f24b91
to
ed8bea7
Compare
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.
Not bad for start, some changes needed, maybe @RonnyPfannschmidt can chime in as well?
tests/test_openshift.py
Outdated
|
||
def mocked_image_data(): | ||
out = [200, {'items': []}] | ||
for i in xrange(fauxfactory.gen_integer(2, 20)): |
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.
xrange is not supported in python3
tests/test_openshift.py
Outdated
# If you prefer to use a real provider, fill HOSTNAME, USERNAME and TOKEN | ||
HOSTNAME = None | ||
USERNAME = None | ||
TOKEN = None |
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.
These should probably be pulled from environment.
tests/test_openshift.py
Outdated
assert wait_for(lambda: label_key in resource.list_labels(), | ||
message="Waiting for label {} of {} {} to be exist..." | ||
.format(label_key, resource.__class__.__name__, resource.name), | ||
delay=5, timeout='1M') |
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.
make a newline before lambda, that will free up some space on the line, and use type(resource)
instead of resource.__class__
. That may then fit the .format
on the previous line, making it look better.
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.
And wait_for returns a tuple if I am not mistaken, you may want to grab the actual value returned. Or you can just remove the assert and go with TimedOutError.
tests/test_openshift.py
Outdated
provider.o_api.get.return_value = provider.api.get.return_value = mocked_image_data() | ||
provider.list_docker_image() | ||
provider.list_image() | ||
provider.list_image_openshift() |
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.
this should check that the output is correct.
tests/test_openshift.py
Outdated
gen_service.provider.api.get.return_value = [200, { | ||
'spec': { | ||
'sessionAffinity': 'ClientIP', | ||
'clusterIP': '123.456.789.101' |
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.
This should probably use some valid local network IP.
tests/test_openshift.py
Outdated
res = gen_dc.create() | ||
assert res[0] in (200, 201) | ||
assert wait_for(lambda: gen_dc.exists(), | ||
message="Waiting for dc {} to be exist..." |
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.
Just "to exist", otherwise all your base are belong to us :)
ed8bea7
to
92c23e5
Compare
|
||
|
||
# Specify whether to use a mock provider or real one. | ||
MOCKED = os.environ.get('MOCKED', 'true').lower() == 'true' |
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.
just for documentation we might want to consider putting the mocked/non-mocked variants into plugin objects at some point
while doing that we could also create a better handling than the env vars provide
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.
Issue created: #170
Assigning to mfalesni since he just needs to re-review |