Improve testing of server configuration #11
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3906b3d cd7604c - Add full process calls: verify that the server functions (`uvicorn.run`, `subprocess.run` with Gunicorn) are called with expected arguments. - Add mock for `subprocess.run()`: similar to Uvicorn update in 3906b3d. - Provide Gunicorn with correct `worker_tmp_dir`: The Gunicorn server was previously exiting without need for a mock. However, this was actually because it couldn't find the hard-coded `worker_tmp_dir` (`/dev/shm`), as can be seen when invoking pytest with `pytest -vv --capture=sys`. The solution is to override the hard-coded `worker_tmp_dir` with `GUNICORN_CMD_ARGS="--worker-tmp-dir $DIR"`. Although the full Gunicorn module is not mocked here, it's a good practice to properly configure the Gunicorn server. - Note that the need for a manual keyboard interrupt (`^C`) as described in 3906b3d still remains when running `start.start_server` without mocked `uvicorn.run` and `subprocess.run` functions.
tiangolo/uvicorn-gunicorn-docker#5 tiangolo/uvicorn-gunicorn-starlette-docker#4 tiangolo/uvicorn-gunicorn-fastapi-docker#6 The "auto-tuning" advertised in tiangolo/uvicorn-gunicorn-docker is basically a few lines of the `gunicorn_conf.py` that determine the number of Gunicorn workers to run. It would be helpful to write some unit test cases for this feature, but without being in a separate unit, it is difficult to unit test in isolation. This commit will refactor the performance auto-tuning into a function, `gunicorn_conf.calculate_workers`, and will add unit test cases to verify the resulting number of worker processes.
This commit will update the README with clearer info on auto-tuning of the number of Gunicorn worker processes.
#11 This commit will break `test_start.py test_gunicorn_conf_workers_custom` (the unit test for custom Gunicorn worker process calculation), into multiple units, based on PR feedback from @sourcery-ai. Thanks robots!
#11 02b249e https://github.com/br3ndonland/inboard/actions/runs/263199007 All unit tests were passing locally, but the unit in `test_start.py` `TestConfigureGunicorn` that tested custom Gunicorn worker calculation failed. A minor update was needed to match `gunicorn_conf.py`.
Codecov Report
@@ Coverage Diff @@
## develop #11 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 8 8
Lines 223 220 -3
=========================================
- Hits 223 220 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
#11 This commit will reduce test_start_server_uvicorn_gunicorn_custom_config by decreasing code duplication, based on feedback from @sourcery-ai. Thanks robots!
Sourcery Code Quality Report❌ Merging this PR will decrease code quality in the affected files by 0.13%.
Here are some functions in these files that still need a tune-up:
Legend and ExplanationThe emojis denote the absolute quality of the code:
The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request. Please see our documentation here for details on how these metrics are calculated. We are actively working on this report - lots more documentation and extra metrics to come! Let us know what you think of it by mentioning @sourcery-ai in a comment. |
|
@sourcery-ai Thanks for your help on this PR! The code quality report helped me tune up my code before merging. Is the code quality report updated each time I push a new commit to a PR? If so, it would help to have the PR comment show up as edited, like Codecov does. |
br3ndonland
added a commit
that referenced
this pull request
Sep 21, 2020
#11 The README explains, "If either `MAX_WORKERS` or `WEB_CONCURRENCY` are set to 1, the total number of workers will be 1, overriding the default minimum of 2." This was true for `MAX_WORKERS`, but `WEB_CONCURRENCY` was still set to minimum 2. This commit will allow `WEB_CONCURRENCY` (number of Gunicorn workers) to have a minimum of 1.
br3ndonland
added a commit
that referenced
this pull request
Sep 21, 2020
br3ndonland
added a commit
that referenced
this pull request
Oct 31, 2020
#11 Mock `subprocess.run` and `uvicorn.run` from `inboard.start` instead of from the installed package.
br3ndonland
added a commit
that referenced
this pull request
Apr 18, 2021
#11 These tests weren't really testing the start script, so they shouldn't be in test_start.py. Moving the tests to test_gunicorn_conf.py also helps emulate the directory structure of the source code, so tests are more intuitively found.
br3ndonland
added a commit
that referenced
this pull request
Apr 18, 2021
#11 02b249e fd60470 https://docs.python.org/3/reference/expressions.html#boolean-operations The `gunicorn_conf.calculate_workers()` method returns a desired number of Gunicorn workers, based on the arguments passed in. The arguments are read from environment variables. The calculation is deceptively complex, and a variety of edge cases emerged over time. Commit fd60470 fixed the web concurrency calculation, but not the max workers calculation. There should have been an `else use_max_workers if max_workers_str` condition. The tests were correspondingly complex and convoluted, testing various interrelated conditions, and becoming less and less readable. This commit will refactor the Gunicorn worker calculation and tests. The result is more readable (and correct) code. Refactor the `gunicorn_conf.calculate_workers()` method arguments: - Remove `_str` from function arguments: arguments are type-annotated, and it is redundant to add `_str` to arguments annotated as strings. - Rename the `web_concurrency_str` function argument to `total_workers`: the "web concurrency" name is a legacy Gunicorn environment variable, and doesn't really convey what the setting does. "Web concurrency" is a total number of workers, so just call it `total_workers`. - Make the `workers_per_core` argument a keyword argument (kwarg): the corresponding environment variable `WORKERS_PER_CORE` has a default, so set the same default on the kwarg. - Move the cpu cores calculation into the function body: not necessary to set a number of cores, so just use `multiprocessing.cpu_count()`. - Keep same order of arguments and argument type-annotations to avoid breaking existing functionality and to ensure backwards-compatibility. Refactor the `gunicorn_conf.calculate_workers()` method logic, by using `if` expressions to more clearly evaluate each condition, then returning the correct value by using `or` to evaluate Boolean conditions in order. Improve test parametrization to more thoroughly test use cases, and refactor tests so that each one tests an isolated use case: - Check defaults - Set maximum number of workers - Set total number of workers ("web concurrency") - Set desired number of workers per core - Set both maximum number of workers and total number of workers
br3ndonland
added a commit
that referenced
this pull request
Apr 18, 2021
#11 https://docs.gunicorn.org/en/latest/settings.html#print-config https://docs.pytest.org/en/latest/how-to/capture-stdout-stderr.html The Gunicorn configuration file was technically covered by unit tests, but the individual settings were not tested. This commit will add tests of the Gunicorn settings. The tests use Gunicorn's `--print-config` option to load and output the configuration. The pytest fixture `capfd` is used to read the Gunicorn output.
This was referenced Apr 18, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Unit test coverage is at 100%, but testing doesn't stop there. It is still important to test that the Uvicorn and Gunicorn servers are being properly configured with environment variables and configuration files. This PR will provide improvements to the unit tests for the Uvicorn and Gunicorn servers, while maintaining test coverage at 100%.
Changes
Gunicorn
uvicorn.runfor Uvicorn,subprocess.runfor Gunicorn) are called with expected arguments.subprocess.run(): similar to Uvicorn update in 3906b3d.worker_tmp_dir: The Gunicorn server was previously exiting without need for a mock. However, this was actually because it couldn't find the hard-codedworker_tmp_dir(/dev/shm), as can be seen when invoking pytest withpytest -vv --capture=sys. The solution is to override the hard-codedworker_tmp_dirwithGUNICORN_CMD_ARGS="--worker-tmp-dir $DIR". Although the full Gunicorn module is not mocked here, it's a good practice to properly configure the Gunicorn server.gunicorn_conf.pythat determine the number of Gunicorn workers to run. It would be helpful to write some unit test cases for this feature, but without being in a separate unit, it is difficult to unit test in isolation.gunicorn_conf.calculate_workers, and will add unit test cases totest_start.pyto verify the resulting number of worker processes.General
Path()is used forshutilarguments instead of stringtmp_path_factorygunicorn_conf.pyto temporary directorytest_start.pywith new Gunicorn pathsPROCESS_MANAGERenvironment variable ininboard/app/base/main.py, but no default was set.PROCESS_MANAGERis not set.Related
3906b3d
cd7604c
#8
https://github.com/tiangolo/uvicorn-gunicorn-docker/releases/tag/0.3.0
tiangolo/uvicorn-gunicorn-docker#5
tiangolo/uvicorn-gunicorn-starlette-docker#4
tiangolo/uvicorn-gunicorn-fastapi-docker#6