Code cleanup for test infrastructure. (#59046)

* Remove useless object inheritance in test code.
* Remove unnecessary pass statements from test code.
* Comment on why certain pylint rules are ignored.
* Add more pylint test contexts.
* Fix import order.
* Remove unused variables.
* Remove unnecessary pass statement.
* Fix bad continuations.
* Remove unnecessary elif.
* Allow legitimate broad except statements.
* Allow legitimate protected access.
* Clean up names to make pylint pass.

test/runner/lib/cache.py

@@ -3,7 +3,7 @@ from __future__ import (absolute_import, division, print_function)
 __metaclass__ = type
 
 
-class CommonCache(object):
+class CommonCache:
     """Common cache."""
     def __init__(self, args):
         """

test/runner/lib/changes.py

@@ -42,10 +42,9 @@ class InvalidBranch(ApplicationError):
 
 class ChangeDetectionNotSupported(ApplicationError):
     """Exception for cases where change detection is not supported."""
-    pass
 
 
-class ShippableChanges(object):
+class ShippableChanges:
     """Change information for Shippable build."""
     def __init__(self, args, git):
         """
@@ -120,7 +119,7 @@ class ShippableChanges(object):
         return last_successful_commit
 
 
-class LocalChanges(object):
+class LocalChanges:
     """Change information for local work."""
     def __init__(self, args, git):
         """

test/runner/lib/classification.py

@@ -170,7 +170,7 @@ def categorize_changes(args, paths, verbose_command=None):
     return changes
 
 
-class PathMapper(object):
+class PathMapper:
     """Map file paths to test commands and targets."""
     def __init__(self, args):
         """

test/runner/lib/cloud/__init__.py

@@ -388,24 +388,21 @@ class CloudEnvironment(CloudBase):
 
     def setup(self):
         """Setup which should be done once per environment instead of once per test target."""
-        pass
 
     @abc.abstractmethod
     def get_environment_config(self):
         """
         :rtype: CloudEnvironmentConfig
         """
-        pass
 
     def on_failure(self, target, tries):
         """
         :type target: IntegrationTarget
         :type tries: int
         """
-        pass
 
 
-class CloudEnvironmentConfig(object):
+class CloudEnvironmentConfig:
     """Configuration for the environment."""
     def __init__(self, env_vars=None, ansible_vars=None, module_defaults=None, callback_plugins=None):
         """

test/runner/lib/cloud/opennebula.py

@@ -19,7 +19,6 @@ class OpenNebulaCloudProvider(CloudProvider):
 
     def filter(self, targets, exclude):
         """ no need to filter modules, they can either run from config file or from fixtures"""
-        pass
 
     def setup(self):
         """Setup the cloud resource before delegation and register a cleanup callback."""

test/runner/lib/cloud/tower.py

@@ -185,7 +185,7 @@ class TowerCloudEnvironment(CloudEnvironment):
         )
 
 
-class TowerConfig(object):
+class TowerConfig:
     """Tower settings."""
     def __init__(self, values):
         self.version = values.get('version')

test/runner/lib/core_ci.py

@@ -37,7 +37,7 @@ AWS_ENDPOINTS = {
 }
 
 
-class AnsibleCoreCI(object):
+class AnsibleCoreCI:
     """Client for Ansible Core CI services."""
     def __init__(self, args, platform, version, stage='prod', persist=True, load=True, name=None, provider=None):
         """
@@ -537,7 +537,7 @@ class CoreHttpError(HttpError):
         self.remote_stack_trace = remote_stack_trace
 
 
-class SshKey(object):
+class SshKey:
     """Container for SSH key used to connect to remote instances."""
     KEY_NAME = 'id_rsa'
     PUB_NAME = 'id_rsa.pub'
@@ -574,7 +574,7 @@ class SshKey(object):
                 self.pub_contents = pub_fd.read().strip()
 
 
-class InstanceConnection(object):
+class InstanceConnection:
     """Container for remote instance status and connection details."""
     def __init__(self, running, hostname, port, username, password):
         """

test/runner/lib/diff.py

@@ -21,7 +21,7 @@ def parse_diff(lines):
     return DiffParser(lines).files
 
 
-class FileDiff(object):
+class FileDiff:
     """Parsed diff for a single file."""
     def __init__(self, old_path, new_path):
         """
@@ -47,7 +47,7 @@ class FileDiff(object):
         return self.old.is_complete and self.new.is_complete
 
 
-class DiffSide(object):
+class DiffSide:
     """Parsed diff for a single 'side' of a single file."""
     def __init__(self, path, new):
         """
@@ -134,7 +134,7 @@ class DiffSide(object):
         return ['%s:%4d %s' % (self.path, line[0], line[1]) for line in lines]
 
 
-class DiffParser(object):
+class DiffParser:
     """Parse diff lines."""
     def __init__(self, lines):
         """

test/runner/lib/executor.py

@@ -1795,7 +1795,7 @@ def get_python_interpreter(args, configs, name):
     return python_interpreter
 
 
-class EnvironmentDescription(object):
+class EnvironmentDescription:
     """Description of current running environment."""
     def __init__(self, args):
         """Initialize snapshot of environment configuration.

test/runner/lib/git.py

@@ -10,7 +10,7 @@ from lib.util import (
 )
 
 
-class Git(object):
+class Git:
     """Wrapper around git command-line tools."""
     def __init__(self, root=None):  # type: (t.Optional[str]) -> None
         self.git = 'git'

test/runner/lib/http.py

@@ -33,7 +33,7 @@ from lib.util_common import (
 )
 
 
-class HttpClient(object):
+class HttpClient:
     """Make HTTP requests via curl."""
     def __init__(self, args, always=False, insecure=False, proxy=None):
         """
@@ -146,7 +146,7 @@ class HttpClient(object):
         return HttpResponse(method, url, status_code, body)
 
 
-class HttpResponse(object):
+class HttpResponse:
     """HTTP response from curl."""
     def __init__(self, method, url, status_code, response):
         """

test/runner/lib/integration/__init__.py

@@ -255,7 +255,7 @@ def integration_test_config_file(args, env_config, integration_dir):
         yield path
 
 
-class IntegrationEnvironment(object):
+class IntegrationEnvironment:
     """Details about the integration environment."""
     def __init__(self, integration_dir, inventory_path, ansible_config, vars_file):
         self.integration_dir = integration_dir

test/runner/lib/manage_ci.py

@@ -32,7 +32,7 @@ from lib.config import (
 )
 
 
-class ManageWindowsCI(object):
+class ManageWindowsCI:
     """Manage access to a Windows instance provided by Ansible Core CI."""
     def __init__(self, core_ci):
         """
@@ -56,7 +56,6 @@ class ManageWindowsCI(object):
         """Used in delegate_remote to setup the host, no action is required for Windows.
         :type python_version: str
         """
-        pass
 
     def wait(self):
         """Wait for instance to respond to ansible ping."""
@@ -136,7 +135,7 @@ class ManageWindowsCI(object):
         raise ApplicationError('Failed transfer: %s -> %s' % (src, dst))
 
 
-class ManageNetworkCI(object):
+class ManageNetworkCI:
     """Manage access to a network instance provided by Ansible Core CI."""
     def __init__(self, core_ci):
         """
@@ -177,7 +176,7 @@ class ManageNetworkCI(object):
                                (self.core_ci.platform, self.core_ci.version, self.core_ci.instance_id))
 
 
-class ManagePosixCI(object):
+class ManagePosixCI:
     """Manage access to a POSIX instance provided by Ansible Core CI."""
     def __init__(self, core_ci):
         """

test/runner/lib/metadata.py

@@ -17,7 +17,7 @@ from lib.diff import (
 )
 
 
-class Metadata(object):
+class Metadata:
     """Metadata object for passing data to delegated tests."""
     def __init__(self):
         """Initialize metadata."""
@@ -102,7 +102,7 @@ class Metadata(object):
         return metadata
 
 
-class ChangeDescription(object):
+class ChangeDescription:
     """Description of changes."""
     def __init__(self):
         self.command = ''  # type: str

test/runner/lib/pytar.py

@@ -28,7 +28,6 @@ class TarFilter(ABC):
         :type item: tarfile.TarInfo
         :rtype: tarfile.TarInfo | None
         """
-        pass
 
 
 class DefaultTarFilter(TarFilter):

test/runner/lib/sanity/__init__.py

@@ -196,10 +196,9 @@ class SanityFailure(TestFailure):
 
 class SanityMessage(TestMessage):
     """Single sanity test message for one file."""
-    pass
 
 
-class SanityTargets(object):
+class SanityTargets:
     """Sanity test target information."""
     def __init__(self, include, exclude, require):
         """
@@ -363,7 +362,6 @@ class SanitySingleVersion(SanityFunc):
         :type targets: SanityTargets
         :rtype: TestResult
         """
-        pass
 
 
 class SanityMultipleVersion(SanityFunc):
@@ -376,7 +374,6 @@ class SanityMultipleVersion(SanityFunc):
         :type python_version: str
         :rtype: TestResult
         """
-        pass
 
 
 SANITY_TESTS = (

test/runner/lib/sanity/pylint.py

@@ -153,6 +153,8 @@ class PylintTest(SanitySingleVersion):
             return context_filter
 
         add_context(remaining_paths, 'ansible-test', filter_path('test/runner/'))
+        add_context(remaining_paths, 'validate-modules', filter_path('test/sanity/validate-modules/'))
+        add_context(remaining_paths, 'sanity', filter_path('test/sanity/'))
         add_context(remaining_paths, 'units', filter_path('test/units/'))
         add_context(remaining_paths, 'test', filter_path('test/'))
         add_context(remaining_paths, 'hacking', filter_path('hacking/'))

test/runner/lib/target.py

@@ -392,7 +392,7 @@ def analyze_integration_target_dependencies(integration_targets):
     return dependencies
 
 
-class CompletionTarget(object):
+class CompletionTarget:
     """Command-line argument completion target base class."""
     __metaclass__ = abc.ABCMeta
 

test/runner/lib/test.py

@@ -55,7 +55,7 @@ def calculate_confidence(path, line, metadata):
     return 50
 
 
-class TestResult(object):
+class TestResult:
     """Base class for test results."""
     def __init__(self, command, test, python_version=None):
         """
@@ -96,23 +96,19 @@ class TestResult(object):
 
     def write_console(self):
         """Write results to console."""
-        pass
 
     def write_lint(self):
         """Write lint results to stdout."""
-        pass
 
     def write_bot(self, args):
         """
         :type args: TestConfig
         """
-        pass
 
     def write_junit(self, args):
         """
         :type args: TestConfig
         """
-        pass
 
     def create_path(self, directory, extension):
         """
@@ -419,7 +415,7 @@ class TestFailure(TestResult):
         return message
 
 
-class TestMessage(object):
+class TestMessage:
     """Single test message for one file."""
     def __init__(self, message, path, line=0, column=0, level='error', code=None, confidence=None):
         """

test/runner/lib/util.py

@@ -513,7 +513,7 @@ def generate_password():
     return password
 
 
-class Display(object):
+class Display:
     """Manages color console output."""
     clear = '\033[0m'
     red = '\033[31m'
@@ -626,12 +626,10 @@ class Display(object):
 
 class ApplicationError(Exception):
     """General application error."""
-    pass
 
 
 class ApplicationWarning(Exception):
     """General application warning which interrupts normal program flow."""
-    pass
 
 
 class SubprocessError(ApplicationError):

test/runner/lib/util_common.py

@@ -24,7 +24,7 @@ from lib.util import (
 )
 
 
-class CommonConfig(object):
+class CommonConfig:
     """Configuration common to all commands."""
     def __init__(self, args, command):
         """

test/sanity/code-smell/botmeta.py

@@ -14,8 +14,6 @@ from voluptuous.humanize import humanize_error
 
 from ansible.module_utils.six import string_types
 
-list_string_types = list(string_types)
-
 
 def main():
     """Validate BOTMETA"""
@@ -27,10 +25,12 @@ def main():
     except yaml.error.MarkedYAMLError as ex:
         print('%s:%d:%d: YAML load failed: %s' % (path, ex.context_mark.line + 1, ex.context_mark.column + 1, re.sub(r'\s+', ' ', str(ex))))
         sys.exit()
-    except Exception as ex:
+    except Exception as ex:  # pylint: disable=broad-except
         print('%s:%d:%d: YAML load failed: %s' % (path, 0, 0, re.sub(r'\s+', ' ', str(ex))))
         sys.exit()
 
+    list_string_types = list(string_types)
+
     files_schema = Any(
         Schema(*string_types),
         Schema({

test/sanity/code-smell/deprecated-config.py

@@ -25,10 +25,10 @@ import os
 import re
 import sys
 
-import yaml
-
 from distutils.version import StrictVersion
 
+import yaml
+
 import ansible.config
 
 from ansible.plugins.loader import fragment_loader
@@ -39,32 +39,32 @@ DOC_RE = re.compile(b'^DOCUMENTATION', flags=re.M)
 ANSIBLE_MAJOR = StrictVersion('.'.join(ansible_version.split('.')[:2]))
 
 
-def find_deprecations(o, path=None):
-    if not isinstance(o, (list, dict)):
+def find_deprecations(obj, path=None):
+    if not isinstance(obj, (list, dict)):
         return
 
     try:
-        items = o.items()
+        items = obj.items()
     except AttributeError:
-        items = enumerate(o)
+        items = enumerate(obj)
 
-    for k, v in items:
+    for key, value in items:
         if path is None:
             this_path = []
         else:
             this_path = path[:]
 
-        this_path.append(k)
+        this_path.append(key)
 
-        if k != 'deprecated':
-            for result in find_deprecations(v, path=this_path):
+        if key != 'deprecated':
+            for result in find_deprecations(value, path=this_path):
                 yield result
         else:
             try:
-                version = v['version']
+                version = value['version']
                 this_path.append('version')
             except KeyError:
-                version = v['removed_in']
+                version = value['removed_in']
                 this_path.append('removed_in')
             if StrictVersion(version) <= ANSIBLE_MAJOR:
                 yield (this_path, version)
@@ -75,12 +75,12 @@ def main():
     for path in sys.argv[1:] or sys.stdin.read().splitlines():
         with open(path, 'rb') as f:
             try:
-                mm = mmap.mmap(f.fileno(), 0, access=mmap.ACCESS_READ)
+                mm_file = mmap.mmap(f.fileno(), 0, access=mmap.ACCESS_READ)
             except ValueError:
                 continue
-            if DOC_RE.search(mm):
+            if DOC_RE.search(mm_file):
                 plugins.append(path)
-            mm.close()
+            mm_file.close()
 
     for plugin in plugins:
         data = {}

test/sanity/code-smell/no-illegal-filenames.py

@@ -57,8 +57,8 @@ ILLEGAL_END_CHARS = [
 
 def check_path(path, is_dir=False):
     type_name = 'directory' if is_dir else 'file'
-    parent, file_name = os.path.split(path)
-    name, ext = os.path.splitext(file_name)
+    file_name = os.path.basename(path)
+    name = os.path.splitext(file_name)[0]
 
     if name.upper() in ILLEGAL_NAMES:
         print("%s: illegal %s name %s" % (path, type_name, name.upper()))

test/sanity/code-smell/required-and-default-attributes.py

@@ -22,7 +22,7 @@ def main():
 
                 if match:
                     print('%s:%d:%d: use only one of `default` or `required` with `FieldAttribute`' % (
-                          path, line + 1, match.start(1) + 1))
+                        path, line + 1, match.start(1) + 1))
 
 
 if __name__ == '__main__':

test/sanity/code-smell/shebang.py

@@ -92,9 +92,9 @@ def main():
                 is_integration = True
 
                 if dirname.endswith('/library') or dirname.endswith('/plugins/modules') or dirname in (
-                    # non-standard module library directories
-                    'test/integration/targets/module_precedence/lib_no_extension',
-                    'test/integration/targets/module_precedence/lib_with_extension',
+                        # non-standard module library directories
+                        'test/integration/targets/module_precedence/lib_no_extension',
+                        'test/integration/targets/module_precedence/lib_with_extension',
                 ):
                     is_module = True
             elif dirname == 'plugins/modules':

test/sanity/compile/compile.py

@@ -20,7 +20,7 @@ def main():
         try:
             parser.suite(source)
         except SyntaxError:
-            ex_type, ex, ex_traceback = sys.exc_info()
+            ex = sys.exc_info()[1]
             status = 1
             message = ex.text.splitlines()[0].strip()
             sys.stdout.write("%s:%d:%d: SyntaxError: %s\n" % (path, ex.lineno, ex.offset, message))

test/sanity/import/importer.py

@@ -12,9 +12,9 @@ import warnings
 
 try:
     import importlib.util
-    imp = None
+    imp = None  # pylint: disable=invalid-name
 except ImportError:
-    importlib = None
+    importlib = None  # pylint: disable=invalid-name
     import imp
 
 try:
@@ -28,10 +28,9 @@ import ansible.module_utils.common.removed
 
 class ImporterAnsibleModuleException(Exception):
     """Exception thrown during initialization of ImporterAnsibleModule."""
-    pass
 
 
-class ImporterAnsibleModule(object):
+class ImporterAnsibleModule:
     """Replacement for AnsibleModule to support import testing."""
     def __init__(self, *args, **kwargs):
         raise ImporterAnsibleModuleException()
@@ -40,7 +39,7 @@ class ImporterAnsibleModule(object):
 # stop Ansible module execution during AnsibleModule instantiation
 ansible.module_utils.basic.AnsibleModule = ImporterAnsibleModule
 # no-op for _load_params since it may be called before instantiating AnsibleModule
-ansible.module_utils.basic._load_params = lambda *args, **kwargs: {}
+ansible.module_utils.basic._load_params = lambda *args, **kwargs: {}  # pylint: disable=protected-access
 # no-op for removed_module since it is called in place of AnsibleModule instantiation
 ansible.module_utils.common.removed.removed_module = lambda *args, **kwargs: None
 
@@ -142,7 +141,7 @@ def test_python_module(path, base_dir, messages, ansible_module):
         report_message(error, messages)
 
 
-class Capture(object):
+class Capture:
     """Captured output and/or exception."""
     def __init__(self):
         self.stdout = StringIO()

test/sanity/pylint/config/ansible-test

@@ -10,11 +10,9 @@ disable=
     too-many-nested-blocks,
     too-many-return-statements,
     too-many-statements,
-    unnecessary-pass,
-    unused-import,
-    useless-object-inheritance,
-    consider-using-dict-comprehension,
-    consider-using-set-comprehension,
+    unused-import,  # pylint does not understand PEP 484 type hints
+    consider-using-dict-comprehension,  # requires Python 2.6, which we still support
+    consider-using-set-comprehension,  # requires Python 2.6, which we still support
 
 [BASIC]
 

test/sanity/pylint/config/sanity

@@ -0,0 +1,40 @@
+[MESSAGES CONTROL]
+
+disable=
+    too-few-public-methods,
+    too-many-arguments,
+    too-many-branches,
+    too-many-instance-attributes,
+    too-many-lines,
+    too-many-locals,
+    too-many-nested-blocks,
+    too-many-return-statements,
+    too-many-statements,
+    missing-docstring,
+    unused-import,  # pylint does not understand PEP 484 type hints
+    consider-using-dict-comprehension,  # requires Python 2.6, which we still support
+    consider-using-set-comprehension,  # requires Python 2.6, which we still support
+
+[BASIC]
+
+bad-names=foo,
+          bar,
+          baz,
+          toto,
+          tutu,
+          tata,
+          _,
+
+good-names=i,
+           j,
+           k,
+           f,
+           e,
+           ex,
+           Run,
+           C,
+           __metaclass__,
+
+module-rgx=[a-z_][a-z0-9_-]{2,40}$
+method-rgx=[a-z_][a-z0-9_]{2,40}$
+function-rgx=[a-z_][a-z0-9_]{2,40}$

test/sanity/pylint/plugins/blacklist.py

@@ -9,7 +9,7 @@ from pylint.checkers import BaseChecker
 from pylint.interfaces import IAstroidChecker
 
 
-class BlacklistEntry(object):
+class BlacklistEntry:
     """Defines a import blacklist entry."""
     def __init__(self, alternative, modules_only=False, names=None, ignore_paths=None):
         """

test/sanity/pylint/plugins/deprecated.py

@@ -70,7 +70,7 @@ class AnsibleDeprecatedChecker(BaseChecker):
                         if len(node.keywords) == 1 and keyword.arg is None:
                             # This is likely a **kwargs splat
                             return
-                        elif keyword.arg == 'version':
+                        if keyword.arg == 'version':
                             if isinstance(keyword.value.value, astroid.Name):
                                 # This is likely a variable
                                 return

test/sanity/pylint/plugins/string_format.py

@@ -74,7 +74,7 @@ class AnsibleStringFormatChecker(BaseChecker):
         if node.starargs or node.kwargs:
             return
         try:
-            fields, num_args, manual_pos = parse_format_method_string(strnode.value)
+            num_args = parse_format_method_string(strnode.value)[1]
         except utils.IncompleteFormatString:
             return
 

test/sanity/validate-modules/main.py

@@ -94,7 +94,7 @@ class ReporterEncoder(json.JSONEncoder):
         return json.JSONEncoder.default(self, o)
 
 
-class Reporter(object):
+class Reporter:
     def __init__(self):
         self.files = OrderedDict()
 
@@ -1699,7 +1699,7 @@ def main():
         sys.exit(reporter.json(warnings=args.warnings, output=args.output))
 
 
-class GitCache(object):
+class GitCache:
     def __init__(self, base_branch):
         self.base_branch = base_branch
 

test/sanity/yamllint/yamllinter.py

@@ -21,7 +21,7 @@ def main():
     checker.report()
 
 
-class YamlChecker(object):
+class YamlChecker:
     """Wrapper around yamllint that supports YAML embedded in Ansible modules."""
     def __init__(self):
         self.messages = []
@@ -177,7 +177,7 @@ class YamlChecker(object):
                 column=ex.offset,
                 level='error',
             ))
-        except Exception as ex:
+        except Exception as ex:  # pylint: disable=broad-except
             self.messages.append(dict(
                 code='python-parse-error',
                 message=str(ex),