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

Change e2e Tests Retry to iteration count #351

Closed
joe-kimmel-vmw opened this issue Aug 25, 2021 · 3 comments · Fixed by #1453
Closed

Change e2e Tests Retry to iteration count #351

joe-kimmel-vmw opened this issue Aug 25, 2021 · 3 comments · Fixed by #1453
Labels
carvel-accepted This issue should be considered for future work and that the triage process has been completed good first issue An issue that will be a good candidate for a new contributor kind/cleanup engineering focused non-feature work priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. snack This issue has been identified as lightweight and potentially quick to deliver.

Comments

@joe-kimmel-vmw
Copy link
Contributor

Currently our e2e tests use a time-based retry (see e.g. https://github.com/vmware-tanzu/carvel-kapp-controller/blob/develop/test/e2e/kappcontroller/app_secret_configmap_reconcile_test.go#L98 ) , but time is notoriously slippery, especially in "the cloud" (see also https://gist.github.com/timvisee/fcda9bbdff88d45cc9061606b4b923ca ).

Very likely we can replace the existing retry function with a counter that takes an integer (e.g. 10) instead of a time duration (e.g. 10 * time.seconds) and still calls Sleep(1) ten times.

Ideally after this task none of our e2e tests will rely on comparing a start time with the current time to determine whether they've retried enough, but if there's one or two cases that are thornier i think an incremental improvement will also be greatly appreciated.

@joe-kimmel-vmw joe-kimmel-vmw added good first issue An issue that will be a good candidate for a new contributor carvel-triage This issue has not yet been reviewed for validity labels Aug 25, 2021
@aaronshurley aaronshurley added carvel-accepted This issue should be considered for future work and that the triage process has been completed priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. snack This issue has been identified as lightweight and potentially quick to deliver. and removed carvel-triage This issue has not yet been reviewed for validity labels Aug 25, 2021
@aaronshurley
Copy link
Contributor

Thanks for creating this @joe-kimmel-vmw! Accepting this and prioritizing it as important-longterm since this will provide engineering improvements but isn't something that we need to address too soon. Moving it towards the bottom of the prioritized backlog. We're open to contributions!

@joe-kimmel-vmw joe-kimmel-vmw added the kind/cleanup engineering focused non-feature work label Aug 26, 2021
@satyazzz123
Copy link

hello @joe-kimmel-vmw I am new to carvel-dev can you guide me a little on how to resolve this issue . Thank you.

@jignyasamishra
Copy link
Contributor

@joe-kimmel-vmw I have raised a PR . Please have a look. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
carvel-accepted This issue should be considered for future work and that the triage process has been completed good first issue An issue that will be a good candidate for a new contributor kind/cleanup engineering focused non-feature work priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. snack This issue has been identified as lightweight and potentially quick to deliver.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants