feat: dynaconf fixture with environments #8

jstavel · 2024-01-09T21:26:38Z

added documentation how to use environments.
Added a few cases for various environments to run the tests.

ptoscano

thanks for the PR! while there are some things to fix/change, it is a good starting approach

regarding the CI failures you noticed:

the codebase is formatted with black: the currently tested version is 23.3.0, however I don't think a different version will format in a different way (it can be easy to fix, anyway); simply have black installed, and run black . in the top-level of the sources before doing a commit
there are REUSE [1] markers for licenses and copyright notices, see also the FAQ for more info; you'll likely run into this when creating new files
ideally, please use Signed-off-by markers in commits; this is not enforced yet

(yes, all the above need to be in a CONTRIBUTING.md file, I have a draft here)

I added inline comments for all the things I noticed so far; additional recommendations are:

please try to keep the commits minimal: style changes (empty lines added/removed) in unrelated parts of the codebase ought to not be part of the PR (better do them separately, if needed)
the commit message itself needs to be a little more verbose than the current one, explaining what is being added/removed/changed, and possibly also the why/reasons

(also, since the additional commits after the first are fixups of the first commit, they will need to be squashed; this can be done as last step, once the PR is ready, so no need to think about it for now; I will remind it as last step)

Thanks!

ptoscano · 2024-01-10T12:05:49Z

.github/workflows/ci.yml

@@ -37,7 +37,7 @@ jobs:

      - name: Run tests
        run: |
-          pytest 
+          pytest


extra change, unrelated to the changes; I pushed a small commit to fix it, after checking whether other files had trailing spaces

ptoscano · 2024-01-10T12:08:14Z

pytest_client_tools/config.py

+@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


this greatly reimplements what the existing test_config fixture does already; neither the PR text nor the commit message say why it is duplicated like this (even copy&pasting the existing code), so please explain what is the reason behind this

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.

No, TestConfig is not redundant, and it does not create any new middleware. As I said, its purpose is to group anything related to the configuration of the test session, i.e. what is read using dynaconf plus additional logic related to that.

ptoscano · 2024-01-10T12:08:30Z

pytest_client_tools/plugin.py

+
+

extra changes, unrelated (please drop)

ptoscano · 2024-01-11T06:38:35Z

CONFIGURATION.md

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.

ptoscano · 2024-01-11T06:40:29Z

CONFIGURATION.md

+## 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.


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 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.

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.

pytest_client_tools/plugin.py

ptoscano · 2024-01-11T07:11:54Z

pytest_client_tools/plugin.py

+    envnames = [mark.args[0] for mark in item.iter_markers(name="env")]
+    the_environment = _settings.get("env_for_dynaconf") or "development"
+    if envnames and the_environment not in envnames:
+        pytest.skip(
+            f"test requires a dynaconf environment to be one of those: {envnames}",
+        )


I think that, rather than implementing it here, it can be implemented as proper marker; something like this:

def pytest_client_tools_env(*envnames): return pytest.mark.skipif( the_current_environment not in envnames, reason="different environment" )

I do not understand how to write this decorator. I've copied the marker env from https://docs.pytest.org/en/7.1.x/example/markers.html#custom-marker-and-command-line-option-to-control-test-runs They sudgested to use explicit markers creation since pytest --list-markers will list them. Is it OK for a code to remain as it is? https://docs.pytest.org/en/7.1.x/example/markers.html#registering-markers I can move some specific logic out of the plugin.py to config.py.

I do not understand how to write this decorator.

Not sure what do you mean "how to write": what I suggested is a working way, and only needs to get a proper logic/condition to pass to skipif(). Using it is no different than any other decorator/marker:

@pytest_client_tools_env("env1") def test_that_runs_only_on_env1():

It needs to still be added to the markers manually, in the same way of the currently implemented env().

ptoscano · 2024-01-11T07:17:54Z

pytest_client_tools/plugin.py

+
+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"


this is exactly the reason why the TestConfig class (and the test_config fixture) exists: group all the logic related to the "configuration" of tests in a single place; in this case, I'd not want to spread the logic that determines what is the current environment all over the place, but rather implement this once in TestConfig -- something like:

class TestConfig: ... @property def environment(self): return _settings.get("env_for_dynaconf") or DEFAULT_ENVIRONMENT

this way:

DEFAULT_ENVIRONMENT is used there, or even can be defined inline (without a variable) if there is no use for it outside of this function

whatever needs to know what's the current environment can simply query TestConfig.environment without any logic for reading what's the existing one, and determining the fallback

there is 'default_env' property in Dynaconf. It could be used to specify default environment.

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.

As I said, TestConfig does not replace dnyaconf is any way, but rather uses it underneath (composition). Even the documentation will still mention dynaconf, that is not going to change.

Thanks for the suggestion, but please use TestConfig and test_config.

pytest_client_tools/plugin.py

jstavel added 6 commits January 9, 2024 22:24

feat: dynaconf fixture with environments

e4ed849

fixed pre-commit changes

1b87977

fixed pre-commit changes

23396bc

fixed flake8 complains

d50fa06

fixed flake8 complains

7e9456e

fixed a problem with import in plugin file

00ae6f1

ptoscano requested changes Jan 11, 2024

View reviewed changes

fix: more specific comment for env marker

78797b6

jstavel closed this May 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: dynaconf fixture with environments #8

feat: dynaconf fixture with environments #8

jstavel commented Jan 9, 2024

ptoscano left a comment

ptoscano Jan 10, 2024

ptoscano Jan 10, 2024

jstavel Jan 12, 2024

ptoscano Jan 16, 2024

ptoscano Jan 10, 2024

ptoscano Jan 11, 2024

ptoscano Jan 11, 2024

jstavel Jan 12, 2024

ptoscano Jan 16, 2024

ptoscano Jan 11, 2024

jstavel Jan 12, 2024

ptoscano Jan 16, 2024

ptoscano Jan 11, 2024

jstavel Jan 12, 2024

jstavel Jan 12, 2024

ptoscano Jan 16, 2024

feat: dynaconf fixture with environments #8

feat: dynaconf fixture with environments #8

Conversation

jstavel commented Jan 9, 2024

ptoscano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment