Refactor configuration files and tests #31
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
#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.
#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
4914a4b - Remove unnecessary setup section: just read environment variables when instantiating the settings objects that are directly read by Gunicorn. The same result is obtained with half the lines of code (29 -> 14). - Sort settings alphabetically
#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.
#30 This commit will add some minor improvements to the refactor of the Gunicorn and Uvicorn setup in start.py (added in #30). - Pass `app_module` into `start.set_gunicorn_options()`, rather than appending later when running the server - Read environment variables earlier in `start.set_uvicorn_options()` to provide consistent syntax when returning the dictionary at the end: this strategy seems opposed to the one taken in `gunicorn_conf.py` (read environment variables when they are used, rather than before), but the end result is similar (consistent syntax in the settings).
#3 The `configure_logging()` method was previously located in `start.py`, but the logging configuration dictionary was in `logging_conf.py`. This organization was not ideal for readability and separation of concerns. It also created logistical complications, such as requiring the Gunicorn configuration file `gunicorn_conf.py` to import from start.py, so that it could configure logging for Gunicorn. The `start.py` script should be self-contained, and should import other modules and objects from the package, not the other way around. This commit will move `configure_logging()` to `logging_conf.py`, and move the tests to a corresponding module, `test_logging_conf.py`. This matches up nicely with the latest updates to the `gunicorn_conf.py`, by having a setup method at the top of the module, and the settings below.
The loader attribute on a module spec is technically optional, so mypy raises a `[union-attr]` error: Item "_Loader" of "Optional[_Loader]" has no attribute "exec_module" To avoid referencing the potentially undefined `_Loader.exec_module` attribute, this commit will update the syntax to get the attribute with `getattr(spec.loader, "exec_module")` prior to running `exec_module()`. An `AttributeError` will be raised if the attribute does not exist.
#3 The `configure_logging()` method did multiple things: find and import a logging configuration module, load the logging configuration dictionary, and apply the dictionary configuration to the Python logger. This commit will refactor `configure_logging()` into two methods. 1. `find_and_load_logging_conf()` will return a dictionary configuration 2. `configure_logging()` will apply the dictionary configuration The return value of `configure_logging()` will remain the same, so there will not be any changes to the programming API. Also, now that the `configure_logging()` method is in the same module as the logging configuration dictionary, that dictionary will be used by default, instead of finding the module separately.
Mocker objects are sometimes prefixed with `mock_` to indicate that they are separate objects. This is largely unnecessary, so it will be removed from the tests of the logging configuration.
#3 https://docs.pytest.org/en/latest/how-to/capture-stdout-stderr.html https://docs.pytest.org/en/latest/how-to/logging.html In addition to testing the methods that load the logging configuration, it can also be useful to test that logger messages have the expected format. This commit will add tests that output logger messages in various formats and assert that each output matches the expected format. As in the tests of the Gunicorn server output, these tests will use the pytest `capfd` fixture. It is also possible to use the `caplog` fixture, but I have not had as much success with it.
https://docs.pytest.org/en/latest/contents.html Love the new docs! 🎉
- `MAX_WORKERS` - `WEB_CONCURRENCY` - `WORKERS_PER_CORE`
Sourcery Code Quality Report✅ Merging this PR will increase code quality in the affected files by 0.81%.
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. |
Codecov Report
@@ Coverage Diff @@
## develop #31 +/- ##
===========================================
Coverage ? 100.00%
===========================================
Files ? 9
Lines ? 256
Branches ? 0
===========================================
Hits ? 256
Misses ? 0
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
1 task
br3ndonland
added a commit
that referenced
this pull request
May 2, 2021
#31 315c1c1 Commit 315c1c1 improved the calculation of the number of Gunicorn worker processes started by the inboard web server. The `gunicorn_conf.calculate_workers()` method was update to return `use_least or use_max or use_total or use_default`. However, this meant that `use_max`, which is set by the `MAX_WORKERS` environment variable, could potentially be used as the total number of workers, when it should only be used as a maximum, not as a total. This commit will update `gunicorn_conf.calculate_workers()` and related tests so that `MAX_WORKERS` is only used to set a limit on the total, not to set the total itself. The `gunicorn_conf.calculate_workers()` method will now return `use_least or use_total or use_default`.
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
The Gunicorn and logging configuration files were technically covered by unit tests, but the individual settings were not tested. This PR will add a dedicated test module for each configuration file, and add tests to verify the Gunicorn and logging settings.
One of the major benefits of unit testing is that the tests not only verify that code is working as expected, but also reveal where the code is not working as expected. This was the case here, and these updates result in cleaner and more correct source code.
Changes
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.--print-configsetting to load and output the configuration.capfdpytest fixture to read the Gunicorn output.configure_logging()method was previously located instart.py, but the logging configuration dictionary was inlogging_conf.py. This organization was not ideal for readability and separation of concerns. It also created logistical complications, such as requiring the Gunicorn configuration filegunicorn_conf.pyto import from start.py, so that it could configure logging for Gunicorn. Thestart.pyscript should be self-contained, and should import other modules and objects from the package, not the other way around.configure_logging()tologging_conf.py, and move the tests to a corresponding module,test_logging_conf.py. This matches up nicely with the latest updates to thegunicorn_conf.py, by having a setup method at the top of the module, and the settings below.configure_logging()method did multiple things: find and import a logging configuration module, load the logging configuration dictionary, and apply the dictionary configuration to the Python logger.configure_logging()into two methods:find_and_load_logging_conf()will return a dictionary configuration, andconfigure_logging()will apply the dictionary configuration.configure_logging()will remain the same, so there will not be any changes to the programming API.configure_logging()method is in the same module as the logging configuration dictionary, that dictionary will be used by default, instead of finding the module separately.capfdpytest fixture.caplogfixture, but I have not had as much success with it.Related
#3
#11