Rewrite tests #478
Rewrite tests #478
Conversation
This rewrites quite a lot of tests to be much more easily readable. While working on #472 I wanted to check "how do renames behave now?", and found this quite hard as most test cases were >90% "plumbing", and seeing "what file operations does this do?" and "what events do we get?" was not very easy. So refactor the lot, based on some work I did in #472: - Add a bunch of "shell-like" helper functions so you're not forever typing error checks, filepath.Join(), and eventSeparator(). Just touch(t, tmp, "file") will create a file in tmp. - Add eventCollector type which will collect all events in in a slice, replacing the previous "counter". This also ensures that the Watcher is closed within a second (this removes a lot of duplicate code). This is also much more precise than merely counting events; before random events could get emitted but if you weren't counting those then you'd never know. Downside is that some tests are a bit more flaky now as some behaviours are not always consistent in various edge cases; these are pre-existing bugs. - Add Events (plural) type (only for tests), and helper function to create this from a string like: REMOVE /link CREATE /link WRITE /link Which makes seeing which events are received, diffing them, etc. much easier. - Add Parallel() for most tests; reduces runtime on my system from ~12 seconds to ~6 seconds. All in all it reduces the integrations_test.go from 1279 lines to 405 lines and it's quite easy to see which events are expected for which operations, which should make things a lot easier going forward.
|
This PR changes ... a lot. It's probably easiest to look at the new versions of integration_test.go and helpers_test.go instead of the diff: https://github.com/fsnotify/fsnotify/blob/a9c46d1aeb606084031b22e27e03f5c5296fbbcd/helpers_test.go |
helpers_test.go
Outdated
| if i > 0 { | ||
| b.WriteString("\n") | ||
| } | ||
| fmt.Fprintf(b, "%-20s %q", ee.Op.String(), strings.ReplaceAll(ee.Name, `\`, "/")) |
There was a problem hiding this comment.
Ah cheers, I didn't know about that function. Updated it.
|
In some tests, it appears that goroutine is not waiting for termination. This can be caused unexpected behavior when running a series of tests, which can be difficult to figure out. I don't have strong opinion but it have better to be fixed, I think in this or another pull-request. Thanks. |
Which tests specifically do you mean? I think all of those that use the new |
Sorry, I couldn't find the tests except two.
|
|
My apologies @mattn, but I'm really confused what you mean 😅 You're saying the bug is in the eventCollector? I looked over it again, and I'm not sure if I see the problem, but I'm probably missing something. I'll go ahead and merge this for now, so I can get ahead with some other things and add test cases and such; but let me know and I'll fix it up! |
This rewrites quite a lot of tests to be much more easily readable.
While working on #472 I wanted to check "how do renames behave now?",
and found this quite hard as most test cases were >90% "plumbing", and
seeing "what file operations does this do?" and "what events do we get?"
was not very easy.
So refactor the lot, based on some work I did in #472:
Add a bunch of "shell-like" helper functions so you're not forever
typing error checks, filepath.Join(), and eventSeparator(). Just
touch(t, tmp, "file") will create a file in tmp.
Add eventCollector type which will collect all events in in a slice,
replacing the previous "counter". This also ensures that the Watcher
is closed within a second (this removes a lot of duplicate code).
This is also much more precise than merely counting events; before
random events could get emitted but if you weren't counting those then
you'd never know.
Downside is that some tests are a bit more flaky now as some
behaviours are not always consistent in various edge cases; these are
pre-existing bugs.
Add Events (plural) type (only for tests), and helper function to
create this from a string like:
Which makes seeing which events are received, diffing them, etc. much
easier.
Add Parallel() for most tests; reduces runtime on my system from ~12
seconds to ~6 seconds.
All in all it reduces the integrations_test.go from 1279 lines to 405
lines and it's quite easy to see which events are expected for which
operations, which should make things a lot easier going forward.