From 707eea3afab2b7f936c7c822de3d78f25f7dccb8 Mon Sep 17 00:00:00 2001 From: Matt Clay Date: Fri, 20 Mar 2020 16:31:20 -0700 Subject: [PATCH] Add new options to `hacking/shippable/incidental.py` (#68384) * Add `--plugin-path` option to `incidental.py`. * Report on plugins with no test target. * Add `--verbose` option to script. --- hacking/shippable/README.md | 20 +++++ hacking/shippable/incidental.py | 149 ++++++++++++++++++++++++-------- 2 files changed, 132 insertions(+), 37 deletions(-) diff --git a/hacking/shippable/README.md b/hacking/shippable/README.md index deb8b41b3a0..2505c98f48e 100644 --- a/hacking/shippable/README.md +++ b/hacking/shippable/README.md @@ -62,6 +62,26 @@ When incidental tests no longer provide exclusive coverage they can be removed. > CAUTION: Only one incidental test should be removed at a time, as doing so may cause another test to gain exclusive incidental coverage. +#### Incidental Plugin Coverage + +Incidental test coverage is not limited to ``incidental_`` prefixed tests. +For example, incomplete code coverage from a filter plugin's own tests may be covered by an unrelated test. +The ``incidental.py`` script can be used to identify these gaps as well. + +Follow the steps 1 and 2 as outlined in the previous section. +For step 3, add the ``--plugin-path {path_to_plugin}`` option. +Repeat step 3 for as many plugins as desired. + +To report on multiple plugins at once, such as all ``filter`` plugins, the following command can be used: + +```shell +find lib/ansible/plugins/filter -name '*.py' -not -name __init__.py -exec hacking/shippable/incidental.py ansible/ansible/162160 --plugin-path '{}' ';' +``` + +Each report will show the incidental code coverage missing from the plugin's own tests. + +> NOTE: The report does not identify where the incidental coverage comes from. + ### Reading Incidental Coverage Reports Each line of code covered will be included in a report. diff --git a/hacking/shippable/incidental.py b/hacking/shippable/incidental.py index bf260f62eb9..ac47cdffdc3 100755 --- a/hacking/shippable/incidental.py +++ b/hacking/shippable/incidental.py @@ -68,11 +68,6 @@ def parse_args(): default=source, help='path to git repository containing Ansible source') - parser.add_argument('--targets', - type=regex, - default='^incidental_', - help='regex for targets to analyze, default: %(default)s') - parser.add_argument('--skip-checks', action='store_true', help='skip integrity checks, use only for debugging') @@ -82,6 +77,20 @@ def parse_args(): action='store_false', help='ignore cached files') + parser.add_argument('-v', '--verbose', + action='store_true', + help='increase verbosity') + + targets = parser.add_mutually_exclusive_group() + + targets.add_argument('--targets', + type=regex, + default='^incidental_', + help='regex for targets to analyze, default: %(default)s') + + targets.add_argument('--plugin-path', + help='path to plugin to report incidental coverage on') + if argcomplete: argcomplete.autocomplete(parser) @@ -149,18 +158,39 @@ def incidental_report(args): # combine coverage results into a single file combined_path = os.path.join(output_path, 'combined.json') - cached(combined_path, args.use_cache, + cached(combined_path, args.use_cache, args.verbose, lambda: ct.combine(coverage_data.paths, combined_path)) with open(combined_path) as combined_file: combined = json.load(combined_file) + if args.plugin_path: + # reporting on coverage missing from the test target for the specified plugin + # the report will be on a single target + cache_path_format = '%s' + '-for-%s' % os.path.splitext(os.path.basename(args.plugin_path))[0] + target_pattern = '^%s$' % get_target_name_from_plugin_path(args.plugin_path) + include_path = args.plugin_path + missing = True + target_name = get_target_name_from_plugin_path(args.plugin_path) + else: + # reporting on coverage exclusive to the matched targets + # the report can contain multiple targets + cache_path_format = '%s' + target_pattern = args.targets + include_path = None + missing = False + target_name = None + # identify integration test targets to analyze target_names = sorted(combined['targets']) - incidental_target_names = [target for target in target_names if re.search(args.targets, target)] + incidental_target_names = [target for target in target_names if re.search(target_pattern, target)] if not incidental_target_names: - raise ApplicationError('no targets to analyze') + if target_name: + # if the plugin has no tests we still want to know what coverage is missing + incidental_target_names = [target_name] + else: + raise ApplicationError('no targets to analyze') # exclude test support plugins from analysis # also exclude six, which for an unknown reason reports bogus coverage lines (indicating coverage of comments) @@ -169,43 +199,80 @@ def incidental_report(args): # process coverage for each target and then generate a report # save sources for generating a summary report at the end summary = {} + report_paths = {} for target_name in incidental_target_names: - only_target_path = os.path.join(data_path, 'only-%s.json' % target_name) - cached(only_target_path, args.use_cache, - lambda: ct.filter(combined_path, only_target_path, include_targets=[target_name], exclude_path=exclude_path)) + cache_name = cache_path_format % target_name + + only_target_path = os.path.join(data_path, 'only-%s.json' % cache_name) + cached(only_target_path, args.use_cache, args.verbose, + lambda: ct.filter(combined_path, only_target_path, include_targets=[target_name], include_path=include_path, exclude_path=exclude_path)) - without_target_path = os.path.join(data_path, 'without-%s.json' % target_name) - cached(without_target_path, args.use_cache, - lambda: ct.filter(combined_path, without_target_path, exclude_targets=[target_name], exclude_path=exclude_path)) + without_target_path = os.path.join(data_path, 'without-%s.json' % cache_name) + cached(without_target_path, args.use_cache, args.verbose, + lambda: ct.filter(combined_path, without_target_path, exclude_targets=[target_name], include_path=include_path, exclude_path=exclude_path)) + + if missing: + source_target_path = missing_target_path = os.path.join(data_path, 'missing-%s.json' % cache_name) + cached(missing_target_path, args.use_cache, args.verbose, + lambda: ct.missing(without_target_path, only_target_path, missing_target_path, only_gaps=True)) + else: + source_target_path = exclusive_target_path = os.path.join(data_path, 'exclusive-%s.json' % cache_name) + cached(exclusive_target_path, args.use_cache, args.verbose, + lambda: ct.missing(only_target_path, without_target_path, exclusive_target_path, only_gaps=True)) - exclusive_target_path = os.path.join(data_path, 'exclusive-%s.json' % target_name) - cached(exclusive_target_path, args.use_cache, - lambda: ct.missing(only_target_path, without_target_path, exclusive_target_path, only_gaps=True)) + source_expanded_target_path = os.path.join(os.path.dirname(source_target_path), 'expanded-%s' % os.path.basename(source_target_path)) + cached(source_expanded_target_path, args.use_cache, args.verbose, + lambda: ct.expand(source_target_path, source_expanded_target_path)) - exclusive_expanded_target_path = os.path.join(data_path, 'exclusive-expanded-%s.json' % target_name) - cached(exclusive_expanded_target_path, args.use_cache, - lambda: ct.expand(exclusive_target_path, exclusive_expanded_target_path)) + summary[target_name] = sources = collect_sources(source_expanded_target_path, git, coverage_data) - summary[target_name] = sources = collect_sources(exclusive_expanded_target_path, git, coverage_data) + txt_report_path = os.path.join(reports_path, '%s.txt' % cache_name) + cached(txt_report_path, args.use_cache, args.verbose, + lambda: generate_report(sources, txt_report_path, coverage_data, target_name, missing=missing)) - txt_report_path = os.path.join(reports_path, '%s.txt' % target_name) - cached(txt_report_path, args.use_cache, - lambda: generate_report(sources, txt_report_path, coverage_data, target_name)) + report_paths[target_name] = txt_report_path # provide a summary report of results for target_name in incidental_target_names: sources = summary[target_name] + report_path = os.path.relpath(report_paths[target_name]) - print('%s: %d arcs, %d lines, %d files' % ( + print('%s: %d arcs, %d lines, %d files - %s' % ( target_name, sum(len(s.covered_arcs) for s in sources), sum(len(s.covered_lines) for s in sources), len(sources), + report_path, )) - sys.stderr.write('NOTE: This report shows only coverage exclusive to the reported targets. ' - 'As targets are removed, exclusive coverage on the remaining targets will increase.\n') + if not missing: + sys.stderr.write('NOTE: This report shows only coverage exclusive to the reported targets. ' + 'As targets are removed, exclusive coverage on the remaining targets will increase.\n') + + +def get_target_name_from_plugin_path(path): # type: (str) -> str + """Return the integration test target name for the given plugin path.""" + parts = os.path.splitext(path)[0].split(os.path.sep) + plugin_name = parts[-1] + + if path.startswith('lib/ansible/modules/'): + plugin_type = None + elif path.startswith('lib/ansible/plugins/'): + plugin_type = parts[3] + elif path.startswith('lib/ansible/module_utils/'): + plugin_type = parts[2] + elif path.startswith('plugins/'): + plugin_type = parts[1] + else: + raise ApplicationError('Cannot determine plugin type from plugin path: %s' % path) + + if plugin_type is None: + target_name = plugin_name + else: + target_name = '%s_%s' % (plugin_type, plugin_name) + + return target_name class CoverageData: @@ -259,7 +326,7 @@ class CoverageTool: def combine(self, input_paths, output_path): subprocess.check_call(self.analyze_cmd + ['combine'] + input_paths + [output_path]) - def filter(self, input_path, output_path, include_targets=None, exclude_targets=None, exclude_path=None): + def filter(self, input_path, output_path, include_targets=None, exclude_targets=None, include_path=None, exclude_path=None): args = [] if include_targets: @@ -270,6 +337,9 @@ class CoverageTool: for target in exclude_targets: args.extend(['--exclude-target', target]) + if include_path: + args.extend(['--include-path', include_path]) + if exclude_path: args.extend(['--exclude-path', exclude_path]) @@ -320,9 +390,9 @@ def collect_sources(data_path, git, coverage_data): return sources -def generate_report(sources, report_path, coverage_data, target_name): +def generate_report(sources, report_path, coverage_data, target_name, missing): output = [ - 'Target: %s' % target_name, + 'Target: %s (%s coverage)' % (target_name, 'missing' if missing else 'exclusive'), 'GitHub: %stest/integration/targets/%s' % (coverage_data.github_base_url, target_name), ] @@ -374,17 +444,22 @@ def parse_arc(value): return tuple(int(v) for v in value.split(':')) -def cached(path, use_cache, func): +def cached(path, use_cache, show_messages, func): if os.path.exists(path) and use_cache: - sys.stderr.write('%s: cached\n' % path) - sys.stderr.flush() + if show_messages: + sys.stderr.write('%s: cached\n' % path) + sys.stderr.flush() return - sys.stderr.write('%s: generating ... ' % path) - sys.stderr.flush() + if show_messages: + sys.stderr.write('%s: generating ... ' % path) + sys.stderr.flush() + func() - sys.stderr.write('done\n') - sys.stderr.flush() + + if show_messages: + sys.stderr.write('done\n') + sys.stderr.flush() def check_failed(args, message):