-
Notifications
You must be signed in to change notification settings - Fork 115
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
Conversation
trigger: test-robottelo |
09216c3
to
291c4b5
Compare
trigger: test-robottelo |
PRT Result
|
trigger: test-robottelo |
PRT Result
|
trigger: test-robottelo |
PRT Result
|
trigger: test-robottelo |
PRT Result
|
trigger: test-robottelo |
trigger: test-robottelo |
PRT Result
|
trigger: test-robottelo |
Pulled in changes and suggestions from #15125, thanks @ColeHiggins2 ! |
PRT Result
|
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.
Adding a few comments for now, I will probably need another iteration to complete the review.
'value': int(True), | ||
} | ||
) | ||
repo.read().sync() |
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.
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.
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.
Agreed, removed it from here, repos can be synced before or after registration
# no repos given/present, subscription-manager should report error | ||
if len(repos) == 0: | ||
assert client.execute(r'subscription-manager repos --enable \*').status == 1 |
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.
Not sure if we need to test this, it's rather sub-man functionality.
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.
hmm but if sub-man fails to find repos to enable, then that will probably mean errata applicability won't work either, correct?
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, 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?
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.
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?
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.
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
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.
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.
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.
@vsedmik Should we also check some satellite functionality too? Such as making sure no repos enabled in AK, when none are specified?
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.
That would rather be a negative test case for the Activation-key component. I wouldn't put it in Errata testing.
for key in actual_values: | ||
assert expected_values[key] in actual_values[key], 'Expected text not found' |
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.
Since expected_values
and actual_values
are same length (same keys), I guess this should work
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' |
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.
So this assertion fails due to one differing value
E Differing items:
E {'Errata': 'Security errata installable'} != {'Errata': 'Security errata installable clear'}
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.
The existing test already had this key-val looping assertion, so should we keep the same?
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.
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'??
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 am not sure about the clear, first time I am seeing it,
will dig in to see why it's relevant
trigger: test-robottelo |
d666f17
to
a2c1181
Compare
trigger: test-robottelo |
PRT Result
|
Latest push was a minor docstring change, and rebase |
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.
ACK, nice work!
* API-factory setup registered host method * UI::ERRATA (cherry picked from commit 0e90a95)
* API-factory setup registered host method * UI::ERRATA (cherry picked from commit 0e90a95)
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]>
* API-factory setup registered host method * UI::ERRATA
Problem Statement
Fix remaining UI errata failures (8-10 in recent CI 6.15.0/stream)
test_end_to_end
and_pagination
api_factory
method proposed (closed)API-factory: Method to Associate needed entities, then Register the Host #14974
Ongoing Failures
test_positive_content_host_previous_env
: Skipped for now, until BZ #2280882 is resolved.test_positive_apply_for_all_hosts
:Testing locallyFixed- Airgun PR #1387. Added End2End flag for this case, as it is now very thorough and makes use of multiple install methods.PRT Case