reproducer for SAT-35949#21605
Draft
pondrejk wants to merge 1 commit into
Draft
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds a dedicated pytest reproducer for SAT-35949 that updates a capsule’s taxonomies via the Capsule CLI and pauses execution with a breakpoint for manual UI verification of the capsule/organization/location mismatch behavior. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
test_reproduce_capsule_mismatchincludes an unconditionalbreakpoint()which will halt automated test runs; if this needs to remain, gate it behind an environment flag or convert it to a skipped/xfail test to avoid blocking CI. - Consider marking this reproducer test with a specific marker (e.g.
@pytest.mark.reproduceror similar) so it can be easily excluded from standard test runs and clearly identified as non-standard.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `test_reproduce_capsule_mismatch` includes an unconditional `breakpoint()` which will halt automated test runs; if this needs to remain, gate it behind an environment flag or convert it to a skipped/xfail test to avoid blocking CI.
- Consider marking this reproducer test with a specific marker (e.g. `@pytest.mark.reproducer` or similar) so it can be easily excluded from standard test runs and clearly identified as non-standard.
## Individual Comments
### Comment 1
<location path="tests/foreman/cli/test_remoteexecution.py" line_range="1214" />
<code_context>
class TestPullProviderRex:
"""Tests related to remote execution via pull provider (mqtt)"""
+ def test_reproduce_capsule_mismatch(
+ self,
+ module_org,
</code_context>
<issue_to_address>
**suggestion (testing):** This test is a manual reproducer without assertions; consider marking it appropriately or turning it into a regression test.
Right now this only sets up state and pauses, so it doesn’t actually verify the bug or the fix in foreman/pull/10952. For long-term value, either mark it as a manual-only reproducer (e.g. `@pytest.mark.manual` or `pytest.skip("manual reproducer")`) or turn it into a true regression test that asserts the expected capsule/org/location behavior once the flow can be automated.
Suggested implementation:
```python
class TestPullProviderRex:
"""Tests related to remote execution via pull provider (mqtt)"""
@pytest.mark.manual
def test_reproduce_capsule_mismatch(
self,
module_org,
smart_proxy_location,
module_target_sat,
module_capsule_configured_mqtt,
):
"""Manual reproducer: run custom template on host converted to mqtt"""
```
1. Ensure `pytest` is imported at the top of `tests/foreman/cli/test_remoteexecution.py` (e.g. `import pytest`) if it is not already.
2. If your project uses strict pytest markers, register the `manual` marker in `pytest.ini` (or the equivalent config) with something like:
`markers = manual: tests that are intended as manual reproducers and not run in automated test suites`.
</issue_to_address>
### Comment 2
<location path="tests/foreman/cli/test_remoteexecution.py" line_range="1230" />
<code_context>
+ 'location-ids': smart_proxy_location.id,
+ }
+ )
+ breakpoint() # noqa
+
@pytest.mark.upgrade
</code_context>
<issue_to_address>
**issue (bug_risk):** Unconditional `breakpoint()` will block automated test runs; gate or remove it.
This `breakpoint()` will hang CI and other automated test runs if the test is executed outside the narrow repro scenario. Please either remove it once the regression test is in place, or guard it behind an explicit flag (e.g. an env var or pytest option) so normal test runs are unaffected.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| class TestPullProviderRex: | ||
| """Tests related to remote execution via pull provider (mqtt)""" | ||
|
|
||
| def test_reproduce_capsule_mismatch( |
Contributor
There was a problem hiding this comment.
suggestion (testing): This test is a manual reproducer without assertions; consider marking it appropriately or turning it into a regression test.
Right now this only sets up state and pauses, so it doesn’t actually verify the bug or the fix in foreman/pull/10952. For long-term value, either mark it as a manual-only reproducer (e.g. @pytest.mark.manual or pytest.skip("manual reproducer")) or turn it into a true regression test that asserts the expected capsule/org/location behavior once the flow can be automated.
Suggested implementation:
class TestPullProviderRex:
"""Tests related to remote execution via pull provider (mqtt)"""
@pytest.mark.manual
def test_reproduce_capsule_mismatch(
self,
module_org,
smart_proxy_location,
module_target_sat,
module_capsule_configured_mqtt,
):
"""Manual reproducer: run custom template on host converted to mqtt"""- Ensure
pytestis imported at the top oftests/foreman/cli/test_remoteexecution.py(e.g.import pytest) if it is not already. - If your project uses strict pytest markers, register the
manualmarker inpytest.ini(or the equivalent config) with something like:
markers = manual: tests that are intended as manual reproducers and not run in automated test suites.
| 'location-ids': smart_proxy_location.id, | ||
| } | ||
| ) | ||
| breakpoint() # noqa |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Unconditional breakpoint() will block automated test runs; gate or remove it.
This breakpoint() will hang CI and other automated test runs if the test is executed outside the narrow repro scenario. Please either remove it once the regression test is in place, or guard it behind an explicit flag (e.g. an env var or pytest option) so normal test runs are unaffected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem Statement
not to be merged!
Solution
This should help reproduce the issue for verification of theforeman/foreman#10952
Summary by Sourcery
Tests: