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

Toggle event processing through the filesystem #30

Merged
merged 1 commit into from
Dec 21, 2023

Conversation

aochagavia
Copy link
Collaborator

(See the readme for details on how to use the feature)

I'm a bit torn on the testing side of this. The feature watches for creation / deletion of a pause file relative to the cwd. If we simply create a pause file in one of our tests, other tests detect it as well because they all share the same cwd (and if an unrelated test detects the pause file, it stops processing events and fails).

The straightforward solution is to use std::env::set_current_dir in the test that exercises this feature, setting it to a unique directory so we can create the pause file without affecting other tests. However, cargo runs all tests in a single multi-threaded process, so changing the cwd will by default also change it for everyone else (failing to achieve the isolation we want). Fortunately, we can use libc::unshare(libc::CLONE_FS) to make std::env::set_current_dir affect only the current test's thread, finally giving us isolation (it works!). The problem is that libc::unshare is not a portable solution (it doesn't exist on MacOS or Windows, so anyone who wants to contribute and uses a non-Linux system would fail to build the tests locally).

I'm inclined to keep the code as is and maybe add a #[cfg(...)] attribute to avoid compiling this specific test on platforms other than Linux. What do you think?

Here are some alternative options:

  1. Modify the code so the path to the pause file is provided through configuration, then ensure that each test gets a unique path. I'm not a fan of this, because we don't really want to configure the path. It feels like introducing complexity in the production code for the sake of testing.
  2. Extract the filesystem related functionality and expose it through a trait, so we can mock it out in our tests. Similar to the previous point, this feels like introducing complexity in the production code for the sake of testing. Besides, I'd rather test the real thing than a mock.

Side note: it looks like the build already fails on Windows, due to a bencher.dev issue 😢.

Closes #23

@cpu cpu self-requested a review December 20, 2023 15:45
Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

This looks great to me, thanks! TIL about unshare :-) That's a neat trick.

I'm inclined to keep the code as is and maybe add a #[cfg(...)] attribute to avoid compiling this specific test on platforms other than Linux. What do you think?

Totally agree - I think the alternatives you listed are interesting but don't strike the right balance for this project.

See the readme for details on how to use the feature
@aochagavia aochagavia merged commit 3b60390 into main Dec 21, 2023
9 checks passed
@aochagavia aochagavia deleted the postpone-benchmarks branch December 21, 2023 15:51
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.

Consider implementing a way to postpone running benchmarks
2 participants