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

Make MDX tests idempotent #601

Merged
merged 1 commit into from
Oct 6, 2023
Merged

Conversation

SGrondin
Copy link
Collaborator

@SGrondin SGrondin commented Aug 12, 2023

At the moment, developing with dune runtest -w is not the most pleasant experience. The first test run will succeed, but the subsequent runs of many tests (fs.md, process.md, README.md) will fail with various Already_exists and Not_found errors.

This PR cleans up those leftover artifacts to ensure accurate test outcomes.

I first tried to do something more automatic, where it would detect the previous artifacts, so that we won't have to remember to invoke the cleanup function with the right paths, but the result was more brittle than this. I wanted to make sure it would work even if the test gets interrupted, so the cleanup has to happen before the test run, not after it. Please let me know if you can think of a better approach.

Copy link
Collaborator

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

Thanks - this is a nice improvement (not sure why I've never hit this problem myself).

README.md Outdated Show resolved Hide resolved
tests/fs.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@SGrondin
Copy link
Collaborator Author

@talex5 thanks for the comments! I'll add rm_r to Path and make the changes.

@SGrondin
Copy link
Collaborator Author

@talex5 I realize now why I didn't think to put rm_r/rmtree into Path: because it needs to be able to empty directories recursively, and it can't do that very well if symbolic links are present because the Path module functions always follow the symbolic links. I can't use Path.unlink on a symbolic link, I have to use the blocking Unix.unlink in a thread.

But I fully agree that such a function needs to be part of Path, I've already had to implement it 3 times in 3 different codebases. I have a few questions first in order to finish this PR:

  • Q1: Should I add the function to Path as part of this PR? I think yes, since it would keep things simple.
  • Q2: Should I implement the function by calling Unix.unlink in a new systhread (for now until Add a systhread pool of workers for eio_posix #607 is fixed) when removing symbolic links? Or should just forego adding anything to Path for now until Add a systhread pool of workers for eio_posix #607 is fixed?
    • Q3: Wouldn't a Workpool be a clean way to implement a thread pool for blocking system calls? If so I would be interested in making the changes to Eio_posix
  • Q4: the name. I personally prefer rm_r because rmtree doesn't follow OCaml's snake case convention. rm_tree sounds like an alias of the inexistent Tree.rm. To my eyes, rm_r looks more OCaml-y and there's no confusion about its meaning were I not familiar with Eio. Ruby uses rm_r as well. That said, I think rmtree is perfectly acceptable if you insist!

@talex5
Copy link
Collaborator

talex5 commented Aug 18, 2023

I realize now why I didn't think to put rm_r/rmtree into Path: because it needs to be able to empty directories recursively, and it can't do that very well if symbolic links are present because the Path module functions always follow the symbolic links.

Right, this is why we need #599 first. That can be merged once ocaml/opam-repository#24292 is accepted into opam-repository.

I can't use Path.unlink on a symbolic link, I have to use the blocking Unix.unlink in a thread.

Are you sure? Path.unlink works for me.

Regarding the name, Lwt calls this delete_recursively
https://github.com/ocsigen/lwt/blob/2eee2a1b9e386cc0eacf455a5a9356150f920edb/src/unix/lwt_io.mli#L550

Another option would be Path.unlink ~recursive:true.

@SGrondin
Copy link
Collaborator Author

SGrondin commented Aug 18, 2023

I like delete_recursively a lot 👍 Dangerous/powerful functions deserve longer, unambiguous names.

If I recall correctly, Path.unlink is unable to remove a symbolic link if that link points to something outside of cwd. It's why I had to resort to Unix.unlink in this PR. It looked as if Path.unlink was designed to seamlessly see through symbolic links, but now I understand it might just be a bug that occurs when the link points to something outside cwd.

@SGrondin
Copy link
Collaborator Author

I just tested symlinks Path operations every way I could think of, and I can confirm that it all works as expected. My earlier confusion came from the fact that I was statting with Path.with_open_in path Eio.File.stat and that's absolutely going to follow the symlink!

@SGrondin
Copy link
Collaborator Author

I've just made several improvements based on the previous review. I'll complete development of this PR as soon as #599 is merged.

@talex5
Copy link
Collaborator

talex5 commented Sep 26, 2023

Stat support is now merged (as #620), so we can unpause this PR now!

@SGrondin
Copy link
Collaborator Author

Fantastic! I'll probably work on it this Saturday.

Comment on lines +2 to +3
_opam
.ocamlformat
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added for developer comfort 😄
Let me know if you'd rather not and I'll remove it.

@SGrondin
Copy link
Collaborator Author

@talex5 Updated! See my comments on Element about kind and rmtree. If you do decide to make changes to those 2 functions I'll want to update this PR. If not, then consider this PR ready.

@talex5 talex5 force-pushed the idempotent-tests branch 2 times, most recently from b0fccb8 to 0e0d739 Compare October 6, 2023 09:40
Copy link
Collaborator

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

I rebased this on top of #628 and updated it for the new stat support. Looks good to merge once #628 is in - thanks!

@SGrondin
Copy link
Collaborator Author

SGrondin commented Oct 6, 2023

Once this is merged we need to start requesting changes on PRs adding tests without proper usage of the ~clear argument. It's pretty easy to tell just by reading the test.

@talex5 talex5 merged commit 69c622f into ocaml-multicore:main Oct 6, 2023
3 of 5 checks passed
@talex5
Copy link
Collaborator

talex5 commented Oct 6, 2023

Thanks!

@SGrondin SGrondin deleted the idempotent-tests branch October 6, 2023 21:05
talex5 added a commit to talex5/opam-repository that referenced this pull request Nov 2, 2023
CHANGES:

New features / API changes:

- Add `Flow.read_all` (@SGrondin ocaml-multicore/eio#596, reviewed by @talex5 @rbjorklin).

- Add `Path.stat` (@patricoferris @talex5 @avsm ocaml-multicore/eio#617 ocaml-multicore/eio#618 ocaml-multicore/eio#624 ocaml-multicore/eio#620, reviewed by @SGrondin).

- Add `Path.rmtree` (@talex5 ocaml-multicore/eio#627 ocaml-multicore/eio#628, reviewed by @SGrondin).

- Add `Path.mkdirs` and `Path.split` (@patricoferris @talex5 ocaml-multicore/eio#625).

- Add `Eio.File.{seek,sync,truncate}` (@talex5 ocaml-multicore/eio#626).

- Add `Eio.Path.{kind,is_file,is_directory}` (@patricoferris @talex5 ocaml-multicore/eio#623, reviewed by @avsm).

- Switch from CTF to OCaml 5.1 runtime events (@TheLortex @patricoferris @talex5 ocaml-multicore/eio#634 ocaml-multicore/eio#635, reviewed by @avsm).
  This is a minimal initial version.

Documentation:

- Document `File.Stat` record fields (@avsm @talex5 ocaml-multicore/eio#621).

- Update README section about `env` (@talex5 ocaml-multicore/eio#614, reported by @jonsterling).

Build and test changes:

- Add `File.stat` benchmark (@talex5 ocaml-multicore/eio#616).

- Add `Path.stat` benchmark (@patricoferris @talex5 ocaml-multicore/eio#630).

- eio_linux: mark as only available on Linux (@talex5 ocaml-multicore/eio#629).

- Make MDX tests idempotent (@SGrondin ocaml-multicore/eio#601, reviewed by @talex5).

- Allow trailing whitespace in CHANGES.md (@talex5 ocaml-multicore/eio#632).

- Update minimum OCaml version to 5.1 (@talex5 ocaml-multicore/eio#631).

- Generate prototypes for C stubs from ml files (@talex5 ocaml-multicore/eio#615).

- Don't try to compile uring support on centos 7 (@talex5 ocaml-multicore/eio#638, reported by @zenfey).
talex5 added a commit to talex5/opam-repository that referenced this pull request Nov 2, 2023
CHANGES:

New features / API changes:

- Add `Flow.read_all` (@SGrondin ocaml-multicore/eio#596, reviewed by @talex5 @rbjorklin).

- Add `Path.stat` (@patricoferris @talex5 @avsm ocaml-multicore/eio#617 ocaml-multicore/eio#618 ocaml-multicore/eio#624 ocaml-multicore/eio#620, reviewed by @SGrondin).

- Add `Path.rmtree` (@talex5 ocaml-multicore/eio#627 ocaml-multicore/eio#628, reviewed by @SGrondin).

- Add `Path.mkdirs` and `Path.split` (@patricoferris @talex5 ocaml-multicore/eio#625).

- Add `Eio.File.{seek,sync,truncate}` (@talex5 ocaml-multicore/eio#626).

- Add `Eio.Path.{kind,is_file,is_directory}` (@patricoferris @talex5 ocaml-multicore/eio#623, reviewed by @avsm).

- Switch from CTF to OCaml 5.1 runtime events (@TheLortex @patricoferris @talex5 ocaml-multicore/eio#634 ocaml-multicore/eio#635, reviewed by @avsm).
  This is a minimal initial version.

Documentation:

- Document `File.Stat` record fields (@avsm @talex5 ocaml-multicore/eio#621).

- Update README section about `env` (@talex5 ocaml-multicore/eio#614, reported by @jonsterling).

Build and test changes:

- Add `File.stat` benchmark (@talex5 ocaml-multicore/eio#616).

- Add `Path.stat` benchmark (@patricoferris @talex5 ocaml-multicore/eio#630).

- eio_linux: mark as only available on Linux (@talex5 ocaml-multicore/eio#629).

- Make MDX tests idempotent (@SGrondin ocaml-multicore/eio#601, reviewed by @talex5).

- Allow trailing whitespace in CHANGES.md (@talex5 ocaml-multicore/eio#632).

- Update minimum OCaml version to 5.1 (@talex5 ocaml-multicore/eio#631).

- Generate prototypes for C stubs from ml files (@talex5 ocaml-multicore/eio#615).

- Don't try to compile uring support on centos 7 (@talex5 ocaml-multicore/eio#638, reported by @zenfey).
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
CHANGES:

New features / API changes:

- Add `Flow.read_all` (@SGrondin ocaml-multicore/eio#596, reviewed by @talex5 @rbjorklin).

- Add `Path.stat` (@patricoferris @talex5 @avsm ocaml-multicore/eio#617 ocaml-multicore/eio#618 ocaml-multicore/eio#624 ocaml-multicore/eio#620, reviewed by @SGrondin).

- Add `Path.rmtree` (@talex5 ocaml-multicore/eio#627 ocaml-multicore/eio#628, reviewed by @SGrondin).

- Add `Path.mkdirs` and `Path.split` (@patricoferris @talex5 ocaml-multicore/eio#625).

- Add `Eio.File.{seek,sync,truncate}` (@talex5 ocaml-multicore/eio#626).

- Add `Eio.Path.{kind,is_file,is_directory}` (@patricoferris @talex5 ocaml-multicore/eio#623, reviewed by @avsm).

- Switch from CTF to OCaml 5.1 runtime events (@TheLortex @patricoferris @talex5 ocaml-multicore/eio#634 ocaml-multicore/eio#635, reviewed by @avsm).
  This is a minimal initial version.

Documentation:

- Document `File.Stat` record fields (@avsm @talex5 ocaml-multicore/eio#621).

- Update README section about `env` (@talex5 ocaml-multicore/eio#614, reported by @jonsterling).

Build and test changes:

- Add `File.stat` benchmark (@talex5 ocaml-multicore/eio#616).

- Add `Path.stat` benchmark (@patricoferris @talex5 ocaml-multicore/eio#630).

- eio_linux: mark as only available on Linux (@talex5 ocaml-multicore/eio#629).

- Make MDX tests idempotent (@SGrondin ocaml-multicore/eio#601, reviewed by @talex5).

- Allow trailing whitespace in CHANGES.md (@talex5 ocaml-multicore/eio#632).

- Update minimum OCaml version to 5.1 (@talex5 ocaml-multicore/eio#631).

- Generate prototypes for C stubs from ml files (@talex5 ocaml-multicore/eio#615).

- Don't try to compile uring support on centos 7 (@talex5 ocaml-multicore/eio#638, reported by @zenfey).
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.

2 participants