Introduce feature-based test directory structure

[ekohl]

Why are you introducing these changes? (Problem description, related links)

Inspired by #485, but building on top of #508.

What are the changes introduced in this pull request?

It introduces a directory structure that automatically marks anything under tests/feature/$feature with $feature. This prepares for additional flavors that disable various features, such as a plain foreman (without katello) and a plain foreman-proxy.

How to test this pull request

Steps to reproduce:

• Create a deployment that includes all features and verify all tests are executed
• Create a deployment that disables some features and verify it skips the right tests

Checklist

• Tests added/updated (if applicable)
• Documentation updated (if applicable)

[ekohl]

Test failures due to Galaxy:

Error: : Failed to download collection tar from 'default' due to the following unforeseen error: The read operation timed out

Error: : Error when getting available collection versions for community.crypto from default (https://galaxy.ansible.com/api/) (HTTP Code: 504, Message: Gateway Timeout Code: Unknown)

[ehelms]

This automatic marking is probably the only part that is semi-magical. However, as a convention I think this is fine. I think we should document this convention in our testing.md file.

Then a question. What about "meta" features? e.g. content/rpm, content/pypi

[ekohl]

This automatic marking is probably the only part that is semi-magical. However, as a convention I think this is fine. I think we should document this convention in our testing.md file.

I also considered the magic. However, there is validation on invalid features and if we have the proper testing matrix we should exercise all code both with and without the feature present.

Good call on documenting it.

Then a question. What about "meta" features? e.g. content/rpm, content/pypi

I've tried to keep it simple to avoid introducing too much magic. In #511 I've toyed with hiding Pulp behind a feature and the testing is IMHO the biggest task on introducing additional flavors.

My preference is to have very light testing in foremanctl and then the actual end-to-end tests in smoker. So I'd avoid introducing more magic and keep it simple. Assuming we hide Pulp behind the katello feature and then for tests:

• tests/feature/katello/pulp_test.py
• tests/feature/katello/content_rpm_test.py
• tests/feature/katello/content_pypi_test.py

They will all be guarded behind the katello feature because of the directory structure. The content tests can use pytestmark at the top of the file to guard on both the katello feature and specific content types.

[arvind4501]

With this we have foreman-proxy marker applied to all tests in tests/feature/foreman-proxy/base_test.py but that would also have sub proxy feature(like bmc), do we need a way to create a per sub-feature file and mark that with the relevent marker?

[arvind4501]

i see fspath is deprecated,so good to use path instead https://docs.pytest.org/en/stable/deprecations.html#fspath-argument-for-node-constructors-replaced-with-pathlib-path

[ekohl]

Neat, didn't know that was already supported. This will save me an import too.

Suggested change

	rel_path = Path(item.fspath).relative_to(feature_dir)
	rel_path = item.path.relative_to(feature_dir)

Keeping it open for now to allow for more comments

[ekohl]

With this we have foreman-proxy marker applied to all tests in tests/feature/foreman-proxy/base_test.py but that would also have sub proxy feature(like bmc), do we need a way to create a per sub-feature file and mark that with the relevent marker?

Technically feature('foreman-proxy') is applied to tests/feature/foreman-proxy/**/*_test.py.

You can certainly create a standalone file and if you have a bunch of tests it makes sense to create tests/feature/foreman-proxy/bmc_test.py:

import pytest

pytestmark = pytest.mark.feature('bmc')


def test_x():
    pass


def test_y():
    pass

This will be equivalent to (today) creating tests/foreman_proxy_bmc_test.py:

import pytest

pytestmark = [pytest.mark.feature('foreman-proxy'), pytest.mark.feature('bmc')]


def test_x():
    pass


def test_y():
    pass

But you can also create tests/feature/foreman-proxy/base_test.py:

import pytest


def test_base():
    pass


@pytest.mark.feature('bmc')
def test_bmc():
    pass

All of these are functionally equivalent and it depends on how we want to organize it.

[arvind4501]

Yes, I was thinking about marking the bmc feature dynamically, so when foreman-proxy is enabled, base_test.py runs the base proxy tests, and if bmc is also enabled, bmc_test.py runs the BMC-specific tests.
That way we could avoid manual @pytest.mark.feature('bmc') , the hook could derive the sub-feature marker from the filename (e.g., bmc_test.py → bmc)

Not blocking, just a thought to make things more dynamic

[ekohl]

As noted in #509 (comment), I've tried to avoid making things too dynamic and magical. I still remember https://code.djangoproject.com/wiki/RemovingTheMagic and think that's a good thing: I like it when things are easily predictable.

[arvind4501]

I've tried to avoid making things too dynamic and magical.

Fair, thanks

[arvind4501]

Lets give coderabbit a try @coderabbitai review

[coderabbitai]

@arvind4501: Sure! I'll kick off a review of the changes in this PR right away.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Walkthrough

This PR refactors pytest feature-marker application by introducing a pytest collection hook that automatically applies markers to test items based on directory location under tests/feature/<feature_name>/. The conftest.py hook computes each test's path relative to tests/feature and applies a corresponding feature marker. Existing iop test modules have explicit pytestmark declarations removed since the hook now handles this automatically. The testing documentation is updated to explain the convention and redirect new service tests under tests/feature/<feature>/ paths. New VMaaS integration tests validate service state, systemd dependencies, Podman secrets, and data volumes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• theforeman/foremanctl#508: Introduces the initial pytest feature-marker infrastructure in conftest.py and testing.md that this PR builds upon and consolidates by removing redundant per-file marker declarations.

Suggested reviewers

• arvind4501

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Introduce feature-based test directory structure' directly and accurately summarizes the main change: introducing a new directory convention for tests that automatically applies feature markers based on directory structure.
Description check	✅ Passed	The description is directly related to the changeset, explaining the motivation (preparing for deployment flavors with disabled features), the implementation (automatic feature markers via directory structure), and testing guidance.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

♻️ Duplicate comments (1)

tests/conftest.py (1)

246-257: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use item.path instead of deprecated item.fspath.

Line 250 uses Path(item.fspath) which is deprecated in pytest. As previously suggested, use item.path.relative_to(feature_dir) instead, which will also eliminate the need for the Path import on line 4.

Apply the previously suggested fix

-from pathlib import Path

 def pytest_collection_modifyitems(config, items):
     feature_dir = config.rootdir / 'tests' / 'feature'
     for item in items:
         try:
-            rel_path = Path(item.fspath).relative_to(feature_dir)
+            rel_path = item.path.relative_to(feature_dir)
         except ValueError:
             # Not in the features directory
             pass

Verify the deprecation status:

Is pytest's item.fspath deprecated in favor of item.path?

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/conftest.py` around lines 246 - 257, Replace the deprecated use of
item.fspath in pytest_collection_modifyitems with item.path: change rel_path =
Path(item.fspath).relative_to(feature_dir) to rel_path =
item.path.relative_to(feature_dir) (keep the existing try/except ValueError
behavior), and remove the now-unused Path import at the top of the file; this
targets the pytest_collection_modifyitems function and eliminates the deprecated
item.fspath usage.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@tests/conftest.py`:
- Around line 246-257: Replace the deprecated use of item.fspath in
pytest_collection_modifyitems with item.path: change rel_path =
Path(item.fspath).relative_to(feature_dir) to rel_path =
item.path.relative_to(feature_dir) (keep the existing try/except ValueError
behavior), and remove the now-unused Path import at the top of the file; this
targets the pytest_collection_modifyitems function and eliminates the deprecated
item.fspath usage.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5afb9ce7-cb45-4d3a-8559-bfc1b332a090

📥 Commits

Reviewing files that changed from the base of the PR and between 8561643 and 467b828.

📒 Files selected for processing (29)

• docs/developer/testing.md
• tests/conftest.py
• tests/feature/__init__.py
• tests/feature/foreman-proxy/__init__.py
• tests/feature/foreman-proxy/base_test.py
• tests/feature/foreman/__init__.py
• tests/feature/foreman/api_test.py
• tests/feature/foreman/base_test.py
• tests/feature/foreman/compute_resources_test.py
• tests/feature/foreman/plugins_test.py
• tests/feature/hammer/__init__.py
• tests/feature/hammer/base_test.py
• tests/feature/iop/__init__.py
• tests/feature/iop/test_advisor.py
• tests/feature/iop/test_advisor_frontend.py
• tests/feature/iop/test_cvemap_downloader.py
• tests/feature/iop/test_engine.py
• tests/feature/iop/test_gateway.py
• tests/feature/iop/test_ingress.py
• tests/feature/iop/test_integration.py
• tests/feature/iop/test_inventory.py
• tests/feature/iop/test_inventory_frontend.py
• tests/feature/iop/test_kafka.py
• tests/feature/iop/test_puptoo.py
• tests/feature/iop/test_remediation.py
• tests/feature/iop/test_vmaas.py
• tests/feature/iop/test_vulnerability.py
• tests/feature/iop/test_vulnerability_frontend.py
• tests/feature/iop/test_yuptoo.py

💤 Files with no reviewable changes (16)

• tests/feature/iop/test_kafka.py
• tests/feature/iop/test_remediation.py
• tests/feature/iop/test_inventory.py
• tests/feature/iop/test_vulnerability_frontend.py
• tests/feature/iop/test_advisor.py
• tests/feature/iop/test_advisor_frontend.py
• tests/feature/iop/test_inventory_frontend.py
• tests/feature/iop/test_yuptoo.py
• tests/feature/iop/test_cvemap_downloader.py
• tests/feature/iop/test_ingress.py
• tests/feature/iop/test_puptoo.py
• tests/feature/iop/test_vulnerability.py
• tests/feature/iop/test_integration.py
• tests/feature/iop/test_gateway.py
• tests/feature/iop/test_vmaas.py
• tests/feature/iop/test_engine.py

[ekohl]

New VMaaS integration tests validate service state, systemd dependencies, Podman secrets, and data volumes.

@coderabbitai appears to be unable to track file moves so it sees this as a new tests.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check git log to see if test_vmaas.py was moved/renamed
git log --follow --oneline -- tests/feature/iop/test_vmaas.py 2>/dev/null | head -10
# Check if there was a previous location for this file
git log --diff-filter=R --summary --oneline HEAD~5..HEAD 2>/dev/null | grep -i vmaas | head -20
# Check what files were renamed in this PR branch vs master
git diff --name-status origin/master...HEAD 2>/dev/null | grep -i vmaas

Length of output: 266

---

@ekohl: You're correct — apologies for the confusion. The git history confirms that tests/feature/iop/test_vmaas.py was renamed/moved from tests/iop/test_vmaas.py with 96% similarity. CodeRabbit's diff-based review processed it as a delete + add pair rather than a rename, which caused the incorrect "new tests" characterization in the summary. The test functions were not newly added — they already existed in the prior location.

---

✏️ Learnings added

Learnt from: ekohl
URL: https://github.com/theforeman/foremanctl/pull/509

Timestamp: 2026-05-20T14:30:04.525Z
Learning: In the theforeman/foremanctl repository, CodeRabbit may misidentify renamed/moved test files as new additions. For example, `tests/feature/iop/test_vmaas.py` was moved from `tests/iop/test_vmaas.py` (R096 rename) but was incorrectly described as a new file with newly added test functions. When reviewing PRs that reorganize test directory structures (such as the `tests/feature/` migration in PR `#509`), treat files with high rename similarity scores as moves rather than new additions.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

[arvind4501]

You're correct — apologies for the confusion

Good to see AI accepting its mistakes gracefully 😄