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.

Sign up for GitHub

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

Jump to bottom

Close-loop bz2209968 #14158

Merged
merged 2 commits into from
Mar 13, 2024
Merged

Close-loop bz2209968 #14158

merged 2 commits into from
Mar 13, 2024

Conversation

lhellebr
Copy link
Contributor

Problem Statement

Close-loop bz2209968

@lhellebr lhellebr added the No-CherryPick PR doesnt need CherryPick to previous branches label Feb 22, 2024
@lhellebr lhellebr marked this pull request as ready for review February 22, 2024 11:46
@lhellebr lhellebr requested review from a team as code owners February 22, 2024 11:46
@lhellebr
Copy link
Contributor Author

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

@lhellebr
Copy link
Contributor Author

Requires SatelliteQE/airgun#1262

@lhellebr
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_jobinvocation.py::test_positive_bz2209968
airgun: 1262

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 5821
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/ui/test_jobinvocation.py::test_positive_bz2209968
Test Result : ================== 1 failed, 13 warnings in 846.12s (0:14:06) ==================

@Satellite-QE Satellite-QE added the PRT-Failed Indicates that latest PRT run is failed for the PR label Feb 22, 2024
@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 5822
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/ui/test_jobinvocation.py::test_positive_bz2209968
Test Result : ================== 1 failed, 13 warnings in 861.41s (0:14:21) ==================

@lhellebr
Copy link
Contributor Author

Dependency PR broken by SatelliteQE/airgun#1226 after rebasing it. It worked locally and the change is not that related.

rplevka
rplevka requested changes Feb 27, 2024
View reviewed changes
Copy link
Member

@rplevka rplevka left a comment

Choose a reason for hiding this comment

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

i think there is some room to consolidate the code to make it self-contained and compact

robottelo/utils/datafactory.py
@@ -367,6 +367,24 @@ def valid_hostgroups_list():
]


@filtered_datapoint
def valid_hostgroups_list_short():
"""Generates a list of valid host group names. Shorter so they can be nested.
Copy link
Member

Choose a reason for hiding this comment

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

I am not quite sold on this - you declare its properties quite vaguely ( shorter), but the reality is, you have a test dependent on this that requires this to be no shorter than 8 items.
If someone decides that it is not short enough and decides to make it even shorter, your new test would start to raise index errors.

I'd suggest to ditch this and simply generate your dataset in a local fixture.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

short stands for a list of short names, not short list of names. There is a similar fixture that has longer names, i.e. greater value in randint.
There are also other tests that require a fixture to have at least X members, valid_hostgroups_list being no exception.

robottelo/utils/datafactory.py
gen_string('latin1', random.randint(1, 15)),
gen_string('numeric', random.randint(1, 15)),
gen_string('utf8', random.randint(1, 15)),
gen_string('html', random.randint(1, 15)),
Copy link
Member

Choose a reason for hiding this comment

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

isn't the name containing html tags going to crash your test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They should be escaped

tests/foreman/ui/test_jobinvocation.py Outdated Show resolved Hide resolved
@lhellebr
Copy link
Contributor Author

lhellebr commented Mar 1, 2024

trigger: test-robottelo
pytest: tests/foreman/ui/test_jobinvocation.py::test_positive_bz2209968
airgun: 1262

@lhellebr lhellebr requested a review from rplevka March 1, 2024 10:10
@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 5910
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/ui/test_jobinvocation.py::test_positive_bz2209968 --external-logging
Test Result : ================== 1 passed, 13 warnings in 823.15s (0:13:43) ==================

@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 Mar 1, 2024
@lhellebr
Copy link
Contributor Author

lhellebr commented Mar 4, 2024

@rplevka comments addressed

@lhellebr lhellebr requested a review from a team March 5, 2024 08:53
ogajduse
ogajduse requested changes Mar 12, 2024
View reviewed changes
Copy link
Member

@ogajduse ogajduse left a comment

Choose a reason for hiding this comment

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

I left a few comments there, but LGTM overall.

tests/foreman/ui/test_jobinvocation.py Outdated Show resolved Hide resolved
tests/foreman/ui/test_jobinvocation.py Outdated Show resolved Hide resolved
tests/foreman/ui/test_jobinvocation.py Outdated Show resolved Hide resolved
@lhellebr
Copy link
Contributor Author

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

@devendra104
Copy link
Member

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

@lhellebr
Copy link
Contributor Author

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

1 similar comment
@ogajduse
Copy link
Member

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

@ogajduse
Copy link
Member

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

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6047
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/ui/test_jobinvocation.py::test_positive_hostgroups_full_nested_names --external-logging
Test Result : ================== 1 passed, 3 warnings in 1185.99s (0:19:45) ==================

@ogajduse ogajduse merged commit 944fe3e into SatelliteQE:master Mar 13, 2024
7 of 8 checks passed
shweta83 pushed a commit to shweta83/robottelo that referenced this pull request Apr 10, 2024
@lhellebr @shweta83
Close-loop bz2209968 (SatelliteQE#14158)
1df5f2a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogajduse ogajduse ogajduse approved these changes

@rplevka rplevka Awaiting requested review from rplevka

Assignees

@lhellebr lhellebr

Labels
No-CherryPick PR doesnt need CherryPick to previous branches PRT-Passed Indicates that latest PRT run is passed for the PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lhellebr @Satellite-QE @devendra104 @ogajduse @rplevka