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

Fix flaky write_with_sse_kms_key_id_ok test #1140

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

c-hagem
Copy link
Contributor

@c-hagem c-hagem commented Nov 15, 2024

Fixes the flakiness of write_with_sse_kms_key_id_ok test, which was caused by not properly unmounting and dropping child.

This is not a breaking change; no changelog entry required (as this just fixes a test).

Before this change, this test fails in ~10 out of 100 runs, after this change it fails 0 times out of 100 runs.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

Signed-off-by: Christian Hagemeier <[email protected]>
@c-hagem c-hagem temporarily deployed to PR integration tests November 15, 2024 10:12 — with GitHub Actions Inactive
@c-hagem c-hagem temporarily deployed to PR integration tests November 15, 2024 10:12 — with GitHub Actions Inactive
@c-hagem c-hagem temporarily deployed to PR integration tests November 15, 2024 10:12 — with GitHub Actions Inactive
@c-hagem c-hagem temporarily deployed to PR integration tests November 15, 2024 10:12 — with GitHub Actions Inactive
@c-hagem c-hagem temporarily deployed to PR integration tests November 15, 2024 10:12 — with GitHub Actions Inactive
@c-hagem c-hagem temporarily deployed to PR integration tests November 15, 2024 10:12 — with GitHub Actions Inactive
@c-hagem c-hagem temporarily deployed to PR integration tests November 15, 2024 10:12 — with GitHub Actions Inactive
let f_name = "f.txt";
write_to_file(mount_point.path(), f_name).expect("should be able to write to the file");
unmount(&mount_point);
wait_for_exit(child);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the right example of an error which you're intending to fix?

In which case, I am not sure if I understood the fix. The test fails before wait_for_exit: in write_to_file, called on line 833. Do you mind explaining the fix in more detail? (it's a good practice to add a comment in code with such explanation)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we should add a comment since it looks like we're doing the right thing everywhere else. However, I agree that the fix here is currently unclear and we should make sure that the PR description (and later this will be the commit) includes why this was a problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, no comments needed and if it helps to resolve issues locally, it would be great to merge this and see if it helps in the CI. Just curious about the idea behind the fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've set this to merge for now since its bringing this test consistent with the other tests. I'm still surprised that this would fix the issue.

@dannycjones dannycjones added this pull request to the merge queue Nov 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 26, 2024
@dannycjones dannycjones added this pull request to the merge queue Nov 26, 2024
Merged via the queue into awslabs:main with commit 896a10b Nov 26, 2024
23 checks passed
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