test clearing the host_owner setting after user deletion #19968

pondrejk · 2025-10-14T10:02:04Z

Problem Statement

to cover theforeman/foreman#10719

Solution

Related Issues

sourcery-ai

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

Consider capturing and restoring the original host_owner setting in teardown to ensure test isolation and avoid side effects on other tests.
Rename test_positive_host_owner to something more descriptive (e.g., test_host_owner_setting_cleared_on_user_deletion) to clearly convey the behavior under test.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- Consider capturing and restoring the original host_owner setting in teardown to ensure test isolation and avoid side effects on other tests.
- Rename test_positive_host_owner to something more descriptive (e.g., test_host_owner_setting_cleared_on_user_deletion) to clearly convey the behavior under test.

## Individual Comments

### Comment 1
<location> `tests/foreman/cli/test_user.py:585-588` </location>
<code_context>
+        :expectedresults: host_owner is cleared when the assigned user is deleted
+
+        """
+        user = target_sat.cli_factory.user()
+        target_sat.cli.Settings.set({'name': "host_owner", 'value': user['login']})
+        host_owner = target_sat.cli.Settings.list({'search': 'name=host_owner'})[0]
+        assert host_owner['value'] == user['login']
+        target_sat.cli.User.delete({'id': user['id']})
+        host_owner = target_sat.cli.Settings.list({'search': 'name=host_owner'})[0]
</code_context>

<issue_to_address>
**suggestion (testing):** Add cleanup for created user and settings to avoid test pollution.

Please add teardown steps or use fixtures to remove the created user and reset the host_owner setting after the test completes to prevent residual state from affecting other tests.
</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.}

sourcery-ai · 2025-10-14T10:02:51Z

tests/foreman/cli/test_user.py

+        user = target_sat.cli_factory.user()
+        target_sat.cli.Settings.set({'name': "host_owner", 'value': user['login']})
+        host_owner = target_sat.cli.Settings.list({'search': 'name=host_owner'})[0]
+        assert host_owner['value'] == user['login']


suggestion (testing): Add cleanup for created user and settings to avoid test pollution.

Please add teardown steps or use fixtures to remove the created user and reset the host_owner setting after the test completes to prevent residual state from affecting other tests.

ekohl · 2025-10-14T12:37:39Z

tests/foreman/cli/test_user.py

+
+        """
+        user = target_sat.cli_factory.user()
+        target_sat.cli.Settings.set({'name': "host_owner", 'value': user['login']})


I think you need to set the type, whether it's Users or Usergroups. Something like:

Suggested change

target_sat.cli.Settings.set({'name': "host_owner", 'value': user['login']})

target_sat.cli.Settings.set({'name': "host_owner", 'value': f'{user['login']}-Users'})

seems it must be user['id']

pondrejk · 2025-10-14T14:41:54Z

trigger: test-robottelo
pytest: tests/foreman/cli/test_user.py -k test_positive_host_owner
theforeman:
    foreman: 10719

Satellite-QE · 2025-10-14T15:10:00Z

PRT Result

Build Number: 13162
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/cli/test_user.py -k test_positive_host_owner --external-logging
Test Result : ========== 1 passed, 25 deselected, 5 warnings in 1219.59s (0:20:19) ===========

chris1984

Looks fine based on the PR, left a suggestion but i'm not an expert with Python.

chris1984 · 2025-10-15T19:34:07Z

tests/foreman/cli/test_user.py

+        host_owner = target_sat.cli.Settings.list({'search': 'name=host_owner'})[0]
+        assert host_owner['value'] == f'{user["id"]}-Users'
+        target_sat.cli.User.delete({'id': user['id']})
+        host_owner = target_sat.cli.Settings.list({'search': 'name=host_owner'})[0]


What do you think of:

host_owner = target_sat.cli.Settings.list({'search': 'name=host_owner'})[0] assert host_owner['value'] == '', f'Host owner is not cleared: {host_owner["value"]}'

this is perfectly fine, but I'd like to clear the value if the assertion fails, hence the try except block

ekohl · 2025-10-15T21:08:36Z

tests/foreman/cli/test_user.py

+        try:
+            assert host_owner['value'] == ''
+        except AssertionError:
+            target_sat.cli.Settings.set({'name': "host_owner", 'value': ''})


Shouldn't this be under a finally so it's cleared regardless of whether an exception is raised or not?

hm, should be fine if it's not raised, though if there's some other exception kind finally is indeed better

ekohl · 2025-10-15T21:09:33Z

tests/foreman/cli/test_user.py

+        :expectedresults: host_owner is cleared when the assigned user is deleted
+
+        """
+        user = target_sat.cli_factory.user()


This is using the CLI, but wouldn't using the plain API be easier and faster?

would be faster to execute, not sure if easier as the factory creates names for us. There are no strict rules for this, but since we are in the cli module for user, I opt to create user through cli :)

adamruzicka · 2025-10-16T14:14:00Z

trigger: test-robottelo
pytest: tests/foreman/cli/test_user.py -k test_positive_host_owner
theforeman:
  foreman: 10719

Satellite-QE · 2025-10-16T14:45:24Z

PRT Result

Build Number: 13216
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/cli/test_user.py -k test_positive_host_owner --external-logging
Test Result : ========== 1 passed, 25 deselected, 5 warnings in 1151.44s (0:19:11) ===========

pondrejk · 2025-10-20T08:49:11Z

trigger: test-robottelo
pytest: tests/foreman/cli/test_user.py -k test_positive_host_owner

Satellite-QE · 2025-10-20T09:13:14Z

PRT Result

Build Number: 13233
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/cli/test_user.py -k test_positive_host_owner --external-logging
Test Result : =========== 1 failed, 25 deselected, 5 warnings in 790.30s (0:13:10) ===========

pondrejk · 2025-10-20T12:04:45Z

ah, prt won't pass until snap is out

pondrejk self-assigned this Oct 14, 2025

pondrejk requested a review from a team as a code owner October 14, 2025 10:02

pondrejk added the No-CherryPick PR doesnt need CherryPick to previous branches label Oct 14, 2025

sourcery-ai bot reviewed Oct 14, 2025

View reviewed changes

pondrejk mentioned this pull request Oct 14, 2025

Fixes #38819 - Clear host_owner on user[group] destroy theforeman/foreman#10719

Merged

ekohl reviewed Oct 14, 2025

View reviewed changes

pondrejk force-pushed the hostowner branch from 714de91 to 00114de Compare October 14, 2025 14:31

Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Oct 14, 2025

pondrejk force-pushed the hostowner branch from 00114de to 31ea6bd Compare October 15, 2025 07:32

Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label Oct 15, 2025

chris1984 reviewed Oct 15, 2025

View reviewed changes

ekohl reviewed Oct 15, 2025

View reviewed changes

Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Oct 16, 2025

test clearing the host_owner setting after user deletion

a4921ad

pondrejk force-pushed the hostowner branch from 31ea6bd to a4921ad Compare October 20, 2025 08:44

Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label Oct 20, 2025

Satellite-QE added the PRT-Failed Indicates that latest PRT run is failed for the PR label Oct 20, 2025

	target_sat.cli.Settings.set({'name': "host_owner", 'value': user['login']})
	target_sat.cli.Settings.set({'name': "host_owner", 'value': f'{user['login']}-Users'})

test clearing the host_owner setting after user deletion #19968

Are you sure you want to change the base?

test clearing the host_owner setting after user deletion #19968

Conversation

pondrejk commented Oct 14, 2025

Problem Statement

Solution

Related Issues

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

sourcery-ai bot Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

ekohl Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

pondrejk Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

pondrejk commented Oct 14, 2025

Uh oh!

Satellite-QE commented Oct 14, 2025

Uh oh!

chris1984 left a comment

Choose a reason for hiding this comment

Uh oh!

chris1984 Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

pondrejk Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekohl Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

pondrejk Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekohl Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

pondrejk Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

adamruzicka commented Oct 16, 2025

Uh oh!

Satellite-QE commented Oct 16, 2025

Uh oh!

pondrejk commented Oct 20, 2025

Uh oh!

Satellite-QE commented Oct 20, 2025

Uh oh!

pondrejk commented Oct 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pondrejk Oct 20, 2025 •

edited

Loading

pondrejk Oct 20, 2025 •

edited

Loading