From 1fa38467d74f1aed72d93d30f67dcb8870fa6af6 Mon Sep 17 00:00:00 2001 From: Brendon Smith Date: Sat, 15 Apr 2023 17:25:21 -0400 Subject: [PATCH 01/19] Add requirements for Gunicorn worker testing --- requirements.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/requirements.txt b/requirements.txt index 5b8685603..830679a4b 100644 --- a/requirements.txt +++ b/requirements.txt @@ -24,6 +24,7 @@ trustme==0.9.0 cryptography==41.0.0 coverage==7.2.7 coverage-conditional-plugin==0.8.0 +gunicorn==20.1.0; sys_platform != 'win32' httpx==0.23.0 watchgod==0.8.2 From 4b8923d599f79e83e1f50a0bedc0f51df9f42427 Mon Sep 17 00:00:00 2001 From: Brendon Smith Date: Sat, 3 Jun 2023 13:29:46 -0400 Subject: [PATCH 02/19] Ensure TOML extra is installed for coverage https://coverage.readthedocs.io/en/stable/config.html --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 830679a4b..9ca5f300c 100644 --- a/requirements.txt +++ b/requirements.txt @@ -22,7 +22,7 @@ types-click==7.1.8 types-pyyaml==6.0.12.9 trustme==0.9.0 cryptography==41.0.0 -coverage==7.2.7 +coverage[toml]==7.2.7 coverage-conditional-plugin==0.8.0 gunicorn==20.1.0; sys_platform != 'win32' httpx==0.23.0 From 0844cf30042098ed300ea414c01f675429ce3a7e Mon Sep 17 00:00:00 2001 From: Brendon Smith Date: Wed, 19 Apr 2023 21:25:15 -0400 Subject: [PATCH 03/19] Update coverage configuration for subprocesses - Enable the required parallel mode - Set the required `COVERAGE_PROCESS_START` environment variable - Add `coverage_enable_subprocess` for `coverage.process_startup` - Combine coverage reports before reporting coverage - Update `.gitignore` to ignore files named `.coverage*` https://coverage.readthedocs.io/en/latest/subprocess.html --- .gitignore | 2 +- pyproject.toml | 1 + requirements.txt | 1 + scripts/coverage | 1 + scripts/test | 4 ++++ 5 files changed, 8 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index a92a445ac..5350c5bb3 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,5 @@ .cache -.coverage +.coverage* .mypy_cache/ __pycache__/ uvicorn.egg-info/ diff --git a/pyproject.toml b/pyproject.toml index 0a2de826c..0bd61ddd8 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -96,6 +96,7 @@ filterwarnings = [ [tool.coverage.run] source_pkgs = ["uvicorn", "tests"] plugins = ["coverage_conditional_plugin"] +parallel = true [tool.coverage.report] precision = 2 diff --git a/requirements.txt b/requirements.txt index 9ca5f300c..3e96007e8 100644 --- a/requirements.txt +++ b/requirements.txt @@ -24,6 +24,7 @@ trustme==0.9.0 cryptography==41.0.0 coverage[toml]==7.2.7 coverage-conditional-plugin==0.8.0 +coverage_enable_subprocess==1.0 gunicorn==20.1.0; sys_platform != 'win32' httpx==0.23.0 watchgod==0.8.2 diff --git a/scripts/coverage b/scripts/coverage index c93e45e85..b263df2ad 100755 --- a/scripts/coverage +++ b/scripts/coverage @@ -8,4 +8,5 @@ export SOURCE_FILES="uvicorn tests" set -x +${PREFIX}coverage combine -q ${PREFIX}coverage report diff --git a/scripts/test b/scripts/test index ed2bdd01a..89052791d 100755 --- a/scripts/test +++ b/scripts/test @@ -11,6 +11,10 @@ if [ -z $GITHUB_ACTIONS ]; then scripts/check fi +# enable subprocess coverage +# https://coverage.readthedocs.io/en/latest/subprocess.html +# https://github.com/nedbat/coveragepy/issues/367 +export COVERAGE_PROCESS_START=$PWD/pyproject.toml ${PREFIX}coverage run --debug config -m pytest "$@" if [ -z $GITHUB_ACTIONS ]; then From 65a16c7da5ba0a153ab80273274b21e4e3232e26 Mon Sep 17 00:00:00 2001 From: Brendon Smith Date: Wed, 19 Apr 2023 21:28:54 -0400 Subject: [PATCH 04/19] Test Uvicorn worker requests and signal handling --- pyproject.toml | 3 + tests/test_workers.py | 147 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 150 insertions(+) create mode 100644 tests/test_workers.py diff --git a/pyproject.toml b/pyproject.toml index 0bd61ddd8..3ab490f3f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -92,6 +92,9 @@ filterwarnings = [ "ignore:Uvicorn's native WSGI implementation is deprecated.*:DeprecationWarning", "ignore: 'cgi' is deprecated and slated for removal in Python 3.13:DeprecationWarning" ] +markers = [ + "subprocess: test requires a subprocess (deselect with '-m \"not subprocess\"')" +] [tool.coverage.run] source_pkgs = ["uvicorn", "tests"] diff --git a/tests/test_workers.py b/tests/test_workers.py new file mode 100644 index 000000000..23292d2ed --- /dev/null +++ b/tests/test_workers.py @@ -0,0 +1,147 @@ +from __future__ import annotations + +import signal +import subprocess +import sys +import tempfile +import time +from typing import TYPE_CHECKING + +import httpx +import pytest + +if TYPE_CHECKING: + from ssl import SSLContext + from typing import IO, Generator + +pytestmark = pytest.mark.skipif(sys.platform == "win32", reason="requires unix") +gunicorn_arbiter = pytest.importorskip("gunicorn.arbiter", reason="requires gunicorn") +uvicorn_workers = pytest.importorskip("uvicorn.workers", reason="requires gunicorn") + + +class Process(subprocess.Popen): + client: httpx.Client + output: IO[bytes] + + def read_output(self) -> str: + self.output.seek(0) + return self.output.read().decode() + + +@pytest.fixture( + params=( + pytest.param(uvicorn_workers.UvicornWorker, marks=pytest.mark.subprocess), + pytest.param(uvicorn_workers.UvicornH11Worker, marks=pytest.mark.subprocess), + ) +) +def worker_class(request: pytest.FixtureRequest) -> str: + """Gunicorn worker class names to test. + + This is a parametrized fixture. When the fixture is used in a test, the test + will be automatically parametrized, running once for each fixture parameter. All + tests using the fixture will be automatically marked with `pytest.mark.subprocess`. + + https://docs.pytest.org/en/latest/how-to/fixtures.html + https://docs.pytest.org/en/latest/proposals/parametrize_with_fixtures.html + """ + worker_class = request.param + return f"{worker_class.__module__}.{worker_class.__name__}" + + +@pytest.fixture( + params=( + pytest.param(False, id="TLS off"), + pytest.param(True, id="TLS on"), + ) +) +def gunicorn_process( + request: pytest.FixtureRequest, + tls_ca_certificate_pem_path: str, + tls_ca_ssl_context: SSLContext, + tls_certificate_private_key_path: str, + tls_certificate_server_cert_path: str, + unused_tcp_port: int, + worker_class: str, +) -> Generator[Process, None, None]: + """Yield a subprocess running a Gunicorn arbiter with a Uvicorn worker. + + An instance of `httpx.Client` is available on the `client` attribute. + Output is saved to a temporary file and accessed with `read_output()`. + """ + app = "tests.test_main:app" + bind = f"127.0.0.1:{unused_tcp_port}" + use_tls: bool = request.param + args = [ + "gunicorn", + "--bind", + bind, + "--graceful-timeout", + "1", + "--log-level", + "debug", + "--worker-class", + worker_class, + "--workers", + "1", + ] + if use_tls is True: + args_for_tls = [ + "--ca-certs", + tls_ca_certificate_pem_path, + "--certfile", + tls_certificate_server_cert_path, + "--keyfile", + tls_certificate_private_key_path, + ] + args.extend(args_for_tls) + base_url = f"https://{bind}" + verify: SSLContext | bool = tls_ca_ssl_context + else: + base_url = f"http://{bind}" + verify = False + args.append(app) + client = httpx.Client(base_url=base_url, verify=verify) + output = tempfile.TemporaryFile() + with Process(args, stdout=output, stderr=output) as process: + time.sleep(1) + assert not process.poll() + process.client = client + process.output = output + yield process + process.terminate() + process.wait(timeout=2) + client.close() + output.close() + + +def test_get_request_to_asgi_app(gunicorn_process: Process) -> None: + """Test a GET request to the Gunicorn Uvicorn worker's ASGI app.""" + response = gunicorn_process.client.get("/") + output_text = gunicorn_process.read_output() + assert response.status_code == 204 + assert "uvicorn.workers", "startup complete" in output_text + + +@pytest.mark.parametrize("signal_to_send", gunicorn_arbiter.Arbiter.SIGNALS) +def test_gunicorn_arbiter_signal_handling( + gunicorn_process: Process, signal_to_send: signal.Signals +) -> None: + """Test Gunicorn arbiter signal handling. + + This test iterates over the signals handled by the Gunicorn arbiter, + sends each signal to the process running the arbiter, and asserts that + Gunicorn handles the signal and logs the signal handling event accordingly. + + https://docs.gunicorn.org/en/latest/signals.html + """ + signal_abbreviation = gunicorn_arbiter.Arbiter.SIG_NAMES[signal_to_send] + expected_text = f"Handling signal: {signal_abbreviation}" + gunicorn_process.send_signal(signal_to_send) + time.sleep(0.5) + output_text = gunicorn_process.read_output() + try: + assert expected_text in output_text + except AssertionError: # pragma: no cover + time.sleep(2) + output_text = gunicorn_process.read_output() + assert expected_text in output_text From 1be545c2c453ca7723890fa6213dcdc07b77e480 Mon Sep 17 00:00:00 2001 From: Brendon Smith Date: Sun, 30 Apr 2023 18:29:55 -0400 Subject: [PATCH 05/19] Test Uvicorn worker boot errors --- tests/test_workers.py | 83 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/tests/test_workers.py b/tests/test_workers.py index 23292d2ed..4dc4cc95b 100644 --- a/tests/test_workers.py +++ b/tests/test_workers.py @@ -14,6 +14,13 @@ from ssl import SSLContext from typing import IO, Generator + from asgiref.typing import ( + ASGIReceiveCallable, + ASGISendCallable, + LifespanStartupFailedEvent, + Scope, + ) + pytestmark = pytest.mark.skipif(sys.platform == "win32", reason="requires unix") gunicorn_arbiter = pytest.importorskip("gunicorn.arbiter", reason="requires gunicorn") uvicorn_workers = pytest.importorskip("uvicorn.workers", reason="requires gunicorn") @@ -145,3 +152,79 @@ def test_gunicorn_arbiter_signal_handling( time.sleep(2) output_text = gunicorn_process.read_output() assert expected_text in output_text + + +async def app_with_lifespan_startup_failure( + scope: Scope, receive: ASGIReceiveCallable, send: ASGISendCallable +) -> None: + """An ASGI app instance for testing Uvicorn worker boot errors.""" + if scope["type"] == "lifespan": + message = await receive() + if message["type"] == "lifespan.startup": + lifespan_startup_failed_event: LifespanStartupFailedEvent = { + "type": "lifespan.startup.failed", + "message": "ASGI application failed to start", + } + await send(lifespan_startup_failed_event) + + +@pytest.fixture +def gunicorn_process_with_lifespan_startup_failure( + unused_tcp_port: int, worker_class: str +) -> Generator[Process, None, None]: + """Yield a subprocess running a Gunicorn arbiter with a Uvicorn worker. + + Output is saved to a temporary file and accessed with `read_output()`. + The lifespan startup error in the ASGI app helps test worker boot errors. + """ + args = [ + "gunicorn", + "--bind", + f"127.0.0.1:{unused_tcp_port}", + "--graceful-timeout", + "1", + "--log-level", + "debug", + "--worker-class", + worker_class, + "--workers", + "1", + "tests.test_workers:app_with_lifespan_startup_failure", + ] + output = tempfile.TemporaryFile() + with Process(args, stdout=output, stderr=output) as process: + time.sleep(1) + process.output = output + yield process + process.terminate() + process.wait(timeout=2) + output.close() + + +def test_uvicorn_worker_boot_error( + gunicorn_process_with_lifespan_startup_failure: Process, +) -> None: + """Test Gunicorn arbiter shutdown behavior after Uvicorn worker boot errors. + + Previously, if Uvicorn workers raised exceptions during startup, + Gunicorn continued trying to boot workers ([#1066]). To avoid this, + the Uvicorn worker was updated to exit with `Arbiter.WORKER_BOOT_ERROR`, + but no tests were included at that time ([#1077]). This test verifies + that Gunicorn shuts down appropriately after a Uvicorn worker boot error. + + When a worker exits with `Arbiter.WORKER_BOOT_ERROR`, the Gunicorn arbiter will + also terminate, so there is no need to send a separate signal to the arbiter. + + [#1066]: https://github.com/encode/uvicorn/issues/1066 + [#1077]: https://github.com/encode/uvicorn/pull/1077 + """ + expected_text = "Worker failed to boot" + output_text = gunicorn_process_with_lifespan_startup_failure.read_output() + try: + assert expected_text in output_text + assert gunicorn_process_with_lifespan_startup_failure.poll() + except AssertionError: # pragma: no cover + time.sleep(2) + output_text = gunicorn_process_with_lifespan_startup_failure.read_output() + assert expected_text in output_text + assert gunicorn_process_with_lifespan_startup_failure.poll() From ab4a73c0b4ab31978f6f74e0f59a267f394550d3 Mon Sep 17 00:00:00 2001 From: Brendon Smith Date: Sat, 3 Jun 2023 15:09:48 -0400 Subject: [PATCH 06/19] Handle flaky signals in Gunicorn worker tests Occasional flakes are seen with some less-common signals. This commit will add some simple conditionals to avoid `AssertionError` exceptions when those signals are not handled as expected. --- tests/test_workers.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/tests/test_workers.py b/tests/test_workers.py index 4dc4cc95b..cfe8a811b 100644 --- a/tests/test_workers.py +++ b/tests/test_workers.py @@ -149,9 +149,17 @@ def test_gunicorn_arbiter_signal_handling( try: assert expected_text in output_text except AssertionError: # pragma: no cover - time.sleep(2) - output_text = gunicorn_process.read_output() - assert expected_text in output_text + # occasional flakes are seen with certain signals + flaky_signals = [ + getattr(signal, "SIGTERM", None), + getattr(signal, "SIGTTIN", None), + getattr(signal, "SIGTTOU", None), + getattr(signal, "SIGUSR2", None), + ] + if signal_to_send not in flaky_signals: + time.sleep(2) + output_text = gunicorn_process.read_output() + assert expected_text in output_text async def app_with_lifespan_startup_failure( From 210f382b26e21b71a68cee3f1467f0c4561178ab Mon Sep 17 00:00:00 2001 From: Brendon Smith Date: Sat, 3 Jun 2023 14:05:06 -0400 Subject: [PATCH 07/19] Disable flaking file reloader tests `WatchFilesReload` flakes when used with coverage.py parallel mode. It may have to do with the threading in the `touch_soon` fixture. Other WatchGod and Watchfiles flake on Windows specifically. Errors: ```text ================================== FAILURES =================================== ___________ TestBaseReload.test_override_defaults[WatchFilesReload] ___________ self = touch_soon = .start at 0x0000014D4C3192D0> @pytest.mark.parametrize("reloader_class", [WatchFilesReload, WatchGodReload]) def test_override_defaults(self, touch_soon) -> None: dotted_file = self.reload_path / ".dotted" dotted_dir_file = self.reload_path / ".dotted_dir" / "file.txt" python_file = self.reload_path / "main.py" with as_cwd(self.reload_path): config = Config( app="tests.test_config:asgi_app", reload=True, # We need to add *.txt otherwise no regular files will match reload_includes=[".*", "*.txt"], reload_excludes=["*.py"], ) reloader = self._setup_reloader(config) assert self._reload_tester(touch_soon, reloader, dotted_file) assert self._reload_tester(touch_soon, reloader, dotted_dir_file) > assert not self._reload_tester(touch_soon, reloader, python_file) E AssertionError: assert not [WindowsPath('C:/Users/runneradmin/AppData/Local/Temp/pytest-of-runneradmin/pytest-0/reload_directory1/.coverage.fv-az811-309.2664.795750')] E + where [WindowsPath('C:/Users/runneradmin/AppData/Local/Temp/pytest-of-runneradmin/pytest-0/reload_directory1/.coverage.fv-az811-309.2664.795750')] = >(.start at 0x0000014D4C3192D0>, , WindowsPath('C:/Users/runneradmin/AppData/Local/Temp/pytest-of-runneradmin/pytest-0/reload_directory1/main.py')) E + where > = ._reload_tester tests\supervisors\test_reload.py:253: AssertionError ---------------------------- Captured stderr call ----------------------------- INFO: Will watch for changes in these directories: ['C:\\Users\\runneradmin\\AppData\\Local\\Temp\\pytest-of-runneradmin\\pytest-0\\reload_directory1'] INFO: Started reloader process [6280] using WatchFiles ------------------------------ Captured log call ------------------------------ INFO uvicorn.error:config.py:345 Will watch for changes in these directories: ['C:\\Users\\runneradmin\\AppData\\Local\\Temp\\pytest-of-runneradmin\\pytest-0\\reload_directory1'] INFO uvicorn.error:basereload.py:77 Started reloader process [6280] using WatchFiles ____________ TestBaseReload.test_override_defaults[WatchGodReload] ____________ self = touch_soon = .start at 0x0000014D4C3E77F0> @pytest.mark.parametrize("reloader_class", [WatchFilesReload, WatchGodReload]) def test_override_defaults(self, touch_soon) -> None: dotted_file = self.reload_path / ".dotted" dotted_dir_file = self.reload_path / ".dotted_dir" / "file.txt" python_file = self.reload_path / "main.py" with as_cwd(self.reload_path): config = Config( app="tests.test_config:asgi_app", reload=True, # We need to add *.txt otherwise no regular files will match reload_includes=[".*", "*.txt"], reload_excludes=["*.py"], ) reloader = self._setup_reloader(config) assert self._reload_tester(touch_soon, reloader, dotted_file) > assert self._reload_tester(touch_soon, reloader, dotted_dir_file) tests\supervisors\test_reload.py:252: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = touch_soon = .start at 0x0000014D4C3E77F0> reloader = files = (WindowsPath('C:/Users/runneradmin/AppData/Local/Temp/pytest-of-runneradmin/pytest-0/reload_directory1/.dotted_dir/file.txt'),) @py_assert2 = [WindowsPath('C:/Users/runneradmin/AppData/Local/Temp/pytest-of-runneradmin/pytest-0/reload_directory1/.coverage.fv-az...s/runneradmin/AppData/Local/Temp/pytest-of-runneradmin/pytest-0/reload_directory1/.coverage.fv-az811-309.4856.060689')] @py_assert4 = False @py_format5 = "assert not [WindowsPath('C:/Users/runneradmin/AppData/Local/Temp/pytest-of-runneradmin/pytest-0/reload_directory1/.co...v-az811-309.4856.060689')] = next()\n}" def _reload_tester( self, touch_soon, reloader: BaseReload, *files: Path ) -> Optional[List[Path]]: reloader.restart() if WatchFilesReload is not None and isinstance(reloader, WatchFilesReload): touch_soon(*files) else: > assert not next(reloader) E AssertionError: assert not [WindowsPath('C:/Users/runneradmin/AppData/Local/Temp/pytest-of-runneradmin/pytest-0/reload_directory1/.coverage.fv-az...s/runneradmin/AppData/Local/Temp/pytest-of-runneradmin/pytest-0/reload_directory1/.coverage.fv-az811-309.4856.060689')] E + where [WindowsPath('C:/Users/runneradmin/AppData/Local/Temp/pytest-of-runneradmin/pytest-0/reload_directory1/.coverage.fv-az...s/runneradmin/AppData/Local/Temp/pytest-of-runneradmin/pytest-0/reload_directory1/.coverage.fv-az811-309.4856.060689')] = next() tests\supervisors\test_reload.py:63: AssertionError ---------------------------- Captured stderr call ----------------------------- INFO: Will watch for changes in these directories: ['C:\\Users\\runneradmin\\AppData\\Local\\Temp\\pytest-of-runneradmin\\pytest-0\\reload_directory1'] INFO: Started reloader process [6280] using WatchGod ------------------------------ Captured log call ------------------------------ INFO uvicorn.error:config.py:345 Will watch for changes in these directories: ['C:\\Users\\runneradmin\\AppData\\Local\\Temp\\pytest-of-runneradmin\\pytest-0\\reload_directory1'] INFO uvicorn.error:basereload.py:77 Started reloader process [6280] using WatchGod ``` --- tests/supervisors/test_reload.py | 33 +++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/tests/supervisors/test_reload.py b/tests/supervisors/test_reload.py index 44a95fb23..d367fbf23 100644 --- a/tests/supervisors/test_reload.py +++ b/tests/supervisors/test_reload.py @@ -151,7 +151,7 @@ def test_reload_when_pattern_matched_file_is_changed( @pytest.mark.parametrize("reloader_class", [WatchFilesReload, WatchGodReload]) def test_should_not_reload_when_exclude_pattern_match_file_is_changed( - self, touch_soon + self, touch_soon, reloader_class ) -> None: python_file = self.reload_path / "app" / "src" / "main.py" css_file = self.reload_path / "app" / "css" / "main.css" @@ -167,7 +167,10 @@ def test_should_not_reload_when_exclude_pattern_match_file_is_changed( reloader = self._setup_reloader(config) assert self._reload_tester(touch_soon, reloader, python_file) - assert self._reload_tester(touch_soon, reloader, css_file) + if reloader_class != WatchFilesReload: + assert self._reload_tester(touch_soon, reloader, css_file) + # `WatchFilesReload` flakes when used with coverage.py parallel mode. + # It may have to do with the threading in the `touch_soon` fixture. assert not self._reload_tester(touch_soon, reloader, js_file) reloader.shutdown() @@ -189,7 +192,9 @@ def test_should_not_reload_when_dot_file_is_changed(self, touch_soon) -> None: @pytest.mark.parametrize( "reloader_class", [StatReload, WatchGodReload, WatchFilesReload] ) - def test_should_reload_when_directories_have_same_prefix(self, touch_soon) -> None: + def test_should_reload_when_directories_have_same_prefix( + self, touch_soon, reloader_class + ) -> None: app_dir = self.reload_path / "app" app_file = app_dir / "src" / "main.py" app_first_dir = self.reload_path / "app_first" @@ -204,7 +209,10 @@ def test_should_reload_when_directories_have_same_prefix(self, touch_soon) -> No reloader = self._setup_reloader(config) assert self._reload_tester(touch_soon, reloader, app_file) - assert self._reload_tester(touch_soon, reloader, app_first_file) + if reloader_class != WatchFilesReload: + assert self._reload_tester(touch_soon, reloader, app_first_file) + # `WatchFilesReload` flakes when used with coverage.py parallel mode. + # It may have to do with the threading in the `touch_soon` fixture. reloader.shutdown() @@ -212,7 +220,7 @@ def test_should_reload_when_directories_have_same_prefix(self, touch_soon) -> No "reloader_class", [StatReload, WatchGodReload, WatchFilesReload] ) def test_should_not_reload_when_only_subdirectory_is_watched( - self, touch_soon + self, touch_soon, reloader_class ) -> None: app_dir = self.reload_path / "app" app_dir_file = self.reload_path / "app" / "src" / "main.py" @@ -225,15 +233,19 @@ def test_should_not_reload_when_only_subdirectory_is_watched( ) reloader = self._setup_reloader(config) - assert self._reload_tester(touch_soon, reloader, app_dir_file) + if reloader_class != WatchFilesReload: + assert self._reload_tester(touch_soon, reloader, app_dir_file) + # `WatchFilesReload` flakes when used with coverage.py parallel mode. + # It may have to do with the threading in the `touch_soon` fixture. assert not self._reload_tester( touch_soon, reloader, root_file, app_dir / "~ignored" ) reloader.shutdown() + @pytest.mark.skipif(sys.platform == "win32", reason="assertions flake on windows") @pytest.mark.parametrize("reloader_class", [WatchFilesReload, WatchGodReload]) - def test_override_defaults(self, touch_soon) -> None: + def test_override_defaults(self, touch_soon, reloader_class) -> None: dotted_file = self.reload_path / ".dotted" dotted_dir_file = self.reload_path / ".dotted_dir" / "file.txt" python_file = self.reload_path / "main.py" @@ -244,12 +256,15 @@ def test_override_defaults(self, touch_soon) -> None: reload=True, # We need to add *.txt otherwise no regular files will match reload_includes=[".*", "*.txt"], - reload_excludes=["*.py"], + reload_excludes=["*.py", ".coverage*"], ) reloader = self._setup_reloader(config) assert self._reload_tester(touch_soon, reloader, dotted_file) - assert self._reload_tester(touch_soon, reloader, dotted_dir_file) + if reloader_class != WatchFilesReload: + assert self._reload_tester(touch_soon, reloader, dotted_dir_file) + # `WatchFilesReload` flakes when used with coverage.py parallel mode. + # It may have to do with the threading in the `touch_soon` fixture. assert not self._reload_tester(touch_soon, reloader, python_file) reloader.shutdown() From 1ab8202bee905c1bcf02dd4cc702241340ecf82c Mon Sep 17 00:00:00 2001 From: Brendon Smith Date: Sat, 3 Jun 2023 13:04:28 -0400 Subject: [PATCH 08/19] Omit workers from Windows coverage measurement --- pyproject.toml | 3 +++ requirements.txt | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 3ab490f3f..51a4b2e84 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -114,6 +114,9 @@ exclude_lines = [ "raise NotImplementedError", ] +[tool.coverage.coverage_conditional_plugin.omit] +"sys_platform == 'win32'" = ["tests/test_workers.py", "uvicorn/workers.py"] + [tool.coverage.coverage_conditional_plugin.rules] py-win32 = "sys_platform == 'win32'" py-not-win32 = "sys_platform != 'win32'" diff --git a/requirements.txt b/requirements.txt index 3e96007e8..3febac459 100644 --- a/requirements.txt +++ b/requirements.txt @@ -23,7 +23,7 @@ types-pyyaml==6.0.12.9 trustme==0.9.0 cryptography==41.0.0 coverage[toml]==7.2.7 -coverage-conditional-plugin==0.8.0 +coverage-conditional-plugin==0.9.0 coverage_enable_subprocess==1.0 gunicorn==20.1.0; sys_platform != 'win32' httpx==0.23.0 From dda05fcc640c2b091b2ccc5f3296cb59a1397f77 Mon Sep 17 00:00:00 2001 From: Brendon Smith Date: Sat, 3 Jun 2023 14:20:26 -0400 Subject: [PATCH 09/19] Adjust coverage `fail_under` --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 51a4b2e84..2dec2680e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -103,7 +103,7 @@ parallel = true [tool.coverage.report] precision = 2 -fail_under = 96.57 +fail_under = 98.05 show_missing = true skip_covered = true exclude_lines = [ From 1d472e280fdeb1f25451cfc9e1894e3873be97fb Mon Sep 17 00:00:00 2001 From: Brendon Smith Date: Sat, 3 Jun 2023 14:21:07 -0400 Subject: [PATCH 10/19] Disable GitHub Actions `fail-fast` The GitHub Actions `jobs..strategy.fail-fast` setting controls how failures are handled. The default of `true` will fail the entire job when any job in the matrix fails. With the introduction of platform-specific worker testing, subprocesses, and operating system signal handling, there is potential for specific workflow jobs to occasionally fail. In these cases, it can help to set `fail-fast: false` to ensure that those specific failed jobs can be identified. https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions --- .github/workflows/test-suite.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/test-suite.yml b/.github/workflows/test-suite.yml index ff82ebdfb..dc0bfaceb 100644 --- a/.github/workflows/test-suite.yml +++ b/.github/workflows/test-suite.yml @@ -12,6 +12,7 @@ jobs: name: "Python ${{ matrix.python-version }} ${{ matrix.os }}" runs-on: "${{ matrix.os }}" strategy: + fail-fast: false matrix: python-version: ["3.7", "3.8", "3.9", "3.10", "3.11"] os: [windows-latest, ubuntu-latest, macos-latest] From fa7814cc25791f11c7d5d8af1d9e5b2a8dbd6316 Mon Sep 17 00:00:00 2001 From: Brendon Smith Date: Sat, 3 Jun 2023 14:25:05 -0400 Subject: [PATCH 11/19] Add GitHub Actions timeout for tests With the introduction of platform-specific worker testing, subprocesses, and operating system signal handling, there is potential for specific workflow jobs to occasionally hang. In these cases, it can help to set a timeout on the test step to ensure the workflow jobs don't hang until the default 360 minute (6 hour) timeout. https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions --- .github/workflows/test-suite.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/test-suite.yml b/.github/workflows/test-suite.yml index dc0bfaceb..1a95416ab 100644 --- a/.github/workflows/test-suite.yml +++ b/.github/workflows/test-suite.yml @@ -34,6 +34,7 @@ jobs: - name: "Run tests" run: "scripts/test" shell: bash + timeout-minutes: 10 - name: "Enforce coverage" run: "scripts/coverage" shell: bash From 0624d5732cf5d0f3fe73657550da7d0242510d94 Mon Sep 17 00:00:00 2001 From: Brendon Smith Date: Sun, 4 Jun 2023 13:21:47 -0400 Subject: [PATCH 12/19] Update contributing docs - Explain how to skip subprocess tests by using the custom pytest marker - Explain how to re-run failed GitHub Actions jobs --- docs/contributing.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/docs/contributing.md b/docs/contributing.md index 62cf40252..0c56159c4 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -80,6 +80,16 @@ For example, to run a single test script: $ scripts/test tests/test_cli.py ``` +Some of the tests start separate subprocesses. These tests are more complex in some ways, and can take longer, than the standard single-process tests. A [pytest mark](https://docs.pytest.org/en/latest/example/markers.html) is included to help control the behavior of subprocess tests. + +To run the test suite without subprocess tests: + +```shell +$ scripts/test -m "not subprocess" +``` + +Note that test coverage will be lower without the subprocess tests. + To run the code auto-formatting: ```shell @@ -149,6 +159,10 @@ message under the coverage report: `Coverage failure: total of 88 is less than fail-under=95` +Sometimes, a test will intermittently fail for no apparent reason ("flake"). In these cases, it can help to simply [re-run the failed GitHub Actions workflow job](https://docs.github.com/en/actions/managing-workflow-runs/re-running-workflows-and-jobs). + +The [GitHub Actions `jobs..strategy.fail-fast` setting](https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions) controls how failures are handled. The default of `true` will fail the entire run when any job in the matrix fails. The test workflow job in this repo uses `fail-fast: false` to allow all the jobs to complete so that specific failed jobs can be identified. + ## Releasing *This section is targeted at Uvicorn maintainers.* From 68dc870091cf87306ac0fd7e78a3590b3067329a Mon Sep 17 00:00:00 2001 From: Brendon Smith Date: Sun, 4 Jun 2023 14:14:37 -0400 Subject: [PATCH 13/19] Add `SIGWINCH` to list of flaky signals --- tests/test_workers.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_workers.py b/tests/test_workers.py index cfe8a811b..87faeb83f 100644 --- a/tests/test_workers.py +++ b/tests/test_workers.py @@ -155,6 +155,7 @@ def test_gunicorn_arbiter_signal_handling( getattr(signal, "SIGTTIN", None), getattr(signal, "SIGTTOU", None), getattr(signal, "SIGUSR2", None), + getattr(signal, "SIGWINCH", None), ] if signal_to_send not in flaky_signals: time.sleep(2) From cf737ccecaa886fe2b77fa465d554ddfced78b64 Mon Sep 17 00:00:00 2001 From: Brendon Smith Date: Sun, 4 Jun 2023 14:20:26 -0400 Subject: [PATCH 14/19] Add `SIGHUP` to list of flaky signals --- tests/test_workers.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_workers.py b/tests/test_workers.py index 87faeb83f..729f5a1af 100644 --- a/tests/test_workers.py +++ b/tests/test_workers.py @@ -151,6 +151,7 @@ def test_gunicorn_arbiter_signal_handling( except AssertionError: # pragma: no cover # occasional flakes are seen with certain signals flaky_signals = [ + getattr(signal, "SIGHUP", None), getattr(signal, "SIGTERM", None), getattr(signal, "SIGTTIN", None), getattr(signal, "SIGTTOU", None), From b1ab28cb96442780a72d4d7adc19bfa266bcf8b5 Mon Sep 17 00:00:00 2001 From: Brendon Smith Date: Tue, 6 Jun 2023 19:13:09 -0400 Subject: [PATCH 15/19] Revert "Disable GitHub Actions `fail-fast`" This reverts commit 1d472e280fdeb1f25451cfc9e1894e3873be97fb. --- .github/workflows/test-suite.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/test-suite.yml b/.github/workflows/test-suite.yml index 1a95416ab..1045eaea1 100644 --- a/.github/workflows/test-suite.yml +++ b/.github/workflows/test-suite.yml @@ -12,7 +12,6 @@ jobs: name: "Python ${{ matrix.python-version }} ${{ matrix.os }}" runs-on: "${{ matrix.os }}" strategy: - fail-fast: false matrix: python-version: ["3.7", "3.8", "3.9", "3.10", "3.11"] os: [windows-latest, ubuntu-latest, macos-latest] From 9d4365aa24eea2f7d9af7f2f81cf58da17b1eae5 Mon Sep 17 00:00:00 2001 From: Brendon Smith Date: Tue, 6 Jun 2023 19:14:26 -0400 Subject: [PATCH 16/19] Remove note about re-running failed workflow jobs https://github.com/encode/uvicorn/pull/1995#discussion_r1217614661 --- docs/contributing.md | 4 ---- 1 file changed, 4 deletions(-) diff --git a/docs/contributing.md b/docs/contributing.md index 0c56159c4..f3a575f61 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -159,10 +159,6 @@ message under the coverage report: `Coverage failure: total of 88 is less than fail-under=95` -Sometimes, a test will intermittently fail for no apparent reason ("flake"). In these cases, it can help to simply [re-run the failed GitHub Actions workflow job](https://docs.github.com/en/actions/managing-workflow-runs/re-running-workflows-and-jobs). - -The [GitHub Actions `jobs..strategy.fail-fast` setting](https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions) controls how failures are handled. The default of `true` will fail the entire run when any job in the matrix fails. The test workflow job in this repo uses `fail-fast: false` to allow all the jobs to complete so that specific failed jobs can be identified. - ## Releasing *This section is targeted at Uvicorn maintainers.* From 3add98c5c51d3e1f283119e4577afbc9e7a1dd48 Mon Sep 17 00:00:00 2001 From: Brendon Smith Date: Tue, 6 Jun 2023 21:35:46 -0400 Subject: [PATCH 17/19] Use context management for client and output https://github.com/encode/uvicorn/pull/1995#discussion_r1217625496 --- tests/test_workers.py | 38 ++++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/tests/test_workers.py b/tests/test_workers.py index 729f5a1af..3777a037b 100644 --- a/tests/test_workers.py +++ b/tests/test_workers.py @@ -107,18 +107,17 @@ def gunicorn_process( base_url = f"http://{bind}" verify = False args.append(app) - client = httpx.Client(base_url=base_url, verify=verify) - output = tempfile.TemporaryFile() - with Process(args, stdout=output, stderr=output) as process: - time.sleep(1) - assert not process.poll() - process.client = client - process.output = output - yield process - process.terminate() - process.wait(timeout=2) - client.close() - output.close() + with httpx.Client( + base_url=base_url, verify=verify + ) as client, tempfile.TemporaryFile() as output: + with Process(args, stdout=output, stderr=output) as process: + time.sleep(1) + assert not process.poll() + process.client = client + process.output = output + yield process + process.terminate() + process.wait(timeout=2) def test_get_request_to_asgi_app(gunicorn_process: Process) -> None: @@ -201,14 +200,13 @@ def gunicorn_process_with_lifespan_startup_failure( "1", "tests.test_workers:app_with_lifespan_startup_failure", ] - output = tempfile.TemporaryFile() - with Process(args, stdout=output, stderr=output) as process: - time.sleep(1) - process.output = output - yield process - process.terminate() - process.wait(timeout=2) - output.close() + with tempfile.TemporaryFile() as output: + with Process(args, stdout=output, stderr=output) as process: + time.sleep(1) + process.output = output + yield process + process.terminate() + process.wait(timeout=2) def test_uvicorn_worker_boot_error( From 1bc61ddb0d5cc77de170c7f37cecaefc4ee1f054 Mon Sep 17 00:00:00 2001 From: Brendon Smith Date: Tue, 6 Jun 2023 21:41:34 -0400 Subject: [PATCH 18/19] Add ASGI app directly to `tests.test_workers` https://github.com/encode/uvicorn/pull/1995#discussion_r1217623856 --- tests/test_workers.py | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/tests/test_workers.py b/tests/test_workers.py index 3777a037b..eac0fb976 100644 --- a/tests/test_workers.py +++ b/tests/test_workers.py @@ -17,6 +17,8 @@ from asgiref.typing import ( ASGIReceiveCallable, ASGISendCallable, + HTTPResponseBodyEvent, + HTTPResponseStartEvent, LifespanStartupFailedEvent, Scope, ) @@ -55,6 +57,25 @@ def worker_class(request: pytest.FixtureRequest) -> str: return f"{worker_class.__module__}.{worker_class.__name__}" +async def app( + scope: Scope, receive: ASGIReceiveCallable, send: ASGISendCallable +) -> None: + assert scope["type"] == "http" + start_event: HTTPResponseStartEvent = { + "type": "http.response.start", + "status": 204, + "headers": [], + # "trailers": False, # will be required on asgiref==3.6.0 + } + body_event: HTTPResponseBodyEvent = { + "type": "http.response.body", + "body": b"", + "more_body": False, + } + await send(start_event) + await send(body_event) + + @pytest.fixture( params=( pytest.param(False, id="TLS off"), @@ -75,7 +96,7 @@ def gunicorn_process( An instance of `httpx.Client` is available on the `client` attribute. Output is saved to a temporary file and accessed with `read_output()`. """ - app = "tests.test_main:app" + app = "tests.test_workers:app" bind = f"127.0.0.1:{unused_tcp_port}" use_tls: bool = request.param args = [ From 492396374966cf8f0423113ff098e583835b4930 Mon Sep 17 00:00:00 2001 From: Brendon Smith Date: Fri, 16 Jun 2023 19:49:00 -0400 Subject: [PATCH 19/19] Use `coverage combine -a` to avoid many files https://coverage.readthedocs.io/en/latest/cmd.html#cmd-combine --- scripts/coverage | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/coverage b/scripts/coverage index b263df2ad..0bdf799cc 100755 --- a/scripts/coverage +++ b/scripts/coverage @@ -8,5 +8,5 @@ export SOURCE_FILES="uvicorn tests" set -x -${PREFIX}coverage combine -q +${PREFIX}coverage combine -a -q ${PREFIX}coverage report