Use module-scoped fixtures for new_upgrades/test_classparameter.py#21650
Draft
tpapaioa wants to merge 1 commit into
Draft
Use module-scoped fixtures for new_upgrades/test_classparameter.py#21650tpapaioa wants to merge 1 commit into
tpapaioa wants to merge 1 commit into
Conversation
tpapaioa
added
Upgrades
Contributor
Reviewer's GuideRefactors the puppet class parameter upgrade tests to use a module-scoped shared Satellite fixture and a single setup run, while parameterizing the assertions over a static data table to improve runtime and parallel behavior. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Author
|
Collaborator
|
PRT Result |
Satellite-QE
added
the
PRT-Passed
![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 1 issue, and left some high level feedback:
- The
test_post_puppet_class_parameter_data_and_typeparametrization can be simplified by usingenumerate(VALID_SC_PARAMS_DATA, start=1)directly and generating more descriptiveids(e.g. based onsc_type) rather thanrange(...), which will also make test output easier to read. - The module-scoped setup fixture calls private attributes/methods (
_swap_nailgun,_session); if there is a public helper or API to reset the client/session for the upgraded Satellite, it would be more robust to use that instead of relying on private internals.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `test_post_puppet_class_parameter_data_and_type` parametrization can be simplified by using `enumerate(VALID_SC_PARAMS_DATA, start=1)` directly and generating more descriptive `ids` (e.g. based on `sc_type`) rather than `range(...)`, which will also make test output easier to read.
- The module-scoped setup fixture calls private attributes/methods (`_swap_nailgun`, `_session`); if there is a public helper or API to reset the client/session for the upgraded Satellite, it would be more robust to use that instead of relying on private internals.
## Individual Comments
### Comment 1
<location path="tests/new_upgrades/test_classparameter.py" line_range="94-96" />
<code_context>
@pytest.mark.puppet_upgrades
+@pytest.mark.parametrize(
+ 'sc_data',
+ [p for p in enumerate(VALID_SC_PARAMS_DATA)],
+ ids=range(1, len(VALID_SC_PARAMS_DATA) + 1),
+)
</code_context>
<issue_to_address>
**suggestion (testing):** Parametrization over `enumerate(VALID_SC_PARAMS_DATA)` could be made clearer and more descriptive for test readability
`sc_data` is currently an `(index, my_param)` tuple from `enumerate(VALID_SC_PARAMS_DATA)`, with numeric `ids` from `range(1, ...)`. This works but makes parameter handling and failure output harder to interpret.
Consider either:
- Parametrizing two arguments explicitly:
```python
@pytest.mark.parametrize(
"index, sc_param",
[(i, data) for i, data in enumerate(VALID_SC_PARAMS_DATA)],
ids=[f"{i+1}-{data['sc_type']}" for i, data in enumerate(VALID_SC_PARAMS_DATA)],
)
```
- Or using `pytest.param` with explicit ids based on `sc_type`.
Both approaches make CI failures self-describing without mapping numeric ids back to the constant list.
Suggested implementation:
```python
@pytest.mark.puppet_upgrades
@pytest.mark.parametrize(
"index, sc_param",
[(i, data) for i, data in enumerate(VALID_SC_PARAMS_DATA)],
ids=[f"{i+1}-{data['sc_type']}" for i, data in enumerate(VALID_SC_PARAMS_DATA)],
)
```
1. Update the corresponding test function signature to accept `index` and `sc_param` instead of the previous single `sc_data` argument (if it existed), e.g. `def test_...(index, sc_param, ...)`.
2. Adjust any test body references that used `sc_data` to instead use `index` or `sc_param` as appropriate, for example:
- If you previously unpacked `sc_data` as `(idx, param)`, you can now use `index` and `sc_param` directly.
- Any logging or assertion messages can now include `sc_param["sc_type"]` to take advantage of the more descriptive parametrization.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+94
to
+96
| @pytest.mark.parametrize( | ||
| 'sc_data', | ||
| [p for p in enumerate(VALID_SC_PARAMS_DATA)], |
Contributor
There was a problem hiding this comment.
suggestion (testing): Parametrization over enumerate(VALID_SC_PARAMS_DATA) could be made clearer and more descriptive for test readability
sc_data is currently an (index, my_param) tuple from enumerate(VALID_SC_PARAMS_DATA), with numeric ids from range(1, ...). This works but makes parameter handling and failure output harder to interpret.
Consider either:
- Parametrizing two arguments explicitly:
@pytest.mark.parametrize( "index, sc_param", [(i, data) for i, data in enumerate(VALID_SC_PARAMS_DATA)], ids=[f"{i+1}-{data['sc_type']}" for i, data in enumerate(VALID_SC_PARAMS_DATA)], )
- Or using
pytest.paramwith explicit ids based onsc_type.
Both approaches make CI failures self-describing without mapping numeric ids back to the constant list.
Suggested implementation:
@pytest.mark.puppet_upgrades
@pytest.mark.parametrize(
"index, sc_param",
[(i, data) for i, data in enumerate(VALID_SC_PARAMS_DATA)],
ids=[f"{i+1}-{data['sc_type']}" for i, data in enumerate(VALID_SC_PARAMS_DATA)],
)- Update the corresponding test function signature to accept
indexandsc_paraminstead of the previous singlesc_dataargument (if it existed), e.g.def test_...(index, sc_param, ...). - Adjust any test body references that used
sc_datato instead useindexorsc_paramas appropriate, for example:- If you previously unpacked
sc_dataas(idx, param), you can now useindexandsc_paramdirectly. - Any logging or assertion messages can now include
sc_param["sc_type"]to take advantage of the more descriptive parametrization.
- If you previously unpacked
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
2 participants
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
Solution
Refactor the puppet class parameter tests to use module-scoped fixtures, where all of the puppet imports and initial class parameter setup is done first on a single Satellite, which is then used by all of the tests, whether run in parallel or sequentially by xdist. (See the tests and fixtures in tests/new_upgrades/test_pulp.py for an example of how this is already done.)
Related Issues
SAT-40414
Summary by Sourcery
Refactor puppet class parameter upgrade tests to use module-scoped fixtures and shared setup to reduce Satellite deployments and speed up execution.
Enhancements:
Tests: