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

Shorten Timeouts for E2e Tests #113

Merged
merged 2 commits into from
Dec 31, 2023
Merged

Shorten Timeouts for E2e Tests #113

merged 2 commits into from
Dec 31, 2023

Conversation

razo7
Copy link
Member

@razo7 razo7 commented Dec 14, 2023 •
edited
Loading

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Dec 14, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Dec 14, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: razo7

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

The pull request process is described 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

@razo7
Copy link
Member Author

razo7 commented Dec 14, 2023

/test 4.14-openshift-e2e
/test 4.15-openshift-e2e

@slintes
Copy link
Member

slintes commented Dec 14, 2023

because of

won't be needed after ...

this should not be merged before the other PR, otherwise we miss a test...

/hold

@clobrano
Copy link
Contributor

checkFarLogs is already removed in #106

@razo7
Copy link
Member Author

razo7 commented Dec 17, 2023

this should not be merged before the other PR, otherwise we miss a test...

It is a step in a test, and not a full test but I agree that it is better to wait.
However, having the checkFarLogs step will fail the e2e due to the #112 change since checkFarLogs checks the logs of the first FAR running pod (e.g., see failing e2e tests at #107), when we have now two running pods at any time.

checkFarLogs is already removed in #106

Correct, so either we merge this PR or the other one, otherwise every e2e test could fail due to the #112 change and we can't merge the remaining PRs.
I lean to merge this small PR first rather than the other large one.

@clobrano
Copy link
Contributor

I lean to merge this small PR first rather than the other large one.

Please do not. It's been already a bit tricky to rebase #106 onto #108, I'd rather get the first in before moving on with further changes to e2e tests.

@razo7
Copy link
Member Author

razo7 commented Dec 27, 2023

/test 4.14-openshift-e2e
/test 4.15-openshift-e2e

@razo7
Copy link
Member Author

razo7 commented Dec 27, 2023

/test 4.14-openshift-e2e

mshitrit
mshitrit reviewed Dec 27, 2023
View reviewed changes
test/e2e/far_e2e_test.go Outdated
Comment on lines 45 to 46
timeoutShort = "5s" // this timeout is used after all the other steps have been succesfult
pollShort = "250ms"
Copy link
Member

Choose a reason for hiding this comment

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

why change the var names with suffix Short , I think in this context long/short time are very subjective (especially since the "long" time doesn't exist anymore as a baseline to compare to) terms so I prefer a more meaningful name.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, it was just short from my POV.

@razo7
Copy link
Member Author

razo7 commented Dec 27, 2023

/test 4.14-openshift-e2e
/test 4.15-openshift-e2e

@razo7
Copy link
Member Author

razo7 commented Dec 27, 2023

/test 4.15-openshift-e2e

mshitrit
mshitrit reviewed Dec 28, 2023
View reviewed changes
test/e2e/far_e2e_test.go Outdated
timeoutTaint = "2s" // Timeout for checking the FAR taint
timeoutReboot = "6m0s" // fencing with fence_aws should be completed within 6 minutes
timeoutAfterReboot = "5s" // Timeout for verifying steps after the node has been rebooted
pollShort = "250ms"
Copy link
Member

@mshitrit mshitrit Dec 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think you've missed that one (i.e pollShort)

@razo7 razo7 force-pushed the improvements-e2e branch 2 times, most recently from fc9e9bf to 4a3a73c Compare December 28, 2023 09:14
@razo7
Use human readable time and minimize timeouts and poll intervals
79723a2 
Use shorter version of time as 6m0s instead of 6 * time.Minute. Minimize timeouts and poll intervals for shorter tests.
@razo7
Use meaningful timeout and poll vars
0922432 
Replace timeoutShort with timeoutTaint, and timeoutAfterReboot. Replace pollShort with pollTaint, and pollAfterReboot
@razo7
Copy link
Member Author

razo7 commented Dec 28, 2023

/test 4.14-openshift-e2e
/test 4.15-openshift-e2e

@mshitrit
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Dec 31, 2023
@razo7 razo7 marked this pull request as ready for review December 31, 2023 09:19
@razo7
Copy link
Member Author

razo7 commented Dec 31, 2023

/unhold
/retest

@openshift-merge-bot openshift-merge-bot bot merged commit 60b9d54 into medik8s:main Dec 31, 2023
18 checks passed
@razo7 razo7 deleted the improvements-e2e branch January 7, 2024 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mshitrit mshitrit mshitrit left review comments

@beekhof beekhof Awaiting requested review from beekhof

@clobrano clobrano Awaiting requested review from clobrano

Assignees

@mshitrit mshitrit

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@razo7 @slintes @clobrano @mshitrit @openshift-merge-robot