-
Notifications
You must be signed in to change notification settings - Fork 44
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 some tests for Docker repository C. #53
Conversation
5a8da68
to
f6e7224
Compare
@@ -0,0 +1,88 @@ | |||
""" |
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.
Nitpick: can I get an encoding declaration, a change in format and unicode literals?
# coding=utf-8
"""Test CRUD for Docker repositories.
This module contains tests for creating Docker repositories. It is intended to
also contain read, update, and delete tests.
"""
from __future__ import unicode_literals
# other imports here
(#24 is a related low-priority issue.)
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.
Sure thing, this is fixed now.
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.
👍
FYI, none of these modules will be included in the documentation right now. You can prove that to yourself by executing Do you have any test output? I can run these tests against my local VMs, but it's handy to have some basic evidence that tests are working before I review the tests. 😄 (For the record, I create a test matrix like this before merging any significant PR.) Again, don't stress out about providing a full test matrix. I can easily do that. |
This PR looks fine. If you can take a few minutes to address my nitpicks, I'll happily add in the documentation stubs, run a test matrix and merge this. |
(The RUD will be added later…)
f6e7224
to
766b4c9
Compare
I need to go fix some Docker bugs that are "on fire" so I don't have time to do the docs yet. Also, that link to the test matrix is a 404, but will you accept this statement as evidence? I ran these tests and they passed:
The failures happened before I wrote any code, so it seems that there is a pre-existing problem. |
Fixes are available on the master branch. |
Merged as 129f682. Thank you! |
(The RUD will be added later…)