code review 8202043: go.exp/fsnotify: make test more robust if directory was...

go.exp/fsnotify: use proper temporary directories in tests

[albert.strasheim, 2013-03-30 21:04:25]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go.exp

[dave_cheney.net, 2013-04-01 12:46:14]

Hi, I'm not certain this is the correct fix. Why are the directories present before the test run in the first place ? It looks like there is a constant name, and the directory is being created relative to the source. Shouldn't the directories be mktemp -d 's ? A while back I started this package which might have some testing harness that is useful to create the structure for doing these sort of tests. Maybe it is useful, https://github.com/davecheney/fswatch/blob/master/fswatch_test.go. Cheers Dave

[albert.strasheim, 2013-04-01 12:52:15]

On 2013/04/01 12:46:14, dfc wrote: > I'm not certain this is the correct fix. Why are the directories present before > the test run in the first place ? It looks like there is a constant name, and > the directory is being created relative to the source. Shouldn't the directories > be mktemp -d 's ? Thanks, I agree. I'll look at fixing it to use ioutil.Tempdir. The need for this fix came about when I was running a large batch of tests at the same time and sometimes killing them. This was the only test I found that left stuff behind in a way that prevented it from passing when you reran it. Regards Albert

[dave_cheney.net, 2013-04-01 12:53:47]

> Thanks, I agree. I'll look at fixing it to use ioutil.Tempdir. > > The need for this fix came about when I was running a large batch of > tests at the same time and sometimes killing them. This was the only > test I found that left stuff behind in a way that prevented it from > passing when you reran it. That is what I suspected. Maybe it is as simple as changing the value of tempDir inside the test to be a _real_ temporary value. 

[albert.strasheim, 2013-04-06 12:30:38]

PTAL

[albert.strasheim, 2013-04-06 12:43:47]

I noticed that TestFsnotifyDeleteWatchedDir is leaking a file descriptor: strace -f -q -e inotify_init,open,close ./fsnotify.test -test.cpu=1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1 -test.v -test.run=TestFsnotifyDeleteWatchedDir SIGABRT shows that all the readEvents goroutines (which are in charge of closing the inotify fd) are still stuck in their read syscall. This seems like a problem with the current design of Watcher. readEvents is blocked and can't see the closed done channel.

[gobot, 2013-12-20 16:21:39]

Replacing golang-dev with golang-codereviews.

[nathany, 2014-01-30 02:57:15]

On 2013/12/20 16:21:39, gobot wrote: > Replacing golang-dev with golang-codereviews. Chris Howey changed the tests to use a temp dir a while back: https://github.com/howeyc/fsnotify/issues/57 I've submitted a CL to bring all the changes made on GitHub here: https://codereview.appspot.com/58500043

[gobot, 2014-01-30 22:36:31]

R=golang-dev (assigned by dave@cheney.net)

[nathany, 2014-02-01 04:26:17]

On 2013/04/06 12:43:47, albert.strasheim wrote: > I noticed that TestFsnotifyDeleteWatchedDir is leaking a file descriptor: > > strace -f -q -e inotify_init,open,close ./fsnotify.test > -test.cpu=1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1 > -test.v -test.run=TestFsnotifyDeleteWatchedDir > > SIGABRT shows that all the readEvents goroutines (which are in charge of closing > the inotify fd) are still stuck in their read syscall. > > This seems like a problem with the current design of Watcher. readEvents is > blocked and can't see the closed done channel. I believe there was an attempt to fix this, but it was reverted: https://github.com/howeyc/fsnotify/commit/7beb45183d2ab48bfc9616d9a019513a123... Perhaps consider filing a new issue under go.exp/fsnotify? https://code.google.com/p/go/issues/list?q=fsnotify Once CL https://codereview.appspot.com/58500043 is applied, all development should be happening at go.exp/fsnotify, hopefully avoiding duplication of effort. I'm active on the GitHub project to send any issues or contributors this way.

[gobot, 2014-02-24 01:00:57]

R=close (assigned by dave@cheney.net)