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

Ignore some filesystem events emitted by inotify #86

Merged
merged 3 commits into from
Mar 5, 2024

Conversation

jwilger
Copy link
Contributor

@jwilger jwilger commented Feb 20, 2024

The :isdir, :closed, and :attribute events likely don't need to be monitored to run tests, and at least the :attribute event is causing problems for projects that use the Domo library for validation. This is because the Domo library causes a delayed file read event to occur on some elixir files in the lib directory, which causes their atime attribute to be updated. This is happening after the test run kicks off, and then it causes a second test run to occur.

The :isdir, :closed, and :attribute events likely don't need to be
monitored to run tests, and at least the :attribute event is causing
problems for projects that use the Domo library for validation. This is
because the Domo library causes a delayed file read event to occur on
some elixir files in the lib directory, which causes their atime
attribute to be updated. This is happening after the test run kicks off,
and then it causes a second test run to occur.
Copy link
Owner

@randycoulman randycoulman left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I think the concept is good, but I'd like to adjust the implementation a bit so that it also has the intended effect on MacOS. See my comment below.

@@ -11,6 +11,8 @@ defmodule MixTestInteractive.Watcher do

require Logger

@ignore_events MapSet.new([:isdir, :closed, :attribute, :undefined])
Copy link
Owner

Choose a reason for hiding this comment

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

This list looks reasonable for Linux and Windows-based systems, but not for MacOS systems. The FSMac backend supports a significantly larger set of possible events.

I wonder if it would be better to instead list the events that we care about and then reverse the logic below. For the existing backends, that would likely be: [:created, :deleted, :modified, :removed, :renamed].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense.

mix.exs Outdated
@@ -1,7 +1,7 @@
defmodule MixTestInteractive.MixProject do
use Mix.Project

@version "2.0.3"
@version "2.0.4-dev.1"
Copy link
Owner

Choose a reason for hiding this comment

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

Please don't adjust the version number here. I'll do that as part of the release process.

Because the list of events that should trigger a test run is potentially
much smaller than the list of all possible filesystem events,
particularly on platforms such as MacOS, it is much simpler to specify
the list of events we *do* want to process rather than the list of
events we want to ignore.

N.B. the `FileSystem` module does not completely normalize the names of
the events between platforms, so our list of trigger events includes a
few items that look similar in order to support all platforms.
Copy link
Owner

@randycoulman randycoulman left a comment

Choose a reason for hiding this comment

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

Changes look great! Merging; I'll get a release with these changes out shortly.

Comment on lines +21 to +22
:moved_from,
:moved_to,
Copy link
Owner

Choose a reason for hiding this comment

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

Interesting! Which backend emits these two events? I didn't see them when I was looking at backends earlier.

@randycoulman randycoulman merged commit 4ab110a into randycoulman:main Mar 5, 2024
5 checks passed
@randycoulman
Copy link
Owner

Released in v2.0.4. Thanks again for the contribution!

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