Arvados

7179-test-mocks has some beginnings (I hope):

• 8dd5480 adds a TestableVolume interface.
• 20abe07 adds a generic "test any TestableVolume" tester that currently just has two new tests for Delete().

Comments for branch 7179-test-mocks at commit 5dd88c53ca3357f96bb98ad286d0fb0a52ef5f54

volume.go:

• loc is guaranteed to consist of 32 or more lowercase hex digits: I think this is a bit confusing because the first token “must” be 32 hex digits? Can it say it will be 32 hex digits with optional hints? Also include the link https://dev.arvados.org/projects/arvados/wiki/Keep_manifest_format?

• “Get should not verify the integrity of the returned data” : can we add, the caller is expected to do this verification?

• This is a bit confusing: “a future call to Mtime() will return a timestamp newer than {now minus one second}” — Why the “minus one second”? Let’s say I just did a touch at exactly at 9AM, and invoke Mtime within a few milliseconds, what would I get?

• “IndexTo writes a complete list of locators” : Can we say “IndexTo returns a complete list of locators in the given writer …”

• “String returns an identifying label for this volume” : Why was this called “String” instead of “Name” or “Identifier”?

• In VolumeManager interface, can you please add new lines between each exposed method (similar to the Volume interface). It will greatly help improve readability of the the interface and the comments

• Can you please add a comment for “RRVolumeManager”? (Peter said it is “Round Robin Volume Manager” and I always like to know what an acronym represents …)

• Would it be too much to generalize some of the errors involved? Would it make sense to move the following from keepstore into volume.go and list the potential errors for each method? For example, Touch would / should return a “MethodDisabledError” if it is ReadOnly volume?

var (
    BadRequestError     = &KeepError{400, "Bad Request"}
    UnauthorizedError   = &KeepError{401, "Unauthorized"}
    CollisionError      = &KeepError{500, "Collision"}
    RequestHashError    = &KeepError{422, "Hash mismatch in request"}
    PermissionError     = &KeepError{403, "Forbidden"}
    DiskHashError       = &KeepError{500, "Hash mismatch in stored data"}
    ExpiredError        = &KeepError{401, "Expired permission signature"}
    NotFoundError       = &KeepError{404, "Not Found"}
    GenericError        = &KeepError{500, "Fail"}
    FullError           = &KeepError{503, "Full"}
    SizeRequiredError   = &KeepError{411, "Missing Content-Length"}
    TooLongError        = &KeepError{413, "Block is too large"}
    MethodDisabledError = &KeepError{405, "Method disabled"}
)

volume_generic_test.go

• Please add a comment for “type TestableVolumeFactory”

• Please add a comment to “fun DoGenericVolumeTests”. Even though it is very clear once you read the file, it does not hurt to help by telling the reader that one would add any methods to be part of the Generic tests in here and add a func with that name in the file …

• I don’t mind just adding a one line comment for each of the included test methods as well. For example, why the block is old and that it is expect not to be deleted … I think one should be able to just read the comments and understand what a test does, than reading the code of the test to see what it is doing …

• Can you please add new lines for each method in volume_test.go => TestableVolume interface to improve readability?

volume_unix_test.go

• Does the test “TestGetNotFound” need to create the data using “v.Put(TEST_HASH, TEST_BLOCK)”?

Radhika Chippada wrote:

Comments for branch 7179-test-mocks at commit 5dd88c53ca3357f96bb98ad286d0fb0a52ef5f54

volume.go:

• loc is guaranteed to consist of 32 or more lowercase hex digits: I think this is a bit confusing because the first token “must” be 32 hex digits?

I figure it won't be too long before we want to support other hash algorithms like SHA-1 and SHA-256, and I didn't want to add yet another obstacle by guaranteeing all hashes will be 32 digits.

Can it say it will be 32 hex digits with optional hints? Also include the link https://dev.arvados.org/projects/arvados/wiki/Keep_manifest_format?

Hm. We don't include the size in filenames and I don't think we have any plans to. (Any particular reason we should allow for this?)

Other hints surely shouldn't make it this far down in the stack.

• “Get should not verify the integrity of the returned data” : can we add, the caller is expected to do this verification?

Yes, added.

• This is a bit confusing: “a future call to Mtime() will return a timestamp newer than {now minus one second}” — Why the “minus one second”? Let’s say I just did a touch at exactly at 9AM, and invoke Mtime within a few milliseconds, what would I get?

Updated comment, with examples... better?

• “IndexTo writes a complete list of locators” : Can we say “IndexTo returns a complete list of locators in the given writer …”

I think "writes" is more accurate than "returns"... Tried to be like http://golang.org/pkg/io/#WriteString except that the words "to writer" didn't seem to fit anywhere, or be especially helpful...

• “String returns an identifying label for this volume” : Why was this called “String” instead of “Name” or “Identifier”?

Satisfies the fmt.Stringer interface. :)

• In VolumeManager interface, can you please add new lines between each exposed method (similar to the Volume interface). It will greatly help improve readability of the the interface and the comments

Yes, done.

• Can you please add a comment for “RRVolumeManager”? (Peter said it is “Round Robin Volume Manager” and I always like to know what an acronym represents …)

Yes, done.

• Would it be too much to generalize some of the errors involved? Would it make sense to move the following from keepstore into volume.go and list the potential errors for each method? For example, Touch would / should return a “MethodDisabledError” if it is ReadOnly volume?

I tried to capture the cases where it really matters which error is returned. Are there cases I missed? (Apart from that, I think error reporting is more helpful when the error types/messages aren't restricted to a finite set...)

• Please add a comment for “type TestableVolumeFactory”

• Please add a comment to “fun DoGenericVolumeTests”. Even though it is very clear once you read the file, it does not hurt to help by telling the reader that one would add any methods to be part of the Generic tests in here and add a func with that name in the file …

• I don’t mind just adding a one line comment for each of the included test methods as well. For example, why the block is old and that it is expect not to be deleted … I think one should be able to just read the comments and understand what a test does, than reading the code of the test to see what it is doing …

• Can you please add new lines for each method in volume_test.go => TestableVolume interface to improve readability?

Did all those, thanks.

volume_unix_test.go

• Does the test “TestGetNotFound” need to create the data using “v.Put(TEST_HASH, TEST_BLOCK)”?

I just preserved the old test. (I'm sure it would pass without it, but it does seems reasonable to test "block not found" on a non-empty volume. Perhaps we should be testing on both empty and non-empty?)

Fine with the location comment with that explanation. The comments are much clearer now. Thanks.

Branch 7179-test-mocks LGTM

commit commig:241f0bcdacbf83b587bff9ff45985e720bde9f0b

Branch 7179-generic-volume-tests is ready for review.

• Added tests that I could think of

• Per our conversation, moved some tests from volume_unix_test.go since they are generic enough and do not do anything unix volume specific. These are:
  • TestGet
  • TestIndexTo
  • TestPutTouch
  • TestGetSerialized
  • TestPutSerialized
  • Some of these tests (for ex: TestIndexTo) test more than one thing (for ex: in this test, testing no prefix and with prefix in one). To help keep review simple, I left them as is (for now). I can break it into multiple tests if need be.

• Since volume.go (interface) is referring to VolumeStatus, moved the VolumeStatus struct from handlers.go into volume.go (and borrowed the comments as well). I think it is good to have this here in the interface file. I can move it back to handlers.go if there is any objection to this.

testPutBlockWithDifferentContent says "Whether Put with the same loc with different content fails or succeeds is implementation dependent." But the spec says: "Put must either overwrite the existing data with the new data or return a non-nil error." So, if the second Put() returns a non-nil error, Get() must return the second data block.

The spec doesn't specify what Get() is supposed to return after Put() returns an error: apparently it's OK for it to return "foo" or an empty string or a mix of old and new data. We should probably tighten the spec (and the test) here so it's only acceptable for a subsequent Get to return "old data", "new data", or "error":

• Get must return TEST_BLOCK_2, if v.Put(TEST_HASH, TEST_BLOCK_2) returned a nil err
• Get must return either TEST_BLOCK or TEST_BLOCK_2 or a non-nil err, if v.Put(TEST_HASH, TEST_BLOCK_2) returned a non-nil err

Pushed the above changes in 143a5f3 -- does this look OK?

testUpdateReadOnly should try writing TEST_HASH_2 (instead of TEST_HASH, which already exists by that point) -- then it should call Get() and make sure TEST_HASH_2 didn't get written.

In the long run I'm not sure about the strategy of offering a separate set of tests for writable volumes. Currently the only difference between Writable and non-Writable volumes is that we don't call Put, Compare, or Touch on non-Writable volumes -- and if we did call them, we'd expect them to do exactly what Writable volumes do, i.e., return an error if the thing can't be done. (If we call Put on a volume that claimed to be non-Writable, and the Put works, is the volume implementation broken?) ...but I think this is fine for now.

testTouchNoSuchBlock seems to test the opposite of what it says it tests. I think we need one test that's identical to testPutAndTouch but using Touch instead of Put (hopefully doing something like getFunc() in volume_unix.go, rather than copying & pasting code), plus one test that does what testTouchNoSuchBlock says it's going to do (i.e., don't PUT first, and fail on err == nil instead of err != nil).

testPutSerialized testGetSerialized never actually tested anything about "serialize", but now they don't even turn on the "serialize" flag. I think we should:

• Move the "todo" comments back to volume_unix_test.go ("serialize" is a UnixVolume thing, not a Volume thing).
• Rename testPutSerialized to testPutConcurrent, and same for Get. They're not perfect, but at least they make some attempt to test overlapping operations from multiple goroutines, which is important and afaict not covered elsewhere.
• Add a test just like TestUnixVolumeWithGenericTests but with NewTestableUnixVolume(t, true, false). It won't test whether serialize itself is doing anything, but it will test that turning on serialize doesn't break anything else.

Pushed the above changes in 143a5f3 -- does this look OK?

That commit by Tom was good.

testUpdateReadOnly should try writing TEST_HASH_2 (instead of TEST_HASH, which already exists by that point) -- then it should call Get() and make sure TEST_HASH_2 didn't get written.

Done

In the long run I'm not sure about the strategy of offering a separate set of tests for writable volumes. Currently the only difference between Writable and non-Writable volumes is that we don't call Put, Compare, or Touch on non-Writable volumes -- and if we did call them, we'd expect them to do exactly what Writable volumes do, i.e., return an error if the thing can't be done. (If we call Put on a volume that claimed to be non-Writable, and the Put works, is the volume implementation broken?) ...but I think this is fine for now.

Updated tests to not need separate set of tests. Now each test checks whether it is for read-only or writable mode and returns if the mode does not match. Also, updated all tests to use PutRaw where applicable (non-put and non-delete tests) so that most tests are executed in both modes.

testTouchNoSuchBlock seems to test the opposite of what it says it tests ... plus one test that does what testTouchNoSuchBlock says it's going to do (i.e., don't PUT first, and fail on err == nil instead of err != nil).

Corrected the typo

I think we need one test that's identical to testPutAndTouch but using Touch instead of Put (hopefully doing something like getFunc() in volume_unix.go, rather than copying & pasting code)

This was not clear to me and hence not addressed.

testPutSerialized testGetSerialized never actually tested anything about "serialize", but now they don't even turn on the "serialize" flag. I think we should:

Move the "todo" comments back to volume_unix_test.go ("serialize" is a UnixVolume thing, not a Volume thing).

Done

Rename testPutSerialized to testPutConcurrent, and same for Get. They're not perfect, but at least they make some attempt to test overlapping operations from multiple goroutines, which is important and afaict not covered elsewhere.

Done

Add a test just like TestUnixVolumeWithGenericTests but with NewTestableUnixVolume(t, true, false). It won't test whether serialize itself is doing anything, but it will test that turning on serialize doesn't break anything else.

Done

• Also, ran golint and addressed most of the suggestions. A few such as "never_delete" are left alone.