Skip to content

Conversation

emanuel-skrenkovic
Copy link
Contributor

#62 (comment)

PR to tests fsnotify style test scripts branched the main branch, as discussed in link above.

@arp242
Copy link
Member

arp242 commented May 3, 2024

It seems the main bit that's missing here is that it doesn't actually print and parse the list of events? It seems always 0:

        have:
        	0        /
        	0        /file
        want:
        	0        /file

Here's what I would do:

  1. Fix that.
  2. Just remove all tests, as the behaviours seem too different.
  3. Start adding them back one-by-one, fixing up the Output, removing the cross-platform bits that are no longer needed.

You can also use a different approach if that works better for you, but the end result should be something that has tests for at least a bunch of common situations: "make a new directory", "remove watched directory", etc. etc.

The tests are based on on the behaviour of a singular watcher across paths. fsevents event streams are simply not used in such a manner - they natively work recursively. There is no need to test for nested path handling.
The test runs inconsistently. At times fsevents reports all the delete events, at other times it only reports the root directory remove. Removing for now.
@emanuel-skrenkovic
Copy link
Contributor Author

emanuel-skrenkovic commented May 4, 2024

Added some tests now. The tests that are here run and pass (locally).

Behaviour is different from fsnotify, and I've mostly just replaced the test asserts with what is happening. These aren't really correctness tests at this point, just regression.

I also haven't really had anything to compare against, as there are not too many other tests here. That is something to keep in mind.

I was watching what was happening for each test though, and the events emitted do make sense with regards to what the test does, and what I could find on how fsevents work on mac os.

@arp242
Copy link
Member

arp242 commented May 4, 2024

Behaviour is different from fsnotify, and I've mostly just replaced the test asserts with what is happening. These aren't really correctness tests at this point, just regression. I also haven't really had anything to compare against, as there are not too many other tests here. That is something to keep in mind.

Yeah, that's fine. The important bit here is to ensure the behaviour doesn't radically change and that it doesn't panic.

Also, can you add a test like so (completely untested code for illustrative purposes):

func TestMany(t *testing.T) {
	tmp := t.TempDir()
	var wg sync.WaitGroup
	wg.Add(300)
	for i := 0; i<100; i++ {
		i := i
		go func() { defer wg.Done(); echoAppend(fmt.Sprintf(%s/file-%d", tmp, i)) }()
		go func() { defer wg.Done(); mkdir(fmt.Sprintf(%s/dir-%d", tmp, i)) }()
		go func() { defer wg.Done(); echoAppend(fmt.Sprintf(%s/dir-%d/file-%d", tmp, i)) }()
	}
	wg.Wait()

	wg.Add(200)
	for i := 0; i<100; i++ {
		i := i
		go func() { defer wg.Done(); remove(fmt.Sprintf(%s/file-%d", tmp, i)) }()
		go func() { defer wg.Done(); removeAll(fmt.Sprintf(%s/dir-%d", tmp, i)) }()
	}
	wg.Wait()

	// Check if you have 100 writes for new files, 100 removes, etc.
}

That is, create 100 files and directories simultaneously in a watched directory, then remove them, and ensure we've got the right events.

After that and the minor comments it should be good to go.

@emanuel-skrenkovic
Copy link
Contributor Author

Alright, I think this is mostly it.

I also wrote a short description of the test scripts in the readme.

I think I went through all your PR comments, also cleaned up some comments in code to remove stuff that was made irrelevant by being different from initial fsnotify code.

Let me know if I've missed something.

@arp242 arp242 merged commit 1e71d45 into fsnotify:main May 7, 2024
@arp242
Copy link
Member

arp242 commented May 7, 2024

Seems good; thanks!

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