Proposal: Replace nox for testing automation instead of tox and/or Makefile

[slimreaper35]

https://nox.thea.codes/en/stable

Notes

• Each session is defined as a Python function with the @nox.session decorator
• reuse_venv=True is used to speed up the execution of sessions by reusing the existing venv
• Docstrings below each session function are used as session descriptions in the help output
• The module docstring is used in the help output when running nox -l, --list too
• The *session.posargs is used to pass additional arguments to the command
• The silent=True argument is used to suppress the output of the command only if the command was successful
• To use external commands, set the external=True argument in the session.run() method
  (it should work without the external=True argument, but it's better to be explicit and silence the warning)
• The only command from Makefile that is not included here is make venv
  (it probably doesn't make sense to have a session for that, it would require contributors to install nox first)
• I made some minor adjustments to pytest commands to make them more readable and clean

---

Maintainers will complete the following section

• Commit messages are descriptive enough
• Code coverage from testing does not decrease and new code is covered
• Docs updated (if applicable)
• Docs links in the code are still valid (if docs were updated)

Note: if the contribution is external (not from an organization member), the CI
pipeline will not run automatically. After verifying that the CI is safe to run:

• approve GitHub Actions workflows by clicking a button
• approve the Red Hat Trusted App Pipeline container build by commenting /ok-to-test
  (as is the standard for Pipelines as Code)

[eskultety]

@slimreaper35 this is a very shallow top-level review I've done without trying it locally myself, but from the first look it looks fine, definitely doesn't hinder readability when compared to the declarative tox.ini config, so from that POV seems like a solid alternative to what we have today.

[eskultety]

I wonder if anyone's ever used this feature. If one suspects a problem originating in an old base image then --pull-always and --no-cache should be in their docker arsenal. I think we can safely drop this, it was just a weird kludge that doesn't really relate to the project itself in any way.
That said, since this existed as a Makefile target, it should be dropped either prior this conversion or after, not in the same commit, the migration should be a 1:1 migration of Makefile + tox, maybe even split this particular commit in 2 - first migrate tox.ini to nox, then Makefile (or vice versa), then remove the corresponding origins.

[a-ovchinnikov]

Such targets add clues to how cachi2 could be tested. I suggest keeping them in place until there is a superb troubleshooting document.

[eskultety]

Such targets add clues to how cachi2 could be tested. I suggest keeping them in place until there is a superb troubleshooting document.

I don't think this is useful in any way, we're pretty much the only project I've encountered that tried to wrap some of podman's flags to do this, anyone even remotely familiar with podman would not really use this. I mean, I think with this @slimreaper35's effort it's time that we drop some of these kludges altogether and only define what's necessary for the project to decrease clutter, improve transparency, promote flexibility in a way where every user is motivated to use standard invocation mechanisms and be in full control over the configuration of such an invocation if what I just wrote makes sense at all. In this very case I don't think any extensive documentation is needed at all, having a pristine image is just one of the troubleshooting and debugging approaches, we're not wrapping other common invocations.

[eskultety]

nitpick:
["3.9", "3.10", "3.11", "3.12"]

In order to avoid changing multiple files at the same time, since noxfile is a Python script we could introduce a simple helper that would parse the requires-python field from the pyproject.toml file and enumerate the version set from the specifier (based on packaging.specifiers.SpecifierSet), i.e. if we adjust the specifier to >=3.9, <=3.12 such helper would yield this exact list. That way, any time we add a new supported version of Python (e.g. 3.13) or deprecate the lower boundary (i.e. 3.9 in this case), noxfile will likely not need an adjustment because of such update.

[slimreaper35]

Do you mean something like that ?

def _get_supported_python_versions() -> list[str]:
    """
    Read pyproject.toml and return a list of all supported Python versions
    based on the requires-python specification.
    """
    pyproject_path = Path("pyproject.toml")
    pyproject = tomli.loads(pyproject_path.read_text())
    requires_python = pyproject["project"]["requires-python"]

    specifiers = SpecifierSet(requires_python)
    min_minor = None
    max_minor = None

    for spec in specifiers:
        minor = int(re.match(r"3\.(\d+)", spec.version).group(1))

        if spec.operator in (">=", ">"):
            if min_minor is None or minor > min_minor:
                min_minor = minor
        elif spec.operator in ("<=", "<"):
            if max_minor is None or minor < max_minor:
                max_minor = minor

    return [f"3.{minor}" for minor in range(min_minor, max_minor + 1)]

[eskultety]

Aren't environment variables automatically passed through? I would expect us to only define the integration_tests target and let it be tweaked with an env variable, that's how I used to it anyway prior this work. I think documenting all the env vars we use anywhere in the project is paramount for that. Unless it doesn't work the way I imagine it does/should in which case my argument becomes invalid.

[slimreaper35]

With the growing amount of our custom env variables, this process becomes much more complicated. Recently, I wanted to generate data for integration tests and I had to define two or more mouthful env variables for that. The next day I would forget the exact names and I could start again looking for them somewhere in the code base.

I would rather have an abstraction layer as it is in this file. As I mentioned in the PR template each session's docstring is displayed in the help message. That seems enough IMO. Just keep it simple.

[a-ovchinnikov]

Having multiple targets adds to documentation of the project and provides another insight into possible ways of running tests and into what could be tweaked when investigating an issue.

[eskultety]

Fair enough, I guess I need to think about this (after trying) harder and potentially propose a follow-up PR if anything comes out of that thought process, so you can disregard my earlier comment for the purposes of getting the PR accepted.

[eskultety]

Similarly here, noxfile is IMO opaque, meaning that nobody will bother looking into it unless they need to add or fix a target, so I think documenting the usage of these env vars to adjust the behaviour of the test target is IMO more transparent than essentially defining "sugar coated" targets which just hinder readability; I think less is more in this case.

[a-ovchinnikov]

This looks so neat!

[a-ovchinnikov]

I wonder if it is a good idea to also drop --check flag?

[slimreaper35]

why ?

[a-ovchinnikov]

To automatically apply black suggestions. What is the normal workflow btw? Does everyone generate a separate diff, inspect it and then apply?

[eskultety]

To automatically apply black suggestions. What is the normal workflow btw? Does everyone generate a separate diff, inspect it and then apply?

Yes, I do that. The reason is that it would only make sense during a rebase, if you run it over a patch series you still have to do your due diligence and apply individual changes and still do a rebase to squash those changes to the right commit. Until we run UTs+linters in a rebase in our GH CI (we'd still have to tweak the action so that commit amends are allowed, do we want that in general?) I don't think we want automatic handling of these.

[a-ovchinnikov]

What do you think about adding

nox.options.stop_on_first_error = True

?

[slimreaper35]

I don't know. That would mean different behavior compared to GH actions.

[a-ovchinnikov]

My reasoning is that it is nice to fail as fast as possible when working on a change locally. I.e. if there is a flake8 issue I know that I need to fix it or it will cascade to other linters, it does not make sense to wait until all targets are done, especially given that nox does not seem to be able to run tests in parallel (unlike tox). I wonder if there is a reverse generator nox_to_tox specifically to leverage parallel target execution?

The original request could be achieved with a local alias though, so I won't insist on it if I am the only one who has this preference

[eskultety]

My reasoning is that it is nice to fail as fast as possible when working on a change locally.

I understand the sentiment and also see some value in this, but then

if there is a flake8 issue I know that I need to fix it or it will cascade to other linters, it does not make sense to wait until all targets are done

This isn't often true for black/isort errors, so e.g. what happens for me often is that I have flake8 + formatter errors reported and currently all it takes is one execution of tox to tell me about all of the errors immediately and I don't need to run tox more than twice per reported error (1 initial and 1 to verify). Linters are pretty fast IMO, most time is consumed by mypy and UTs which this setting would not solve. Also, I think the overall team sentiment is to move off of these individual linters onto Ruff which will just render all this irrelevant given that it's written in a compiled language and so, again, the only real bottleneck in nox execution will be the UTs.

[a-ovchinnikov]

If local DNF set to true by default then something must start it, or at least ensure it is up and running. Maybe false should be assumed as the default?

[slimreaper35]

Well, it depends on how important these special tests are and how we define the sessions. I would assume if I want to run all integration tests, that all integration tests would run.

[a-ovchinnikov]

Good point, but you have defined this env not for all_tests target, but on a higher level. I have probably missed it, but does it still respect environment variables settings?

[eskultety]

Good point, but you have defined this env not for all_tests target, but on a higher level

Fully agreed, I don't want those servers to be spawned most of the time on my local machine, so this should only ever be defaulting to true for the all_tests target.

[slimreaper35]

Done.

[a-ovchinnikov]

Non-blocker, observation: here Python runs podman which starts a container in which shell executes a script (which, in turn, runs Python among other things). What do you think about at least separating the inner shell script into a variable?

[a-ovchinnikov]

A bit controversial style suggestion: what do you think about something like

cmd = "isort --check --diff --color cachi2 tests noxfile.py"
session.run(*cmd.split(), *session.posargs, silent=True)

Maybe it is just me, but the actual underlying command is easier to read this way.

[slimreaper35]

I am okay with both styles.

[eskultety]

Matter of taste - how about we format invocations that fit within the 100 columns on a single line and only split those which would not?

[a-ovchinnikov]

What do you think about making UTs silent as well? (This would require to somehow exfiltrate coverage which will get clobbered along with tests output otherwise)

[slimreaper35]

It is doable, but if one test fails we lose the colorful output from pytest.