Repository: aaugustin/websockets
Pull request #1255 (2022-10-31 21:16:14)

Add a script to measure coverage per module.
This makes it possible to increase coverage threshold to "each module has 100% branch coverage from its own tests".

This implementation supports `make maxi_cov` and `tox -e maxi_cov`. It is also enabled in CI.


Patch:
diff --git a/Makefile b/Makefile
index cd8095a..ac5d6a4 100644
--- a/Makefile
+++ b/Makefile
@@ -1,4 +1,4 @@
-.PHONY: default style test coverage build clean
+.PHONY: default style test coverage maxi_cov build clean
 
 export PYTHONASYNCIODEBUG=1
 export PYTHONPATH=src
@@ -20,6 +20,11 @@ coverage:
 	coverage html
 	coverage report --show-missing --fail-under=100
 
+maxi_cov:
+	python tests/maxi_cov.py
+	coverage html
+	coverage report --show-missing --fail-under=100
+
 build:
 	python setup.py build_ext --inplace
 
diff --git a/setup.cfg b/setup.cfg
index 74efc80..4c1f609 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -21,7 +21,9 @@ lines_after_imports = 2
 [coverage:run]
 branch = True
 omit =
-    */__main__.py
+    # */websockets matches src/websockets and .tox/**/site-packages/websockets
+    */websockets/__main__.py
+    tests/maxi_cov.py
 
 [coverage:paths]
 source =
diff --git a/src/websockets/exceptions.py b/src/websockets/exceptions.py
index 0c4fc51..291b6d2 100644
--- a/src/websockets/exceptions.py
+++ b/src/websockets/exceptions.py
@@ -120,11 +120,15 @@ class ConnectionClosed(WebSocketException):
 
     @property
     def code(self) -> int:
-        return 1006 if self.rcvd is None else self.rcvd.code
+        if self.rcvd is None:
+            return 1006
+        return self.rcvd.code
 
     @property
     def reason(self) -> str:
-        return "" if self.rcvd is None else self.rcvd.reason
+        if self.rcvd is None:
+            return ""
+        return self.rcvd.reason
 
 
 class ConnectionClosedError(ConnectionClosed):
diff --git a/tox.ini b/tox.ini
index 2f37cdc..0fcab4d 100644
--- a/tox.ini
+++ b/tox.ini
@@ -21,6 +21,12 @@ commands =
     python -m coverage report --show-missing --fail-under=100
 deps = coverage
 
+[testenv:maxi_cov]
+commands =
+    python tests/maxi_cov.py {envsitepackagesdir}
+    python -m coverage report --show-missing --fail-under=100
+deps = coverage
+
 [testenv:black]
 commands = black --check src tests
 deps = black


Test patch:
diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml
index 4c1b1e8..4d5cc3c 100644
--- a/.github/workflows/tests.yml
+++ b/.github/workflows/tests.yml
@@ -9,8 +9,8 @@ on:
       - main
 
 jobs:
-  main:
-    name: Run code quality checks
+  coverage:
+    name: Run test coverage checks
     runs-on: ubuntu-latest
     steps:
     - name: Check out repository
@@ -23,6 +23,21 @@ jobs:
       run: pip install tox
     - name: Run tests with coverage
       run: tox -e coverage
+    - name: Run tests with per-module coverage
+      run: tox -e maxi_cov
+
+  quality:
+    name: Run code quality checks
+    runs-on: ubuntu-latest
+    steps:
+    - name: Check out repository
+      uses: actions/checkout@v3
+    - name: Install Python 3.x
+      uses: actions/setup-python@v4
+      with:
+        python-version: "3.x"
+    - name: Install tox
+      run: pip install tox
     - name: Check code formatting
       run: tox -e black
     - name: Check code style
@@ -34,7 +49,9 @@ jobs:
 
   matrix:
     name: Run tests on Python ${{ matrix.python }}
-    needs: main
+    needs:
+      - coverage
+      - quality
     runs-on: ubuntu-latest
     strategy:
       matrix:
diff --git a/tests/extensions/test_base.py b/tests/extensions/test_base.py
index 0daa342..ba8657b 100644
--- a/tests/extensions/test_base.py
+++ b/tests/extensions/test_base.py
@@ -1,40 +1,4 @@
-from websockets.exceptions import NegotiationError
 from websockets.extensions.base import *  # noqa
 
 
 # Abstract classes don't provide any behavior to test.
-
-
-class ClientNoOpExtensionFactory:
-    name = "x-no-op"
-
-    def get_request_params(self):
-        return []
-
-    def process_response_params(self, params, accepted_extensions):
-        if params:
-            raise NegotiationError()
-        return NoOpExtension()
-
-
-class ServerNoOpExtensionFactory:
-    name = "x-no-op"
-
-    def __init__(self, params=None):
-        self.params = params or []
-
-    def process_request_params(self, params, accepted_extensions):
-        return self.params, NoOpExtension()
-
-
-class NoOpExtension:
-    name = "x-no-op"
-
-    def __repr__(self):
-        return "NoOpExtension()"
-
-    def decode(self, frame, *, max_size=None):
-        return frame
-
-    def encode(self, frame):
-        return frame
diff --git a/tests/extensions/test_permessage_deflate.py b/tests/extensions/test_permessage_deflate.py
index 3cc9172..d433762 100644
--- a/tests/extensions/test_permessage_deflate.py
+++ b/tests/extensions/test_permessage_deflate.py
@@ -21,7 +21,7 @@ from websockets.frames import (
     Frame,
 )
 
-from .test_base import ClientNoOpExtensionFactory, ServerNoOpExtensionFactory
+from .utils import ClientNoOpExtensionFactory, ServerNoOpExtensionFactory
 
 
 class ExtensionTestsMixin:
diff --git a/tests/extensions/utils.py b/tests/extensions/utils.py
index 1eabc16..24fb74b 100644
--- a/tests/extensions/utils.py
+++ b/tests/extensions/utils.py
@@ -46,6 +46,41 @@ class ServerOpExtensionFactory:
         return [("op", self.op)], OpExtension(self.op)
 
 
+class NoOpExtension:
+    name = "x-no-op"
+
+    def __repr__(self):
+        return "NoOpExtension()"
+
+    def decode(self, frame, *, max_size=None):
+        return frame
+
+    def encode(self, frame):
+        return frame
+
+
+class ClientNoOpExtensionFactory:
+    name = "x-no-op"
+
+    def get_request_params(self):
+        return []
+
+    def process_response_params(self, params, accepted_extensions):
+        if params:
+            raise NegotiationError()
+        return NoOpExtension()
+
+
+class ServerNoOpExtensionFactory:
+    name = "x-no-op"
+
+    def __init__(self, params=None):
+        self.params = params or []
+
+    def process_request_params(self, params, accepted_extensions):
+        return self.params, NoOpExtension()
+
+
 class Rsv2Extension:
     name = "x-rsv2"
 
diff --git a/tests/legacy/test_client_server.py b/tests/legacy/test_client_server.py
index 3004c68..8ee7187 100644
--- a/tests/legacy/test_client_server.py
+++ b/tests/legacy/test_client_server.py
@@ -37,7 +37,7 @@ from websockets.legacy.http import read_response
 from websockets.legacy.server import *
 from websockets.uri import parse_uri
 
-from ..extensions.test_base import (
+from ..extensions.utils import (
     ClientNoOpExtensionFactory,
     NoOpExtension,
     ServerNoOpExtensionFactory,
diff --git a/tests/maxi_cov.py b/tests/maxi_cov.py
new file mode 100755
index 0000000..5acb979
--- /dev/null
+++ b/tests/maxi_cov.py
@@ -0,0 +1,148 @@
+#!/usr/bin/env python
+
+"""Measure coverage of each module by its test module."""
+
+import glob
+import os.path
+import subprocess
+import sys
+
+
+UNMAPPED_SRC_FILES = ["websockets/version.py"]
+UNMAPPED_TEST_FILES = ["tests/test_exports.py"]
+
+IGNORED_FILES = [
+    # */websockets matches src/websockets and .tox/**/site-packages/websockets.
+    # There are no tests for the __main__ module.
+    "*/websockets/__main__.py",
+    # This approach isn't applicable to the test suite of the legacy
+    # implementation, due to the huge test_client_server test module.
+    "*/websockets/legacy/*",
+    "tests/legacy/*",
+    # Test utilities don't fit anywhere because they are shared.
+    "tests/extensions/utils.py",
+    "tests/utils.py",
+    # There is no point measure the coverage of this script.
+    "tests/maxi_cov.py",
+]
+
+
+def check_environment():
+    """Check that prerequisites for running this script are met."""
+    try:
+        import websockets  # noqa: F401
+    except ImportError:
+        print("failed to import websockets; is src on PYTHONPATH?")
+        return False
+    try:
+        import coverage  # noqa: F401
+    except ImportError:
+        print("failed to locate Coverage.py; is it installed?")
+        return False
+    return True
+
+
+def get_mapping(src_dir="src"):
+    """Return a dict mapping each source file to its test file."""
+
+    # List source and test files.
+
+    src_files = glob.glob(
+        os.path.join(src_dir, "websockets/**/*.py"),
+        recursive=True,
+    )
+
+    test_files = glob.glob(
+        "tests/**/*.py",
+        recursive=True,
+    )
+
+    src_files = [
+        os.path.relpath(src_file, src_dir)
+        for src_file in sorted(src_files)
+        if os.path.basename(src_file) != "__init__.py"
+        and os.path.basename(src_file) != "__main__.py"
+        and "legacy" not in os.path.dirname(src_file)
+    ]
+    test_files = [
+        test_file
+        for test_file in sorted(test_files)
+        if os.path.basename(test_file) != "__init__.py"
+        and os.path.basename(test_file).startswith("test_")
+        and "legacy" not in os.path.dirname(test_file)
+    ]
+
+    # Map source files to test files.
+
+    mapping = {}
+    unmapped_test_files = []
+
+    for test_file in test_files:
+        dir_name, file_name = os.path.split(test_file)
+        assert dir_name.startswith("tests")
+        assert file_name.startswith("test_")
+        src_file = os.path.join(
+            "websockets" + dir_name[len("tests") :],
+            file_name[len("test_") :],
+        )
+        if src_file in src_files:
+            mapping[src_file] = test_file
+        else:
+            unmapped_test_files.append(test_file)
+
+    unmapped_src_files = list(set(src_files) - set(mapping))
+
+    # Ensure that all files are mapped.
+
+    assert unmapped_src_files == UNMAPPED_SRC_FILES
+    assert unmapped_test_files == UNMAPPED_TEST_FILES
+
+    return mapping
+
+
+def run_coverage(mapping, src_dir="src"):
+    # Initialize a new coverage measurement session. The --source option
+    # includes all files in the report, even if they're never imported.
+    print("\nInitializing session\n", flush=True)
+    subprocess.run(
+        [
+            sys.executable,
+            "-m",
+            "coverage",
+            "run",
+            "--source",
+            ",".join([os.path.join(src_dir, "websockets"), "tests"]),
+            "--omit",
+            ",".join(IGNORED_FILES),
+            "-m",
+            "unittest",
+        ]
+        + UNMAPPED_TEST_FILES,
+        check=True,
+    )
+    # Append coverage of each source module by the corresponding test module.
+    for src_file, test_file in mapping.items():
+        print(f"\nTesting {src_file} with {test_file}\n", flush=True)
+        subprocess.run(
+            [
+                sys.executable,
+                "-m",
+                "coverage",
+                "run",
+                "--append",
+                "--include",
+                ",".join([os.path.join(src_dir, src_file), test_file]),
+                "-m",
+                "unittest",
+                test_file,
+            ],
+            check=True,
+        )
+
+
+if __name__ == "__main__":
+    if not check_environment():
+        sys.exit(1)
+    src_dir = sys.argv[1] if len(sys.argv) == 2 else "src"
+    mapping = get_mapping(src_dir)
+    run_coverage(mapping, src_dir)
diff --git a/tests/test_connection.py b/tests/test_connection.py
index 44f5bc3..3858d25 100644
--- a/tests/test_connection.py
+++ b/tests/test_connection.py
@@ -1,3 +1,4 @@
+import logging
 import unittest.mock
 
 from websockets.connection import *
@@ -1712,3 +1713,25 @@ class ExtensionsTests(ConnectionTestCase):
         server.extensions = [Rsv2Extension()]
         server.receive_data(b"\xaa\x80\x00\x44\x88\xcc")
         self.assertEqual(server.events_received(), [Frame(OP_PONG, b"")])
+
+
+class MiscTests(unittest.TestCase):
+    def test_client_default_logger(self):
+        client = Connection(CLIENT)
+        logger = logging.getLogger("websockets.client")
+        self.assertIs(client.logger, logger)
+
+    def test_server_default_logger(self):
+        server = Connection(SERVER)
+        logger = logging.getLogger("websockets.server")
+        self.assertIs(server.logger, logger)
+
+    def test_client_custom_logger(self):
+        logger = logging.getLogger("test")
+        client = Connection(CLIENT, logger=logger)
+        self.assertIs(client.logger, logger)
+
+    def test_server_custom_logger(self):
+        logger = logging.getLogger("test")
+        server = Connection(SERVER, logger=logger)
+        self.assertIs(server.logger, logger)
diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py
index 3ede25f..a0f9dfc 100644
--- a/tests/test_exceptions.py
+++ b/tests/test_exceptions.py
@@ -157,3 +157,13 @@ class ExceptionsTests(unittest.TestCase):
         ]:
             with self.subTest(exception=exception):
                 self.assertEqual(str(exception), exception_str)
+
+    def test_connection_closed_attributes_backwards_compatibility(self):
+        exception = ConnectionClosed(Close(1000, "OK"), None, None)
+        self.assertEqual(exception.code, 1000)
+        self.assertEqual(exception.reason, "OK")
+
+    def test_connection_closed_attributes_backwards_compatibility_defaults(self):
+        exception = ConnectionClosed(None, None, None)
+        self.assertEqual(exception.code, 1006)
+        self.assertEqual(exception.reason, "")