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

Errata WIP for remaining UI Failures #15085

Merged
merged 2 commits into from
Jun 21, 2024

Conversation

damoore044
Copy link
Contributor

@damoore044 damoore044 commented May 16, 2024

Problem Statement

Fix remaining UI errata failures (8-10 in recent CI 6.15.0/stream)

Ongoing Failures

  • test_positive_content_host_previous_env: Skipped for now, until BZ #2280882 is resolved.
  • test_positive_apply_for_all_hosts: Testing locally Fixed- Airgun PR #1387. Added End2End flag for this case, as it is now very thorough and makes use of multiple install methods.

PRT Case

trigger: test-robottelo
pytest: tests/foreman/ui/test_errata.py
airgun: 1387

@damoore044 damoore044 added No-CherryPick PR doesnt need CherryPick to previous branches Stream Introduced in or relating directly to Satellite Stream/Master labels May 16, 2024
@damoore044 damoore044 self-assigned this May 16, 2024
@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_errata.py

@damoore044 damoore044 force-pushed the Errata-WIP branch 2 times, most recently from 09216c3 to 291c4b5 Compare May 16, 2024 17:57
@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_errata.py

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6968
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/ui/test_errata.py --external-logging
Test Result : ======= 9 failed, 9 passed, 1842 warnings, 1 error in 6026.68s (1:40:26) =======

@Satellite-QE Satellite-QE added the PRT-Failed Indicates that latest PRT run is failed for the PR label May 16, 2024
@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_errata.py

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6975
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/ui/test_errata.py --external-logging
Test Result : ====== 4 failed, 5 passed, 1764 warnings, 10 errors in 5999.92s (1:39:59) ======

@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_errata.py

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6992
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/ui/test_errata.py --external-logging
Test Result : ====== 5 failed, 13 passed, 1861 warnings, 1 error in 6449.62s (1:47:29) =======

@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_errata.py

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6994
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/ui/test_errata.py --external-logging
Test Result : ====== 8 failed, 8 passed, 1853 warnings, 2 errors in 5132.86s (1:25:32) =======

@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_errata.py

@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_errata.py

@damoore044 damoore044 added 6.15.z Introduced in or relating directly to Satellite 6.15 CherryPick PR needs CherryPick to previous branches and removed No-CherryPick PR doesnt need CherryPick to previous branches labels May 17, 2024
@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6997
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/ui/test_errata.py --external-logging
Test Result : ====== 8 failed, 10 passed, 2099 warnings, 2 errors in 5952.10s (1:39:12) ======

@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_errata.py
airgun: 1387

@damoore044
Copy link
Contributor Author

Pulled in changes and suggestions from #15125, thanks @ColeHiggins2 !

@damoore044 damoore044 requested review from a team June 17, 2024 18:09
@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 7413
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/ui/test_errata.py --external-logging
Test Result : ========== 17 passed, 1 skipped, 1960 warnings in 5935.16s (1:38:55) ===========

@Satellite-QE Satellite-QE added PRT-Passed Indicates that latest PRT run is passed for the PR and removed PRT-Failed Indicates that latest PRT run is failed for the PR labels Jun 17, 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.

Adding a few comments for now, I will probably need another iteration to complete the review.

'value': int(True),
}
)
repo.read().sync()
Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing against syncing it here, but if we don't set enable_repos param, we need to sync it elsewhere. To me it feels a bit unrelated to enabling the repo at AK. Like two different things.
If you decide to keep it maybe just update the docstring that the repo gets synced here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, removed it from here, repos can be synced before or after registration

tests/foreman/ui/test_errata.py Outdated Show resolved Hide resolved
Comment on lines +227 to +242
# no repos given/present, subscription-manager should report error
if len(repos) == 0:
assert client.execute(r'subscription-manager repos --enable \*').status == 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we need to test this, it's rather sub-man functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm but if sub-man fails to find repos to enable, then that will probably mean errata applicability won't work either, correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but here we just assert that it really fails when no repos provided.
If len(repos) == 0 means that we have no repos where we would install or apply errata from anyway. So is it allowed variant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gotcha yeah I see, code is a little different but my thought process was this;
if we want to register host with no repos at time of reg
(allowed & default variant of the chost fixture)...
I wanted to ensure sub-manager also found no repos to enable.
Seems unnecessary though?

Copy link
Contributor Author

@damoore044 damoore044 Jun 20, 2024

Choose a reason for hiding this comment

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

Because after registration, we can always add repos to cv and publish/promote, and use subman as normal after. So by default (no repos) I also wanted to check subman found nothing

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, gotcha. It just appeared odd to me that we check sub-man functionality here, not Sat (like do we want to fail Satellite test when sub-man is broken actually). But we rely on sub-man repos and others anyway, so I don't mind to keep it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vsedmik Should we also check some satellite functionality too? Such as making sure no repos enabled in AK, when none are specified?

Copy link
Contributor

Choose a reason for hiding this comment

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

That would rather be a negative test case for the Activation-key component. I wouldn't put it in Errata testing.

tests/foreman/ui/test_errata.py Outdated Show resolved Hide resolved
tests/foreman/ui/test_errata.py Outdated Show resolved Hide resolved
tests/foreman/ui/test_errata.py Outdated Show resolved Hide resolved
tests/foreman/ui/test_errata.py Outdated Show resolved Hide resolved
Comment on lines +1609 to +1586
for key in actual_values:
assert expected_values[key] in actual_values[key], 'Expected text not found'
Copy link
Contributor

Choose a reason for hiding this comment

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

Since expected_values and actual_values are same length (same keys), I guess this should work

Suggested change
for key in actual_values:
assert expected_values[key] in actual_values[key], 'Expected text not found'
assert expected_values == actual_values, 'Expected text not found'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this assertion fails due to one differing value

E    Differing items:
E    {'Errata': 'Security errata installable'} != {'Errata': 'Security errata installable clear'}

Copy link
Contributor Author

@damoore044 damoore044 Jun 20, 2024

Choose a reason for hiding this comment

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

The existing test already had this key-val looping assertion, so should we keep the same?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or we could revise our expectation if we get something unexpected but it seems to be correct anyway.

        expected_values = {
            'Status': 'Error',
            'Errata': 'Security errata installable clear',
            'Subscription': 'Fully entitled',
        }

Btw, why 'clear'??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure about the clear, first time I am seeing it,
will dig in to see why it's relevant

@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label Jun 20, 2024
@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_errata.py

@damoore044 damoore044 force-pushed the Errata-WIP branch 2 times, most recently from d666f17 to a2c1181 Compare June 20, 2024 19:34
@damoore044
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_errata.py

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 7462
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/ui/test_errata.py --external-logging
Test Result : ========== 17 passed, 1 skipped, 2166 warnings in 6961.59s (1:56:01) ===========

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Jun 20, 2024
@damoore044
Copy link
Contributor Author

Latest push was a minor docstring change, and rebase

@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label Jun 21, 2024
Copy link
Member

@JacobCallahan JacobCallahan left a comment

Choose a reason for hiding this comment

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

ACK, nice work!

@JacobCallahan JacobCallahan merged commit 0e90a95 into SatelliteQE:master Jun 21, 2024
9 checks passed
github-actions bot pushed a commit that referenced this pull request Jun 21, 2024
* API-factory setup registered host method

* UI::ERRATA

(cherry picked from commit 0e90a95)
damoore044 added a commit that referenced this pull request Jun 24, 2024
* API-factory setup registered host method

* UI::ERRATA

(cherry picked from commit 0e90a95)
JacobCallahan pushed a commit that referenced this pull request Jun 24, 2024
Errata WIP for remaining UI Failures (#15085)

* API-factory setup registered host method

* UI::ERRATA

(cherry picked from commit 0e90a95)

Co-authored-by: David Moore <[email protected]>
jyejare pushed a commit to jyejare/robottelo that referenced this pull request Oct 19, 2024
* API-factory setup registered host method

* UI::ERRATA
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 CherryPick PR needs CherryPick to previous branches Stream Introduced in or relating directly to Satellite Stream/Master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants