Stabilize tests (#636)

Makefile

@@ -12,6 +12,7 @@ PYDEPS = \
 	psutil \
 	pyopenssl \
 	pytest \
+	pytest-xdist \
 	setuptools
 # dev deps
 ifndef GITHUB_ACTIONS
@@ -32,7 +33,7 @@ endif
 # In not in a virtualenv, add --user options for install commands.
 INSTALL_OPTS = `$(PYTHON) -c "import sys; print('' if hasattr(sys, 'real_prefix') else '--user')"`
 TEST_PREFIX = PYTHONWARNINGS=always
-PYTEST_ARGS = -v --tb=native -o cache_dir=/tmp/pyftpdlib-pytest-cache
+PYTEST_ARGS = -v -s --tb=short
 NUM_WORKERS = `$(PYTHON) -c "import os; print(os.cpu_count() or 1)"`
 
 # if make is invoked with no arg, default to `make help`
@@ -111,47 +112,36 @@ setup-dev-env: ## Install GIT hooks, pip, test deps (also upgrades them).
 # ===================================================================
 
 test:  ## Run all tests. To run a specific test: do "make test ARGS=pyftpdlib.test.test_functional.TestFtpStoreData"
-	${MAKE} install
 	$(TEST_PREFIX) $(PYTHON) -m pytest $(PYTEST_ARGS) $(ARGS)
 
 test-parallel:  ## Run all tests in parallel.
-	${MAKE} install
 	$(TEST_PREFIX) $(PYTHON) -m pytest $(PYTEST_ARGS) -n auto --dist loadgroup $(ARGS)
 
 test-functional:  ## Run functional FTP tests.
-	${MAKE} install
 	$(TEST_PREFIX) $(PYTHON) -m pytest $(PYTEST_ARGS) $(ARGS) pyftpdlib/test/test_functional.py
 
 test-functional-ssl:  ## Run functional FTPS tests.
-	${MAKE} install
 	$(TEST_PREFIX) $(PYTHON) -m pytest $(PYTEST_ARGS) $(ARGS) pyftpdlib/test/test_functional_ssl.py
 
 test-servers:  ## Run tests for FTPServer and its subclasses.
-	${MAKE} install
 	$(TEST_PREFIX) $(PYTHON) -m pytest $(PYTEST_ARGS) $(ARGS) pyftpdlib/test/test_servers.py
 
 test-authorizers:  ## Run tests for authorizers.
-	${MAKE} install
 	$(TEST_PREFIX) $(PYTHON) -m pytest $(PYTEST_ARGS) $(ARGS) pyftpdlib/test/test_authorizers.py
 
 test-filesystems:  ## Run filesystem tests.
-	${MAKE} install
 	$(TEST_PREFIX) $(PYTHON) -m pytest $(PYTEST_ARGS) $(ARGS) pyftpdlib/test/test_filesystems.py
 
 test-ioloop:  ## Run IOLoop tests.
-	${MAKE} install
 	$(TEST_PREFIX) $(PYTHON) -m pytest $(PYTEST_ARGS) $(ARGS) pyftpdlib/test/test_ioloop.py
 
 test-cli:  ## Run miscellaneous tests.
-	${MAKE} install
 	$(TEST_PREFIX) $(PYTHON) -m pytest $(PYTEST_ARGS) $(ARGS) pyftpdlib/test/test_cli.py
 
 test-lastfailed:  ## Run previously failed tests
-	${MAKE} install
 	$(TEST_PREFIX) $(PYTHON) -m pytest $(PYTEST_ARGS) --last-failed $(ARGS)
 
 test-coverage:  ## Run test coverage.
-	${MAKE} install
 	rm -rf .coverage htmlcov
 	$(TEST_PREFIX) $(PYTHON) -m coverage run -m pytest $(PYTEST_ARGS) $(ARGS)
 	$(PYTHON) -m coverage report

pyftpdlib/authorizers.py

@@ -396,9 +396,11 @@ def _is_rejected_user(self, username):
 # ===================================================================
 
 try:
-    import crypt
-    import pwd
-    import spwd
+    with warnings.catch_warnings():
+        warnings.simplefilter("ignore")
+        import crypt
+        import pwd
+        import spwd
 except ImportError:
     pass
 else:

pyftpdlib/ioloop.py

@@ -172,6 +172,16 @@ def reheapify(self):
         self._tasks = [x for x in self._tasks if not x.cancelled]
         heapq.heapify(self._tasks)
 
+    def close(self):
+        for x in self._tasks:
+            try:
+                if not x.cancelled:
+                    x.cancel()
+            except Exception:
+                logger.error(traceback.format_exc())
+        del self._tasks[:]
+        self._cancellations = 0
+
 
 class _CallLater:
     """Container object which instance is returned by ioloop.call_later()."""
@@ -417,13 +427,7 @@ def close(self):
         self.socket_map.clear()
 
         # free scheduled functions
-        for x in self.sched._tasks:
-            try:
-                if not x.cancelled:
-                    x.cancel()
-            except Exception:
-                logger.error(traceback.format_exc())
-        del self.sched._tasks[:]
+        self.sched.close()
 
 
 # ===================================================================

pyftpdlib/test/__init__.py

@@ -3,7 +3,6 @@
 # found in the LICENSE file.
 
 
-import atexit
 import contextlib
 import functools
 import logging
@@ -21,29 +20,26 @@
 
 import psutil
 
+import pyftpdlib.servers
 from pyftpdlib.authorizers import DummyAuthorizer
 from pyftpdlib.handlers import FTPHandler
 from pyftpdlib.ioloop import IOLoop
 from pyftpdlib.servers import FTPServer
 
 
-# --- platforms
-
 HERE = os.path.realpath(os.path.abspath(os.path.dirname(__file__)))
 ROOT_DIR = os.path.realpath(os.path.join(HERE, '..', '..'))
+
 PYPY = '__pypy__' in sys.builtin_module_names
-GITHUB_ACTIONS = 'GITHUB_ACTIONS' in os.environ or 'CIBUILDWHEEL' in os.environ
-CI_TESTING = GITHUB_ACTIONS
-COVERAGE = 'COVERAGE_RUN' in os.environ
-# are we a 64 bit process?
-IS_64BIT = sys.maxsize > 2**32
 OSX = sys.platform.startswith("darwin")
 POSIX = os.name == 'posix'
 BSD = "bsd" in sys.platform
 WINDOWS = os.name == 'nt'
-LOG_FMT = "[%(levelname)1.1s t: %(threadName)-15s p: %(processName)-25s "
-LOG_FMT += "@%(module)-12s: %(lineno)-4s] %(message)s"
 
+GITHUB_ACTIONS = 'GITHUB_ACTIONS' in os.environ or 'CIBUILDWHEEL' in os.environ
+CI_TESTING = GITHUB_ACTIONS
+COVERAGE = 'COVERAGE_RUN' in os.environ
+PYTEST_PARALLEL = "PYTEST_XDIST_WORKER" in os.environ  # `make test-parallel`
 
 # Attempt to use IP rather than hostname (test suite will run a lot faster)
 try:
@@ -66,6 +62,12 @@
     GLOBAL_TIMEOUT *= 3
     NO_RETRIES *= 3
 
+SUPPORTS_IPV4 = None  # set later
+SUPPORTS_IPV6 = None  # set later
+SUPPORTS_MULTIPROCESSING = hasattr(pyftpdlib.servers, 'MultiprocessFTPServer')
+if BSD or OSX and GITHUB_ACTIONS:
+    SUPPORTS_MULTIPROCESSING = False  # XXX: it's broken!!
+
 
 class PyftpdlibTestCase(unittest.TestCase):
     """All test classes inherit from this one."""
@@ -107,6 +109,8 @@ def close_client(session):
 def try_address(host, port=0, family=socket.AF_INET):
     """Try to bind a socket on the given host:port and return True
     if that has been possible."""
+    # Note: if IPv6 fails on Linux do:
+    # $ sudo sh -c 'echo 0 > /proc/sys/net/ipv6/conf/all/disable_ipv6'
     try:
         with contextlib.closing(socket.socket(family)) as sock:
             sock.bind((host, port))
@@ -176,16 +180,6 @@ def touch(name):
         return f.name
 
 
-def configure_logging():
-    """Set pyftpdlib logger to "WARNING" level."""
-    handler = logging.StreamHandler()
-    formatter = logging.Formatter(fmt=LOG_FMT)
-    handler.setFormatter(formatter)
-    logger = logging.getLogger('pyftpdlib')
-    logger.setLevel(logging.WARNING)
-    logger.addHandler(handler)
-
-
 def disable_log_warning(fun):
     """Temporarily set FTP server's logging level to ERROR."""
 
@@ -202,18 +196,6 @@ def wrapper(self, *args, **kwargs):
     return wrapper
 
 
-def cleanup():
-    """Cleanup function executed on interpreter exit."""
-    map = IOLoop.instance().socket_map
-    for x in list(map.values()):
-        try:
-            sys.stderr.write("garbage: %s\n" % repr(x))
-            x.close()
-        except Exception:  # noqa
-            pass
-    map.clear()
-
-
 class retry:
     """A retry decorator."""
 
@@ -274,17 +256,27 @@ def wrapper(cls, *args, **kwargs):
         return wrapper
 
 
-def retry_on_failure(retries=NO_RETRIES):
+def retry_on_failure(fun):
     """Decorator which runs a test function and retries N times before
     actually failing.
     """
 
-    def logfun(exc):
-        print("%r, retrying" % exc, file=sys.stderr)  # NOQA
+    @functools.wraps(fun)
+    def wrapper(self, *args, **kwargs):
+        for x in range(NO_RETRIES):
+            try:
+                return fun(self, *args, **kwargs)
+            except AssertionError as exc:
+                if x + 1 >= NO_RETRIES:
+                    raise
+                msg = "%r, retrying" % exc
+                print(msg, file=sys.stderr)  # NOQA
+                if PYTEST_PARALLEL:
+                    warnings.warn(msg, ResourceWarning, stacklevel=2)
+                self.tearDown()
+                self.setUp()
 
-    return retry(
-        exception=AssertionError, timeout=None, retries=retries, logfun=logfun
-    )
+    return wrapper
 
 
 def call_until(fun, expr, timeout=GLOBAL_TIMEOUT):
@@ -339,7 +331,8 @@ def assert_free_resources(parent_pid=None):
     children = this_proc.children()
     if children:
         warnings.warn(
-            "some children didn't terminate %r" % str(children),
+            "some children didn't terminate (pid=%r) %r"
+            % (os.getpid(), str(children)),
             UserWarning,
             stacklevel=2,
         )
@@ -358,7 +351,8 @@ def assert_free_resources(parent_pid=None):
         ]
         if cons:
             warnings.warn(
-                "some connections didn't close %r" % str(cons),
+                "some connections didn't close (pid=%r) %r"
+                % (os.getpid(), str(cons)),
                 UserWarning,
                 stacklevel=2,
             )
@@ -416,7 +410,7 @@ def reset_server_opts():
         klass.max_cons_per_ip = 0
 
 
-class ThreadedTestFTPd(threading.Thread):
+class FtpdThreadWrapper(threading.Thread):
     """A threaded FTP server used for running tests.
     This is basically a modified version of the FTPServer class which
     wraps the polling loop into a thread.
@@ -460,7 +454,7 @@ def stop(self):
 
 if POSIX:
 
-    class MProcessTestFTPd(multiprocessing.Process):
+    class FtpdMultiprocWrapper(multiprocessing.Process):
         """Same as above but using a sub process instead."""
 
         handler = FTPHandler
@@ -489,11 +483,4 @@ def stop(self):
 
 else:
     # Windows
-    MProcessTestFTPd = ThreadedTestFTPd
-
-
-@atexit.register
-def exit_cleanup():
-    for name in os.listdir(ROOT_DIR):
-        if name.startswith(TESTFN_PREFIX):
-            safe_rmpath(os.path.join(ROOT_DIR, name))
+    FtpdMultiprocWrapper = FtpdThreadWrapper

pyftpdlib/test/conftest.py

@@ -14,27 +14,40 @@
 tearDown(), setUpClass(), tearDownClass() methods for each test class.
 """
 
+import atexit
+import os
 import threading
 import warnings
 
+import psutil
 import pytest
 
+from pyftpdlib.ioloop import IOLoop
+
+from . import POSIX
+from . import ROOT_DIR
+from . import TESTFN_PREFIX
+from . import safe_rmpath
+
+
+# set it to True to raise an exception instead of warning
+FAIL = False
+this_proc = psutil.Process()
+
 
 def collect_resources():
-    # Note: files and sockets are already collected by pytest, so no
-    # need to use psutil for it.
     res = {}
     res["threads"] = set(threading.enumerate())
+    if POSIX:
+        res["num_fds"] = this_proc.num_fds()
+    # res["cons"] = set(this_proc.net_connections(kind="all"))
+    # res["files"] = set(this_proc.open_files())
     return res
 
 
-def setup(origin):
-    ctx = collect_resources()
-    ctx["_origin"] = origin
-    return ctx
-
-
 def warn(msg):
+    if FAIL:
+        raise RuntimeError(msg)
     warnings.warn(msg, ResourceWarning, stacklevel=3)
 
 
@@ -61,7 +74,36 @@ def assert_closed_resources(setup_ctx, request):
                 warn(msg)
 
 
+def assert_closed_ioloop():
+    inst = IOLoop.instance()
+    if inst.socket_map:
+        warn(f"unclosed ioloop socket map {inst.socket_map}")
+    if inst.sched._tasks:
+        warn(f"unclosed ioloop tasks {inst.sched._tasks}")
+
+
+# ---
+
+
+def setup_method(origin):
+    ctx = collect_resources()
+    ctx["_origin"] = origin
+    return ctx
+
+
+def teardown_method(setup_ctx, request):
+    assert_closed_resources(setup_ctx, request)
+    assert_closed_ioloop()
+
+
 @pytest.fixture(autouse=True, scope="function")
 def for_each_test_method(request):
-    ctx = setup(request.node.nodeid)
-    request.addfinalizer(lambda: assert_closed_resources(ctx, request))
+    ctx = setup_method(request.node.nodeid)
+    request.addfinalizer(lambda: teardown_method(ctx, request))
+
+
+@atexit.register
+def on_exit():
+    for name in os.listdir(ROOT_DIR):
+        if name.startswith(TESTFN_PREFIX):
+            safe_rmpath(os.path.join(ROOT_DIR, name))