tests: add @payload_type decorator for DNF VM provisioning#1310
Draft
KKoukiou wants to merge 1 commit into
Draft
tests: add @payload_type decorator for DNF VM provisioning#1310KKoukiou wants to merge 1 commit into
KKoukiou wants to merge 1 commit into
Conversation
That replaces the payload_type in the provision attribute. The custom provision is incompatible with non-destructive tests. This fixes the Payload Basic test being skipped with: "Cannot provision multiple machines if a specific machine address is specified"
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
setUp, thepayload_typedecorator will overwrite any existingpayload_typeinself.provisionfor all machines; if mixed payloads or preconfigured payloads are ever needed, consider only setting it when not already present or scoping it to a specific machine. - The
payload_typedecorator currently lowercases and blindly assigns the mode; adding simple validation (e.g., restricting to known payload types likeliveimg/dnf) would help catch typos or unsupported values early.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `setUp`, the `payload_type` decorator will overwrite any existing `payload_type` in `self.provision` for all machines; if mixed payloads or preconfigured payloads are ever needed, consider only setting it when not already present or scoping it to a specific machine.
- The `payload_type` decorator currently lowercases and blindly assigns the mode; adding simple validation (e.g., restricting to known payload types like `liveimg`/`dnf`) would help catch typos or unsupported values early.
## Individual Comments
### Comment 1
<location path="test/anacondalib.py" line_range="70-75" />
<code_context>
def setUp(self):
method = getattr(self, self._testMethodName)
+ payload_type = getattr(method, "payload_type", None)
+ if payload_type and not self.is_nondestructive():
+ provision = dict(self.provision or {"machine1": {}})
+ self.provision = {
+ key: {**dict(opts), "payload_type": payload_type}
+ for key, opts in provision.items()
+ }
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests that exercise the new `payload_type` setup behavior (destructive vs. non-destructive and default provision).
The new `setUp` logic is central to payload behavior but isn't directly tested. Please add tests that:
- For a destructive test decorated with `@payload_type("dnf")`, assert that each machine in `self.provision` includes `{"payload_type": "dnf"}` while existing fields (e.g. `memory`, `image`) remain unchanged.
- For a non-destructive test (`is_nondestructive()` returns `True`), assert that `@payload_type(...)` does not modify `self.provision`.
- When `self.provision` starts as `None`, assert that the implicit `{"machine1": {}}` default is used and receives the expected `payload_type`.
A focused test (or helper) around `AnacondaTestCase.setUp` would help prevent regressions in decorator-driven provisioning.
Suggested implementation:
```python
def setUp(self):
method = getattr(self, self._testMethodName)
payload_type = getattr(method, "payload_type", None)
if payload_type and not self.is_nondestructive():
provision = dict(self.provision or {"machine1": {}})
self.provision = {
key: {**dict(opts), "payload_type": payload_type}
for key, opts in provision.items()
}
class _PayloadTypeTestCase(AnacondaTestCase):
"""Helper TestCase to exercise payload_type-driven provisioning."""
def is_nondestructive(self):
# Default to destructive; individual tests can override via subclassing
return False
class PayloadTypeSetupTests(unittest.TestCase):
def test_destructive_test_applies_payload_type_to_all_machines(self):
class DestructiveCase(_PayloadTypeTestCase):
@payload_type("dnf")
def test_method(self):
pass
case = DestructiveCase("test_method")
case.provision = {
"machine1": {"memory": "2G", "image": "base.img"},
"machine2": {"memory": "4G", "image": "advanced.img"},
}
case.setUp()
self.assertEqual(
{
"machine1": {
"memory": "2G",
"image": "base.img",
"payload_type": "dnf",
},
"machine2": {
"memory": "4G",
"image": "advanced.img",
"payload_type": "dnf",
},
},
case.provision,
)
def test_nondestructive_test_does_not_modify_provision(self):
class NonDestructiveCase(_PayloadTypeTestCase):
def is_nondestructive(self):
return True
@payload_type("dnf")
def test_method(self):
pass
original_provision = {
"machine1": {"memory": "2G"},
"machine2": {"memory": "4G"},
}
case = NonDestructiveCase("test_method")
case.provision = copy.deepcopy(original_provision)
case.setUp()
self.assertEqual(original_provision, case.provision)
def test_default_provision_used_when_none(self):
class DefaultProvisionCase(_PayloadTypeTestCase):
@payload_type("dnf")
def test_method(self):
pass
case = DefaultProvisionCase("test_method")
case.provision = None
case.setUp()
self.assertEqual(
{"machine1": {"payload_type": "dnf"}},
case.provision,
)
```
1. Ensure the following are imported at the top of `test/anacondalib.py` (or adjust to match existing import style/module layout):
- `import unittest` (likely already present)
- `import copy`
- `from anacondalib import AnacondaTestCase, payload_type` (or the appropriate module path where `AnacondaTestCase` and `payload_type` are defined).
2. If `AnacondaTestCase` already has its own tests or base helpers in this file, you may want to place `_PayloadTypeTestCase` and `PayloadTypeSetupTests` near them for coherence.
3. There appear to be two `setUp` definitions in the snippet you shared. The older, no-op `setUp` (`def setUp(self): method = getattr(self, self._testMethodName)`) should likely be removed to avoid confusion; keep only the payload-aware implementation.
</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
+70
to
+75
| payload_type = getattr(method, "payload_type", None) | ||
| if payload_type and not self.is_nondestructive(): | ||
| provision = dict(self.provision or {"machine1": {}}) | ||
| self.provision = { | ||
| key: {**dict(opts), "payload_type": payload_type} | ||
| for key, opts in provision.items() |
There was a problem hiding this comment.
suggestion (testing): Add tests that exercise the new payload_type setup behavior (destructive vs. non-destructive and default provision).
The new setUp logic is central to payload behavior but isn't directly tested. Please add tests that:
- For a destructive test decorated with
@payload_type("dnf"), assert that each machine inself.provisionincludes{"payload_type": "dnf"}while existing fields (e.g.memory,image) remain unchanged. - For a non-destructive test (
is_nondestructive()returnsTrue), assert that@payload_type(...)does not modifyself.provision. - When
self.provisionstarts asNone, assert that the implicit{"machine1": {}}default is used and receives the expectedpayload_type.
A focused test (or helper) around AnacondaTestCase.setUp would help prevent regressions in decorator-driven provisioning.
Suggested implementation:
def setUp(self):
method = getattr(self, self._testMethodName)
payload_type = getattr(method, "payload_type", None)
if payload_type and not self.is_nondestructive():
provision = dict(self.provision or {"machine1": {}})
self.provision = {
key: {**dict(opts), "payload_type": payload_type}
for key, opts in provision.items()
}
class _PayloadTypeTestCase(AnacondaTestCase):
"""Helper TestCase to exercise payload_type-driven provisioning."""
def is_nondestructive(self):
# Default to destructive; individual tests can override via subclassing
return False
class PayloadTypeSetupTests(unittest.TestCase):
def test_destructive_test_applies_payload_type_to_all_machines(self):
class DestructiveCase(_PayloadTypeTestCase):
@payload_type("dnf")
def test_method(self):
pass
case = DestructiveCase("test_method")
case.provision = {
"machine1": {"memory": "2G", "image": "base.img"},
"machine2": {"memory": "4G", "image": "advanced.img"},
}
case.setUp()
self.assertEqual(
{
"machine1": {
"memory": "2G",
"image": "base.img",
"payload_type": "dnf",
},
"machine2": {
"memory": "4G",
"image": "advanced.img",
"payload_type": "dnf",
},
},
case.provision,
)
def test_nondestructive_test_does_not_modify_provision(self):
class NonDestructiveCase(_PayloadTypeTestCase):
def is_nondestructive(self):
return True
@payload_type("dnf")
def test_method(self):
pass
original_provision = {
"machine1": {"memory": "2G"},
"machine2": {"memory": "4G"},
}
case = NonDestructiveCase("test_method")
case.provision = copy.deepcopy(original_provision)
case.setUp()
self.assertEqual(original_provision, case.provision)
def test_default_provision_used_when_none(self):
class DefaultProvisionCase(_PayloadTypeTestCase):
@payload_type("dnf")
def test_method(self):
pass
case = DefaultProvisionCase("test_method")
case.provision = None
case.setUp()
self.assertEqual(
{"machine1": {"payload_type": "dnf"}},
case.provision,
)- Ensure the following are imported at the top of
test/anacondalib.py(or adjust to match existing import style/module layout):import unittest(likely already present)import copyfrom anacondalib import AnacondaTestCase, payload_type(or the appropriate module path whereAnacondaTestCaseandpayload_typeare defined).
- If
AnacondaTestCasealready has its own tests or base helpers in this file, you may want to place_PayloadTypeTestCaseandPayloadTypeSetupTestsnear them for coherence. - There appear to be two
setUpdefinitions in the snippet you shared. The older, no-opsetUp(def setUp(self): method = getattr(self, self._testMethodName)) should likely be removed to avoid confusion; keep only the payload-aware implementation.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a @payload_type decorator to allow specifying the installer VM payload type at the test method level, updating anacondalib.py and migrating existing tests to this new pattern. Feedback highlights a missing propagation of the payload_type to the machine instance for non-destructive tests, which may cause issues with certain helpers. Additionally, a simplification was suggested for the dictionary comprehension in the setUp method to remove redundant dict() calls.
| def setUp(self): | ||
| method = getattr(self, self._testMethodName) | ||
| payload_type = getattr(method, "payload_type", None) | ||
| if payload_type and not self.is_nondestructive(): |
There was a problem hiding this comment.
This implementation correctly avoids modifying self.provision for non-destructive tests to prevent infrastructure errors. However, it fails to propagate the payload_type to the machine instance in those cases. Helpers like Installer and resetPayloadDNF rely on self.machine.payload_type being set. For non-destructive tests, you should ensure self.machine.payload_type = payload_type is called after super().setUp() (around line 101), as self.machine is initialized there.
Comment on lines
+72
to
+76
| provision = dict(self.provision or {"machine1": {}}) | ||
| self.provision = { | ||
| key: {**dict(opts), "payload_type": payload_type} | ||
| for key, opts in provision.items() | ||
| } |
There was a problem hiding this comment.
The dict() calls are redundant here. Dictionary unpacking {**opts} already creates a new dictionary, and provision is only used for iteration, so a shallow copy of self.provision is not needed before the comprehension.
Suggested change
| provision = dict(self.provision or {"machine1": {}}) | |
| self.provision = { | |
| key: {**dict(opts), "payload_type": payload_type} | |
| for key, opts in provision.items() | |
| } | |
| provision = self.provision or {"machine1": {}} | |
| self.provision = { | |
| key: {**opts, "payload_type": payload_type} | |
| for key, opts in provision.items() | |
| } |
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.
That replaces the payload_type in the provision attribute. The custom provision is incompatible with non-destructive tests.
This fixes the Payload Basic test being skipped with: "Cannot provision multiple machines if a specific machine address is specified"
https://cockpit-ci-logs.s3.us-east-1.amazonaws.com/pull-1151-de40b6ef-20260520-073235-fedora-rawhide-boot-dnf/log.html