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

test: Added tests for rhc status #115

Merged
merged 1 commit into from
Sep 5, 2024
Merged

test: Added tests for rhc status #115

merged 1 commit into from
Sep 5, 2024

Conversation

Archana-PandeyM
Copy link

@Archana-PandeyM Archana-PandeyM commented May 27, 2024 •
edited
Loading

One test currently fails due to known issue :https://issues.redhat.com/browse/CCT-525

@Archana-PandeyM Archana-PandeyM force-pushed the arpandey/addTestsRhcSttaus branch from 24b90ad to 3229361 Compare May 27, 2024 10:40
@Archana-PandeyM Archana-PandeyM force-pushed the arpandey/addTestsRhcSttaus branch 3 times, most recently from 8b14e98 to ecc0a3a Compare June 18, 2024 11:52
@Archana-PandeyM Archana-PandeyM force-pushed the arpandey/addTestsRhcSttaus branch 3 times, most recently from 3f904b5 to c29aa0d Compare July 1, 2024 09:15
@Archana-PandeyM Archana-PandeyM marked this pull request as ready for review July 1, 2024 09:16
@Archana-PandeyM Archana-PandeyM force-pushed the arpandey/addTestsRhcSttaus branch from c29aa0d to 3b6efb6 Compare July 1, 2024 09:58
jirihnidek
jirihnidek requested changes Jul 2, 2024
View reviewed changes
Copy link
Contributor

@jirihnidek jirihnidek left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the PR 👍 . I have few comments and proposals.

integration-tests/test_status.py Outdated Show resolved Hide resolved
integration-tests/test_status.py Outdated Show resolved Hide resolved
integration-tests/test_status.py Outdated Show resolved Hide resolved
integration-tests/test_status.py Outdated Show resolved Hide resolved
integration-tests/utils/__init__.py Outdated Show resolved Hide resolved
integration-tests/test_status.py Outdated Show resolved Hide resolved
@Archana-PandeyM Archana-PandeyM force-pushed the arpandey/addTestsRhcSttaus branch 2 times, most recently from 914565f to a4c60fe Compare July 2, 2024 15:07
Lorquas
Lorquas reviewed Jul 8, 2024
View reviewed changes
Copy link
Member

@Lorquas Lorquas left a comment

Choose a reason for hiding this comment

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

Generally good! I have a few questions (see inline)...

integration-tests/utils/__init__.py Outdated Show resolved Hide resolved
integration-tests/test_status.py Show resolved Hide resolved
integration-tests/test_status.py Outdated Show resolved Hide resolved
@Archana-PandeyM Archana-PandeyM requested a review from Lorquas July 9, 2024 13:38
@Archana-PandeyM Archana-PandeyM force-pushed the arpandey/addTestsRhcSttaus branch 5 times, most recently from b6a13c2 to a45046a Compare July 11, 2024 08:43
Lorquas
Lorquas approved these changes Jul 11, 2024
View reviewed changes
Copy link
Member

@Lorquas Lorquas left a comment

Choose a reason for hiding this comment

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

Approved pending discussion on cct-525

@jirihnidek
Copy link
Contributor

/packit build

@jirihnidek
Copy link
Contributor

It seems that merging this PR is still blocked by some issue in insights-client on Fedora.

@Lorquas
Copy link
Member

Lorquas commented Jul 15, 2024

It seems that merging this PR is still blocked by some issue in insights-client on Fedora.

insights-client is having some non-related problems on latest python: https://issues.redhat.com/browse/RHINENG-11283

@jirihnidek
Copy link
Contributor

It seems that merging this PR is still blocked by some issue in insights-client on Fedora.

insights-client is having some non-related problems on latest python: https://issues.redhat.com/browse/RHINENG-11283

OK, then it would be good to skip failing test on Fedora ATM using e.g. @pytest.mark.skipif(platform.freedesktop_os_release()["ID"] == "fedora", reason="RHINENG-11283") and create another card to enable these test, when related issue with insights-client is fixed. It is not good practice to approve failed tests.

@Archana-PandeyM Archana-PandeyM force-pushed the arpandey/addTestsRhcSttaus branch from a45046a to caf6159 Compare July 22, 2024 15:13
@Archana-PandeyM
Copy link
Author

/packit build

@Archana-PandeyM Archana-PandeyM force-pushed the arpandey/addTestsRhcSttaus branch from caf6159 to 48173df Compare July 29, 2024 15:08
@Archana-PandeyM
Copy link
Author

@jirihnidek the problem with fedora build is resolved with insights-core fix! RedHatInsights/insights-core#4154

@Archana-PandeyM Archana-PandeyM requested a review from Lorquas August 6, 2024 05:24
jirihnidek
jirihnidek reviewed Aug 6, 2024
View reviewed changes
integration-tests/test_status.py
4- restart yggdrasil service via systemctl
expected_results:
1- Yggdrasil service should be restarted successfully and set in active state
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually test of yggdrasil and I'm not sure if it is good to have it here, because when yggdrasil could not be restarted on e.g. disconnected system, then we will not be able to fix it in this repository.

jirihnidek
jirihnidek approved these changes Aug 6, 2024
View reviewed changes
Copy link
Contributor

@jirihnidek jirihnidek left a comment

Choose a reason for hiding this comment

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

LGTM, I would also add some test for --format json, but we can add it in the future in another PR.

@Archana-PandeyM Archana-PandeyM marked this pull request as draft August 6, 2024 08:00
@Archana-PandeyM Archana-PandeyM force-pushed the arpandey/addTestsRhcSttaus branch 2 times, most recently from dd0e0ed to 1d0df0a Compare August 14, 2024 13:26
@Archana-PandeyM
Copy link
Author

/packit build

@ptoscano
Copy link
Contributor

/packit test

@Archana-PandeyM Archana-PandeyM force-pushed the arpandey/addTestsRhcSttaus branch from 1d0df0a to 3132c65 Compare September 3, 2024 12:50
@jirihnidek
Copy link
Contributor

/packit build

@Archana-PandeyM Archana-PandeyM force-pushed the arpandey/addTestsRhcSttaus branch from 3132c65 to a0acf09 Compare September 5, 2024 07:39
@Archana-PandeyM Archana-PandeyM marked this pull request as ready for review September 5, 2024 07:41
@Archana-PandeyM
Copy link
Author

Archana-PandeyM commented Sep 5, 2024 •
edited
Loading

" LGTM, I would also add some test for --format json, but we can add it in the future in another PR."

yes we will increase the test coverage to have this tests in another PR.

@jirihnidek jirihnidek merged commit 19d1ee4 into main Sep 5, 2024
23 of 24 checks passed
@jirihnidek jirihnidek deleted the arpandey/addTestsRhcSttaus branch September 5, 2024 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ptoscano ptoscano ptoscano left review comments

@jirihnidek jirihnidek jirihnidek approved these changes

@Lorquas Lorquas Awaiting requested review from Lorquas

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Archana-PandeyM @jirihnidek @Lorquas @ptoscano @archana-redhat