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.

Sign up for GitHub

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

Jump to bottom

Test Gunicorn Worker #1834

Closed
Kludex opened this issue Jan 5, 2023 · 8 comments · Fixed by #2302
Closed

Test Gunicorn Worker #1834

Kludex opened this issue Jan 5, 2023 · 8 comments · Fixed by #2302
Labels
good first issue testing workers/gunicorn

Comments

@Kludex
Copy link
Member

Kludex commented Jan 5, 2023 •
edited by polar-sh bot
Loading

The idea here is to create a setup to test the gunicorn worker classes that we have.

I don't know how is the best way to achieve this, as gunicorn test suite didn't give me much insight on how we can test this integration without unittest.mock. Suggestions are welcome.

If you are interested on working on this, go ahead. Some clarifications:

  1. Avoid integration kind of setup. I want to keep it simple.
  2. Avoid unittest.mock - if inevitable, explain the reason.
  3. Add test coverage up to 100% on the workers.py.

Notes

When doing this, please change those lines:

uvicorn/setup.cfg

Lines 39 to 43 in eec7d22

[coverage:run]
omit = venv/*
include = uvicorn/*, tests/*
plugins =
coverage_conditional_plugin

To something more like: https://github.com/encode/starlette/blob/048643adc21e75b668567fc6bcdd3650b89044ea/setup.cfg#L41-L42

Important

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@br3ndonland
Copy link
Contributor

I can take this on. I agree we should keep it simple and focus on unit testing.

One way I've approached unit testing without mocks is by starting a Gunicorn server with the --print-config option. This does actually start the Gunicorn server, but Gunicorn will just print out the config and then shut down. You can then parse the output and assert that Gunicorn is using the appropriate settings.

I'll think about some other complementary ways to approach testing of the worker.

@br3ndonland
Copy link
Contributor

Just dropping a note here to say that I'm working on this and making some slow progress.

I started off with the --print-config strategy I mentioned in #1834 (comment), but it wasn't very useful. We're not doing much modification of the Gunicorn config here, other than just setting the worker class, and I've also found that this strategy doesn't really test functionality of the server at all. For example, I can run a command like gunicorn --print-config --worker-class "this.worker.DoesNotExist" "example_package.example_module:app", and Gunicorn will start up, print out the config stating that the worker class is this.worker.DoesNotExist, and exit successfully, even though that config would clearly not work.

Instead, I'm working on some testing capabilities that will:

Hope to have a PR for review soon.

@jawang94

This comment was marked as spam.

Sign in to view
@br3ndonland
Copy link
Contributor

br3ndonland commented Apr 15, 2023 •
edited
Loading

I'm working through a few tedious limitations right now. One of them relates to conditional test coverage.

As correctly suggested in the description, we need to change our coverage.py config from include = uvicorn/*, tests/* to source_pkgs = uvicorn, tests. This is because the current include syntax actually excludes uvicorn.workers from test coverage. Switching to source_pkgs will include all the modules.

The limitation is that we can't cover uvicorn.workers or the associated tests on Windows. Gunicorn doesn't test on Windows at all because it depends on various Unix utilities like fcntl.

How do we tell coverage.py, "please exclude uvicorn.workers and tests.test_workers from coverage on Windows"? Seems like there should be some obvious way to do this, but I'm not sure.

Coverage.py has an --omit flag, but I don't think this flag can handle platform-specific conditionals like we need here.

I tried setting pragmas at the top-level of the modules, but coverage-conditional-plugin doesn't support this (wemake-services/coverage-conditional-plugin#2).

Am I missing something? Or will I need to do a PR that resolves wemake-services/coverage-conditional-plugin#2 in order to unblock this?

I guess the simplest option would be to add a # pragma: py-win32 comment to every statement that's not covered. It would add a lot of comments.

@br3ndonland br3ndonland mentioned this issue Apr 22, 2023
Merged
@br3ndonland
Copy link
Contributor

Am I missing something? Or will I need to do a PR that resolves wemake-services/coverage-conditional-plugin#2 in order to unblock this?

PR submitted (wemake-services/coverage-conditional-plugin#221). This feature will enable entire modules to be conditionally omitted from test coverage measurement.

@br3ndonland
Copy link
Contributor

Coding for this is done, and we now have a nice way to omit the worker code from test coverage measurement on Windows, thanks to coverage-conditional-plugin 0.9.0.

PR coming up shortly.

@Kludex
Copy link
Member Author

Kludex commented Jun 4, 2023

Thanks 🙏

@br3ndonland br3ndonland mentioned this issue Jun 4, 2023
Closed
3 tasks
@Kludex
Copy link
Member Author

Kludex commented Mar 30, 2024

I'll deprecate the workers within Uvicorn.

@Kludex Kludex mentioned this issue Apr 13, 2024
Merged
@br3ndonland br3ndonland mentioned this issue Dec 22, 2024
Merged
br3ndonland added a commit to br3ndonland/inboard that referenced this issue Dec 22, 2024
@br3ndonland
Add and test Gunicorn workers (#116)
35d8d86 
This project supports the Gunicorn web server. Gunicorn's server design
includes a primary "arbiter" process that spawns "worker" processes.
Workers are child processes, each with their own running server. Workers
are implemented as Python classes. Custom workers can be supplied.

This project also supports the Uvicorn web server. In the past, Uvicorn
supplied workers for use with Gunicorn. The Uvicorn workers were not
tested. The `uvicorn.workers` module was completely omitted from
coverage measurement due to use of the coverage.py `include` setting
to specify source files.

Efforts were made to test the Uvicorn workers
(encode/uvicorn#1834,
encode/uvicorn#1995),
but the workers were arbitrarily deprecated and moved to
someone's personal project (encode/uvicorn#2302),
instead of an Encode-managed project as would have been expected
(encode/uvicorn#517 (comment))

Rather than introducing a production dependency on a separate Uvicorn
workers package that is not managed by Encode, this commit will add the
Gunicorn workers directly to this project.

This commit will add the code from `uvicorn.workers` to a new module
`inboard/gunicorn_workers.py`. The code will be preserved as it was
prior to deprecation, with a copy of the Uvicorn license and necessary
updates for compliance with the code quality settings in this project:

- Ruff
  [UP008](https://docs.astral.sh/ruff/rules/super-call-with-parameters/)
- Ruff
  [UP035](https://docs.astral.sh/ruff/rules/deprecated-import/)
- mypy `[attr-defined]` - Module "uvicorn.main" does not explicitly
  export attribute "Server"
- mypy `[import-untyped]` - `gunicorn.arbiter`
- mypy `[import-untyped]` - `gunicorn.workers.base`
- mypy `[misc]` - Class cannot subclass "Worker" (has type "Any")
- mypy `[type-arg]` - Missing type parameters for generic type "dict"
  (on `config_kwargs`)

This commit will also add tests of 100% of the Gunicorn worker code to a
new module `tests/test_gunicorn_workers.py`.

A test fixture starts a subprocess running Gunicorn with a Uvicorn worker
and an ASGI app. The subprocess includes an instance of `httpx.Client`
for HTTP requests to the Uvicorn worker's ASGI app, and saves its output
to a temporary file for assertions on `stdout`/`stderr`. Tests can send
operating system signals to the process.

The coverage.py configuration will be updated for subprocess test
coverage measurement. Changes to coverage measurement include:

- Enable the required parallel mode (note that it is important to ensure
  the `.gitignore` ignores files named `.coverage.*` because many coverage
  files are generated when subprocesses are measured in parallel mode)
- Set the required `COVERAGE_PROCESS_START` environment variable
- Add the `coverage_enable_subprocess` package to invoke
  `coverage.process_startup`
- Combine coverage reports before reporting coverage
- Add instructions to `contributing.md` about how to omit subprocess
  tests

Related:

https://github.com/encode/uvicorn/blob/4fd507718eb0313e2de66123e6737b054088f722/LICENSE.md
https://github.com/encode/uvicorn/blob/4fd507718eb0313e2de66123e6737b054088f722/uvicorn/workers.py
encode/uvicorn#517 (comment)
encode/uvicorn#1834
encode/uvicorn#1995
encode/uvicorn#2302

https://coverage.readthedocs.io/en/latest/subprocess.html
https://docs.gunicorn.org/en/latest/design.html
https://docs.gunicorn.org/en/latest/signals.html
https://www.uvicorn.org/deployment/#gunicorn
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue testing workers/gunicorn
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Deprecate the uvicorn.workers module encode/uvicorn
Test workers
3 participants
@Kludex @br3ndonland @jawang94