From ce25789f4b0aaa03c8a6b60715a48e6ebe593491 Mon Sep 17 00:00:00 2001 From: williexu Date: Thu, 21 Jun 2018 18:13:18 -0700 Subject: [PATCH 1/6] fixed parser bug assuming value parsed is part of command --- src/azure-cli-core/azure/cli/core/parser.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/azure-cli-core/azure/cli/core/parser.py b/src/azure-cli-core/azure/cli/core/parser.py index ddc760adab3..c580c7bf5bb 100644 --- a/src/azure-cli-core/azure/cli/core/parser.py +++ b/src/azure-cli-core/azure/cli/core/parser.py @@ -150,18 +150,25 @@ def _check_value(self, action, value): # Override to customize the error message when a argument is not among the available choices # converted value must be one of the choices (if specified) if action.choices is not None and value not in action.choices: - error_msg = "{prog}: '{value}' is not an {prog} command. See '{prog} --help'.".format(prog=self.prog, - value=value) + if not self.command_source: + # parser has no `command_source`, value is part of command itself + error_msg = "{prog}: '{value}' is not in the '{prog}' command-group. See '{prog} --help'.".format( + prog=self.prog, value=value) + else: + # `command_source` indicates command values have been parsed, value is an argument + parameter = action.option_strings[0] if action.option_strings else action.dest + error_msg = "{prog}: '{value}' is not a valid value for '{param}'. See '{prog} --help'.".format( + prog=self.prog, value=value, param=parameter) telemetry.set_user_fault(error_msg) logger.error(error_msg) - candidates = difflib.get_close_matches(value, action.choices, cutoff=0.8) + candidates = difflib.get_close_matches(value, action.choices, cutoff=0.7) if candidates: print_args = { 's': 's' if len(candidates) > 1 else '', 'verb': 'are' if len(candidates) > 1 else 'is', 'value': value } - suggestion_msg = "\nThe most similar command{s} to '{value}' {verb}:\n".format(**print_args) + suggestion_msg = "\nThe most similar choice{s} to '{value}' {verb}:\n".format(**print_args) suggestion_msg += '\n'.join(['\t' + candidate for candidate in candidates]) print(suggestion_msg, file=sys.stderr) From cd6bf4baaf723cecf268d85e39c7247285cb81ed Mon Sep 17 00:00:00 2001 From: williexu Date: Mon, 25 Jun 2018 09:17:57 -0700 Subject: [PATCH 2/6] address feedback --- src/azure-cli-core/azure/cli/core/parser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/azure-cli-core/azure/cli/core/parser.py b/src/azure-cli-core/azure/cli/core/parser.py index c580c7bf5bb..5cf22943210 100644 --- a/src/azure-cli-core/azure/cli/core/parser.py +++ b/src/azure-cli-core/azure/cli/core/parser.py @@ -152,7 +152,7 @@ def _check_value(self, action, value): if action.choices is not None and value not in action.choices: if not self.command_source: # parser has no `command_source`, value is part of command itself - error_msg = "{prog}: '{value}' is not in the '{prog}' command-group. See '{prog} --help'.".format( + error_msg = "{prog}: '{value}' is not in the '{prog}' command group. See '{prog} --help'.".format( prog=self.prog, value=value) else: # `command_source` indicates command values have been parsed, value is an argument From bd3b25beb2aec99df71495bdc151b313bbd7db8a Mon Sep 17 00:00:00 2001 From: williexu Date: Mon, 25 Jun 2018 16:03:52 -0700 Subject: [PATCH 3/6] testing for parser spellcheck --- .../azure/cli/core/tests/test_parser.py | 119 +++++++++++++++++- 1 file changed, 118 insertions(+), 1 deletion(-) diff --git a/src/azure-cli-core/azure/cli/core/tests/test_parser.py b/src/azure-cli-core/azure/cli/core/tests/test_parser.py index 01e8eae1533..8f54f90aeb1 100644 --- a/src/azure-cli-core/azure/cli/core/tests/test_parser.py +++ b/src/azure-cli-core/azure/cli/core/tests/test_parser.py @@ -6,7 +6,8 @@ import mock import unittest from six import StringIO -from azure.cli.core import AzCommandsLoader +from collections import namedtuple +from azure.cli.core import AzCommandsLoader, MainCommandsLoader from azure.cli.core.commands import AzCliCommand from azure.cli.core.parser import AzCliCommandParser @@ -126,6 +127,59 @@ def test_handler(): args = parser.parse_args('test command --opt sNake_CASE'.split()) self.assertEqual(args.opt, 'snake_case') + @mock.patch('importlib.import_module', _mock_import_lib) + @mock.patch('pkgutil.iter_modules', _mock_iter_modules) + @mock.patch('azure.cli.core.commands._load_command_loader', _mock_load_command_loader) + @mock.patch('azure.cli.core.extension.get_extension_modname', _mock_extension_modname) + @mock.patch('azure.cli.core.extension.get_extensions', _mock_get_extensions) + def test_parser_error_spellchecker(self): + cli = TestCli() + main_loader = MainCommandsLoader(cli) + cli.loader = main_loader + cmd_tbl = cli.loader.load_command_table(None) + + parser = cli.parser_cls(cli) + parser.load_command_table(cmd_tbl) + + def mock_log_error(_, msg): + logger_msgs.append(msg) + + def mock_get_close_matches(*args, **kwargs): + import difflib + choices.append(difflib.get_close_matches(*args, **kwargs)) + + logger_msgs = [] + choice_lists = [] + + # run multiple faulty commands and save error logs, as well as close matches + with mock.patch('logging.Logger.error', mock_log_error): + faulty_cmd_args = [ + 'test module1 --opt enum_1', + 'test extension1 --opt enum_1', + 'test foo_bar --opt enum_3', + 'test module --opt enum_3', + 'test extension --opt enum_3' + ] + for text in faulty_cmd_args: + with self.assertRaises(SystemExit): + parser.parse_args(text.split()) + parser.parse_args('test module --opt enum_1'.split()) + + # assert the right type of error msg is logged for command vs argument parsing + for msg in logger_msgs[:2]: + self.assertIn("not in the 'azdev test' command group", msg) + for msg in logger_msgs[3:]: + self.assertIn("not a valid value for '--opt'.", msg) + + # assert the right choices are matched as "close". + # If these don't hold, matching algorithm should be deemed flawed. + for choices in choice_lists[:1]: + self.assertEqual(len(choices), 1) + self.assertEqual(len(choice_lists[2]), 1) + for choices in choice_lists[3:]: + self.assertEqual(len(choices), 2) + self.assertCountEqual(choices, ['enum_1', 'enum_2']) + class VerifyError(object): # pylint: disable=too-few-public-methods @@ -140,5 +194,68 @@ def __call__(self, message): self.called = True +def _mock_import_lib(_): + mock_obj = mock.MagicMock() + mock_obj.__path__ = __name__ + return mock_obj + + +def _mock_iter_modules(_): + return [(None, __name__, None)] + + +def _mock_extension_modname(ext_name, _): + return ext_name + + +def _mock_get_extensions(): + MockExtension = namedtuple('Extension', ['name', 'preview']) + return [MockExtension(name=__name__ + '.ExtCommandsLoader', preview=False), + MockExtension(name=__name__ + '.Ext2CommandsLoader', preview=False)] + + +def _mock_load_command_loader(loader, args, name, prefix): + from enum import Enum + + class TestEnum(Enum): # pylint: disable=too-few-public-methods + enum_1 = 'enum_1' + enum_2 = 'enum_2' + + def test_handler(): + pass + + class TestCommandsLoader(AzCommandsLoader): + def load_command_table(self, args): + super(TestCommandsLoader, self).load_command_table(args) + command = AzCliCommand(loader, 'test module', test_handler) + command.add_argument('opt', '--opt', required=True, **enum_choice_list(TestEnum)) + self.command_table['test module'] = command + return self.command_table + + # A command from an extension + class ExtCommandsLoader(AzCommandsLoader): + + def load_command_table(self, args): + super(ExtCommandsLoader, self).load_command_table(args) + command = AzCliCommand(loader, 'test extension', test_handler) + command.add_argument('opt', '--opt', required=True, **enum_choice_list(TestEnum)) + self.command_table['test extension'] = command + return self.command_table + + if prefix == 'azure.cli.command_modules.': + command_loaders = {'TestCommandsLoader': TestCommandsLoader} + else: + command_loaders = {'ExtCommandsLoader': ExtCommandsLoader} + + module_command_table = {} + for _, loader_cls in command_loaders.items(): + command_loader = loader_cls(cli_ctx=loader.cli_ctx) + command_table = command_loader.load_command_table(args) + if command_table: + module_command_table.update(command_table) + loader.loaders.append(command_loader) # this will be used later by the load_arguments method + return module_command_table + + if __name__ == '__main__': unittest.main() From f61d3ca7862f2461cbf5926a2c6e9cbd73838df2 Mon Sep 17 00:00:00 2001 From: williexu Date: Mon, 25 Jun 2018 16:31:09 -0700 Subject: [PATCH 4/6] fixes to spellchecker test fix --- .../azure/cli/core/tests/test_parser.py | 146 +++++++++--------- 1 file changed, 74 insertions(+), 72 deletions(-) diff --git a/src/azure-cli-core/azure/cli/core/tests/test_parser.py b/src/azure-cli-core/azure/cli/core/tests/test_parser.py index 8f54f90aeb1..4af6d11e68c 100644 --- a/src/azure-cli-core/azure/cli/core/tests/test_parser.py +++ b/src/azure-cli-core/azure/cli/core/tests/test_parser.py @@ -5,6 +5,7 @@ import mock import unittest +import difflib from six import StringIO from collections import namedtuple from azure.cli.core import AzCommandsLoader, MainCommandsLoader @@ -127,6 +128,64 @@ def test_handler(): args = parser.parse_args('test command --opt sNake_CASE'.split()) self.assertEqual(args.opt, 'snake_case') + def _mock_import_lib(_): + mock_obj = mock.MagicMock() + mock_obj.__path__ = __name__ + return mock_obj + + def _mock_iter_modules(_): + return [(None, __name__, None)] + + def _mock_extension_modname(ext_name, ext_dir): + return ext_name + + def _mock_get_extensions(): + MockExtension = namedtuple('Extension', ['name', 'preview']) + return [MockExtension(name=__name__ + '.ExtCommandsLoader', preview=False), + MockExtension(name=__name__ + '.Ext2CommandsLoader', preview=False)] + + def _mock_load_command_loader(loader, args, name, prefix): + from enum import Enum + + class TestEnum(Enum): # pylint: disable=too-few-public-methods + enum_1 = 'enum_1' + enum_2 = 'enum_2' + + def test_handler(): + pass + + class TestCommandsLoader(AzCommandsLoader): + def load_command_table(self, args): + super(TestCommandsLoader, self).load_command_table(args) + command = AzCliCommand(loader, 'test module', test_handler) + command.add_argument('opt', '--opt', required=True, **enum_choice_list(TestEnum)) + self.command_table['test module'] = command + return self.command_table + + # A command from an extension + class ExtCommandsLoader(AzCommandsLoader): + + def load_command_table(self, args): + super(ExtCommandsLoader, self).load_command_table(args) + command = AzCliCommand(loader, 'test extension', test_handler) + command.add_argument('opt', '--opt', required=True, **enum_choice_list(TestEnum)) + self.command_table['test extension'] = command + return self.command_table + + if prefix == 'azure.cli.command_modules.': + command_loaders = {'TestCommandsLoader': TestCommandsLoader} + else: + command_loaders = {'ExtCommandsLoader': ExtCommandsLoader} + + module_command_table = {} + for _, loader_cls in command_loaders.items(): + command_loader = loader_cls(cli_ctx=loader.cli_ctx) + command_table = command_loader.load_command_table(args) + if command_table: + module_command_table.update(command_table) + loader.loaders.append(command_loader) # this will be used later by the load_arguments method + return module_command_table + @mock.patch('importlib.import_module', _mock_import_lib) @mock.patch('pkgutil.iter_modules', _mock_iter_modules) @mock.patch('azure.cli.core.commands._load_command_loader', _mock_load_command_loader) @@ -136,23 +195,25 @@ def test_parser_error_spellchecker(self): cli = TestCli() main_loader = MainCommandsLoader(cli) cli.loader = main_loader + cmd_tbl = cli.loader.load_command_table(None) parser = cli.parser_cls(cli) parser.load_command_table(cmd_tbl) + logger_msgs = [] + choice_lists = [] + original_get_close_matches = difflib.get_close_matches + def mock_log_error(_, msg): logger_msgs.append(msg) def mock_get_close_matches(*args, **kwargs): - import difflib - choices.append(difflib.get_close_matches(*args, **kwargs)) - - logger_msgs = [] - choice_lists = [] + choice_lists.append(original_get_close_matches(*args, **kwargs)) # run multiple faulty commands and save error logs, as well as close matches - with mock.patch('logging.Logger.error', mock_log_error): + with mock.patch('logging.Logger.error', mock_log_error), \ + mock.patch('difflib.get_close_matches', mock_get_close_matches): faulty_cmd_args = [ 'test module1 --opt enum_1', 'test extension1 --opt enum_1', @@ -166,16 +227,20 @@ def mock_get_close_matches(*args, **kwargs): parser.parse_args('test module --opt enum_1'.split()) # assert the right type of error msg is logged for command vs argument parsing - for msg in logger_msgs[:2]: + self.assertEqual(len(logger_msgs), 5) + for msg in logger_msgs[:3]: self.assertIn("not in the 'azdev test' command group", msg) for msg in logger_msgs[3:]: self.assertIn("not a valid value for '--opt'.", msg) # assert the right choices are matched as "close". # If these don't hold, matching algorithm should be deemed flawed. - for choices in choice_lists[:1]: + print(choice_lists) + for x in choice_lists: + print(x) + for choices in choice_lists[:2]: self.assertEqual(len(choices), 1) - self.assertEqual(len(choice_lists[2]), 1) + self.assertEqual(len(choice_lists[2]), 0) for choices in choice_lists[3:]: self.assertEqual(len(choices), 2) self.assertCountEqual(choices, ['enum_1', 'enum_2']) @@ -194,68 +259,5 @@ def __call__(self, message): self.called = True -def _mock_import_lib(_): - mock_obj = mock.MagicMock() - mock_obj.__path__ = __name__ - return mock_obj - - -def _mock_iter_modules(_): - return [(None, __name__, None)] - - -def _mock_extension_modname(ext_name, _): - return ext_name - - -def _mock_get_extensions(): - MockExtension = namedtuple('Extension', ['name', 'preview']) - return [MockExtension(name=__name__ + '.ExtCommandsLoader', preview=False), - MockExtension(name=__name__ + '.Ext2CommandsLoader', preview=False)] - - -def _mock_load_command_loader(loader, args, name, prefix): - from enum import Enum - - class TestEnum(Enum): # pylint: disable=too-few-public-methods - enum_1 = 'enum_1' - enum_2 = 'enum_2' - - def test_handler(): - pass - - class TestCommandsLoader(AzCommandsLoader): - def load_command_table(self, args): - super(TestCommandsLoader, self).load_command_table(args) - command = AzCliCommand(loader, 'test module', test_handler) - command.add_argument('opt', '--opt', required=True, **enum_choice_list(TestEnum)) - self.command_table['test module'] = command - return self.command_table - - # A command from an extension - class ExtCommandsLoader(AzCommandsLoader): - - def load_command_table(self, args): - super(ExtCommandsLoader, self).load_command_table(args) - command = AzCliCommand(loader, 'test extension', test_handler) - command.add_argument('opt', '--opt', required=True, **enum_choice_list(TestEnum)) - self.command_table['test extension'] = command - return self.command_table - - if prefix == 'azure.cli.command_modules.': - command_loaders = {'TestCommandsLoader': TestCommandsLoader} - else: - command_loaders = {'ExtCommandsLoader': ExtCommandsLoader} - - module_command_table = {} - for _, loader_cls in command_loaders.items(): - command_loader = loader_cls(cli_ctx=loader.cli_ctx) - command_table = command_loader.load_command_table(args) - if command_table: - module_command_table.update(command_table) - loader.loaders.append(command_loader) # this will be used later by the load_arguments method - return module_command_table - - if __name__ == '__main__': unittest.main() From 7a1427bdcaadff8f3c4932e670fd2cbda2e7da74 Mon Sep 17 00:00:00 2001 From: williexu Date: Mon, 25 Jun 2018 17:50:09 -0700 Subject: [PATCH 5/6] msg edit in test --- src/azure-cli-core/azure/cli/core/tests/test_parser.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/azure-cli-core/azure/cli/core/tests/test_parser.py b/src/azure-cli-core/azure/cli/core/tests/test_parser.py index 4af6d11e68c..43a2d0004d4 100644 --- a/src/azure-cli-core/azure/cli/core/tests/test_parser.py +++ b/src/azure-cli-core/azure/cli/core/tests/test_parser.py @@ -229,7 +229,8 @@ def mock_get_close_matches(*args, **kwargs): # assert the right type of error msg is logged for command vs argument parsing self.assertEqual(len(logger_msgs), 5) for msg in logger_msgs[:3]: - self.assertIn("not in the 'azdev test' command group", msg) + self.assertIn("not in the", msg) + self.assertIn("command group", msg) for msg in logger_msgs[3:]: self.assertIn("not a valid value for '--opt'.", msg) From f43b0343174d670a22d15392d556791be7835d15 Mon Sep 17 00:00:00 2001 From: Willie Xu Date: Mon, 25 Jun 2018 18:46:21 -0700 Subject: [PATCH 6/6] assertCountEqual not in python2 --- src/azure-cli-core/azure/cli/core/tests/test_parser.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/azure-cli-core/azure/cli/core/tests/test_parser.py b/src/azure-cli-core/azure/cli/core/tests/test_parser.py index 43a2d0004d4..301a612d89a 100644 --- a/src/azure-cli-core/azure/cli/core/tests/test_parser.py +++ b/src/azure-cli-core/azure/cli/core/tests/test_parser.py @@ -236,15 +236,13 @@ def mock_get_close_matches(*args, **kwargs): # assert the right choices are matched as "close". # If these don't hold, matching algorithm should be deemed flawed. - print(choice_lists) - for x in choice_lists: - print(x) for choices in choice_lists[:2]: self.assertEqual(len(choices), 1) self.assertEqual(len(choice_lists[2]), 0) for choices in choice_lists[3:]: self.assertEqual(len(choices), 2) - self.assertCountEqual(choices, ['enum_1', 'enum_2']) + for choice in ['enum_1', 'enum_2']: + self.assertIn(choice, choices) class VerifyError(object): # pylint: disable=too-few-public-methods