Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test workers #1995

Closed
wants to merge 20 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/test-suite.yml
Expand Up @@ -33,6 +33,7 @@ jobs:
- name: "Run tests"
run: "scripts/test"
shell: bash
timeout-minutes: 10
- name: "Enforce coverage"
run: "scripts/coverage"
shell: bash
2 changes: 1 addition & 1 deletion .gitignore
@@ -1,5 +1,5 @@
.cache
.coverage
.coverage*
.mypy_cache/
__pycache__/
uvicorn.egg-info/
Expand Down
10 changes: 10 additions & 0 deletions docs/contributing.md
Expand Up @@ -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"
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to not test by default, and do the opposite: scripts/test -m "subprocess"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can. The way to disable subprocess tests by default is to add the -m "not subprocess" to addopts in pyproject.toml.

The downsides to this approach are:

  • When we do want to run the subprocess tests, test commands and invocation become more complicated. We would have to add another marker called unmarked and add it to all the other tests (Apply a default marker to all unmarked tests pytest-dev/pytest#8573), then do -m "subprocess or unmarked" to run all the tests. I tried it but pytest is still skipping a lot of tests and I'm having trouble with it.
  • Coverage reports will fail by default, which may confuse people.
  • It doesn't match up as well with the pytest docs on custom markers (the docs recommend the "not subprocess" approach for skipping tests with custom markers).

I prefer the "not subprocess" approach overall.

Copy link
Sponsor Member

@Kludex Kludex Jun 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inclined about removing the workers.py module from uvicorn.

```

Note that test coverage will be lower without the subprocess tests.

To run the code auto-formatting:

```shell
Expand Down
11 changes: 9 additions & 2 deletions pyproject.toml
Expand Up @@ -89,14 +89,17 @@ 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"]
plugins = ["coverage_conditional_plugin"]
omit = [
"uvicorn/workers.py",
"uvicorn/__main__.py",
]
parallel = true

[tool.coverage.report]
precision = 2
Expand All @@ -112,7 +115,11 @@ exclude_lines = [
]

[tool.coverage.coverage_conditional_plugin.omit]
"sys_platform == 'win32'" = ["uvicorn/loops/uvloop.py"]
"sys_platform == 'win32'" = [
"tests/test_workers.py",
"uvicorn/loops/uvloop.py",
"uvicorn/workers.py",
]

[tool.coverage.coverage_conditional_plugin.rules]
py-win32 = "sys_platform == 'win32'"
Expand Down
2 changes: 2 additions & 0 deletions requirements.txt
Expand Up @@ -21,6 +21,8 @@ trustme==0.9.0
cryptography==41.0.0
coverage==7.2.7
coverage-conditional-plugin==0.9.0
coverage_enable_subprocess==1.0
gunicorn==20.1.0; sys_platform != 'win32'
httpx==0.23.0
watchgod==0.8.2

Expand Down
1 change: 1 addition & 0 deletions scripts/coverage
Expand Up @@ -8,4 +8,5 @@ export SOURCE_FILES="uvicorn tests"

set -x

${PREFIX}coverage combine -a -q
${PREFIX}coverage report
4 changes: 4 additions & 0 deletions scripts/test
Expand Up @@ -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
Expand Down
33 changes: 24 additions & 9 deletions tests/supervisors/test_reload.py
Expand Up @@ -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"
Expand All @@ -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()
Expand All @@ -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"
Expand All @@ -204,15 +209,18 @@ 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()

@pytest.mark.parametrize(
"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"
Expand All @@ -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"
Expand All @@ -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()
Expand Down