Unit tests for tenant AdminCLI #850
Conversation
| self.datastore_name = datastore[0] | ||
| self.datastore_path = datastore[2] | ||
|
|
||
| if len(datastores) >= 1: |
There was a problem hiding this comment.
this fails when there is only one datastore available in the inventory. Can you please provide a fix or provide an explanation if I have misunderstood?
There was a problem hiding this comment.
It will not fail the test. Some test case need to run with at least two datastores. Those test will be skipped if the condition is not met.
There was a problem hiding this comment.
sorry for the confusion...
let's say we have only one datastore if len(datastores) >= 1: satisfies and failed at line:443.
Those test will be skipped if the condition is not met.
Can you please point me to the line which skips the TestTenant when only one datastore is found in the inventory?
Thanks!
| """ Test tenant functionality """ | ||
| # tenant1 info | ||
| tenant1_name = "test_tenant1" | ||
| vm1_name = 'test_vm1' |
There was a problem hiding this comment.
minor: it is better to randomize the vm name to avoid subsequent failures if cleanup doesn't happen correctly. We have done such exercise recently so it would be great to keep it consistent.
There was a problem hiding this comment.
@shuklanirdesh82 Could you point me what is the change you made to randomize the vm name? I want to take a look. Thanks!
There was a problem hiding this comment.
Comment addressed.
There was a problem hiding this comment.
Thanks @lipingxue !
| @@ -435,6 +444,7 @@ def test_remove_tenants(self): | |||
| description='Some tenant', | |||
| vms=vms, | |||
| privileges=privileges) | |||
|
|
|||
There was a problem hiding this comment.
It seems this file is having only empty newline, please remove this file from the review if it is an accidental change.
There was a problem hiding this comment.
overall looks good. Please squash the commits, and more important, outline somewhere what is covered by the test. All tenant-related CLI with all possible options ? Error cases handled?
Even if it's not compete I think we should merge the tests, but it is hard to understand what is covered and what is not.
| @@ -1097,6 +1105,7 @@ def test_vmdkops_on_different_tenants(self): | |||
|
|
|||
| @unittest.skipIf(len(vmdk_utils.get_datastores()) < 2, | |||
| "second datastore is not found - skipping this test") | |||
| @unittest.skip("Liping remove") | |||
There was a problem hiding this comment.
this looks like something you want to remove :-)
There was a problem hiding this comment.
Few minor changes are required.
-
some of the files are having only
new linechange ...
Can you please remove those file from this PR and keep only unit test related files? -
You may want to rebase with your master to avoid having unrelated changes. Seeing some of changes unrelated to your PR.
Thanks!
|
@shuklanirdesh82 I have already rebase with the master and there is no conflicts with the base branch. |
|
Thanks @lipingxue for addressing comments and providing explanation. You may want to squash the commit as I am seeing changes from other PRs as part of your changes. |
lipingxue
force-pushed
the
tenant_admin_cli.liping
branch
from
ed3de9a
to
2602e2d
Compare
lipingxue
force-pushed
the
tenant_admin_cli.liping
branch
from
2602e2d
to
287fce2
Compare
* Add unit test to test AdminCLI command for "tenant". * Add unit test for AdminCLI command for "tenant vm" and "tenant access".
|
Fixed #830 |
This change add unit tests for tenant AdminCLI.