-
Notifications
You must be signed in to change notification settings - Fork 127
Added test case to verify capsule upgradation with hammer installed #19933
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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:
- Instead of hardcoding “satellite-capsule-6.19” in the post-upgrade check, parameterize the expected capsule version from settings or fixtures so it doesn’t break on version bumps.
- Consider extracting repeated command execution and assertion logic (e.g. yum repo creation, package installation checks) into helper functions or fixtures to reduce duplication and improve readability.
- Double-check the pytest.mark.post_upgrade(depend_on=…) usage to ensure it correctly references the pre-upgrade test (string or marker) so the dependency is actually enforced.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of hardcoding “satellite-capsule-6.19” in the post-upgrade check, parameterize the expected capsule version from settings or fixtures so it doesn’t break on version bumps.
- Consider extracting repeated command execution and assertion logic (e.g. yum repo creation, package installation checks) into helper functions or fixtures to reduce duplication and improve readability.
- Double-check the pytest.mark.post_upgrade(depend_on=…) usage to ensure it correctly references the pre-upgrade test (string or marker) so the dependency is actually enforced.
## Individual Comments
### Comment 1
<location> `tests/upgrades/test_capsulecontent.py:296-297` </location>
<code_context>
+ assert result.status == 0
+ result = module_capsule_configured.execute('yum install rubygem-hammer_cli_katello -y')
+ assert result.status == 0
+ result = module_capsule_configured.execute('rpm -qa | grep -i "hammer"')
+ assert "hammer_cli_katello" in result.stdout
+ assert result.status == 0
+
</code_context>
<issue_to_address>
**suggestion (testing):** Suggestion to verify hammer CLI functionality, not just installation.
Consider adding a test that runs a basic hammer CLI command to ensure it works as expected after installation.
```suggestion
result = module_capsule_configured.execute('yum install rubygem-hammer_cli_katello -y')
assert result.status == 0
# Verify hammer CLI is functional
result = module_capsule_configured.execute('hammer --version')
assert result.status == 0
assert "Hammer CLI" in result.stdout or "hammer" in result.stdout.lower()
```
</issue_to_address>
### Comment 2
<location> `tests/upgrades/test_capsulecontent.py:317-318` </location>
<code_context>
+ 1. The capsule should be upgraded.
+ 2. The hammer should be installed on Capsule.
+ """
+ result = module_capsule_configured.execute('rpm -qa | grep -i "capsule"')
+ assert "satellite-capsule-6.19" in result.stdout
+ assert result.status == 0
+ result = module_capsule_configured.execute('rpm -qa | grep -i "hammer"')
</code_context>
<issue_to_address>
**question (testing):** Question about hardcoded version check for capsule upgrade.
If the expected version changes in future releases, this test may fail. Please consider parameterizing the version or using a more flexible check.
</issue_to_address>
### Comment 3
<location> `tests/upgrades/test_capsulecontent.py:302-303` </location>
<code_context>
+ assert "hammer_cli_katello" in result.stdout
+ assert result.status == 0
+
+ @pytest.mark.post_upgrade(depend_on=test_pre_upgrade_check_capsule_hammer)
+ def test_post_upgrade_check_capsule_hammer(self, module_capsule_configured):
+ """Post-upgrade scenario that verifies if the hammer is installed on Capsule
+ in the post-upgrade.
</code_context>
<issue_to_address>
**suggestion (testing):** Suggestion to add validation for repo sync after upgrade.
Please add a check to confirm the utils repository and its RPMs remain available and properly synced after the upgrade.
Suggested implementation:
```python
result = module_capsule_configured.execute('yum install rubygem-hammer_cli_katello -y')
assert result.status == 0
result = module_capsule_configured.execute('rpm -qa | grep -i "hammer"')
assert "hammer_cli_katello" in result.stdout
assert result.status == 0
# Validate utils repository and RPMs remain available and properly synced after upgrade
result = module_capsule_configured.execute('yum repolist all')
assert "Utils Repository" in result.stdout
assert result.status == 0
# Check for a known RPM from the utils repo (replace 'some-utils-rpm' with actual RPM name if needed)
result = module_capsule_configured.execute('yum list available | grep -i "some-utils-rpm"')
assert result.status == 0
assert "some-utils-rpm" in result.stdout
# Try installing the RPM to confirm availability
result = module_capsule_configured.execute('yum install some-utils-rpm -y')
assert result.status == 0
@pytest.mark.post_upgrade(depend_on=test_pre_upgrade_check_capsule_hammer)
def test_post_upgrade_check_capsule_hammer(self, module_capsule_configured):
```
- Replace "some-utils-rpm" with the actual RPM name from the utils repository that should be validated.
- If the repository name in `yum repolist all` output differs, adjust "Utils Repository" accordingly.
- If you have a helper for repo sync status, consider using it for more robust validation.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@pytest.mark.post_upgrade(depend_on=test_pre_upgrade_check_capsule_hammer) | ||
def test_post_upgrade_check_capsule_hammer(self, module_capsule_configured): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Suggestion to add validation for repo sync after upgrade.
Please add a check to confirm the utils repository and its RPMs remain available and properly synced after the upgrade.
Suggested implementation:
result = module_capsule_configured.execute('yum install rubygem-hammer_cli_katello -y')
assert result.status == 0
result = module_capsule_configured.execute('rpm -qa | grep -i "hammer"')
assert "hammer_cli_katello" in result.stdout
assert result.status == 0
# Validate utils repository and RPMs remain available and properly synced after upgrade
result = module_capsule_configured.execute('yum repolist all')
assert "Utils Repository" in result.stdout
assert result.status == 0
# Check for a known RPM from the utils repo (replace 'some-utils-rpm' with actual RPM name if needed)
result = module_capsule_configured.execute('yum list available | grep -i "some-utils-rpm"')
assert result.status == 0
assert "some-utils-rpm" in result.stdout
# Try installing the RPM to confirm availability
result = module_capsule_configured.execute('yum install some-utils-rpm -y')
assert result.status == 0
@pytest.mark.post_upgrade(depend_on=test_pre_upgrade_check_capsule_hammer)
def test_post_upgrade_check_capsule_hammer(self, module_capsule_configured):
- Replace "some-utils-rpm" with the actual RPM name from the utils repository that should be validated.
- If the repository name in
yum repolist all
output differs, adjust "Utils Repository" accordingly. - If you have a helper for repo sync status, consider using it for more robust validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid this test needs to go into tests/new_upgrades
and be updated accordingly.
The legacy tests/upgrades
are not to be run in CI.
trigger: test-robottelo |
81745f8
to
e2f6967
Compare
I have added to new_upgrades now. |
trigger: test-robottelo |
|
|
PRT Result
|
e2f6967
to
7117b11
Compare
I have added it to |
7117b11
to
ed96980
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposed a couple of nit-picks, otherwise looks good.
repo_config = f"[utils]\nname=Utils Repository\nbaseurl={settings.repos.satutils_repo}\nenabled=1\ngpgcheck=0\n" | ||
result = module_capsule_configured.execute( | ||
f'cat > /etc/yum.repos.d/utils.repo << EOF\n{repo_config}\nEOF' | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the create_custom_repos helper here and lower.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created Custom Repo using create_custom_repos
repo='capsule', | ||
product='capsule', | ||
release='stream', | ||
os_release=9, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we could use os_version (os_release=module_capsule_configured.os_version.major
) so the test becomes future-proof when SAT and CAPS are based on RHEL10+.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replaced with os_release=module_capsule_configured.os_version.major
assert "Upgrade finished." in result.stdout | ||
result = module_capsule_configured.execute('rpm -qa | grep -i "capsule"') | ||
assert result.status == 0 | ||
assert "satellite-capsule-6.19" in result.stdout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar here, we should omit version hard-coding. We could use settings.server.version.release
or just check it matches the original (pre-upgrade) version, since we are using the stream repos.
""" | ||
|
||
|
||
@pytest.mark.upgrade |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This decorator qualifies the test to being additionally run in the "upgade-all-tiers" pipeline (aka on a setup upgraded from n-1 version), apart from being run in the standard CI.
Is it something you intentionally want, or you just wanted to declare this test is related to upgrade? Then I would remove the decorator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to declare this test is related to capsule upgrade. Should i use any other decorator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, I would just note it in the docstring and/or test name (you have that done already).
And please remove the @pytest.mark.upgrade
decorator. Based on this decorator the upgrade all-tiers tests are collected, see the stream Upgrade > sat-stream-rhel9-ystream-upgrade-all-tiers job output in Jenkins how they are collected: py.test <some_options> tests/foreman -m upgrade
And I believe it was not intended in this case.
4. The capsule should be upgraded | ||
5. The hammer should be installed on Capsule | ||
:Verifies: SAT-37909 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This linked issue feels incorrect, can you double-check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, My bad :-) ; adding correct number.
result = module_capsule_configured.execute('rpm -qa | grep -i "hammer"') | ||
assert "hammer_cli_katello" in result.stdout | ||
assert result.status == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to validate something else here after hammer is installed like hammer ping
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scope is to upgrade with a package like Hammer, Not to test the functionality of hammer in this scenario. So it is not required.
capsule = dogfood_repository( | ||
ohsnap=settings.ohsnap, | ||
repo='capsule', | ||
product='capsule', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why this is needed for hammer install? but if you really need it I'd suggest downloading a repofile from ohsnapUtils here on capsule instead of manually creating repofile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, when you checkout module_capsule_configured fixture, the provided capsule instance should have capsule repofile present on it already, so this could be redundant step here to add same repos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No this code do not install hammer. but for capsule upgraded repos.
result = module_capsule_configured.execute('rpm -qa | grep -i "hammer"') | ||
assert "hammer_cli_katello" in result.stdout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use either single quote or double quotes for your tests for better readability and consistency. Single quotes are preferred
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
assert "Successfully updated satellite-maintain" in result.stdout | ||
result = module_capsule_configured.cli.Upgrade.run( | ||
options={ | ||
'whitelist': 'repositories-validate, non-rh-packages, repositories-setup, pulpcore-rpm-datarepair', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'whitelist': 'repositories-validate, non-rh-packages, repositories-setup, pulpcore-rpm-datarepair', | |
'whitelist': 'repositories-validate, repositories-setup, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pulpcore-rpm-datarepair
is required as mentioned in SAT-39207.
Problem Statement
There are no automated checks to verify Capsule Hammer functionality before and after an upgrade. Manual validation is time-consuming and prone to human error.
Solution
Added a new test class TestCapsuleHammer that validates Capsule Hammer in two phases:
Pre-upgrade: Confirms capsule configuration and baseline functionality before upgrade.
Post-upgrade: Runs dependent validation after upgrade to ensure functionality remains intact.
Related Issues