Skip to content
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

[Comp Eval] Updating fixture to SCA and Fixing failing tests #14167

Merged

Conversation

ColeHiggins2
Copy link
Member

Problem Statement

Many tests were failing in the UI due to several issues including outdated code, old non-sca fixtures, and more.

The associated issues were addressed and now passing
RHSAT6-45606: test_positive_generate_all_installed_packages_report
RHSAT6-44209: test_positive_gen_entitlements_reports_multiple_formats
RHSAT6-40833: test_positive_generate_registered_hosts_report

Solution

Update setup_content fixture to use SCA and enabled the custom repo on the Activation Key
docstring changes and manual org.disable for subscription-entitlements report
Rewrite outdated test using new rhel_content fixture

@ColeHiggins2 ColeHiggins2 added CherryPick PR needs CherryPick to previous branches AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing 6.15.z Introduced in or relating directly to Satellite 6.15 labels Feb 23, 2024
@ColeHiggins2 ColeHiggins2 self-assigned this Feb 23, 2024
@ColeHiggins2 ColeHiggins2 requested a review from a team as a code owner February 23, 2024 16:03
and it contains created host with correct data

:CaseImportance: High
"""
# generate Host Status report
os_name = 'comma,' + gen_string('alpha')
Copy link
Contributor

Choose a reason for hiding this comment

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

The comma was there to test correct handling in CSV.

with target_sat.ui_session() as session:
session.organization.select(module_org.name)
session.location.select(module_location.name)
# create multiple hosts to test filtering
Copy link
Contributor

Choose a reason for hiding this comment

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

So you removed testing filtering. It may be ok. Do you test it elsewhere? Is the change intended? You haven't mentioned it. This weakens the test.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was failing so I just decided to use the new way of registering a host. This test is for report templates not host filtering, unless the report templates test filter, I dont think its necessary to keep? what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I also see some ui test for testing positive user access with host filtering, is that the same thing here?

@omkarkhatavkar
Copy link

trigger: test-robottelo
pytest: tests/foreman/ui/ -m e2e

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 5896
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/ui/ -m e2e --external-logging
Test Result : = 10 failed, 26 passed, 800 deselected, 1978 warnings, 5 errors in 13727.37s (3:48:47) =

@Satellite-QE Satellite-QE added the PRT-Failed Indicates that latest PRT run is failed for the PR label Feb 29, 2024
@Satellite-QE Satellite-QE removed the PRT-Failed Indicates that latest PRT run is failed for the PR label Apr 1, 2024
Copy link
Contributor

@vsedmik vsedmik left a comment

Choose a reason for hiding this comment

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

A few proposals and questions posed

)[0]
ak.add_subscriptions(data={'quantity': 1, 'subscription_id': subscription.id})
all_content = ak.product_content(data={'content_access_mode_all': '1'})['results']
content_label = content_label = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
content_label = content_label = [
content_label = [

Comment on lines 248 to 250
client.install_katello_ca(target_sat)
org, ak, _, _ = setup_content
client.register_contenthost(org.label, ak.name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use global registration

Suggested change
client.install_katello_ca(target_sat)
org, ak, _, _ = setup_content
client.register_contenthost(org.label, ak.name)
org, ak, _, _ = setup_content
client.register(org, None, ak.name, target_sat)

Copy link
Contributor

Choose a reason for hiding this comment

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

@ColeHiggins2 Based on the latest discussion in Automation Weekly BS call, this became rather a blocker than suggestion. Can you update the registering method?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

@@ -490,10 +481,12 @@ def test_negative_nonauthor_of_report_cant_download_it(session):


@pytest.mark.tier3
@pytest.mark.no_containers
def test_positive_gen_entitlements_reports_multiple_formats(
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at org.sca_disable() bellow, is this test case really applicable to 6.16? Shouldn't it be raised against 6.15.z branch instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

This change fixes the test in 6.15 and will be removed in 6.16. however, a lot of the other work in this PR still applies to 6.16. so I will just create a separate PR to remove this later in 6.16 as part of the RFE for SCA ONLY

Copy link
Contributor

@vijaysawant vijaysawant left a comment

Choose a reason for hiding this comment

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

Ack
Can you please check this small change?

tests/foreman/ui/test_reporttemplates.py Outdated Show resolved Hide resolved
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.

1 change necessary, looks good otherwise. Can we get PRT as well

query={'search': f'name="{DEFAULT_SUBSCRIPTION_NAME}"'}
)[0]
ak.add_subscriptions(data={'quantity': 1, 'subscription_id': subscription.id})
client.register_contenthost(org.label, None, ak.name, target_sat)
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to change this to use global registration as well

Copy link
Contributor

@vsedmik vsedmik left a comment

Choose a reason for hiding this comment

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

One more suggestion from me. Sam's comment regarding GR and PRT run (at least for the first test since the second one would fail in stream) pending.

Comment on lines 234 to 237
def test_positive_generate_registered_hosts_report(
session, target_sat, setup_content, rhel7_contenthost
):
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's get rid of rhelX_contenthost in favor of rhel_contenthost and parametrize what version we want to test with. Here I would say we could test with all supported versions (as suggested bellow), or pick one at least.

Suggested change
def test_positive_generate_registered_hosts_report(
session, target_sat, setup_content, rhel7_contenthost
):
@pytest.mark.rhel_ver_match(r'^(?!6$)\d+$')
def test_positive_generate_registered_hosts_report(
session, target_sat, module_setup_content, rhel_contenthost
):

I can see the setup_content fixture is module-scoped - good, we need it module-scoped so we don't create new and new content for every parametrized run, just consider renaming it to what it is - module_setup_content.

Copy link
Contributor

@vijaysawant vijaysawant left a comment

Choose a reason for hiding this comment

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

Ack.. lgtm
Penidng few comments of other reviewers and PRT status, please take a look.

@ColeHiggins2 ColeHiggins2 force-pushed the reporting-comp-eval-test-fixes branch from dd3591e to 7aac374 Compare May 10, 2024 15:04
@ColeHiggins2 ColeHiggins2 requested a review from vsedmik May 10, 2024 15:05
@pytest.mark.upgrade
@pytest.mark.tier2
def test_positive_generate_registered_hosts_report(target_sat, module_org, module_location):
def test_positive_generate_registered_hosts_report(
Copy link
Contributor

Choose a reason for hiding this comment

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

lets run PRT on this at least, if that looks good I'm good with ACK

@ColeHiggins2
Copy link
Member Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_reporttemplates.py -k test_positive_generate_registered_hosts_report

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6882
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/ui/test_reporttemplates.py -k test_positive_generate_registered_hosts_report --external-logging
Test Result : ========== 3 passed, 15 deselected, 61 warnings in 1014.84s (0:16:54) ==========

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label May 10, 2024
Copy link
Contributor

@vsedmik vsedmik left a comment

Choose a reason for hiding this comment

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

LGTM

@vsedmik vsedmik merged commit 06e5529 into SatelliteQE:master May 10, 2024
10 checks passed
github-actions bot pushed a commit that referenced this pull request May 10, 2024
* Fixing tests for reporting comp eval

* updating to list comp

* addressing comments

* Addressing comments

* addressing comments and rename setup content

(cherry picked from commit 06e5529)
jyejare pushed a commit to jyejare/robottelo that referenced this pull request Oct 19, 2024
…teQE#14167)

* Fixing tests for reporting comp eval

* updating to list comp

* addressing comments

* Addressing comments

* addressing comments and rename setup content
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.15.z Introduced in or relating directly to Satellite 6.15 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants