Arvados

93d818fb7374b961d7f802c7d89a36162c78f6e7

sdk/go/manifest/manifest.go -> BlockIterWithDuplicates and parseManifestStream are updated to look for errors in manifest_text and return them. Made updates in datamanager/collection to look for these errors.

Several tests to verify API errors and manifest errors during GetCollections have been added. The specific test “An API error is encountered when getting the collection list (for example, its token expires between fetching page N and fetching page N+1)” has not been added due to the difficulty in simulating such as error, but instead other API error simulation tests were added.

sdk/go/arvadostest/testing.go:

testing.go → stub.go

StatusAndBody { ResponseStatus, ResponseBody } → StubResponse { Status, Body } ?

In APIStub: Data → Responses

KeepServerStub seems to be identical to APIStub; if so, just say type KeepServerStub APIStub instead of having two copies of the code. (Apparently I should have noticed this in #7490...)

sdk/go/manifest/manifest.go:

Can we stop using "m" for both "Manifest" and "ManifestStream" -- maybe "ms" for ManifestStream? At least we should fix BlockIterWithDuplicates where m starts out being a *Manifest but is then reused to hold a ManifestStream in the goroutine loop.

• -       for j := range m.FileTokens {
  -               _, _, _, err := parseFileToken(fileTokens[j])
  +       for _, ft := range m.FileTokens {
  +               _, _, _, err := parseFileToken(ft)
  

• Specify in the BlockIterWithDuplicates comment that the caller must check m.Err after the returned channel closes, in order to detect parse errors.
• Set m.Err = err (and don't send anything to blockChannel) if blockdigest.ParseBlockLocator() fails.

• -       expectBlockLocator(t, b, ...
  +       expectBlockLocator(t, blockdigest.BlockLocator{b.Digest, b.Size, b.Hints}, ...
  

sdk/go/manifest/manifest_test.go:

Instead of having one "success" case in TestBlockIterWithBadManifest, just test bad manifests -- and check manifest.Err == nil in all the other test cases that test BlockIterWithDuplicates.

Test: ". acbd18db4cc2f85cedef654fccc4a4d8+3 0:1:file1.txt. acbd18db4cc2f85cedef654fccc4a4d8+3 1:2:file3.txt"

Test: ". acbd18db4cc2f85cedef654fccc4a4d8+3 0:1:file1.txt\n. acbd18db4cc2f85cedef654fccc4a4d8+3 ::file2.txt\n"

Test: ". acbd18db4cc2f85cedef654fccc4a4d8+3 acbd18db4cc2f85cedef654fccc4a4d8+3\n"

• for _ = range manifest.BlockIterWithDuplicates() { }
  

Tests would be a bit less repetitive like this:

-       testData := APITestData{}
-       testData.data = dataMap
-       testData.expectedError = "Invalid stream name: badstreamname" 
-
-       testGetCollectionsAndSummarize(c, testData)
+       testGetCollectionsAndSummarize(c, APITestData{
+               data:          dataMap,
+               expectedError: "Invalid stream name: badstreamname",
+       })

Updated to address all those comments. Just a couple things:

• Good catch about the APIStub and KeepServerStub doing the same thing. After all the cleanup and all, it became obvious. I now have only one and called it ServerStub

• Regarding "This seems to make a copy of b instead of just using b... but I'm not sure why? expectBlockLocator(t, b, ..." Here "b" is manifest.BlockLocator, whereas expectBlockLocator expects a blockdigest.BlockLocator

Thanks.

The second line here is unnecessary (in testGetCollectionsAndSummarize): results can't be nil because it's not a pointer, and even if it was a pointer, the first line would have already crashed if it was nil...

               c.Assert(results.Err, IsNil)
               c.Assert(results, NotNil)

Everything else LGTM, thanks.

LGTM @ cbb8375