Skip to content

irmin-tezos (test_gc): Add check_async_unlinked that checks if a file has been unlinked asynchronously.#2230

Merged
adatario merged 1 commit intomirage:mainfrom
adatario:fix-async-unlink-test
Mar 30, 2023
Merged

irmin-tezos (test_gc): Add check_async_unlinked that checks if a file has been unlinked asynchronously.#2230
adatario merged 1 commit intomirage:mainfrom
adatario:fix-async-unlink-test

Conversation

@adatario
Copy link
Contributor

This fixes flakey test failures introduced by #2221 caused by files still existing that are being unlinked asynchronously (see #2227 (comment)).

The introduced check_asyn_unlinked function takes a timeout argument and checks if the file has been unlinked within the specified timeout period.

Tested locally:

  • Use negative timeout to check failure
  • Use small timeout to reproduce condition that should be catched.

I've run the tests multiple times and can not reproduce the conditions observed in the CI. Still, I believe this should fix the issue.

…le has been unlinked asynchronously.

This fixes flakey test failures introduced by mirage#2221 caused by files
still existing that are being unlinked asynchronously.

The introduced `check_asyn_unlinked` function takes a timeout argument
and checks if the file has been unlinked within the specified timeout
period.
@adatario adatario added the no-changelog-needed No changelog is needed here label Mar 29, 2023
Copy link
Contributor

@art-w art-w left a comment

Choose a reason for hiding this comment

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

Thanks! I agree, it's nice that your solution still tests the real async code... Let's see if it works, we can do something more fancy if we discover that the test is still flaky :)

Copy link
Member

@metanivek metanivek left a comment

Choose a reason for hiding this comment

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

Reasonable solution! We can always re-visit if it turns out to be flaky (my eye is on any upcoming opam release 😅).

Left a joke suggestion (but feel free to implement!)

val check_removed : t -> S.commit -> string -> unit Lwt.t
end

let rec check_async_unlinked ?(timeout = 3.141) file =
Copy link
Member

Choose a reason for hiding this comment

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

😼

Suggested change
let rec check_async_unlinked ?(timeout = 3.141) file =
let rec check_async_unlinked ?(timeout = Float.pi) file =

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:) I'll keep that for a future update.

@adatario adatario merged commit be961dc into mirage:main Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog-needed No changelog is needed here

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants