-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: dynaconf fixture with environments #8
Changes from all commits
e4ed849
1b87977
23396bc
d50fa06
7e9456e
00ae6f1
78797b6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,7 +37,7 @@ jobs: | |
|
||
- name: Run tests | ||
run: | | ||
pytest | ||
pytest | ||
|
||
install-test: | ||
name: "test as installed" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While added documentation is good, I feel like I should make use of the mkdocs bits currently available (and not used yet), so there is a proper documentation available also in other formats. I'll work on this, so we can have a better place for the docs. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
# Configuration and common usecases | ||
## Tests are run in upstream environment | ||
|
||
Pytest will run local candlepin service in a container. | ||
|
||
Dynaconf will load all configuration from a file `settings.toml` | ||
|
||
```bash | ||
$ export ENVIRONMENTS_FOR_DYNACONF=True | ||
$ export ENV_FOR_DYNACONF=upstream | ||
$ | ||
$ pytest -v | ||
``` | ||
Pytest will run all tests that are available for `upstream` | ||
environment. | ||
|
||
There is a section `[upstream]` in the settings file that contains of | ||
all properties that tests require. | ||
|
||
## Tests are run in CI environment | ||
Jenkins does not require dynaconf to use environments. | ||
Jenkins provides all config properties directly - using environment | ||
variables. | ||
|
||
```bash | ||
$ export ENVIRONMENTS_FOR_DYNACONF=False | ||
$ | ||
$ pytest -v | ||
``` | ||
|
||
## Tests are run in stage environment | ||
Properties that describe a development environment must be stored in a | ||
secret file that is not part of the repo. Default name of the file is | ||
`.secrets.toml` | ||
|
||
```bash | ||
$ export ENVIRONMENTS_FOR_DYNACONF=True | ||
$ export ENV_FOR_DYNACONF=stage | ||
$ | ||
$ pytest -v | ||
``` | ||
|
||
`stage` environment is used for most of the tests. It requires all | ||
services to be available. | ||
|
||
All properties are defined in a custom file `.secrets.toml` This file | ||
is not part of this repo. | ||
Comment on lines
+31
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this section seems a bit a duplication of the previous one on upstream environment: all it says different is mentioning the different environment name & context; I'd rather make it generic There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is difference that upstream environment will be already configured in the repo (ie. all config properties will be in a file settings.toml (or generated automatically from test candlepin). So the most important part of this section is note about .secrets.toml that is not part of the repo and it is necessary to create it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The only difference I see is related to the configuration file for dynaconf; since that will need to be explained/documented regardless of the environment used, then the two different sections become basically redundant. Please un-duplicate them, thanks. |
||
|
||
# Example of `.secrets.toml` | ||
|
||
```toml | ||
[stage.candlepin] | ||
hostname=TO_BE_CONFIGURED | ||
|
||
[stage.account] | ||
username=TO_BE_REDACTED | ||
password=TO_BE_REDACTED | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
# SPDX-FileCopyrightText: Red Hat | ||
# SPDX-License-Identifier: MIT | ||
|
||
import pytest | ||
from dynaconf import Dynaconf, Validator | ||
|
||
# | ||
# it is necessary to use define global variable since dynaconf settings object | ||
# is used even in pytest hooks to get config properties | ||
# | ||
_settings = Dynaconf( | ||
envvar_prefix="PYTEST_CLIENT_TOOLS", | ||
settings_files=["settings.toml", ".secrets.toml"], | ||
) | ||
|
||
|
||
@pytest.fixture | ||
def settings(scope="session"): | ||
_settings.validators.register( | ||
Validator("candlepin.host"), | ||
Validator("candlepin.port", gt=0, lt=65536, cast=int, is_type_of=int), | ||
Validator("candlepin.prefix", startswith="/"), | ||
Validator("candlepin.insecure", cast=bool, is_type_of=bool), | ||
Validator("candlepin.username"), | ||
Validator( | ||
"candlepin.password", | ||
must_exist=True, | ||
when=Validator("candlepin.username", must_exist=True), | ||
), | ||
) | ||
_settings.validators.validate() | ||
return _settings | ||
Comment on lines
+17
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this greatly reimplements what the existing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The main reason for this simplification was that one class in a module is redundant in case test_config. It seems to me that a module file is enough to group functions into one namespace. I vote for keeping it super simple. ie. no hidden logic for a tester, upstream visitor to learn. That is a reason to use only well known tools, libraries rather than creating our own classes with another middleware. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,13 +7,15 @@ | |
import logging | ||
import shutil | ||
import subprocess | ||
|
||
jstavel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
import pytest | ||
|
||
|
||
jstavel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
from .candlepin import Candlepin, ping_candlepin | ||
from .insights_client import InsightsClient, INSIGHTS_CLIENT_FILES_TO_SAVE | ||
from .logger import LOGGER | ||
from .podman import Podman | ||
|
||
|
||
Comment on lines
+17
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. extra changes, unrelated (please drop) |
||
from .subscription_manager import ( | ||
SubscriptionManager, | ||
SUBMAN_FILES_TO_SAVE, | ||
|
@@ -22,7 +24,7 @@ | |
from .rhc import Rhc, RHC_FILES_TO_SAVE | ||
from .test_config import TestConfig | ||
from .util import NodeRunningData | ||
|
||
from .config import _settings | ||
|
||
_MARKERS = { | ||
"candlepin": "tests requiring a self-deployed Candlepin", | ||
|
@@ -233,6 +235,11 @@ def pytest_configure(config): | |
for mark, description in _MARKERS.items(): | ||
config.addinivalue_line("markers", f"{mark}: {description}") | ||
config.addinivalue_line("markers", "jira(id): test for jira cards") | ||
config.addinivalue_line( | ||
"markers", | ||
"env(name): required Dynaconf environment for a test", | ||
) | ||
|
||
locale.setlocale(locale.LC_ALL, "C.UTF-8") | ||
|
||
|
||
|
@@ -256,3 +263,12 @@ def pytest_runtest_logfinish(nodeid, location): | |
if node_running_data.logfile.exists(): | ||
node_running_data.artifacts.copy(node_running_data.logfile) | ||
logging.getLogger().handlers.remove(node_running_data.handler) | ||
|
||
|
||
def pytest_runtest_setup(item): | ||
envnames = [mark.args[0] for mark in item.iter_markers(name="env")] | ||
the_environment = _settings.get("env_for_dynaconf") or "development" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is exactly the reason why the class TestConfig:
...
@property
def environment(self):
return _settings.get("env_for_dynaconf") or DEFAULT_ENVIRONMENT this way:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there is 'default_env' property in Dynaconf. It could be used to specify default environment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted to avoid a class in this case - since Dynaconf does the same. Groupping functions into one namespace can be done by using module file and functions in it. It is easier for me to update doc aka 'this is a dynaconf object. See: dynaconf.org for more details how it works' rather that keep doc about test_class updated. Another reason was that when a test code is shallow (ie no additional middleware - it is easier to debug it and it is easier for an upstream user to understand the code'. It was my main reason for this suggestion - KISS. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I said, Thanks for the suggestion, but please use |
||
if envnames and the_environment not in envnames: | ||
pytest.skip( | ||
f"test requires a Dynaconf environment to be one of those: {envnames}", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra change, unrelated to the changes; I pushed a small commit to fix it, after checking whether other files had trailing spaces