Expose user enabled features as pytest markers#508
Conversation
|
Is this a more common failure? |
| @property | ||
| def _obsah_state(self) -> Path: | ||
| # mirror what foremanctl / obsah does | ||
| if state := os.environ.get('OBSAH_STATE'): |
There was a problem hiding this comment.
I am wondering if OBSAH_STATE and OBSAH_BASE will exist until we define them, in case of foremanctl we define them to be used in execution in https://github.com/theforeman/foremanctl/blob/master/foremanctl
There was a problem hiding this comment.
I had assumed that you could still override them, but now that I read foremanctl again I see I was wrong.
My idea was to build towards running things in parallel:
export OBSAH_STATE=$(mktemp -d)
foremanctl deploy --flavor foreman
pytestThen in another shell:
export OBSAH_STATE=$(mktemp -d)
foremanctl deploy --flavor katello
pytestPerhaps it's better to reduce this method to the hardcoded option until we get the other pieces in place.
There was a problem hiding this comment.
So these envs are no use as of today, but when we have support for multiple flavours(which certanily will have diffrent set of features ) we could use envs at runtime to invoke flavour specific ci workflows?
Thinking out loud:- In foreman-installer we had answers file per scenarios, will we have similar kind of parameters files for each flavour, also there was no way of changing scenarios after you deployed with one, will that remain same or could be improved,
This is not related to the PR, but just wanted to write my mind dump out
There was a problem hiding this comment.
So these envs are no use as of today, but when we have support for multiple flavours(which certanily will have diffrent set of features ) we could use envs at runtime to invoke flavour specific ci workflows?
Yes. I was a bit inspired by #494. That uses multiple git checkouts (which has a value in its own) while the approach I had in mind here allowed testing multiple deployments from the same directory. But thinking more about it, perhaps we shouldn't build that partially and either do it completely or not at all.
Thinking out loud:- In foreman-installer we had answers file per scenarios, will we have similar kind of parameters files for each flavour, also there was no way of changing scenarios after you deployed with one, will that remain same or could be improved,
Technically there was a way to switch scenarios, but that switching was never really well implemented / supported. Partially because the underlying system didn't really support it. I think on the end user system we should aim for a single parameters file and not allow switching between flavors.
This is not related to the PR, but just wanted to write my mind dump out
Since we're dumping thoughts: I've been toying with the thought of remote deployments. Ansible supports running on remote hosts so can we imagine some sort of control node where you manage the actual deployment? Then by changing OBSAH_STATE you could perhaps manage your various environments (like test, production, etc).
There was a problem hiding this comment.
can we imagine some sort of control node where you manage the actual deployment?
Yes, that should be possible with ansible, rather than hardcoding quadlet as deployment location we could give a target group(multiple hosts).
| return root / '.var' / 'lib' / 'foremanctl' | ||
|
|
||
| def _read_parameters(self): | ||
| params_file = self._obsah_state / 'parameters.yaml' |
There was a problem hiding this comment.
Not blocking: currently we are reading from parameters.yaml, and today if we add feature it first gets added to parameters.yaml and then the subsequent steps for setup runs in playbook and roles.
The only case i see this becoming a issue is when --add-feature fails in between the run but feature persists in parameters.yaml and then that leaves that feature specific test broken.
I also a relevent PR which persits params after playbook run theforeman/obsah#118
There was a problem hiding this comment.
I'd argue that's a corrupted state and running a test suite on that isn't useful. If foremanctl deploy exits with a non-zero code then IMHO the whole execution should halt.
There was a problem hiding this comment.
After playing with the tests myself I now understand what you mean. The test suite runs --add-feature invalid-feature which invalidates reruns.
I've also found another limitation: it doesn't read the features from the flavor which for now isn't a big deal but will be when you really want to gate all features.
There was a problem hiding this comment.
Should it use foremanctl features --list-enabled to get the list instead of digging in internals?
There was a problem hiding this comment.
Should it use foremanctl features --list-enabled to get the list instead of digging in internals?
Yes that would be a better option as foremanctl features will retrieve features from features.yml.
I've also found another limitation: it doesn't read the features from the flavor which for now isn't a big deal but will be when you really want to gate all features.
This will also be solved if we use foremanctl features to get features list. one thing to note here is that foremanctl features is not aware about flavor while pulling features, so it pulls all features from feeatures.yml, which is fine as of today because we only have one flavor.
But in future we need to make foremanctl features aware of flavor
There was a problem hiding this comment.
But that also brings next challenge which is what if mutiple features needs
dynflowas dependency.
You read my mind. I was thinking about Apache & PostgreSQL and if that could be tracked as an internal feature. It can be pulled in by multiple services (Foreman, Candlepin, Pulp).
There was a problem hiding this comment.
I was thinking about Apache & PostgreSQL and if that could be tracked as an internal feature.
not sure if we would expose postgresql as feature, but we have remote-execution which has dynflow as dependency but we might also have dynflow as dependency for other feature, i think ansible and that way we have two features having dynflow as dependency,
and then with what i said earlier
i am thinking is test user exposed features and conditionalily test internal features based on if the feature (that needs them as dependency, so in case of dynflow, it is remote-execution), is enabled.
becomes a issue as we need to test dynflow if any of these feature is enabled.
I have also asked somewhat similar #444 (comment)
There was a problem hiding this comment.
not sure if we would expose postgresql as feature, but we have
remote-executionwhich hasdynflowas dependency but we might also havedynflowas dependency for other feature, i think ansible and that way we have two features havingdynflowas dependency,
The reason I got to it is introducing a plain foreman-proxy (without content). That doesn't run Apache and PostgreSQL. I could derive it in Ansible code from which other features are enabled but then it's harder in testing. An internal feature could help.
I have also asked somewhat similar #444 (comment)
Good point, I hadn't thought that far ahead yet.
There was a problem hiding this comment.
yes, looking at
foremanctl/tests/foreman_proxy_test.py
Line 31 in 6822461
we will mark it as pytest.mark.feature("foreman-proxy") probabily and i think we can stick to use
foremanctl feartures --list-enabled to get features (which will also bring default flavor feature) for now, and once we see more test required for internal features, we can extend that. what do you think?
There was a problem hiding this comment.
Yes, let's start with foremanctl feartures --list-enabled and refactor as needed. IMHO even without the internal features this is already a useful framework.
ekohl
force-pushed
the
add-feature-test-infra
branch
from
cc85c2a to
54a75eb
Compare
|
Tip For best results, initiate chat on the files or code changes.
This failure is unrelated to the PR's code changes — it's a transient infrastructure issue. A simple re-run of the CI job usually resolves it. If it becomes persistent, some common mitigations include:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis PR refactors the test feature marking system from IOP-specific hardcoding to a configuration-driven approach. A new Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (2)
docs/developer/testing.md (1)
153-153:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix spelling error.
"invidivual" should be "individual".
📝 Proposed fix
-You can mark an invidivual test to be skipped if needed: +You can mark an individual test to be skipped if needed:🤖 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 `@docs/developer/testing.md` at line 153, Typo in the docs: replace the misspelled word "invidivual" with the correct word "individual" in the sentence "You can mark an invidivual test to be skipped if needed:" so the line reads "You can mark an individual test to be skipped if needed:"; search for the exact token "invidivual" in docs/developer/testing.md and update it wherever it appears to maintain consistency.tests/conftest.py (1)
37-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd error handling for YAML parsing and validation.
The current implementation has two error paths that would crash test collection:
yaml.safe_load()can raiseyaml.YAMLErrorif the file is malformed- Line 49 assumes
paramsis a dict, butyaml.safe_load()can return non-dict types (list, string, None, etc.), causingAttributeError🛡️ Proposed fix to add robust error handling
def _read_parameters(self): params_file = self._obsah_state / 'parameters.yaml' if params_file.exists(): - with params_file.open('r') as f: - return yaml.safe_load(f) + try: + with params_file.open('r') as f: + params = yaml.safe_load(f) + if not isinstance(params, dict): + return None + return params + except yaml.YAMLError: + return None return None🤖 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 37 - 50, The _read_parameters/features pair must guard against malformed YAML and unexpected types: wrap yaml.safe_load(params_file) in a try/except catching yaml.YAMLError (and other exceptions from reading) and return None on failure; in features, verify that params is an instance of dict before accessing params.get and ensure the 'features' value is an iterable/list before converting to a set (otherwise treat as empty), and optionally emit a warning/log when parsing or validation fails; update _read_parameters, features, params_file, and yaml.safe_load usage accordingly.
🤖 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 `@docs/developer/testing.md`:
- Line 153: Typo in the docs: replace the misspelled word "invidivual" with the
correct word "individual" in the sentence "You can mark an invidivual test to be
skipped if needed:" so the line reads "You can mark an individual test to be
skipped if needed:"; search for the exact token "invidivual" in
docs/developer/testing.md and update it wherever it appears to maintain
consistency.
In `@tests/conftest.py`:
- Around line 37-50: The _read_parameters/features pair must guard against
malformed YAML and unexpected types: wrap yaml.safe_load(params_file) in a
try/except catching yaml.YAMLError (and other exceptions from reading) and
return None on failure; in features, verify that params is an instance of dict
before accessing params.get and ensure the 'features' value is an iterable/list
before converting to a set (otherwise treat as empty), and optionally emit a
warning/log when parsing or validation fails; update _read_parameters, features,
params_file, and yaml.safe_load usage accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9f6dea4a-b883-433c-aa2c-c0769cfd1414
📒 Files selected for processing (19)
docs/developer/testing.mdtests/conftest.pytests/foreman_proxy_test.pytests/iop/test_advisor.pytests/iop/test_advisor_frontend.pytests/iop/test_cvemap_downloader.pytests/iop/test_engine.pytests/iop/test_gateway.pytests/iop/test_ingress.pytests/iop/test_integration.pytests/iop/test_inventory.pytests/iop/test_inventory_frontend.pytests/iop/test_kafka.pytests/iop/test_puptoo.pytests/iop/test_remediation.pytests/iop/test_vmaas.pytests/iop/test_vulnerability.pytests/iop/test_vulnerability_frontend.pytests/iop/test_yuptoo.py
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
2 suggestions from coderabbit
Thats indeed a good finding and actionable
I don't think we need to do error handling here, Sad part is it did not even considered |
This is something I also had in mind #508 (comment) but the scenario which i pointed out might not ever happen. |
ekohl
force-pushed
the
add-feature-test-infra
branch
from
54a75eb to
21cc56f
Compare
|
I've updated it to call |
ekohl
force-pushed
the
add-feature-test-infra
branch
2 times, most recently
from
c66320c to
eb6a3d9
Compare
This allows guarding tests behind markers.
ekohl
force-pushed
the
add-feature-test-infra
branch
from
eb6a3d9 to
a1e80be
Compare
|
And now with a few force pushes I think I pleased the linter too. |
3 participants
Why are you introducing these changes? (Problem description, related links)
I suggested it in #484 (comment) because I think it sets the base for future extensibility.
What are the changes introduced in this pull request?
It reads
foremanctl featuresto know which features exist and which are enabled. It then configures pytest to expose a feature marker (inpytest_configure) that's then read inpytest_runtest_setup.With all of that set up, the existing iop feature guard is rewritten.
How to test this pull request
Steps to reproduce:
Checklist