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

TFA fix for discovery plugin #15783

Merged
merged 1 commit into from
Aug 29, 2024
Merged

Conversation

amolpati30
Copy link
Contributor

@amolpati30 amolpati30 commented Jul 26, 2024

The failure occurred because the code was deleting the discovery rule, and then attempting to read from the deleted table using the read_all() method. To resolve this issue, I added the read_after_del() method, which reads the page message after a rule is deleted or if there are no more tables.

Dependent PR: SatelliteQE/airgun#1480

@amolpati30 amolpati30 added Easy Fix :) Easiest Fix to review and quick merge request. No-CherryPick PR doesnt need CherryPick to previous branches Stream Introduced in or relating directly to Satellite Stream/Master labels Jul 26, 2024
@amolpati30 amolpati30 requested review from a team as code owners July 26, 2024 17:04
@amolpati30
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_discoveryrule.py -k test_positive_crud_with_non_admin_user
airgun: 1480

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 7882
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/ui/test_discoveryrule.py -k test_positive_crud_with_non_admin_user --external-logging
Test Result : =========== 1 passed, 3 deselected, 16 warnings in 761.02s (0:12:41) ===========

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Jul 26, 2024
@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label Jul 26, 2024
@amolpati30 amolpati30 force-pushed the discoveryrulefix branch 2 times, most recently from 5413fe0 to 276ddd0 Compare July 26, 2024 18:08
Copy link
Member

@ColeHiggins2 ColeHiggins2 left a comment

Choose a reason for hiding this comment

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

Ack, pending airgun merge

@@ -121,7 +121,7 @@ def test_positive_crud_with_non_admin_user(

session.discoveryrule.delete(new_rule_name)
dr_val = session.discoveryrule.read_all()
assert new_rule_name not in [rule['Name'] for rule in dr_val]
assert new_rule_name not in dr_val
Copy link
Contributor

Choose a reason for hiding this comment

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

@amolpati30 you need to fix/update the rest of the tests too. There are 4 more places in the code where this change is needed. For example,

dr_val = session.discoveryrule.read_all()

@amolpati30 amolpati30 force-pushed the discoveryrulefix branch 2 times, most recently from 7fcc48a to 590e55a Compare July 31, 2024 13:57
@amolpati30
Copy link
Contributor Author

"trigger": "test-robottelo"
"pytest": "tests/foreman/ui/test_discoveryrule.py -k 'crud_with_non_admin_user or delete_rule_with_non_admin_user or list_host_based_on_rule_search_query or end_to_end'"
"airgun": "1480"

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 7928
Build Status: SUCCESS
PRT Comment: pytest /opt/app-root/src/robottelo/tests/foreman -v -m build_sanity --external-logging
Test Result : ======= 12 passed, 5526 deselected, 5633 warnings in 2122.04s (0:35:22) ========

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Jul 31, 2024
@@ -154,7 +154,7 @@ def test_negative_delete_rule_with_non_admin_user(
with pytest.raises(ValueError): # noqa: PT011 - TODO Adarsh determine better exception
session.discoveryrule.delete(dr.name)
dr_val = session.discoveryrule.read_all()
assert dr.name in [rule['Name'] for rule in dr_val]
assert dr.name not in dr_val
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
assert dr.name not in dr_val
assert dr.name in dr_val

@amolpati30 You have changed the assertion here. Is this expected?

@amolpati30 amolpati30 closed this Aug 1, 2024
@amolpati30 amolpati30 reopened this Aug 1, 2024
@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label Aug 2, 2024
@amolpati30 amolpati30 marked this pull request as draft August 2, 2024 06:27
@amolpati30 amolpati30 marked this pull request as ready for review August 2, 2024 17:51
@amolpati30
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_discoveryrule.py -k 'crud_with_non_admin_user or delete_rule_with_non_admin_user or list_host_based_on_rule_search_query or end_to_end'
airgun: 1480

@Satellite-QE Satellite-QE removed the PRT-Failed Indicates that latest PRT run is failed for the PR label Aug 9, 2024
@amolpati30 amolpati30 added 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 and removed No-CherryPick PR doesnt need CherryPick to previous branches Stream Introduced in or relating directly to Satellite Stream/Master labels Aug 9, 2024
@jyejare jyejare added the 6.16.z Introduced in or relating directly to Satellite 6.16 label Aug 22, 2024
@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label Aug 23, 2024
@amolpati30
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_discoveryrule.py -k 'crud_with_non_admin_user or end_to_end'
airgun: 1480

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 8264
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/ui/test_discoveryrule.py -k crud_with_non_admin_user or end_to_end --external-logging
Test Result : =========== 2 passed, 2 deselected, 52 warnings in 935.46s (0:15:35) ===========

Copy link
Contributor

@shweta83 shweta83 left a comment

Choose a reason for hiding this comment

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

Ack

@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label Aug 28, 2024
@shweta83
Copy link
Contributor

trigger: test-robottelo
pytest: tests/foreman/ui/test_discoveryrule.py -k 'crud_with_non_admin_user or end_to_end'
airgun: 1480

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 8334
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/ui/test_discoveryrule.py -k crud_with_non_admin_user or end_to_end --external-logging
Test Result : =========== 2 passed, 2 deselected, 49 warnings in 845.80s (0:14:05) ===========

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Aug 28, 2024
@shweta83 shweta83 merged commit 95906c1 into SatelliteQE:master Aug 29, 2024
11 checks passed
github-actions bot pushed a commit that referenced this pull request Aug 29, 2024
fixed in discoveryrule assertion

update in test

(cherry picked from commit 95906c1)
github-actions bot pushed a commit that referenced this pull request Aug 29, 2024
fixed in discoveryrule assertion

update in test

(cherry picked from commit 95906c1)
Gauravtalreja1 pushed a commit that referenced this pull request Aug 29, 2024
TFA fix for discovery plugin (#15783)

fixed in discoveryrule assertion

update in test

(cherry picked from commit 95906c1)

Co-authored-by: amolpati30 <[email protected]>
jyejare pushed a commit to jyejare/robottelo that referenced this pull request Oct 19, 2024
fixed in discoveryrule assertion

update in test
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 6.16.z Introduced in or relating directly to Satellite 6.16 AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing CherryPick PR needs CherryPick to previous branches Easy Fix :) Easiest Fix to review and quick merge request. 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.

6 participants