Add unit tests for coriolisclient.cli: shell, utils and formatter modules
#85
Conversation
Cristi1324
force-pushed
the
add_cli_unit_tests_3
branch
2 times, most recently
from
6cc2d4a to
d09571f
Compare
Cristi1324
changed the title
coriolisclient.cli.shell and coriolisclient.cli.utils modulescoriolisclient.cli shell, utils and formatter modules
Cristi1324
changed the title
coriolisclient.cli shell, utils and formatter modulescoriolisclient.cli: shell, utils and formatter modules
Cristi1324
force-pushed
the
add_cli_unit_tests_3
branch
2 times, most recently
from
77c0323 to
bb62a9e
Compare
| def setUp(self): | ||
| def _get_formatted_data(self, obj): | ||
| return mock.sentinel.data | ||
| setattr(formatter.EntityFormatter, '_get_formatted_data', |
There was a problem hiding this comment.
Instead of doing this, let's use the EntityFormatter as it was supposed to be used, and that is by creating a mock class that inherits from this tested mixin class. You just need to "implement" the _get_formatted_data method and then you can test the rest of the methods as below.
| ): | ||
| obj_list = ["obj2", "obj1"] | ||
| mock_get_sorted_list.return_value = ["obj1", "obj2"] | ||
| mock_get_generic_data.side_effect = [ |
There was a problem hiding this comment.
If you implement _get_formatted_data for your test class, you won't need to mock this, and also you wouldn't have to use next() in results (especially if you just return the passed obj). Let's give it a simpler look please
| result | ||
| ) | ||
|
|
||
| def testget_formatted_entity(self): |
| setattr(args, key, value) | ||
| endpoint_filter_kwargs = {"filter_arg": "mock_filter_arg"} | ||
| mock_get_endpoint_filter_kwargs.return_value = endpoint_filter_kwargs | ||
| call_args = data.get("call_args", None) |
There was a problem hiding this comment.
Pro tip: unless set, get() method's value argument always defaults to None, you don't need to explicitly set it
| result | ||
| ) | ||
|
|
||
| if create_keystone_session_args is None: |
There was a problem hiding this comment.
I'd prefer if you'd change this condition to if args.no_auth:
Makes more sense, because if auth is otherwise set, a session needs to be created, hence create_keystone_session is called
| mock_register_session_argparse_arguments.assert_called_once_with( | ||
| mock_build_option_parser.return_value) | ||
|
|
||
| @mock.patch.object(os, 'environ') |
There was a problem hiding this comment.
Please actually use os.environ instead of mocking it: https://docs.python.org/3/library/os.html#os.environ
| ) | ||
| def test_compose_user_scripts(self, data): | ||
| global_scripts = data["global_scripts"] | ||
| global_scripts = data["global_scripts"] |
There was a problem hiding this comment.
Please remove duplicate line
| }, | ||
| { | ||
| "global_scripts": ["linux="], | ||
| "instance_scripts": ["linux="], |
There was a problem hiding this comment.
Please don't input os_type as instance. It should be something like instance_1
Cristi1324
force-pushed
the
add_cli_unit_tests_3
branch
from
bc98c1f to
cffaca8
Compare
| utils.add_storage_mappings_arguments_to_parser(parser) | ||
| args = parser.parse_args( | ||
| ['--storage-backend-mapping', 'mock_source=mock_destination', | ||
| '--disk-storage-mapping', 'mock_disk_id=mock_destination']) |
There was a problem hiding this comment.
Please add a default_storage_backend also. Great test btw, I love it!
|
|
||
| result = utils.parse_storage_mappings(None) | ||
|
|
||
| self.assertEqual( | ||
| (None, {}, {}), | ||
| result | ||
| ) |
There was a problem hiding this comment.
Please move this into a separate test method, call it test_parse_storageg_mappings_none or something like that.
| ) | ||
|
|
||
| def test_add_args_for_json_option_to_parser(self): | ||
| parser = mock.Mock() |
There was a problem hiding this comment.
I really loved the approach of using an empty ArgumentParser to test these kinds of methods. Can you please also do that here?
| script_path = os.path.join(script_path, 'data') | ||
| script_path = os.path.join(script_path, 'user_scripts.yml') |
There was a problem hiding this comment.
You can simplify this by:
| script_path = os.path.join(script_path, 'data') | |
| script_path = os.path.join(script_path, 'user_scripts.yml') | |
| script_path = os.path.join(script_path, 'data/user_scripts.yml') |
Cristi1324
force-pushed
the
add_cli_unit_tests_3
branch
3 times, most recently
from
24512ac to
917d782
Compare
| "args": { | ||
| "no_auth": "mock_no_auth", | ||
| "os_auth_url": "mock_os_auth_url" | ||
| } | ||
| }, | ||
| { | ||
| "args": { | ||
| "no_auth": "mock_no_auth", | ||
| "os_auth_url": "mock_os_auth_url" |
There was a problem hiding this comment.
These two cases seem to be the same.
Cristi1324
force-pushed
the
add_cli_unit_tests_3
branch
3 times, most recently
from
50fbbb0 to
27b499c
Compare
…rom `coriolisclient/cli/utils.py`
Cristi1324
force-pushed
the
add_cli_unit_tests_3
branch
from
27b499c to
14f2ba8
Compare
2 participants
Add unit tests for the following modules:
coriolisclient.cli.formattercoriolisclient.cli.shellcoriolisclient.cli.utils