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

feat: added integration tests to cover evicted pods in post conditions #1399

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Al-Pragliola
Copy link
Contributor

Tracking issue: RHOAIENG-15614

Description

Seeing that testing in a real cluster can be quite problematic, I added integration tests to cover scenarios with evicted pods, I didn't add a negative test because it takes 5 minutes to fail and the timeout duration is not configurable.

How Has This Been Tested?

make test

Screenshot or short clip

Merge criteria

  • You have read the contributors guide.
  • Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
  • Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

Copy link

codecov bot commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 88.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 20.04%. Comparing base (71f79e2) to head (7546b8d).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
...gration/features/fixtures/cluster_test_fixtures.go 88.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1399      +/-   ##
==========================================
+ Coverage   19.88%   20.04%   +0.15%     
==========================================
  Files         160      160              
  Lines       10818    10843      +25     
==========================================
+ Hits         2151     2173      +22     
- Misses       8440     8442       +2     
- Partials      227      228       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Al-Pragliola
Copy link
Contributor Author

/cc @zdtsw

@openshift-ci openshift-ci bot requested a review from zdtsw November 28, 2024 10:59
objectCleaner.DeleteAll(ctx, dsci)
})

It("should succeed when all pods in the namespace are ready", func(ctx context.Context) {
Copy link
Member

Choose a reason for hiding this comment

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

i guess here is
It("should succeed when expected pod in the namespace is ready", func(ctx context.Context) {

Copy link
Member

Choose a reason for hiding this comment

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

otherwise, i am fine with the change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

condition feature.WaitForPodsToBeReady will be satisfied only if all pods in the namespace are "Ready", in the test case there's only one pod but "should succeed when all pods in the namespace are ready" should still be valid, I can add more pods in the test if you think is necessary

@zdtsw zdtsw requested review from bartoszmajsak and israel-hdez and removed request for StevenTobin November 28, 2024 13:27
@lburgazzoli
Copy link
Contributor

@Al-Pragliola this PR should now targeting main, and probably rebased

@Al-Pragliola Al-Pragliola force-pushed the chore/add-tests-to-cover-evicted-pods-scenario branch from a52d606 to d3dfb03 Compare January 14, 2025 10:51
@Al-Pragliola Al-Pragliola changed the base branch from incubation to main January 14, 2025 10:51
@Al-Pragliola
Copy link
Contributor Author

/retest

@Al-Pragliola
Copy link
Contributor Author

/test opendatahub-operator-e2e

@Al-Pragliola
Copy link
Contributor Author

@Al-Pragliola this PR should now targeting main, and probably rebased

done! thanks @lburgazzoli

@Al-Pragliola
Copy link
Contributor Author

/test opendatahub-operator-e2e

@Al-Pragliola Al-Pragliola force-pushed the chore/add-tests-to-cover-evicted-pods-scenario branch from d3dfb03 to 7546b8d Compare January 14, 2025 17:47
Copy link

openshift-ci bot commented Jan 14, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign ykaliuta for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@zdtsw zdtsw added the testing label Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

3 participants