Convert /tests/policy/plan into pytest-based test

[happz]

This one uses some jq queries, changes cwd, more complicated, but not terribly complex.

Did I mention jq? I added jq. I like jq. I don't like a mess of in and get(..., {}) expressions to inspect a data structure.

Pull Request Checklist

• implement the feature

[graphite-app]

Missing exit code verification. The original bash test expected exit code 3 (rlRun ... 3), but the new pytest test doesn't verify the exit code. The test should fail if the command doesn't exit with code 3.

result = run_tmt('-vv', 'run', '-a', '--policy-file', POLICIES_DIR / 'plan/simple.yaml')
assert result.exit_code == 3, "Expected exit code 3"

Suggested change

	result = run_tmt('-vv', 'run', '-a', '--policy-file', POLICIES_DIR / 'plan/simple.yaml')
	result = run_tmt('-vv', 'run', '-a', '--policy-file', POLICIES_DIR / 'plan/simple.yaml')
	assert result.exit_code == 3, "Expected exit code 3"

Spotted by Graphite



Is this helpful? React 👍 or 👎 to let us know.

[gemini-code-assist]

Code Review

The pull request introduces jq as a dependency in pyproject.toml and adds utility functions cwd, with_cwd, and jq_all in tests/__init__.py for managing current working directory and applying jq filters in tests. Additionally, it modifies the test script tests/policy/main.fmf to use pytest and replaces the bash script with a python script tests/policy/test_plan.py. The review comments suggest using a thread-local variable for better concurrency handling in the cwd context manager and adding a try...finally block in jq_all to handle potential jq exceptions.

[graphite-app]

The version parsing will fail if Click has a pre-release or post-release version (e.g., '8.2.0rc1', '8.2.0.post1'). The code attempts to convert non-numeric version segments to integers, which will raise a ValueError.

# Fix by handling only numeric parts:
import re
_CLICK_VERSION = tuple(
    int(_s) for _s in re.match(r'(\d+\.\d+\.\d+)', 
                               tmt._compat.importlib.metadata.version('click')).group(1).split('.')
)

Alternatively, use packaging.version.parse() for robust version parsing.

Suggested change

	_CLICK_VERSION = tuple(
	int(_s) for _s in tmt._compat.importlib.metadata.version('click').split('.')
	)
	import re
	_CLICK_VERSION = tuple(
	int(_s) for _s in re.match(r'(\d+\.\d+\.\d+)',
	tmt._compat.importlib.metadata.version('click')).group(1).split('.')
	)

Spotted by Graphite



Is this helpful? React 👍 or 👎 to let us know.

[LecrisUT]

Why is this one needed to be exported? It is available from the start https://github.com/python/cpython/blob/v3.9.25/Lib/importlib/metadata.py

[LecrisUT]

Can we please avoid this? I rather help with figuring out what the blackmagic in the yq/jq was used and translate it to python instead.

[happz]

There is no black magic, just a simple filter one can use to inspect a JSON/YAML structure. Native tools like indexing and get() can't keep up in simplicity and readability with .[] | .prepare | .[] | [.how, .order].

Give me a tool as good as a jq filter, and I'd happily use it.

[LecrisUT]

It's more about readability .[] | .prepare | .[] | [.how, .order] vs

def _filter(data: Any):
    for plan in data:
        for prepare in plan["prepare"]
            yield {
                "how": prepare["how"],
                "order": prepare["order"],
            }

And we don't even need the last part because that was there just to make it string-friendly comparable.

[happz]

Yeah, and the Python implementation of that simple filter is absolutely terrible. So much imperative work instead of declaring what I want to get. And the deeper a test would need to peek, with keys that may or may not be present, these custom helpers will get more and more complicated.

Sorry, you can't really convince me Python is better in this area, it's not its strongest suite. My idea is to use a tool that's, IMHO, better suited for the task, that's all.

This discussion cannot be resolved here, we clearly have different views on how the problem should be approached. Added to the hacking session agenda.

[LecrisUT]

I would refer not to extend this one, at the very least not inside this function

[LecrisUT]

Could we maybe avoid this and use --root instead? We could even put it in the run_tmt fixture along with some plan selection and stuff.

[happz]

I'd consider this a different problem. One that can be improved, but I tried to not invent too many new things.