Mark tests for RHEL6 client testing#20760
Mark tests for RHEL6 client testing#20760jameerpathan111 wants to merge 2 commits intoSatelliteQE:6.16.zfrom
Conversation
jameerpathan111
added
No-CherryPick
Reviewer's GuideExtend Robottelo RHEL client test support to include RHEL6 by broadening version markers and relaxing strict success expectations on registration/assertions for RHEL6, plus wiring RHEL6 client repositories into supportability and provisioning fixtures. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
jameerpathan111
changed the title
jameerpathan111
force-pushed
the
rhel6_client
branch
6 times, most recently
from
c54e358 to
c160325
Compare
jameerpathan111
force-pushed
the
rhel6_client
branch
2 times, most recently
from
10be053 to
954293a
Compare
jameerpathan111
changed the title
![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 4 issues, and left some high level feedback:
- There are several places where
os_version.majoris compared to the string'6'instead of the integer6(e.g. intest_positive_global_registration_end_to_endandtest_positive_export_import_consume_incremental_yum_repo), which will never evaluate as expected; align these checks to use a consistent type. - In a few tests you switched to
@pytest.mark.rhel_ver_match([settings.content_host.default_rhel_version])instead of usingrhel_ver_list(e.g. intests/foreman/ui/test_registration.pyandtests/foreman/ui/test_remoteexecution.py), which changes the marker semantics; verify these should berhel_ver_list([settings.content_host.default_rhel_version])for list-style selection.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There are several places where `os_version.major` is compared to the string `'6'` instead of the integer `6` (e.g. in `test_positive_global_registration_end_to_end` and `test_positive_export_import_consume_incremental_yum_repo`), which will never evaluate as expected; align these checks to use a consistent type.
- In a few tests you switched to `@pytest.mark.rhel_ver_match([settings.content_host.default_rhel_version])` instead of using `rhel_ver_list` (e.g. in `tests/foreman/ui/test_registration.py` and `tests/foreman/ui/test_remoteexecution.py`), which changes the marker semantics; verify these should be `rhel_ver_list([settings.content_host.default_rhel_version])` for list-style selection.
## Individual Comments
### Comment 1
<location> `conf/supportability.yaml:5` </location>
<code_context>
default_os_name: "RedHat"
rhel:
- versions: [7,'7_fips', 8, '8_fips', 9, '9_fips']
+ versions: [6, 7, '7_fips', 8, '8_fips', 9, '9_fips']
</code_context>
<issue_to_address>
**question:** Clarify whether a RHEL 6 FIPS variant should be represented in the support matrix
You added `6` but not `'6_fips'`, whereas 7–9 all include FIPS variants. If RHEL 6 FIPS is supported, please add `'6_fips'` for consistency; if it’s explicitly unsupported, this asymmetry is fine but should be intentional.
</issue_to_address>
### Comment 2
<location> `tests/foreman/ui/test_registration.py:425` </location>
<code_context>
-@pytest.mark.rhel_ver_match('8')
+@pytest.mark.rhel_ver_match([settings.content_host.default_rhel_version])
@pytest.mark.no_containers
def test_positive_parse_package_name_url(
</code_context>
<issue_to_address>
**issue (bug_risk):** Decorator `rhel_ver_match` is now passed a list instead of the expected string/regex pattern.
Elsewhere `@pytest.mark.rhel_ver_match` always receives a string/regex, and `@pytest.mark.rhel_ver_list` is the marker that takes lists. Passing a list here will likely break or bypass the marker logic. Please update this (and the similar changes below) to use `rhel_ver_list([settings.content_host.default_rhel_version])`, or keep `rhel_ver_match` with a string/regex, depending on what behavior you need.
</issue_to_address>
### Comment 3
<location> `tests/foreman/ui/test_registration.py:221` </location>
<code_context>
- f'Failed to register host: {rhel_contenthost.hostname}\n'
- f'StdOut: {res.stdout}\nStdErr: {res.stderr}'
- )
+ if rhel_contenthost.os_version.major != '6':
+ assert res.status == 0, (
+ f'Failed to register host: {rhel_contenthost.hostname}\n'
</code_context>
<issue_to_address>
**issue (bug_risk):** Comparing `os_version.major` to the string `'6'` is likely a type mismatch and defeats the RHEL6 special-casing.
Elsewhere `os_version.major` is compared as an int (`!= 6`), so this string comparison (`'6'`) will never match and the assertion will still run on RHEL6, defeating the intended special-casing. Please compare against the integer (`!= 6`) or otherwise align the types, and update any similar `'6'` comparisons (e.g. in `tests/foreman/cli/test_satellitesync.py`).
</issue_to_address>
### Comment 4
<location> `tests/foreman/cli/test_satellitesync.py:2788` </location>
<code_context>
- f'Failed to register host: {rhel_contenthost.hostname}\n'
- f'StdOut: {res.stdout}\nStdErr: {res.stderr}'
- )
+ if rhel_contenthost.os_version.major != '6':
+ assert res.status == 0, (
+ f'Failed to register host: {rhel_contenthost.hostname}\n'
</code_context>
<issue_to_address>
**issue (bug_risk):** Another instance of comparing `os_version.major` to the string `'6'` instead of an integer.
Because this uses `rhel_contenthost.os_version.major != '6'`, it will always be True if `os_version.major` is an int (as implied by other comparisons to `6`). That means the assertion still runs on RHEL6 and the intended relaxation doesn’t take effect. Please align this check with the rest of the suite (e.g. `!= 6` if numeric), or standardize on strings everywhere if the field is actually a string.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| default_os_name: "RedHat" | ||
| rhel: | ||
| versions: [7,'7_fips', 8, '8_fips', 9, '9_fips'] | ||
| versions: [6, 7, '7_fips', 8, '8_fips', 9, '9_fips'] |
There was a problem hiding this comment.
question: Clarify whether a RHEL 6 FIPS variant should be represented in the support matrix
You added 6 but not '6_fips', whereas 7–9 all include FIPS variants. If RHEL 6 FIPS is supported, please add '6_fips' for consistency; if it’s explicitly unsupported, this asymmetry is fine but should be intentional.
|
|
||
|
|
||
| @pytest.mark.rhel_ver_match('9') | ||
| @pytest.mark.rhel_ver_match([settings.content_host.default_rhel_version]) |
There was a problem hiding this comment.
issue (bug_risk): Decorator rhel_ver_match is now passed a list instead of the expected string/regex pattern.
Elsewhere @pytest.mark.rhel_ver_match always receives a string/regex, and @pytest.mark.rhel_ver_list is the marker that takes lists. Passing a list here will likely break or bypass the marker logic. Please update this (and the similar changes below) to use rhel_ver_list([settings.content_host.default_rhel_version]), or keep rhel_ver_match with a string/regex, depending on what behavior you need.
| # run curl | ||
| result = rhel_contenthost.execute(cmd) | ||
| assert result.status == 0 | ||
| if rhel_contenthost.os_version.major != '6': |
There was a problem hiding this comment.
issue (bug_risk): Comparing os_version.major to the string '6' is likely a type mismatch and defeats the RHEL6 special-casing.
Elsewhere os_version.major is compared as an int (!= 6), so this string comparison ('6') will never match and the assertion will still run on RHEL6, defeating the intended special-casing. Please compare against the integer (!= 6) or otherwise align the types, and update any similar '6' comparisons (e.g. in tests/foreman/cli/test_satellitesync.py).
| f'Failed to register host: {rhel_contenthost.hostname}\n' | ||
| f'StdOut: {res.stdout}\nStdErr: {res.stderr}' | ||
| ) | ||
| if rhel_contenthost.os_version.major != '6': |
There was a problem hiding this comment.
issue (bug_risk): Another instance of comparing os_version.major to the string '6' instead of an integer.
Because this uses rhel_contenthost.os_version.major != '6', it will always be True if os_version.major is an int (as implied by other comparisons to 6). That means the assertion still runs on RHEL6 and the intended relaxation doesn’t take effect. Please align this check with the rest of the suite (e.g. != 6 if numeric), or standardize on strings everywhere if the field is actually a string.
jameerpathan111
force-pushed
the
rhel6_client
branch
from
954293a to
2b8b911
Compare
There was a problem hiding this comment.
Pull request overview
This PR expands Robottelo’s client-side test selection to allow running scenarios against RHEL6 content hosts (Satellite 6.16 context), primarily by widening RHEL version markers and relaxing a few registration-related assertions for RHEL6 due to known behavior differences.
Changes:
- Add RHEL6 to the supported content host versions and introduce Satellite Client repo metadata for RHEL6 (ELS).
- Broaden many tests from fixed RHEL lists (e.g., 7/8/9) to “any supported version” and/or to the configured default RHEL content host version.
- Relax/skip specific registration assertions and switch some client commands to be more RHEL6-compatible (e.g.,
dnf→yumin a spot).
Reviewed changes
Copilot reviewed 67 out of 67 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/upgrades/test_subscription.py | Widens upgrade scenario client selection to more RHEL versions via marker change. |
| tests/upgrades/test_repository.py | Widens upgrade scenario client selection to more RHEL versions via marker change. |
| tests/upgrades/test_remoteexecution.py | Widens upgrade scenario client selection to more RHEL versions via marker change. |
| tests/upgrades/test_errata.py | Widens upgrade scenario client selection to more RHEL versions via marker change. |
| tests/upgrades/test_client.py | Switches upgrade client selection to settings.content_host.default_rhel_version. |
| tests/new_upgrades/test_subscription.py | Widens new-upgrade scenario client selection via marker change. |
| tests/new_upgrades/test_remoteexecution.py | Widens client selection and relaxes registration-output assertion on RHEL6. |
| tests/new_upgrades/test_client.py | Relaxes registration-output assertion on RHEL6. |
| tests/foreman/ui/test_rhcloud_inventory.py | Broadens RHEL marker selection for PIT coverage. |
| tests/foreman/ui/test_rhcloud_insights.py | Broadens RHEL marker selection for UI insights coverage. |
| tests/foreman/ui/test_repositories.py | Broadens RHEL marker selection; relaxes subscription assertion on RHEL6. |
| tests/foreman/ui/test_reporttemplates.py | Broadens RHEL marker selection; relaxes registration-output assertion on RHEL6. |
| tests/foreman/ui/test_remoteexecution.py | Uses default RHEL version for selection (via settings) and updates imports. |
| tests/foreman/ui/test_registration.py | Broadens selection and relaxes some assertions for RHEL6 registration behavior. |
| tests/foreman/ui/test_package.py | Switches RHEL marker to be driven by default RHEL version. |
| tests/foreman/ui/test_host.py | Broadens RHEL selection and relaxes some registration assertions for RHEL6. |
| tests/foreman/ui/test_fact.py | Relaxes registration assertion on RHEL6. |
| tests/foreman/ui/test_errata.py | Broadens selection; relaxes fixture error assertion and switches selection to default RHEL. |
| tests/foreman/ui/test_discoveredhost.py | Switches provisioning selection to default RHEL version. |
| tests/foreman/ui/test_computeresource_libvirt.py | Broadens provisioning selection to include RHEL6. |
| tests/foreman/ui/test_ansible.py | Broadens selection and relaxes registration assertions for RHEL6. |
| tests/foreman/ui/test_activationkey.py | Broadens selection and relaxes registration assertions for RHEL6. |
| tests/foreman/sys/test_capsule_loadbalancer.py | Relaxes registration assertions for RHEL6. |
| tests/foreman/longrun/test_remoteexecution.py | Switches longrun selection to default RHEL version. |
| tests/foreman/longrun/test_oscap.py | Adds RHEL6 to OSCAP longrun setup and broadens selection. |
| tests/foreman/endtoend/test_api_endtoend.py | Relaxes registration assertion for RHEL6. |
| tests/foreman/destructive/test_remoteexecution.py | Switches destructive selection to default RHEL version; updates imports. |
| tests/foreman/destructive/test_registration.py | Relaxes registration assertion for RHEL6. |
| tests/foreman/destructive/test_infoblox.py | Switches selection to default RHEL version. |
| tests/foreman/destructive/test_host.py | Broadens selection to include RHEL6. |
| tests/foreman/cli/test_satellitesync.py | Relaxes registration assertion for RHEL6. |
| tests/foreman/cli/test_repository.py | Relaxes registration assertion for RHEL6. |
| tests/foreman/cli/test_repositories.py | Broadens selection to include RHEL6. |
| tests/foreman/cli/test_reporttemplates.py | Broadens selection; relaxes registration assertion for RHEL6. |
| tests/foreman/cli/test_report.py | Broadens selection to include RHEL6. |
| tests/foreman/cli/test_remoteexecution.py | Broadens selection; relaxes registration assertion for RHEL6; adds explicit non-RHEL6 markers for pull-provider tests. |
| tests/foreman/cli/test_registration.py | Broadens selection; relaxes multiple registration assertions for RHEL6; adjusts UUID check. |
| tests/foreman/cli/test_host.py | Broadens selection; relaxes multiple registration assertions for RHEL6; switches some markers to default RHEL. |
| tests/foreman/cli/test_fact.py | Relaxes registration assertions for RHEL6. |
| tests/foreman/cli/test_errata.py | Switches selection to default RHEL version. |
| tests/foreman/cli/test_discoveredhost.py | Switches provisioning selection to default RHEL version; adds import. |
| tests/foreman/cli/test_contentaccess.py | Broadens selection and switches some tests to default RHEL version. |
| tests/foreman/cli/test_computeresource_vmware.py | Broadens selection and switches one test to default RHEL version. |
| tests/foreman/cli/test_computeresource_rhev.py | Broadens selection to include RHEL6. |
| tests/foreman/cli/test_computeresource_libvirt.py | Broadens selection to include RHEL6. |
| tests/foreman/cli/test_bootdisk.py | Broadens selection to include RHEL6. |
| tests/foreman/cli/test_ansible.py | Broadens selection; relaxes registration assertions for RHEL6; switches some markers to default RHEL. |
| tests/foreman/cli/test_activationkey.py | Broadens selection; relaxes assertions for RHEL6; switches one test to default RHEL. |
| tests/foreman/api/test_subscription.py | Relaxes registration assertions for RHEL6; switches one marker to default RHEL. |
| tests/foreman/api/test_repository.py | Unskips an SRPM repo test by commenting out a skip marker. |
| tests/foreman/api/test_reporttemplates.py | Broadens selection; relaxes registration-output assertions for RHEL6; modifies installed-products flow. |
| tests/foreman/api/test_remoteexecution.py | Switches selection to exclude RHEL6 for pull-provider flows; relaxes registration assertion for RHEL6. |
| tests/foreman/api/test_registration.py | Broadens selection; relaxes registration assertions for RHEL6; switches some markers to default RHEL. |
| tests/foreman/api/test_provisioningtemplate.py | Broadens selection to include RHEL6; switches one marker to default RHEL. |
| tests/foreman/api/test_provisioning_puppet.py | Broadens selection to include RHEL6. |
| tests/foreman/api/test_provisioning.py | Broadens provisioning selection to include RHEL6. |
| tests/foreman/api/test_http_proxy.py | Relaxes registration assertion for RHEL6. |
| tests/foreman/api/test_errata.py | Broadens selection; relaxes registration assertions for RHEL6. |
| tests/foreman/api/test_discoveredhost.py | Broadens selection and switches reboot tests to default RHEL; adds import. |
| tests/foreman/api/test_computeresource_vmware.py | Switches selection to default RHEL version. |
| tests/foreman/api/test_capsulecontent.py | Switches distro selection to default RHEL; relaxes registration assertion; replaces dnf with yum. |
| tests/foreman/api/test_ansible.py | Broadens selection; refactors repo selection by detected major; relaxes registration assertions for RHEL6. |
| robottelo/constants/init.py | Adds repo metadata for rhsclient6 (RHEL6 ELS Satellite Client). |
| pytest_fixtures/component/provisioning_template.py | Adjusts kickstart version selection logic for rhel_ver <= 7 (includes RHEL6). |
| pytest_fixtures/component/provision_pxe.py | Adjusts kickstart version selection logic for rhel_ver <= 7 (includes RHEL6). |
| pytest_fixtures/component/provision_capsule_pxe.py | Adjusts kickstart version selection logic for rhel_ver <= 7 (includes RHEL6). |
| conf/supportability.yaml | Adds RHEL6 to supported content host versions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @pytest.mark.rhel_ver_list(r'^[\d]+$') | ||
| def test_positive_custom_products_disabled_by_default( |
There was a problem hiding this comment.
Same issue as elsewhere: rhel_ver_list does not treat its argument as a regex. Using r'^[\d]+$' will not exclude non-numeric variants like 7_fips and may unintentionally expand the test matrix. Prefer rhel_ver_match(r'^\d+$') if you want numeric-only, or drop the marker if you want all supported variants.
| @pytest.mark.e2e | ||
| @pytest.mark.no_containers | ||
| @pytest.mark.pit_client | ||
| @pytest.mark.rhel_ver_match('[^6]') | ||
| @pytest.mark.rhel_ver_list(r'^[\d]+$') | ||
| def test_positive_global_registration_end_to_end( |
There was a problem hiding this comment.
rhel_ver_list does not interpret regex patterns. With r'^[\d]+$' this test will effectively run on all supported versions (including *_fips) due to marker parsing logic, which doesn’t match the intent of “numeric majors only”. Use @pytest.mark.rhel_ver_match(r'^\d+$') instead if you want to target numeric majors.
| @pytest.mark.rhel_ver_match([settings.content_host.default_rhel_version]) | ||
| def test_positive_run_default_job_template( |
There was a problem hiding this comment.
rhel_ver_match takes a regex string; passing [settings.content_host.default_rhel_version] works only accidentally for single-digit majors and fails for multi-digit majors (e.g. 10). Use rhel_ver_list([settings.content_host.default_rhel_version]) or an anchored string regex instead.
| @pytest.mark.rhel_ver_list(r'^[\d]+$') | ||
| def test_positive_installed_products( |
There was a problem hiding this comment.
rhel_ver_list does not process regex patterns. Using r'^[\d]+$' here won’t select only numeric majors and will effectively fall back to running on all supported versions (including *_fips). If you want numeric-only, switch to @pytest.mark.rhel_ver_match(r'^\d+$').
| if rhel_contenthost.os_version.major != 6: | ||
| rhel_contenthost.register(org, default_location, ak.name, target_sat) | ||
| if rhel_contenthost.os_version.major != 6: | ||
| assert rhel_contenthost.subscribed, 'Host registration failed.' | ||
|
|
There was a problem hiding this comment.
When os_version.major == 6, this test skips registration entirely but continues to generate a report for rhel_contenthost.hostname and asserts on the report contents. That will likely fail (host not present) rather than cleanly skipping. Either register the host for RHEL6 too (and relax only the known-broken assertion), or explicitly pytest.skip(...) for RHEL6 here; also the nested if os_version.major != 6 is redundant.
| if rhel_contenthost.os_version.major != 6: | |
| rhel_contenthost.register(org, default_location, ak.name, target_sat) | |
| if rhel_contenthost.os_version.major != 6: | |
| assert rhel_contenthost.subscribed, 'Host registration failed.' | |
| if rhel_contenthost.os_version.major == 6: | |
| pytest.skip('Host registration and installed products report are not supported on RHEL 6.') | |
| rhel_contenthost.register(org, default_location, ak.name, target_sat) | |
| assert rhel_contenthost.subscribed, 'Host registration failed.' |
| if rhel_contenthost.os_version.major != '6': | ||
| assert result.status == 0 |
There was a problem hiding this comment.
os_version.major is used numerically throughout the codebase (e.g. > 7), so it’s an int. Comparing it to the string '6' (here and earlier in this test) will not behave as intended—!= '6' will always be true, and == '6' will never be true. Use integer comparisons (== 6 / != 6) so the RHEL6-specific logic actually takes effect.
| """ | ||
|
|
||
| @pytest.mark.rhel_ver_list([7, 8, 9]) | ||
| @pytest.mark.rhel_ver_list(r'^[\d]+$') |
There was a problem hiding this comment.
rhel_ver_list markers are parsed by substring matching against settings.supportability...versions (see pytest_plugins/fixture_markers.py), not as a regex. Passing r'^[\d]+$' will not filter to numeric versions and will effectively fall back to running on all supported versions (including *_fips). If the goal is “any numeric major”, use @pytest.mark.rhel_ver_match(r'^\d+$') (or remove the marker entirely if you truly want all variants).
|
|
||
|
|
||
| @pytest.mark.rhel_ver_match('9') | ||
| @pytest.mark.rhel_ver_match([settings.content_host.default_rhel_version]) |
There was a problem hiding this comment.
rhel_ver_match expects a regex string (or N-x). Passing a Python list relies on str([10]) == '[10]' being treated as a regex character class, which breaks for multi-digit majors (e.g. 10 won’t match). Prefer @pytest.mark.rhel_ver_list([settings.content_host.default_rhel_version]) or @pytest.mark.rhel_ver_match(f'^{settings.content_host.default_rhel_version}$').
| @pytest.mark.rhel_ver_match([settings.content_host.default_rhel_version]) | ||
| @pytest.mark.no_containers |
There was a problem hiding this comment.
rhel_ver_match expects a regex string. Supplying a list ([settings.content_host.default_rhel_version]) will break for multi-digit majors (e.g. 10) because str([10]) becomes '[10]' (a one-character regex class). Use rhel_ver_list([settings.content_host.default_rhel_version]) or rhel_ver_match(f'^{...}$').
| if rhel_contenthost.os_version.major != '6': | ||
| assert res.status == 0, ( | ||
| f'Failed to register host: {rhel_contenthost.hostname}\n' | ||
| f'StdOut: {res.stdout}\nStdErr: {res.stderr}' | ||
| ) |
There was a problem hiding this comment.
os_version.major is an int; comparing to the string '6' makes the condition always true. This means the status assertion will still run on RHEL 6, defeating the intended exception handling.
1 participant
Problem Statement
We want to check how the robottelo tests are doing for rhel6 client with Satellite 6.16
Solution
Related Issues
Summary by Sourcery
Broaden RHEL client test coverage and supportability to include RHEL6 by relaxing version-specific test markers.
New Features:
Tests:
Summary by Sourcery
Enable RHEL 6 client coverage across robottelo tests by generalizing RHEL version markers and adapting registration expectations.
New Features:
Enhancements:
Tests: