code review 65870043: manualtest/filelock: extend test so only one copy need ...

leveldb/db: move manualtest/filelock to standard go test. A by-product
of this is that developers no longer need to run multiple copies of
filelock, the test spawns additional lockers as necessary.

[wathiede, 2014-02-19 04:10:00]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/leveldb-go

[gobot, 2014-02-19 19:21:48]

R=nigeltao@golang.org (assigned by r@golang.org)

[nigeltao, 2014-02-24 01:13:43]

Thanks for doing this. This was easier then I was expecting.

https://codereview.appspot.com/65870043/diff/40001/manualtest/filelock/main.go
File manualtest/filelock/main.go (right):

https://codereview.appspot.com/65870043/diff/40001/manualtest/filelock/main.g...
manualtest/filelock/main.go:22: func spawn(prog, lockFn string) error {
s/lockFn/lockFilename/ or just s/lockFn/filename/, since I first thought that Fn
was short for function.

https://codereview.appspot.com/65870043/diff/40001/manualtest/filelock/main.g...
manualtest/filelock/main.go:41: fmt.Printf("Locking %s\n", filename)
I think it would help if all of these fmt.Printf messages were prefixed with the
PID of the current process, e.g.:

pid := os.Getpid()
fmt.Printf("%6d: Locking %s\n", pid, filename)

[bradfitz, 2014-02-24 05:16:55]

Or, as a Go test:

https://github.com/camlistore/lock

https://github.com/camlistore/lock/blob/master/lock_test.go

[wathiede, 2014-03-02 03:58:00]

On 2014/02/24 05:16:55, bradfitz wrote:
> Or, as a Go test:
> 
> https://github.com/camlistore/lock
> 
> https://github.com/camlistore/lock/blob/master/lock_test.go

Ok, I've taken Brad's advice and made it a normal Go test.  I also moved it in
with the lock code, it didn't feel right to leave it in the manualtest directory
now that is isn't manual.

I chose not to include the pid information in the logging statements.  Using the
testing framework, you get this 'for free' in the form of additional indention
when a subprocess fails and it's output is printed from the parent process.

[nigeltao, 2014-03-05 05:47:18]

https://codereview.appspot.com/65870043/diff/50001/leveldb/db/file_lock_test.go
File leveldb/db/file_lock_test.go (right):

https://codereview.appspot.com/65870043/diff/50001/leveldb/db/file_lock_test....
leveldb/db/file_lock_test.go:23: // Test locks a file, spawns a second process
that attempts to grab the
s/Test/TestLock/

https://codereview.appspot.com/65870043/diff/50001/leveldb/db/file_lock_test....
leveldb/db/file_lock_test.go:58: t.Logf("Child failed to grab lock as
expected.")
Should you also check that string(out) contains fmt.Sprintf("Could not lock
%s:", filename)?

[wathiede, 2014-03-06 04:42:33]

https://codereview.appspot.com/65870043/diff/50001/leveldb/db/file_lock_test.go
File leveldb/db/file_lock_test.go (right):

https://codereview.appspot.com/65870043/diff/50001/leveldb/db/file_lock_test....
leveldb/db/file_lock_test.go:23: // Test locks a file, spawns a second process
that attempts to grab the
On 2014/03/05 05:47:18, nigeltao wrote:
> s/Test/TestLock/

Done.

https://codereview.appspot.com/65870043/diff/50001/leveldb/db/file_lock_test....
leveldb/db/file_lock_test.go:58: t.Logf("Child failed to grab lock as
expected.")
On 2014/03/05 05:47:18, nigeltao wrote:
> Should you also check that string(out) contains fmt.Sprintf("Could not lock
> %s:", filename)?

Done.

[nigeltao, 2014-03-07 05:01:27]

LGTM, thanks.

[nigeltao, 2014-03-07 05:11:12]

Wait, this doesn't work for me (linux/amd64). Does "go test" pass for you?

$ go test
--- FAIL: TestLock (0.00 seconds)
	file_lock_test.go:48: Locking /tmp/712488037
	file_lock_test.go:55: Spawning child, should fail to grab lock.
	file_lock_test.go:58: Attempt to grab open lock should have failed.
		=== RUN TestLock
		--- PASS: TestLock (0.00 seconds)
			file_lock_test.go:48: Locking /tmp/712488037
			file_lock_test.go:66: Unlocking /tmp/712488037
		PASS
FAIL
exit status 1
FAIL	code.google.com/p/leveldb-go/leveldb/db	0.040s

[wathiede, 2014-03-07 18:30:19]

On 2014/03/07 05:11:12, nigeltao wrote:
> Wait, this doesn't work for me (linux/amd64). Does "go test" pass for you?
> 
> $ go test
> --- FAIL: TestLock (0.00 seconds)
> 	file_lock_test.go:48: Locking /tmp/712488037
> 	file_lock_test.go:55: Spawning child, should fail to grab lock.
> 	file_lock_test.go:58: Attempt to grab open lock should have failed.
> 		=== RUN TestLock
> 		--- PASS: TestLock (0.00 seconds)
> 			file_lock_test.go:48: Locking /tmp/712488037
> 			file_lock_test.go:66: Unlocking /tmp/712488037
> 		PASS
> FAIL
> exit status 1
> FAIL	code.google.com/p/leveldb-go/leveldb/db	0.040s

That is very strange, I did a fresh checkout and hg clpatch'd on my Linux box at
work and this test is passing.  Is it possible you have a dirty checkout or are
running an earlier patch set?

[wathiede, 2014-03-07 21:30:02]

On 2014/03/07 18:30:19, wathiede wrote:
> On 2014/03/07 05:11:12, nigeltao wrote:
> > Wait, this doesn't work for me (linux/amd64). Does "go test" pass for you?
> > 
> > $ go test
> > --- FAIL: TestLock (0.00 seconds)
> > 	file_lock_test.go:48: Locking /tmp/712488037
> > 	file_lock_test.go:55: Spawning child, should fail to grab lock.
> > 	file_lock_test.go:58: Attempt to grab open lock should have failed.
> > 		=== RUN TestLock
> > 		--- PASS: TestLock (0.00 seconds)
> > 			file_lock_test.go:48: Locking /tmp/712488037
> > 			file_lock_test.go:66: Unlocking /tmp/712488037
> > 		PASS
> > FAIL
> > exit status 1
> > FAIL	code.google.com/p/leveldb-go/leveldb/db	0.040s
> 
> That is very strange, I did a fresh checkout and hg clpatch'd on my Linux box
at
> work and this test is passing.  Is it possible you have a dirty checkout or
are
> running an earlier patch set?

Here's a clue (on Linux):
$ ~/Downloads/go-tip/bin/go version
go version devel +26aa53304a48 Fri Mar 07 16:08:12 2014 -0500 linux/amd64

$ ~/Downloads/go-tip/bin/go test
--- FAIL: TestLock (0.00 seconds) 
	file_lock_test.go:48: Locking /tmp/019631719
	file_lock_test.go:55: Spawning child, should fail to grab lock.
	file_lock_test.go:58: Attempt to grab open lock should have failed.
		=== RUN TestLock
		--- PASS: TestLock (0.00 seconds)
			file_lock_test.go:48: Locking /tmp/019631719
			file_lock_test.go:66: Unlocking /tmp/019631719
		PASS
FAIL
exit status 1
FAIL	code.google.com/p/leveldb-go/leveldb/db	0.047s

$ ~/Downloads/go/bin/go version    
go version go1.2.1 linux/amd64

$ ~/Downloads/go/bin/go test    
PASS
ok  	code.google.com/p/leveldb-go/leveldb/db	0.055s



Are you running with tip?  I think maybe this test is working well :)

[wathiede, 2014-03-07 21:41:51]

On 2014/03/07 21:30:02, wathiede wrote:
> On 2014/03/07 18:30:19, wathiede wrote:
> > On 2014/03/07 05:11:12, nigeltao wrote:
> > > Wait, this doesn't work for me (linux/amd64). Does "go test" pass for you?
> > > 
> > > $ go test
> > > --- FAIL: TestLock (0.00 seconds)
> > > 	file_lock_test.go:48: Locking /tmp/712488037
> > > 	file_lock_test.go:55: Spawning child, should fail to grab lock.
> > > 	file_lock_test.go:58: Attempt to grab open lock should have failed.
> > > 		=== RUN TestLock
> > > 		--- PASS: TestLock (0.00 seconds)
> > > 			file_lock_test.go:48: Locking /tmp/712488037
> > > 			file_lock_test.go:66: Unlocking /tmp/712488037
> > > 		PASS
> > > FAIL
> > > exit status 1
> > > FAIL	code.google.com/p/leveldb-go/leveldb/db	0.040s
> > 
> > That is very strange, I did a fresh checkout and hg clpatch'd on my Linux
box
> at
> > work and this test is passing.  Is it possible you have a dirty checkout or
> are
> > running an earlier patch set?
> 
> Here's a clue (on Linux):
> $ ~/Downloads/go-tip/bin/go version
> go version devel +26aa53304a48 Fri Mar 07 16:08:12 2014 -0500 linux/amd64
> 
> $ ~/Downloads/go-tip/bin/go test
> --- FAIL: TestLock (0.00 seconds) 
> 	file_lock_test.go:48: Locking /tmp/019631719
> 	file_lock_test.go:55: Spawning child, should fail to grab lock.
> 	file_lock_test.go:58: Attempt to grab open lock should have failed.
> 		=== RUN TestLock
> 		--- PASS: TestLock (0.00 seconds)
> 			file_lock_test.go:48: Locking /tmp/019631719
> 			file_lock_test.go:66: Unlocking /tmp/019631719
> 		PASS
> FAIL
> exit status 1
> FAIL	code.google.com/p/leveldb-go/leveldb/db	0.047s
> 
> $ ~/Downloads/go/bin/go version    
> go version go1.2.1 linux/amd64
> 
> $ ~/Downloads/go/bin/go test    
> PASS
> ok  	code.google.com/p/leveldb-go/leveldb/db	0.055s
> 
> 
> 
> Are you running with tip?  I think maybe this test is working well :)

Ok, one last data point (on FreeBSD):
$ ~/Downloads/go-tip/bin/go version
go version devel +26aa53304a48 Fri Mar 07 16:08:12 2014 -0500 freebsd/amd64

$ ~/Downloads/go-tip/bin/go test
PASS
ok      code.google.com/p/leveldb-go/leveldb/db 0.018s

$ ~/Downloads/go/bin/go version
go version go1.2 freebsd/amd64

$ ~/Downloads/go/bin/go test
PASS
ok      code.google.com/p/leveldb-go/leveldb/db 0.016s
$ 

So I'm guessing something with syscall handling on Linux at tip has changed, but
it isn't a problem on FreeBSD.

[wathiede, 2014-03-11 00:11:54]

On 2014/03/07 21:41:51, wathiede wrote:
> On 2014/03/07 21:30:02, wathiede wrote:
> > On 2014/03/07 18:30:19, wathiede wrote:
> > > On 2014/03/07 05:11:12, nigeltao wrote:
> > > > Wait, this doesn't work for me (linux/amd64). Does "go test" pass for
you?
> > > > 
> > > > $ go test
> > > > --- FAIL: TestLock (0.00 seconds)
> > > > 	file_lock_test.go:48: Locking /tmp/712488037
> > > > 	file_lock_test.go:55: Spawning child, should fail to grab lock.
> > > > 	file_lock_test.go:58: Attempt to grab open lock should have failed.
> > > > 		=== RUN TestLock
> > > > 		--- PASS: TestLock (0.00 seconds)
> > > > 			file_lock_test.go:48: Locking /tmp/712488037
> > > > 			file_lock_test.go:66: Unlocking /tmp/712488037
> > > > 		PASS
> > > > FAIL
> > > > exit status 1
> > > > FAIL	code.google.com/p/leveldb-go/leveldb/db	0.040s
> > > 
> > > That is very strange, I did a fresh checkout and hg clpatch'd on my Linux
> box
> > at
> > > work and this test is passing.  Is it possible you have a dirty checkout
or
> > are
> > > running an earlier patch set?
> > 
> > Here's a clue (on Linux):
> > $ ~/Downloads/go-tip/bin/go version
> > go version devel +26aa53304a48 Fri Mar 07 16:08:12 2014 -0500 linux/amd64
> > 
> > $ ~/Downloads/go-tip/bin/go test
> > --- FAIL: TestLock (0.00 seconds) 
> > 	file_lock_test.go:48: Locking /tmp/019631719
> > 	file_lock_test.go:55: Spawning child, should fail to grab lock.
> > 	file_lock_test.go:58: Attempt to grab open lock should have failed.
> > 		=== RUN TestLock
> > 		--- PASS: TestLock (0.00 seconds)
> > 			file_lock_test.go:48: Locking /tmp/019631719
> > 			file_lock_test.go:66: Unlocking /tmp/019631719
> > 		PASS
> > FAIL
> > exit status 1
> > FAIL	code.google.com/p/leveldb-go/leveldb/db	0.047s
> > 
> > $ ~/Downloads/go/bin/go version    
> > go version go1.2.1 linux/amd64
> > 
> > $ ~/Downloads/go/bin/go test    
> > PASS
> > ok  	code.google.com/p/leveldb-go/leveldb/db	0.055s
> > 
> > 
> > 
> > Are you running with tip?  I think maybe this test is working well :)
> 
> Ok, one last data point (on FreeBSD):
> $ ~/Downloads/go-tip/bin/go version
> go version devel +26aa53304a48 Fri Mar 07 16:08:12 2014 -0500 freebsd/amd64
> 
> $ ~/Downloads/go-tip/bin/go test
> PASS
> ok      code.google.com/p/leveldb-go/leveldb/db 0.018s
> 
> $ ~/Downloads/go/bin/go version
> go version go1.2 freebsd/amd64
> 
> $ ~/Downloads/go/bin/go test
> PASS
> ok      code.google.com/p/leveldb-go/leveldb/db 0.016s
> $ 
> 
> So I'm guessing something with syscall handling on Linux at tip has changed,
but
> it isn't a problem on FreeBSD.

This is the CL that seems to break it, although I have no idea why:

https://code.google.com/p/go/source/detail?r=a8b97f205736

[nigeltao, 2014-03-11 09:30:52]

On Tue, Mar 11, 2014 at 11:11 AM,  <couchmoney@gmail.com> wrote:
> This is the CL that seems to break it, although I have no idea why:
>
> https://code.google.com/p/go/source/detail?r=a8b97f205736

Huh, mysterious GC bugs.

Anyway, thanks for looking into this.

[nigeltao, 2014-03-11 09:31:24]

*** Submitted as
https://code.google.com/p/leveldb-go/source/detail?r=be79c82176ff ***

    leveldb/db: move manualtest/filelock to standard go test. A by-product
of this is that developers no longer need to run multiple copies of
filelock, the test spawns additional lockers as necessary.

LGTM=nigeltao
R=golang-codereviews, gobot, nigeltao, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/65870043

Committer: Nigel Tao <nigeltao@golang.org>