Merge pull request #285 from clumio-oss/typing

Add more typing to the codebase and various fixes.

.github/workflows/ci.yml

@@ -31,7 +31,7 @@ jobs:
       - name: Install
         run: |
           python -m pip install --upgrade pip
-          pip install --upgrade .[dev]
+          pip install --upgrade '.[dev]'
 
       - name: Format
         run: black --check --diff green example

Makefile

@@ -1,5 +1,9 @@
+# SHELL ensures more consistent behavior between OSes.
+SHELL=/bin/bash
+
 VERSION=$(shell cat green/VERSION)
 
+
 clean: clean-message clean-silent
 
 clean-message:
@@ -22,7 +26,7 @@ test: test-versions test-installed test-coverage
 	@echo "\n(test) completed\n"
 
 test-local:
-	@pip3 install --upgrade -e .[dev]
+	@pip3 install --upgrade -e '.[dev]'
 	@make test-installed
 	make test-versions
 	make test-coverage
@@ -32,9 +36,15 @@ test-on-containers: clean-silent
 	@# Run the tests on pristine containers to isolate us from the local environment.
 	@for version in 3.8 3.9 3.10 3.11 3.12.0; do \
 	    docker run --rm -it -v `pwd`:/green python:$$version \
-	        bash -c "python --version; cd /green && pip install -e .[dev] && ./g green" ; \
+	        bash -c "python --version; cd /green && pip install -e '.[dev]' && ./g green" ; \
 	done
 
+test-coverage-on-container: clean-silent
+	@# Run the tests on pristine containers to isolate us from the local environment.
+	docker run --rm -it -v `pwd`:/green python:3.12.0 \
+	    bash -c "cd /green && pip install -e '.[dev]' && ./g 3 -r -vvvv green"
+
+
 test-coverage:
 	@# Generate coverage files for travis builds (don't clean after this!)
 	@make clean-silent
@@ -95,3 +105,6 @@ release-unsafe:
 	twine upload --username CleanCut dist/green-$(VERSION).tar.gz
 
 release: release-test release-tag release-unsafe
+
+# Declare all targets as phony so that make will always run them.
+.PHONY: $(MAKECMDGOALS)

green/VERSION

@@ -1 +1 @@
-4.0.0
+4.0.1-dev

green/cmdline.py

@@ -1,12 +1,16 @@
+from __future__ import annotations
+
+
 import os
 import sys
 import tempfile
+from typing import Sequence
 
 # Importing from green (other than config) is done after coverage initialization
 import green.config as config
 
 
-def _main(argv, testing):
+def _main(argv: Sequence[str] | None, testing: bool) -> int:
     args = config.parseArguments(argv)
     args = config.mergeConfig(args, testing)
 
@@ -77,9 +81,9 @@ def _main(argv, testing):
     return int(not result.wasSuccessful())
 
 
-def main(argv=None, testing=False):
+def main(argv: Sequence[str] | None = None, testing: bool = False):
     # create the temp dir only once (i.e., not while in the recursed call)
-    if os.environ.get("TMPDIR") is None:  # pragma: nocover
+    if not os.environ.get("TMPDIR"):  # pragma: nocover
         try:
             with tempfile.TemporaryDirectory() as temp_dir_for_tests:
                 try:
@@ -94,7 +98,7 @@ def main(argv=None, testing=False):
                 # "Directory not empty" when trying to delete the temp dir can just be a warning
                 print(f"warning: {os_error.strerror}")
             else:
-                raise (os_error)
+                raise os_error
     else:
         return _main(argv, testing)
 

green/command.py

@@ -1,3 +1,5 @@
+from __future__ import annotations
+
 import sys
 
 from setuptools import Command

green/config.py

@@ -1,8 +1,11 @@
 # We have to use this entire file before we can turn coverage on, so we exclude
 # it from coverage.  We still have tests, though!
 
+from __future__ import annotations
+
 import argparse  # pragma: no cover
 import configparser  # pragma: no cover
+from typing import Sequence
 
 import coverage  # pragma: no cover
 
@@ -77,7 +80,9 @@ def __call__(self, action):
         self.options.extend(action.option_strings[0:2])
 
 
-def parseArguments(argv=None):  # pragma: no cover
+def parseArguments(
+    argv: Sequence[str] | None = None,
+) -> argparse.Namespace:  # pragma: no cover
     """
     I parse arguments in sys.argv and return the args object.  The parser
     itself is available as args.parser.

green/djangorunner.py

@@ -8,6 +8,8 @@
     TEST_RUNNER="green.djangorunner.DjangoRunner"
 """
 
+from __future__ import annotations
+
 from argparse import Namespace
 import os
 import sys

green/examples.py

@@ -1,3 +1,5 @@
+from __future__ import annotations
+
 import sys
 import unittest
 

green/exceptions.py

@@ -1,2 +1,5 @@
+from __future__ import annotations
+
+
 class InitializerOrFinalizerError(Exception):
     pass

green/junit.py

@@ -1,3 +1,5 @@
+from __future__ import annotations
+
 from lxml.etree import Element, tostring as to_xml
 
 

green/loader.py

@@ -1,6 +1,6 @@
 from __future__ import annotations
 
-from collections import OrderedDict
+import pathlib
 from doctest import DocTestSuite
 from fnmatch import fnmatch
 import functools
@@ -12,16 +12,27 @@
 import sys
 import unittest
 import traceback
+from typing import Iterable, TYPE_CHECKING
 
 from green.output import debug
 from green import result
 from green.suite import GreenTestSuite
 
+if TYPE_CHECKING:
+    from types import ModuleType
+    from unittest import TestSuite, TestCase
+
 python_file_pattern = re.compile(r"^[_a-z]\w*?\.py$", re.IGNORECASE)
 python_dir_pattern = re.compile(r"^[_a-z]\w*?$", re.IGNORECASE)
 
 
 class GreenTestLoader(unittest.TestLoader):
+    """
+    This class is responsible for loading tests according to various criteria
+    and returning them wrapped in a GreenTestSuite. The super class wraps with
+    TestSuite.
+    """
+
     suiteClass = GreenTestSuite
 
     def loadTestsFromTestCase(self, testCaseClass):
@@ -44,7 +55,7 @@ def filter_test_methods(attrname):
             test_case_names = ["runTest"]
         return flattenTestSuite(map(testCaseClass, test_case_names))
 
-    def loadFromModuleFilename(self, filename):
+    def loadFromModuleFilename(self, filename: str):
         dotted_module, parent_dir = findDottedModuleAndParentDir(filename)
         # Adding the parent path of the module to the start of sys.path is
         # the closest we can get to an absolute import in Python that I can
@@ -153,13 +164,15 @@ def discover(self, current_path, file_pattern="test*.py", top_level_dir=None):
 
         return flattenTestSuite(suite) if suite.countTestCases() else None
 
-    def loadTargets(self, targets, file_pattern="test*.py"):
-        # If a string was passed in, put it into a list.
-        if type(targets) != list:
+    def loadTargets(
+        self, targets: Iterable[str] | str, file_pattern: str = "test*.py"
+    ) -> GreenTestSuite | None:
+        # If a string was passed in, put it into a tuple.
+        if isinstance(targets, str):
             targets = [targets]
 
         # Make sure there are no duplicate entries, preserving order
-        target_dict = OrderedDict()
+        target_dict = {}
         for target in targets:
             target_dict[target] = True
         targets = target_dict.keys()
@@ -325,15 +338,18 @@ def toProtoTestList(
     return test_list
 
 
-def toParallelTargets(suite, targets: list[str]) -> list[str]:
+def toParallelTargets(suite, targets: Iterable[str]) -> list[str]:
     """
     Produce a list of targets which should be tested in parallel.
 
     For the most part, this will be a list of test modules.
     The exception is when a dotted name representing something more granular
     than a module was input (like an individual test case or test method).
     """
-    targets = [x for x in targets if x != "."]
+    if isinstance(targets, str):
+        # This should not happen, but mypy treats str as a valid sequence of strings.
+        targets = (targets,)
+    targets = (x for x in targets if x != ".")
     # First, convert the suite to a proto test list - proto tests nicely
     # parse things like the fully dotted name of the test and the
     # finest-grained module it belongs to, which simplifies our job.
@@ -371,9 +387,9 @@ def toParallelTargets(suite, targets: list[str]) -> list[str]:
     return parallel_targets
 
 
-def getCompletions(target):
+def getCompletions(target: list[str] | str):
     # This option expects 0 or 1 targets
-    if type(target) == list:
+    if not isinstance(target, str):
         target = target[0]
     if target == ".":
         target = ""
@@ -382,11 +398,12 @@ def getCompletions(target):
 
     # First try the completion as-is.  It might be at a valid spot.
     loader = GreenTestLoader()
+    # FIXME: We do not pass file_pattern here, ignoring `--file-pattern`?
     test_suite = loader.loadTargets(target)
     if not test_suite:
         # Next, try stripping to the previous '.'
         last_dot_idx = target.rfind(".")
-        to_complete = None
+        to_complete: str | Iterable[str] = ""
         if last_dot_idx > 0:
             to_complete = target[:last_dot_idx]
         elif len(target):
@@ -415,43 +432,41 @@ def getCompletions(target):
                 dotted_name = dotted_name[:idx]
                 if dotted_name.startswith(target):
                     dotted_names.add(dotted_name)
-                else:  # pragma: nocover -- occurs in Python <= 3.7, but can't find a reasonable way to cover this line when it doesn't in Python > 3.7
+                else:  # pragma: no cover -- occurs in Python <= 3.7, but can't find a reasonable way to cover this line when it doesn't in Python > 3.7
                     break
     return "\n".join(sorted(list(dotted_names)))
 
 
-def isPackage(file_path):
+def isPackage(file_path: pathlib.Path) -> bool:
     """
     Determine whether or not a given path is a (sub)package or not.
     """
-    return os.path.isdir(file_path) and os.path.isfile(
-        os.path.join(file_path, "__init__.py")
-    )
+    return file_path.is_dir() and (file_path / "__init__.py").is_file()
 
 
-def findDottedModuleAndParentDir(file_path):
+def findDottedModuleAndParentDir(file_path: str | pathlib.Path) -> tuple[str, str]:
     """
     I return a tuple (dotted_module, parent_dir) where dotted_module is the
     full dotted name of the module with respect to the package it is in, and
     parent_dir is the absolute path to the parent directory of the package.
 
-    If the python file is not part of a package, I return (None, None).
-
-    For for filepath /a/b/c/d.py where b is the package, ('b.c.d', '/a')
+    For filepath '/a/b/c/d.py' where b is the package, ('b.c.d', '/a')
     would be returned.
     """
-    if not os.path.isfile(file_path):
+    path = pathlib.Path(file_path)
+    if not path.is_file():
         raise ValueError(f"'{file_path}' is not a file.")
-    parent_dir = os.path.dirname(os.path.abspath(file_path))
-    dotted_module = os.path.basename(file_path).replace(".py", "")
+    parent_dir = path.absolute().parent
+    dotted_module = path.stem
     while isPackage(parent_dir):
-        dotted_module = os.path.basename(parent_dir) + "." + dotted_module
-        parent_dir = os.path.dirname(parent_dir)
+        dotted_module = f"{parent_dir.stem}.{dotted_module}"
+        parent_dir = parent_dir.parent
     debug(f"Dotted module: {parent_dir} -> {dotted_module}", 2)
-    return (dotted_module, parent_dir)
+    # TODO: consider returning the Path object for the parent directory.
+    return dotted_module, str(parent_dir)
 
 
-def isTestCaseDisabled(test_case_class, method_name):
+def isTestCaseDisabled(test_case_class: TestCase, method_name: str):
     """
     I check to see if a method on a TestCase has been disabled via nose's
     convention for disabling a TestCase.  This makes it so that users can
@@ -461,24 +476,28 @@ def isTestCaseDisabled(test_case_class, method_name):
     return getattr(test_method, "__test__", "not nose") is False
 
 
-def flattenTestSuite(test_suite, module=None):
-    # Look for a `doctest_modules` list and attempt to add doctest tests to the
-    # suite of tests that we are about to flatten.
+def flattenTestSuite(
+    test_suite: list[TestSuite] | TestSuite, module: ModuleType | None = None
+) -> GreenTestSuite:
+    """
+    Look for a `doctest_modules` list and attempt to add doctest tests to the
+    suite of tests that we are about to flatten.
+    """
     # todo: rename this function to something more appropriate.
-    suites = [test_suite]
+    suites = test_suite if isinstance(test_suite, list) else [test_suite]
     doctest_modules = getattr(module, "doctest_modules", ())
     for doctest_module in doctest_modules:
-        suite = DocTestSuite(doctest_module)
-        suite.injected_module = module.__name__
-        suites.append(suite)
+        doc_suite = DocTestSuite(doctest_module)
+        # Forcing an injected_module onto DocTestSuite.
+        doc_suite.injected_module = module.__name__  # type: ignore
+        suites.append(doc_suite)
 
     # Now extract all tests from the suite hierarchies and flatten them into a
     # single suite with all tests.
-    tests = []
+    tests: list[TestCase | TestSuite] = []
     for suite in suites:
-        injected_module = None
-        if getattr(suite, "injected_module", None):
-            injected_module = suite.injected_module
+        # injected_module is not present in DocTestSuite.
+        injected_module: str | None = getattr(suite, "injected_module", None)
         for test in suite:
             if injected_module:
                 # For doctests, inject the test module name so we can later