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

@@ -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.

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)
+        cache_name = cache_path_format % 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))
 
-        without_target_path = os.path.join(data_path, 'without-%s.json' % target_name)
+        only_target_path = os.path.join(data_path, 'only-%s.json' % cache_name)
-        cached(without_target_path, args.use_cache,
+        cached(only_target_path, args.use_cache, args.verbose,
-               lambda: ct.filter(combined_path, without_target_path, exclude_targets=[target_name], exclude_path=exclude_path))
+               lambda: ct.filter(combined_path, only_target_path, include_targets=[target_name], include_path=include_path, exclude_path=exclude_path))
 
-        exclusive_target_path = os.path.join(data_path, 'exclusive-%s.json' % target_name)
+        without_target_path = os.path.join(data_path, 'without-%s.json' % cache_name)
-        cached(exclusive_target_path, args.use_cache,
+        cached(without_target_path, args.use_cache, args.verbose,
-               lambda: ct.missing(only_target_path, without_target_path, exclusive_target_path, only_gaps=True))
+               lambda: ct.filter(combined_path, without_target_path, exclude_targets=[target_name], include_path=include_path, exclude_path=exclude_path))
 
-        exclusive_expanded_target_path = os.path.join(data_path, 'exclusive-expanded-%s.json' % target_name)
+        if missing:
-        cached(exclusive_expanded_target_path, args.use_cache,
+            source_target_path = missing_target_path = os.path.join(data_path, 'missing-%s.json' % cache_name)
-               lambda: ct.expand(exclusive_target_path, exclusive_expanded_target_path))
+            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))
 
-        summary[target_name] = sources = collect_sources(exclusive_expanded_target_path, git, coverage_data)
+        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))
 
-        txt_report_path = os.path.join(reports_path, '%s.txt' % target_name)
+        summary[target_name] = sources = collect_sources(source_expanded_target_path, git, coverage_data)
-        cached(txt_report_path, args.use_cache,
+
-               lambda: generate_report(sources, txt_report_path, coverage_data, target_name))
+        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))
+
+        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. '
+    if not missing:
-                     'As targets are removed, exclusive coverage on the remaining targets will increase.\n')
+        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)
+        if show_messages:
-        sys.stderr.flush()
+            sys.stderr.write('%s: cached\n' % path)
+            sys.stderr.flush()
         return
 
-    sys.stderr.write('%s: generating ... ' % path)
+    if show_messages:
-    sys.stderr.flush()
+        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):