-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add PDCClient tests #137
Add PDCClient tests #137
Conversation
7f5ff35
to
ee581f1
Compare
Keeping compatibility with Python 2.6 sucks - still have to fix some tests. |
Yes, I think we can consider to remove the 2.6 test in the future and add update the 3.3 to 3.5.
|
cc0b57c
to
7522947
Compare
While adding tests for rest of the API I found few issues. Fixes are included.
Added.
This is already tested on few places. I want to have simple and small test methods for testing just specific part of API. |
Here's the pip installs for pdc-client from PyPI for the last month -- a single 2.6 and no 3.3 (both are EOL):
By the way, you could add both 3.5 and 3.6 to Travis etc. More reasons for dropping old ones: 2.6
3.2
3.3
|
tests/api/tests.py
Outdated
"description": "RHEL 7 Workstation", | ||
"id": 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.
Is the cpe returned data in our product something like below one?
[
{
"id": 1,
"cpe": "cpe:test",
"description": "1234"
}
]
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.
The mocked server returns paged response when making GET HTTP request to rest_api/v1/cpe/
.
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.
So the data is like the data in db, got that, thx.
def test_set_comment(self): | ||
self.client.set_comment('TEST') | ||
self.client.cpes[1] += {'description': 'TEST'} | ||
self.assertEqual(_MockPDCServerRequestHandler.last_comment, 'TEST') |
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 we should also add the getitem way in each of the test function, like client["cpes"].
This is already tested on few places. I want to have simple and small test methods for testing just specific part of API.
I think we can add codes here to test patch as item, not mean add more test functions.
self.client['cpes'][1] += {'description': 'TEST1'}
self.assertEqual(_MockPDCServerRequestHandler.last_comment, 'TEST1')
Maybe you mean self.client.cpes[1] is the case that gets resource by item "[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.
OK, I'll add that. Also patching using +=
for attribute would be nice.
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.
cool
I just found I keep the comment there for several days, but not submit it,sorry about that... |
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.
Nice patch. Just one minor thing related to the test case
tests/api/tests.py
Outdated
ssl_verify=False, | ||
) | ||
|
||
def test_request_response(self): |
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 don't quite understand this case. Is this testing the Mock server setup correctly? I think other cases already cover this.
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, I'll remove it. It was the first test just to verify the server is running.
I simplified the server code a lot (no regexps and should be easier to modify). I also added few more tests for PUT and PATCH. |
pdc_client/__init__.py
Outdated
@@ -361,17 +369,14 @@ def worker(): | |||
response = self.client(*args, **kwargs) | |||
if isinstance(response, list): | |||
yield response | |||
else: | |||
elif _is_page(response): |
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.
If you change like this, it seems that when the response is a list, the "while true" seems not break?
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.
Nice catch. Fixed and added test for this.
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.
cool
tests/api/tests.py
Outdated
"description": "RHEL 7 Workstation", | ||
"id": 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.
So the data is like the data in db, got that, thx.
Reivewed again. LGTM |
Fixes strigifying PDCClient. Fixes throwing NoResultsError.
No description provided.