From 6ec0b4ec86c35442b68aaaf8ac683f1ab40901fd Mon Sep 17 00:00:00 2001
From: Matt Clay <matt@mystile.com>
Date: Fri, 23 Aug 2019 18:08:21 -0700
Subject: [PATCH] Sanity test updates for collections support. (#61248)

* Run no-unwanted-files sanity test only on Ansible.

Since collections should be able to use binary modules there is not really any limit on what could exist in a collection `plugins` directory.

* Add support for symlinks in sanity target lists:

- Sanity tests that need to analyze symlinks can do so using the supplied target list.
- Tests that analyze directories will now only look at symlinks if requested.
- Directory symlinks will now be seen as directories instead of files.

* Enable symlinks on filename based sanity tests.

Sanity tests that evalulate filenames instead of content should include symlinks.

* Update symlinks sanity test.

Use the sanity test target list now that it can include symlinks.
---
 .../code-smell/no-illegal-filenames.json      |  1 +
 .../_data/sanity/code-smell/symlinks.json     |  3 +-
 .../_data/sanity/code-smell/symlinks.py       | 31 +++-------
 test/lib/ansible_test/_internal/cover.py      |  4 +-
 .../_internal/provider/layout/__init__.py     | 26 ++++++---
 .../_internal/provider/source/git.py          |  7 +++
 .../_internal/provider/source/installed.py    |  2 +
 .../_internal/provider/source/unversioned.py  |  7 +++
 .../ansible_test/_internal/sanity/__init__.py | 56 +++++++++++++++----
 test/lib/ansible_test/_internal/target.py     | 40 ++++++++-----
 .../sanity/code-smell/no-unwanted-files.json  |  1 +
 .../sanity/code-smell/no-unwanted-files.py    |  0
 test/sanity/code-smell/obsolete-files.json    |  1 +
 13 files changed, 120 insertions(+), 59 deletions(-)
 rename test/{lib/ansible_test/_data => }/sanity/code-smell/no-unwanted-files.json (70%)
 rename test/{lib/ansible_test/_data => }/sanity/code-smell/no-unwanted-files.py (100%)

diff --git a/test/lib/ansible_test/_data/sanity/code-smell/no-illegal-filenames.json b/test/lib/ansible_test/_data/sanity/code-smell/no-illegal-filenames.json
index cc4333bff5d..6f13c86b305 100644
--- a/test/lib/ansible_test/_data/sanity/code-smell/no-illegal-filenames.json
+++ b/test/lib/ansible_test/_data/sanity/code-smell/no-illegal-filenames.json
@@ -1,4 +1,5 @@
 {
     "include_directories": true,
+    "include_symlinks": true,
     "output": "path-message"
 }
diff --git a/test/lib/ansible_test/_data/sanity/code-smell/symlinks.json b/test/lib/ansible_test/_data/sanity/code-smell/symlinks.json
index 593b765d14a..6f13c86b305 100644
--- a/test/lib/ansible_test/_data/sanity/code-smell/symlinks.json
+++ b/test/lib/ansible_test/_data/sanity/code-smell/symlinks.json
@@ -1,4 +1,5 @@
 {
-    "no_targets": true,
+    "include_directories": true,
+    "include_symlinks": true,
     "output": "path-message"
 }
diff --git a/test/lib/ansible_test/_data/sanity/code-smell/symlinks.py b/test/lib/ansible_test/_data/sanity/code-smell/symlinks.py
index 3f54759ba9f..8151d482e1f 100755
--- a/test/lib/ansible_test/_data/sanity/code-smell/symlinks.py
+++ b/test/lib/ansible_test/_data/sanity/code-smell/symlinks.py
@@ -3,34 +3,19 @@ from __future__ import (absolute_import, division, print_function)
 __metaclass__ = type
 
 import os
+import sys
 
 
 def main():
-    skip_dirs = set([
-        '.tox',
-    ])
+    for path in sys.argv[1:] or sys.stdin.read().splitlines():
+        if not os.path.islink(path.rstrip(os.path.sep)):
+            continue
 
-    for root, dirs, files in os.walk('.'):
-        for skip_dir in skip_dirs:
-            if skip_dir in dirs:
-                dirs.remove(skip_dir)
+        if not os.path.exists(path):
+            print('%s: broken symlinks are not allowed' % path)
 
-        if root == '.':
-            root = ''
-        elif root.startswith('./'):
-            root = root[2:]
-
-        for file in files:
-            path = os.path.join(root, file)
-
-            if not os.path.exists(path):
-                print('%s: broken symlinks are not allowed' % path)
-
-        for directory in dirs:
-            path = os.path.join(root, directory)
-
-            if os.path.islink(path):
-                print('%s: symlinks to directories are not allowed' % path)
+        if path.endswith(os.path.sep):
+            print('%s: symlinks to directories are not allowed' % path)
 
 
 if __name__ == '__main__':
diff --git a/test/lib/ansible_test/_internal/cover.py b/test/lib/ansible_test/_internal/cover.py
index 4b38a26e491..c1e096f5536 100644
--- a/test/lib/ansible_test/_internal/cover.py
+++ b/test/lib/ansible_test/_internal/cover.py
@@ -58,7 +58,9 @@ def command_coverage_combine(args):
     groups = {}
 
     if args.all or args.stub:
-        sources = sorted(os.path.abspath(target.path) for target in walk_compile_targets())
+        # excludes symlinks of regular files to avoid reporting on the same file multiple times
+        # in the future it would be nice to merge any coverage for symlinks into the real files
+        sources = sorted(os.path.abspath(target.path) for target in walk_compile_targets(include_symlinks=False))
     else:
         sources = []
 
diff --git a/test/lib/ansible_test/_internal/provider/layout/__init__.py b/test/lib/ansible_test/_internal/provider/layout/__init__.py
index fc7de7119a9..3e895efa6ad 100644
--- a/test/lib/ansible_test/_internal/provider/layout/__init__.py
+++ b/test/lib/ansible_test/_internal/provider/layout/__init__.py
@@ -25,17 +25,27 @@ class Layout:
                  ):  # type: (...) -> None
         self.root = root
 
-        self.__paths = paths
-        self.__tree = paths_to_tree(paths)
+        self.__paths = paths  # contains both file paths and symlinked directory paths (ending with os.path.sep)
+        self.__files = [path for path in paths if not path.endswith(os.path.sep)]  # contains only file paths
+        self.__paths_tree = paths_to_tree(self.__paths)
+        self.__files_tree = paths_to_tree(self.__files)
 
-    def all_files(self):  # type: () -> t.List[str]
+    def all_files(self, include_symlinked_directories=False):  # type: (bool) -> t.List[str]
         """Return a list of all file paths."""
-        return self.__paths
+        if include_symlinked_directories:
+            return self.__paths
 
-    def walk_files(self, directory):  # type: (str) -> t.List[str]
+        return self.__files
+
+    def walk_files(self, directory, include_symlinked_directories=False):  # type: (str, bool) -> t.List[str]
         """Return a list of file paths found recursively under the given directory."""
+        if include_symlinked_directories:
+            tree = self.__paths_tree
+        else:
+            tree = self.__files_tree
+
         parts = directory.rstrip(os.sep).split(os.sep)
-        item = get_tree_item(self.__tree, parts)
+        item = get_tree_item(tree, parts)
 
         if not item:
             return []
@@ -54,13 +64,13 @@ class Layout:
     def get_dirs(self, directory):  # type: (str) -> t.List[str]
         """Return a list directory paths found directly under the given directory."""
         parts = directory.rstrip(os.sep).split(os.sep)
-        item = get_tree_item(self.__tree, parts)
+        item = get_tree_item(self.__files_tree, parts)
         return [os.path.join(directory, key) for key in item[0].keys()] if item else []
 
     def get_files(self, directory):  # type: (str) -> t.List[str]
         """Return a list of file paths found directly under the given directory."""
         parts = directory.rstrip(os.sep).split(os.sep)
-        item = get_tree_item(self.__tree, parts)
+        item = get_tree_item(self.__files_tree, parts)
         return item[1] if item else []
 
 
diff --git a/test/lib/ansible_test/_internal/provider/source/git.py b/test/lib/ansible_test/_internal/provider/source/git.py
index 5b530b641fd..59ad6bdea75 100644
--- a/test/lib/ansible_test/_internal/provider/source/git.py
+++ b/test/lib/ansible_test/_internal/provider/source/git.py
@@ -10,6 +10,10 @@ from ...git import (
     Git,
 )
 
+from ...util import (
+    to_bytes,
+)
+
 from . import (
     SourceProvider,
 )
@@ -30,4 +34,7 @@ class GitSource(SourceProvider):
         deleted_paths = git.get_file_names(['--deleted'])
         paths = sorted(set(paths) - set(deleted_paths))
 
+        # directory symlinks are reported by git as regular files but they need to be treated as directories
+        paths = [path + os.path.sep if os.path.isdir(to_bytes(path)) else path for path in paths]
+
         return paths
diff --git a/test/lib/ansible_test/_internal/provider/source/installed.py b/test/lib/ansible_test/_internal/provider/source/installed.py
index 0823a6ee6f7..d24a6e3dd84 100644
--- a/test/lib/ansible_test/_internal/provider/source/installed.py
+++ b/test/lib/ansible_test/_internal/provider/source/installed.py
@@ -38,4 +38,6 @@ class InstalledSource(SourceProvider):
             paths.extend([os.path.join(rel_root, file_name) for file_name in file_names
                           if not os.path.splitext(file_name)[1] in kill_extensions])
 
+            # NOTE: directory symlinks are ignored as there should be no directory symlinks for an install
+
         return paths
diff --git a/test/lib/ansible_test/_internal/provider/source/unversioned.py b/test/lib/ansible_test/_internal/provider/source/unversioned.py
index ade6e01be3e..ef116ff3828 100644
--- a/test/lib/ansible_test/_internal/provider/source/unversioned.py
+++ b/test/lib/ansible_test/_internal/provider/source/unversioned.py
@@ -10,6 +10,10 @@ from ...constants import (
     TIMEOUT_PATH,
 )
 
+from ...util import (
+    to_bytes,
+)
+
 from . import (
     SourceProvider,
 )
@@ -75,4 +79,7 @@ class UnversionedSource(SourceProvider):
             paths.extend([os.path.join(rel_root, file_name) for file_name in file_names
                           if not os.path.splitext(file_name)[1] in kill_extensions and file_name not in kill_files])
 
+            # include directory symlinks since they will not be traversed and would otherwise go undetected
+            paths.extend([os.path.join(rel_root, dir_name) + os.path.sep for dir_name in dir_names if os.path.islink(to_bytes(dir_name))])
+
         return paths
diff --git a/test/lib/ansible_test/_internal/sanity/__init__.py b/test/lib/ansible_test/_internal/sanity/__init__.py
index 5de1a50f8de..027ebed02ff 100644
--- a/test/lib/ansible_test/_internal/sanity/__init__.py
+++ b/test/lib/ansible_test/_internal/sanity/__init__.py
@@ -163,6 +163,8 @@ def command_sanity(args):
                 else:
                     raise Exception('Unsupported test type: %s' % type(test))
 
+                all_targets = targets.targets
+
                 if test.all_targets:
                     usable_targets = targets.targets
                 elif test.no_targets:
@@ -170,12 +172,12 @@ def command_sanity(args):
                 else:
                     usable_targets = targets.include
 
-                if test.include_directories:
-                    usable_targets += tuple(TestTarget(path, None, None, '') for path in paths_to_dirs([target.path for target in usable_targets]))
+                all_targets = SanityTargets.filter_and_inject_targets(test, all_targets)
+                usable_targets = SanityTargets.filter_and_inject_targets(test, usable_targets)
 
                 usable_targets = sorted(test.filter_targets(list(usable_targets)))
                 usable_targets = settings.filter_skipped_targets(usable_targets)
-                sanity_targets = SanityTargets(targets.targets, tuple(usable_targets))
+                sanity_targets = SanityTargets(tuple(all_targets), tuple(usable_targets))
 
                 if usable_targets or test.no_targets:
                     if isinstance(test, SanityCodeSmellTest):
@@ -266,10 +268,7 @@ class SanityIgnoreParser:
         display.info('Read %d sanity test ignore line(s) for %s from: %s' % (len(lines), ansible_label, self.relative_path), verbosity=1)
 
         for test in sanity_get_tests():
-            test_targets = list(targets)
-
-            if test.include_directories:
-                test_targets += tuple(TestTarget(path, None, None, '') for path in paths_to_dirs([target.path for target in test_targets]))
+            test_targets = SanityTargets.filter_and_inject_targets(test, targets)
 
             paths_by_test[test.name] = set(target.path for target in test.filter_targets(test_targets))
 
@@ -488,10 +487,9 @@ class SanityIgnoreProcessor:
 
         if self.test.no_targets or self.test.all_targets:
             # tests which do not accept a target list, or which use all targets, always return all possible errors, so all ignores can be checked
-            paths = [target.path for target in SanityTargets.get_targets()]
-
-            if self.test.include_directories:
-                paths.extend(paths_to_dirs(paths))
+            targets = SanityTargets.get_targets()
+            test_targets = SanityTargets.filter_and_inject_targets(self.test, targets)
+            paths = [target.path for target in test_targets]
 
         for path in paths:
             path_entry = self.ignore_entries.get(path)
@@ -562,6 +560,29 @@ class SanityTargets:
         _include = walk_internal_targets(_targets, include, exclude, require)
         return SanityTargets(_targets, _include)
 
+    @staticmethod
+    def filter_and_inject_targets(test, targets):  # type: (SanityTest, t.Iterable[TestTarget]) -> t.List[TestTarget]
+        """Filter and inject targets based on test requirements and the given target list."""
+        test_targets = list(targets)
+
+        if not test.include_symlinks:
+            # remove all symlinks unless supported by the test
+            test_targets = [target for target in test_targets if not target.symlink]
+
+        if not test.include_directories or not test.include_symlinks:
+            # exclude symlinked directories unless supported by the test
+            test_targets = [target for target in test_targets if not target.path.endswith(os.path.sep)]
+
+        if test.include_directories:
+            # include directories containing any of the included files
+            test_targets += tuple(TestTarget(path, None, None, '') for path in paths_to_dirs([target.path for target in test_targets]))
+
+            if not test.include_symlinks:
+                # remove all directory symlinks unless supported by the test
+                test_targets = [target for target in test_targets if not target.symlink]
+
+        return test_targets
+
     @staticmethod
     def get_targets():  # type: () -> t.Tuple[TestTarget, ...]
         """Return a tuple of sanity test targets. Uses a cached version when available."""
@@ -613,6 +634,11 @@ class SanityTest(ABC):
         """True if the test targets should include directories."""
         return False
 
+    @property
+    def include_symlinks(self):  # type: () -> bool
+        """True if the test targets should include symlinks."""
+        return False
+
     @property
     def supported_python_versions(self):  # type: () -> t.Optional[t.Tuple[str, ...]]
         """A tuple of supported Python versions or None if the test does not depend on specific Python versions."""
@@ -655,6 +681,7 @@ class SanityCodeSmellTest(SanityTest):
             self.__all_targets = self.config.get('all_targets')  # type: bool
             self.__no_targets = self.config.get('no_targets')  # type: bool
             self.__include_directories = self.config.get('include_directories')  # type: bool
+            self.__include_symlinks = self.config.get('include_symlinks')  # type: bool
         else:
             self.output = None
             self.extensions = []
@@ -666,6 +693,7 @@ class SanityCodeSmellTest(SanityTest):
             self.__all_targets = False
             self.__no_targets = True
             self.__include_directories = False
+            self.__include_symlinks = False
 
         if self.no_targets:
             mutually_exclusive = (
@@ -676,6 +704,7 @@ class SanityCodeSmellTest(SanityTest):
                 'ignore_self',
                 'all_targets',
                 'include_directories',
+                'include_symlinks',
             )
 
             problems = sorted(name for name in mutually_exclusive if getattr(self, name))
@@ -698,6 +727,11 @@ class SanityCodeSmellTest(SanityTest):
         """True if the test targets should include directories."""
         return self.__include_directories
 
+    @property
+    def include_symlinks(self):  # type: () -> bool
+        """True if the test targets should include symlinks."""
+        return self.__include_symlinks
+
     def filter_targets(self, targets):  # type: (t.List[TestTarget]) -> t.List[TestTarget]
         """Return the given list of test targets, filtered to include only those relevant for the test."""
         if self.no_targets:
diff --git a/test/lib/ansible_test/_internal/target.py b/test/lib/ansible_test/_internal/target.py
index 44f689e0259..469986c5263 100644
--- a/test/lib/ansible_test/_internal/target.py
+++ b/test/lib/ansible_test/_internal/target.py
@@ -172,18 +172,19 @@ def walk_units_targets():
     return walk_test_targets(path=data_context().content.unit_path, module_path=data_context().content.unit_module_path, extensions=('.py',), prefix='test_')
 
 
-def walk_compile_targets():
+def walk_compile_targets(include_symlinks=True):
     """
+    :type include_symlinks: bool
     :rtype: collections.Iterable[TestTarget]
     """
-    return walk_test_targets(module_path=data_context().content.module_path, extensions=('.py',), extra_dirs=('bin',))
+    return walk_test_targets(module_path=data_context().content.module_path, extensions=('.py',), extra_dirs=('bin',), include_symlinks=include_symlinks)
 
 
 def walk_sanity_targets():
     """
     :rtype: collections.Iterable[TestTarget]
     """
-    return walk_test_targets(module_path=data_context().content.module_path)
+    return walk_test_targets(module_path=data_context().content.module_path, include_symlinks=True, include_symlinked_directories=True)
 
 
 def walk_posix_integration_targets(include_hidden=False):
@@ -245,19 +246,21 @@ def load_integration_prefixes():
     return prefixes
 
 
-def walk_test_targets(path=None, module_path=None, extensions=None, prefix=None, extra_dirs=None):
+def walk_test_targets(path=None, module_path=None, extensions=None, prefix=None, extra_dirs=None, include_symlinks=False, include_symlinked_directories=False):
     """
     :type path: str | None
     :type module_path: str | None
     :type extensions: tuple[str] | None
     :type prefix: str | None
     :type extra_dirs: tuple[str] | None
+    :type include_symlinks: bool
+    :type include_symlinked_directories: bool
     :rtype: collections.Iterable[TestTarget]
     """
     if path:
-        file_paths = data_context().content.walk_files(path)
+        file_paths = data_context().content.walk_files(path, include_symlinked_directories=include_symlinked_directories)
     else:
-        file_paths = data_context().content.all_files()
+        file_paths = data_context().content.all_files(include_symlinked_directories=include_symlinked_directories)
 
     for file_path in file_paths:
         name, ext = os.path.splitext(os.path.basename(file_path))
@@ -268,12 +271,12 @@ def walk_test_targets(path=None, module_path=None, extensions=None, prefix=None,
         if prefix and not name.startswith(prefix):
             continue
 
-        if os.path.islink(to_bytes(file_path)):
-            # special case to allow a symlink of ansible_release.py -> ../release.py
-            if file_path != 'lib/ansible/module_utils/ansible_release.py':
-                continue
+        symlink = os.path.islink(to_bytes(file_path.rstrip(os.path.sep)))
 
-        yield TestTarget(file_path, module_path, prefix, path)
+        if symlink and not include_symlinks:
+            continue
+
+        yield TestTarget(file_path, module_path, prefix, path, symlink)
 
     file_paths = []
 
@@ -283,10 +286,12 @@ def walk_test_targets(path=None, module_path=None, extensions=None, prefix=None,
                 file_paths.append(file_path)
 
     for file_path in file_paths:
-        if os.path.islink(to_bytes(file_path)):
+        symlink = os.path.islink(to_bytes(file_path.rstrip(os.path.sep)))
+
+        if symlink and not include_symlinks:
             continue
 
-        yield TestTarget(file_path, module_path, prefix, path)
+        yield TestTarget(file_path, module_path, prefix, path, symlink)
 
 
 def analyze_integration_target_dependencies(integration_targets):
@@ -315,7 +320,7 @@ def analyze_integration_target_dependencies(integration_targets):
     # this use case is supported, but discouraged
     for target in integration_targets:
         for path in data_context().content.walk_files(target.path):
-            if not os.path.islink(path):
+            if not os.path.islink(to_bytes(path.rstrip(os.path.sep))):
                 continue
 
             real_link_path = os.path.realpath(path)
@@ -442,18 +447,23 @@ class DirectoryTarget(CompletionTarget):
 
 class TestTarget(CompletionTarget):
     """Generic test target."""
-    def __init__(self, path, module_path, module_prefix, base_path):
+    def __init__(self, path, module_path, module_prefix, base_path, symlink=None):
         """
         :type path: str
         :type module_path: str | None
         :type module_prefix: str | None
         :type base_path: str
+        :type symlink: bool | None
         """
         super(TestTarget, self).__init__()
 
+        if symlink is None:
+            symlink = os.path.islink(to_bytes(path.rstrip(os.path.sep)))
+
         self.name = path
         self.path = path
         self.base_path = base_path + '/' if base_path else None
+        self.symlink = symlink
 
         name, ext = os.path.splitext(os.path.basename(self.path))
 
diff --git a/test/lib/ansible_test/_data/sanity/code-smell/no-unwanted-files.json b/test/sanity/code-smell/no-unwanted-files.json
similarity index 70%
rename from test/lib/ansible_test/_data/sanity/code-smell/no-unwanted-files.json
rename to test/sanity/code-smell/no-unwanted-files.json
index f508e3e18e6..7a89ebbe748 100644
--- a/test/lib/ansible_test/_data/sanity/code-smell/no-unwanted-files.json
+++ b/test/sanity/code-smell/no-unwanted-files.json
@@ -1,4 +1,5 @@
 {
+    "include_symlinks": true,
     "prefixes": [
         "lib/"
     ],
diff --git a/test/lib/ansible_test/_data/sanity/code-smell/no-unwanted-files.py b/test/sanity/code-smell/no-unwanted-files.py
similarity index 100%
rename from test/lib/ansible_test/_data/sanity/code-smell/no-unwanted-files.py
rename to test/sanity/code-smell/no-unwanted-files.py
diff --git a/test/sanity/code-smell/obsolete-files.json b/test/sanity/code-smell/obsolete-files.json
index 7c527df382a..02d3920457b 100644
--- a/test/sanity/code-smell/obsolete-files.json
+++ b/test/sanity/code-smell/obsolete-files.json
@@ -1,4 +1,5 @@
 {
+    "include_symlinks": true,
     "prefixes": [
         "test/runner/",
         "test/sanity/ansible-doc/",