test: automate subnet CLI parameter scenarios

[Alleny244]

Problem Statement

Subnet CLI parameter tests were stubbed, so subnet set-parameter behavior was not covered in automation.

Solution

Implemented those tests: make_subnet → set_parameter → info (JSON) → assert → delete subnet; negative case expects CLIReturnCodeError and empty parameters.

Related Issues

[sourcery-ai]

Reviewer's Guide

Automates previously stubbed subnet CLI parameter tests by turning them into real, data-driven CLI scenarios that create subnets, set parameters via Subnet.set_parameter, and verify persistence and validation via Subnet.info and error handling.

File-Level Changes

Change	Details	Files
Implement real positive test for creating a subnet parameter with multiple comma-separated values and validating stored value.	Remove stub marker and add module_target_sat fixture parameter to the test. Create a subnet using cli_factory.make_subnet(). Generate a random parameter name and three random value components, join them with commas, and set the parameter via Subnet.set_parameter. Fetch subnet details with Subnet.info(..., output_format='json'), locate the created parameter, and assert the stored value matches the comma-separated string. Clean up by deleting the created subnet via Subnet.delete.	tests/foreman/cli/test_subnet.py
Implement real positive test for creating a subnet parameter whose name contains multiple segments separated by valid separators and verifying persistence.	Remove stub marker and add module_target_sat fixture parameter to the test. Create a subnet via cli_factory.make_subnet(). Build a parameter name from three random components joined by '/', with a random value, and set it via Subnet.set_parameter. Retrieve subnet info in JSON, find the parameter by name, and assert the stored value matches the expected string. Delete the subnet after verification.	tests/foreman/cli/test_subnet.py
Implement real negative test ensuring subnet parameters with invalid separators in the name are rejected and not persisted.	Remove stub marker and add module_target_sat fixture parameter to the test. Create a subnet via cli_factory.make_subnet(). Construct a parameter name using space-separated segments and a random value. Use pytest.raises(CLIReturnCodeError) around Subnet.set_parameter to assert the CLI rejects the invalid name. Verify via Subnet.info that no parameters are stored on the subnet and delete the subnet.	tests/foreman/cli/test_subnet.py
Implement real upgrade-marked positive test that a subnet can have multiple parameters set and all are persisted correctly.	Remove stub marker and add module_target_sat fixture parameter to the test while keeping the @pytest.mark.upgrade marker. Create a subnet via cli_factory.make_subnet(). Generate three (name, value) pairs and call Subnet.set_parameter for each to add multiple parameters to the same subnet. Fetch subnet info in JSON, assert the number of parameters matches the number created, and verify each parameter’s stored value matches the expected value. Delete the subnet after assertions.	tests/foreman/cli/test_subnet.py

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• Subnet creation and deletion logic is duplicated across the new tests; consider extracting a small helper/fixture to create a subnet and ensure it is always deleted (e.g., via a context manager or try/finally) to avoid leaks on failures.
• When locating parameters in params, you currently use if param_name in parameter['name']; if parameter names can be similar, switching to an equality check (==) would make the tests more precise and less brittle.
• In the negative separator test, asserting len(params) == 0 assumes there are no other parameters on the subnet; it would be more robust to assert specifically that no parameter with param_name exists in params.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- Subnet creation and deletion logic is duplicated across the new tests; consider extracting a small helper/fixture to create a subnet and ensure it is always deleted (e.g., via a context manager or `try/finally`) to avoid leaks on failures.
- When locating parameters in `params`, you currently use `if param_name in parameter['name']`; if parameter names can be similar, switching to an equality check (`==`) would make the tests more precise and less brittle.
- In the negative separator test, asserting `len(params) == 0` assumes there are no other parameters on the subnet; it would be more robust to assert specifically that no parameter with `param_name` exists in `params`.

## Individual Comments

### Comment 1
<location path="tests/foreman/cli/test_subnet.py" line_range="288-289" />
<code_context>
+    )
+    subnet_info = module_target_sat.cli.Subnet.info({'id': subnet['id']}, output_format='json')
+    params = subnet_info['parameters']
+    param = next(parameter for parameter in params if param_name in parameter['name'])
+    stored = param['value']
+    stored_str = str(stored).strip()
</code_context>
<issue_to_address>
**suggestion:** Use exact name matching instead of substring matching when locating the created parameter

Using `if param_name in parameter['name']` can match the wrong parameter when names share substrings (e.g. `param`, `param-1`, `param-2`). To avoid false positives and ensure the test validates the intended parameter, use exact matching: `parameter['name'] == param_name`.

```suggestion
    params = subnet_info['parameters']
    param = next(parameter for parameter in params if parameter['name'] == param_name)
```
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion: Use exact name matching instead of substring matching when locating the created parameter

Using if param_name in parameter['name'] can match the wrong parameter when names share substrings (e.g. param, param-1, param-2). To avoid false positives and ensure the test validates the intended parameter, use exact matching: parameter['name'] == param_name.

Suggested change

	params = subnet_info['parameters']
	param = next(parameter for parameter in params if param_name in parameter['name'])
	params = subnet_info['parameters']
	param = next(parameter for parameter in params if parameter['name'] == param_name)

[Alleny244]

trigger: test-robottelo
pytest: tests/foreman/cli/test_subnet.py -k "multiple_values or multiple_names or invalid_separator or multiple_parameters"

[Satellite-QE]

PRT Result

Build Number: 15097
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/cli/test_subnet.py -k "multiple_values or multiple_names or invalid_separator or multiple_parameters" --external-logging
Test Result : ========== 4 passed, 35 deselected, 31 warnings in 882.35s (0:14:42) ===========

[evgeni]

I think I lean towards dropping these stubbed tests instead of implementing them, but not clicking "request changes" as I am not 100% sure.

[evgeni]

if the assert fails, this won't be executed, leaving the subnet behind and possibly interfering with other tests.

you can add a finalizer instead (will need to add the request fixture too):

Suggested change

	module_target_sat.cli.Subnet.delete({'id': subnet['id']})
	request.addfinalizer(lambda: module_target_sat.cli.Subnet.delete({'id': subnet['id']}))

[evgeni]

this BZ is long closed and doesn't really relate to the things that we're testing here (it was just marked here as the test would fail before the BZ was fixed), so I think we can remove this marker from all tests.

[evgeni]

I'm trying to understand what the intent was for this test in #4941. The name is "create_with_parameter_…" which implies the parameter should be already set during creation, but that's not possible, you can only set parameters on existing subnets. In #21127 you already removed test_positive_create_with_parameter as a duplicate of test_positive_subnet_CRUD_parameters and the only difference here is that the value contains a comma, which doesn't sound too useful to me?

One could interpret "multiple values" as "the value is an array" (we have parameter_type, which is not covered in tests for subnets at all), but then "multiple names" in the test below still makes no real sense?