From d54bc09fae6ed02bf0da12501c1fb38cbe2f4b5e Mon Sep 17 00:00:00 2001 From: Matt Clay Date: Mon, 13 Feb 2017 18:49:36 -0800 Subject: [PATCH] Consider module_utils deps when running tests. (#21382) * Skip pep8 analysis when --explain is used. * Fix return type annotations. * Match line length requirement of PEP 8 config. * Consider module_utils deps when running tests. --- test/runner/lib/classification.py | 88 +++++---- test/runner/lib/delegation.py | 2 +- test/runner/lib/executor.py | 5 +- test/runner/lib/import_analysis.py | 167 ++++++++++++++++++ test/runner/lib/util.py | 2 +- test/sanity/code-smell/pylint-ansible-test.sh | 2 +- 6 files changed, 229 insertions(+), 37 deletions(-) create mode 100644 test/runner/lib/import_analysis.py diff --git a/test/runner/lib/classification.py b/test/runner/lib/classification.py index 14376d12cc1..9145e55171f 100644 --- a/test/runner/lib/classification.py +++ b/test/runner/lib/classification.py @@ -3,6 +3,7 @@ from __future__ import absolute_import, print_function import os +import time from lib.target import ( walk_module_targets, @@ -17,6 +18,10 @@ from lib.util import ( display, ) +from lib.import_analysis import ( + get_python_module_utils_imports, +) + def categorize_changes(paths, verbose_command=None): """ @@ -35,6 +40,26 @@ def categorize_changes(paths, verbose_command=None): 'network-integration': set(), } + additional_paths = set() + + for path in paths: + dependent_paths = mapper.get_dependent_paths(path) + + if not dependent_paths: + continue + + display.info('Expanded "%s" to %d dependent file(s):' % (path, len(dependent_paths)), verbosity=1) + + for dependent_path in dependent_paths: + display.info(dependent_path, verbosity=1) + additional_paths.add(dependent_path) + + additional_paths -= set(paths) # don't count changed paths as additional paths + + if additional_paths: + display.info('Expanded %d changed file(s) into %d additional dependent file(s).' % (len(paths), len(additional_paths))) + paths = sorted(set(paths) | additional_paths) + display.info('Mapping %d changed file(s) to tests.' % len(paths)) for path in paths: @@ -98,6 +123,35 @@ class PathMapper(object): self.prefixes = load_integration_prefixes() + self.python_module_utils_imports = {} # populated on first use to reduce overhead when not needed + + def get_dependent_paths(self, path): + """ + :type path: str + :rtype: list[str] + """ + name, ext = os.path.splitext(os.path.split(path)[1]) + + if path.startswith('lib/ansible/module_utils/'): + if ext == '.py': + return self.get_python_module_utils_usage(name) + + return [] + + def get_python_module_utils_usage(self, name): + """ + :type name: str + :rtype: list[str] + """ + if not self.python_module_utils_imports: + display.info('Analyzing python module_utils imports...') + before = time.time() + self.python_module_utils_imports = get_python_module_utils_imports(self.compile_targets) + after = time.time() + display.info('Processed %d python module_utils in %d second(s).' % (len(self.python_module_utils_imports), after - before)) + + return sorted(self.python_module_utils_imports.get(name, set())) + def classify(self, path): """ :type path: str @@ -174,39 +228,7 @@ class PathMapper(object): } if ext == '.py': - network_utils = ( - 'netcfg', - 'netcli', - 'network_common', - 'network', - ) - - if name in network_utils: - return { - 'network-integration': 'network/', # target all network platforms - 'units': 'all', - } - - if name in self.prefixes and self.prefixes[name] == 'network': - network_target = 'network/%s/' % name - - if network_target in self.integration_targets_by_alias: - return { - 'network-integration': network_target, - 'units': 'all', - } - - display.warning('Integration tests for "%s" not found.' % network_target) - - return { - 'units': 'all', - } - - return { - 'integration': 'all', - 'network-integration': 'all', - 'units': 'all', - } + return minimal # already expanded using get_dependent_paths if path.startswith('lib/ansible/plugins/connection/'): if name == '__init__': diff --git a/test/runner/lib/delegation.py b/test/runner/lib/delegation.py index c351e181f72..7f195d97b06 100644 --- a/test/runner/lib/delegation.py +++ b/test/runner/lib/delegation.py @@ -341,7 +341,7 @@ def generate_command(args, path, options, exclude, require): :type options: dict[str, int] :type exclude: list[str] :type require: list[str] - :return: list[str] + :rtype: list[str] """ options['--color'] = 1 diff --git a/test/runner/lib/executor.py b/test/runner/lib/executor.py index 544259bb138..7ec8543653b 100644 --- a/test/runner/lib/executor.py +++ b/test/runner/lib/executor.py @@ -165,7 +165,7 @@ def generate_egg_info(args): def generate_pip_install(command): """ :type command: str - :return: list[str] | None + :rtype: list[str] | None """ constraints = 'test/runner/requirements/constraints.txt' requirements = 'test/runner/requirements/%s.txt' % command @@ -861,6 +861,9 @@ def command_sanity_pep8(args, targets): if stderr: raise SubprocessError(cmd=cmd, status=status, stderr=stderr) + if args.explain: + return + pattern = '^(?P[^:]*):(?P[0-9]+):(?P[0-9]+): (?P[A-Z0-9]{4}) (?P.*)$' results = [re.search(pattern, line).groupdict() for line in stdout.splitlines()] diff --git a/test/runner/lib/import_analysis.py b/test/runner/lib/import_analysis.py new file mode 100644 index 00000000000..aeebccf6e8d --- /dev/null +++ b/test/runner/lib/import_analysis.py @@ -0,0 +1,167 @@ +"""Analyze python import statements.""" + +from __future__ import absolute_import, print_function + +import ast +import os +import uuid + +from lib.util import ( + display, + ApplicationError, +) + + +def get_python_module_utils_imports(compile_targets): + """Return a dictionary of python file paths mapped to sets of module_utils names. + :type compile_targets: list[TestTarget] + :rtype: dict[str, set[str]] + """ + module_utils_files = (os.path.splitext(filename) for filename in os.listdir('lib/ansible/module_utils')) + module_utils = sorted(name[0] for name in module_utils_files if name[0] != '__init__' and name[1] == '.py') + + imports_by_target_path = {} + + for target in compile_targets: + imports_by_target_path[target.path] = extract_python_module_utils_imports(target.path, module_utils) + + def recurse_import(import_name, depth=0, seen=None): + """Recursively expand module_utils imports from module_utils files. + :type import_name: str + :type depth: int + :type seen: set[str] | None + :rtype set[str] + """ + display.info('module_utils import: %s%s' % (' ' * depth, import_name), verbosity=4) + + if seen is None: + seen = set([import_name]) + + results = set([import_name]) + + import_path = os.path.join('lib/ansible/module_utils', '%s.py' % import_name) + + for name in sorted(imports_by_target_path.get(import_path, set())): + if name in seen: + continue + + seen.add(name) + + matches = sorted(recurse_import(name, depth + 1, seen)) + + for result in matches: + results.add(result) + + return results + + for module_util in module_utils: + # recurse over module_utils imports while excluding self + module_util_imports = recurse_import(module_util) + module_util_imports.remove(module_util) + + # add recursive imports to all path entries which import this module_util + for target_path in imports_by_target_path: + if module_util in imports_by_target_path[target_path]: + for module_util_import in sorted(module_util_imports): + if module_util_import not in imports_by_target_path[target_path]: + display.info('%s inherits import %s via %s' % (target_path, module_util_import, module_util), verbosity=6) + imports_by_target_path[target_path].add(module_util_import) + + imports = dict([(module_util, set()) for module_util in module_utils]) + + for target_path in imports_by_target_path: + for module_util in imports_by_target_path[target_path]: + imports[module_util].add(target_path) + + for module_util in sorted(imports): + if not len(imports[module_util]): + display.warning('No imports found which use the "%s" module_util.' % module_util) + + return imports + + +def extract_python_module_utils_imports(path, module_utils): + """Return a list of module_utils imports found in the specified source file. + :type path: str + :type module_utils: set[str] + :rtype: set[str] + """ + with open(path, 'r') as module_fd: + code = module_fd.read() + + try: + tree = ast.parse(code) + except SyntaxError as ex: + # Setting the full path to the filename results in only the filename being given for str(ex). + # As a work-around, set the filename to a UUID and replace it in the final string output with the actual path. + ex.filename = str(uuid.uuid4()) + error = str(ex).replace(ex.filename, path) + raise ApplicationError('AST parse error: %s' % error) + + finder = ModuleUtilFinder(path, module_utils) + finder.visit(tree) + return finder.imports + + +class ModuleUtilFinder(ast.NodeVisitor): + """AST visitor to find valid module_utils imports.""" + def __init__(self, path, module_utils): + """Return a list of module_utils imports found in the specified source file. + :type path: str + :type module_utils: set[str] + """ + super(ModuleUtilFinder, self).__init__() + + self.path = path + self.module_utils = module_utils + self.imports = set() + + # noinspection PyPep8Naming + # pylint: disable=locally-disabled, invalid-name + def visit_Import(self, node): + """ + :type node: ast.Import + """ + self.generic_visit(node) + + for alias in node.names: + if alias.name.startswith('ansible.module_utils.'): + # import ansible.module_utils.MODULE[.MODULE] + self.add_import(alias.name.split('.')[2], node.lineno) + + # noinspection PyPep8Naming + # pylint: disable=locally-disabled, invalid-name + def visit_ImportFrom(self, node): + """ + :type node: ast.ImportFrom + """ + self.generic_visit(node) + + if not node.module: + return + + if node.module == 'ansible.module_utils': + for alias in node.names: + # from ansible.module_utils import MODULE[, MODULE] + self.add_import(alias.name, node.lineno) + elif node.module.startswith('ansible.module_utils.'): + # from ansible.module_utils.MODULE[.MODULE] + self.add_import(node.module.split('.')[2], node.lineno) + + def add_import(self, name, line_number): + """ + :type name: str + :type line_number: int + """ + if name in self.imports: + return # duplicate imports are ignored + + if name not in self.module_utils: + if self.path.startswith('test/'): + return # invalid imports in tests are ignored + + raise Exception('%s:%d Invalid module_util import: %s' % (self.path, line_number, name)) + + display.info('%s:%d imports module_utils: %s' % (self.path, line_number, name), verbosity=5) + + self.imports.add(name) diff --git a/test/runner/lib/util.py b/test/runner/lib/util.py index 0326c27089b..f500ba480eb 100644 --- a/test/runner/lib/util.py +++ b/test/runner/lib/util.py @@ -227,7 +227,7 @@ def deepest_path(path_a, path_b): """Return the deepest of two paths, or None if the paths are unrelated. :type path_a: str :type path_b: str - :return: str | None + :rtype: str | None """ if path_a == '.': path_a = '' diff --git a/test/sanity/code-smell/pylint-ansible-test.sh b/test/sanity/code-smell/pylint-ansible-test.sh index 5cc56050a9c..cf59733a0fa 100755 --- a/test/sanity/code-smell/pylint-ansible-test.sh +++ b/test/sanity/code-smell/pylint-ansible-test.sh @@ -2,7 +2,7 @@ cd test/runner/ -pylint --max-line-length=120 --reports=n ./*.py ./*/*.py \ +pylint --max-line-length=160 --reports=n ./*.py ./*/*.py \ --jobs 2 \ --rcfile /dev/null \ --function-rgx '[a-z_][a-z0-9_]{2,40}$' \