-
Notifications
You must be signed in to change notification settings - Fork 59
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 initial unit tests for gateway.py #253
base: main
Are you sure you want to change the base?
Conversation
test/test_gateway.py
Outdated
return self.run_ceph_mock_return.pop() | ||
|
||
@mock.patch.object(gateway.subprocess, 'check_output') | ||
def test__run_ceph_cmd(self, check_output): |
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.
s/test__run_ceph_cmd/test_run_ceph_cmd/ ?
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 method being tested starts with an underscore _run_ceph_cmd
. Hence the double underscore in the test name. I'm happy to change the test name though if you'd prefer test_run_ceph_cmd
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.
Yeah, please.
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.
At the same time could update the README
to give some explaination and a example for the 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.
Hi @lxbsz, thank you for the review. I can't think what would be relevant to put in the README since this PR is just adding some unit tests or are you suggesting I add doc strings to the tests?
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 am how to run the test. You need to update the exact command and what should others do to run the test.
Such as the Curl Examples:
.
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 thought the README was aimed at users who want to install, configure and use ceph-iscsi. The Curl Examples
in the README is aimed at users who want to interact with the api via curl. Running unit tests seems to me to only be of interest to developers. Having said that I could add something like
diff --git a/README b/README
index fe32bd2..b0c7455 100644
--- a/README
+++ b/README
@@ -135,3 +135,9 @@ here:
http://docs.ceph.com/docs/master/rbd/iscsi-target-cli/
can be used to create a iscsi-gateway.cfg and create a target.
+
+## Running Unit Tests
+
+The unit tests for ceph-isci can be run with:
+
+```tox -e py3```
Is that the sort of thing you are looking for?
9d697e1
to
de69fff
Compare
Add some intial unit tests to gateway.py
de69fff
to
9e89d4b
Compare
Add some intial unit tests to gateway.py