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

Rework process dump cri-containerd tests #1267

Merged
merged 2 commits into from
Jan 11, 2022

Conversation

dcantah
Copy link
Contributor

@dcantah dcantah commented Jan 7, 2022

This change aims to de-flake the Windows process dump test by continuously checking for a .dmp instead of waiting some arbitrary amount of time and then checking once. While we haven't seen any flakes with the Linux version of this test for generating core dumps, the same logic is employed for the LCOW test as well to keep it consistent.

This change aims to deflake the  Windows process dump test by continuously
checking for a .dmp instead of waiting some arbitrary amount of time and
then checking once. While we haven't seen any flakes with the Linux version
of this test for generating coredumps, the same logic is employed for the
LCOW test as well.

Signed-off-by: Daniel Canter <dcanter@microsoft.com>
@dcantah dcantah requested a review from a team as a code owner January 7, 2022 02:50
@dcantah
Copy link
Contributor Author

dcantah commented Jan 7, 2022

@ambarve Assigning you as you were the one who found this originally I believe 😆

Comment on lines 1492 to 1495
for {
if done {
break
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we could use for !done here instead

Copy link
Contributor Author

@dcantah dcantah Jan 11, 2022

Choose a reason for hiding this comment

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

Oh great point, I'll change and check in after

Copy link
Contributor

@katiewasnothere katiewasnothere left a comment

Choose a reason for hiding this comment

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

lgtm

Change from breaking out of for loop when done to a `for !done {}` approach.

Signed-off-by: Daniel Canter <dcanter@microsoft.com>
@dcantah dcantah merged commit 5e7e3bb into microsoft:master Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants