Skip to content

Conversation

pondrejk
Copy link
Contributor

Problem Statement

this was consistently causing teardown issues, the fixtures were not a good fit for two parent scenario

Solution

doesn't seem right to make a one occurrence fixture, so handling teardown within test

Related Issues

@pondrejk pondrejk self-assigned this Oct 15, 2025
@pondrejk pondrejk requested a review from a team as a code owner October 15, 2025 15:00
@pondrejk pondrejk added CherryPick PR needs CherryPick to previous branches AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing Stream Introduced in or relating directly to Satellite Stream/Master 6.18.z Introduced in or relating directly to Satellite 6.18 labels Oct 15, 2025
@pondrejk
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/cli/test_location.py -k test_positive_update_parennt

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there - I've reviewed your changes - here's some feedback:

  • Initialize location, parent_location, and parent_location_2 before the try block (or guard their use) so that your finally cleanup can’t trigger an UnboundLocalError if creation fails.
  • Extract the repetitive delete-and-exists logic into a shared helper or pytest finalizer to reduce boilerplate and improve readability.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Initialize `location`, `parent_location`, and `parent_location_2` before the `try` block (or guard their use) so that your `finally` cleanup can’t trigger an UnboundLocalError if creation fails.
- Extract the repetitive delete-and-exists logic into a shared helper or pytest finalizer to reduce boilerplate and improve readability.

## Individual Comments

### Comment 1
<location> `tests/foreman/cli/test_location.py:363-364` </location>
<code_context>

</code_context>

<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))

<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.

Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals

Some ways to fix this:

* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.

> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.

Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>

### Comment 2
<location> `tests/foreman/cli/test_location.py:365-366` </location>
<code_context>

</code_context>

<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))

<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.

Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals

Some ways to fix this:

* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.

> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.

Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>

### Comment 3
<location> `tests/foreman/cli/test_location.py:367-368` </location>
<code_context>

</code_context>

<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))

<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.

Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals

Some ways to fix this:

* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.

> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.

Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@sambible
Copy link
Contributor

trigger: test-robottelo
pytest: tests/foreman/cli/test_location.py -k test_positive_update_parent

Copy link
Contributor

@sambible sambible left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK pending PRT.

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 13220
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/cli/test_location.py -k test_positive_update_parent --external-logging
Test Result : =========== 1 passed, 7 deselected, 5 warnings in 1783.41s (0:29:43) ===========

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Oct 16, 2025
@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label Oct 20, 2025
@pondrejk
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/cli/test_location.py -k test_positive_update_parent

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 13234
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/cli/test_location.py -k test_positive_update_parent --external-logging
Test Result : =========== 1 passed, 7 deselected, 5 warnings in 697.61s (0:11:37) ============

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Oct 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.18.z Introduced in or relating directly to Satellite 6.18 AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing CherryPick PR needs CherryPick to previous branches PRT-Passed Indicates that latest PRT run is passed for the PR Stream Introduced in or relating directly to Satellite Stream/Master

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants