Added a unit test for the BlobStoreWriter.

[agautam478]

What changed?
Added a test for the BlobstoreWriter since it didn't have any.

Why?
It's an attempt at improving our code coverage and introduce better testing practices. Tried the table test for the first time here. This is something that will simplify tedious and repetitive unit tests in Go (Golang) when testing functions with multiple scenarios and data.

How did you test it?
Yes ran locally.

Potential risks
NA

Release notes
NA

Documentation Changes
NA

[taylanisikdemir]

avoid extra indentation by handling special case and returning.

if tc.expectedErr {
  assert.Error(t, err)
  return
}

// rest of the code goes here without an else { } wrapper

[Groxx]

this would be an ideal case for require.NoError(t, err) fwiw. that'll stop the test immediately.

(this is also one of the reasons why embedding Assertions is problematic. it hides whether things are asserts or requires, and that's a VERY important detail for this reason and for concurrency-safety reasons (you cannot safely require on a background thread))

[taylanisikdemir]

do these assert calls include a message to pinpoint where it failed? more simple way to write assertions in native go is

if flushedKeys == nil {
  t.Error("Expected flushedKeys to be not nil")
}

what does assert.NotNil show in test output when flushedKeys is nil?

[agautam478]

It will show a test failure like this:

--- FAIL: TestBlobstoreWriter (0.00s)
Error Trace: xyz
Error: Expected value not to be nil.

[Groxx]

assert and t.* calls do print the line and a partial trace that failed, yeah.

generally we prefer assert/require because it's a safer habit and almost always less code - they produce more useful output if you don't spend extra time making custom messages for every assertion.

[Groxx]

same as assert.NotNil though in this case you probably want require.NotNil since it'll crash and fail to produce that error message if it runs line 86

[agautam478]

@taylanisikdemir any opinions?

[taylanisikdemir]

Not only this line but all the validations could just be written using t.Error/t.Fatal which forces us to write explicit error message. On the other hand the message is optional in assert/require packages. e.g.

// require.NotNil signature
NotNil(t TestingT, object interface{}, msgAndArgs ...interface{})

Regarding the termination when flushedKeys is nil, yes let's fix that by changing this to t.Fatal instead of t.Error

[Groxx]

hmmm. tbh this feels kinda misleading to me - this same Unmarshal won't work if you .Add twice because of how it's serializing in getBlobstoreWriteFn(...), but "multiple Add()s" is the normal, expected use here.

to be more accurate it'd probably need to use blobstoreiterator to deserialize. though that'd require using a reconciliation/entity.Entity which does a fair amount of irrelevant things here...

---

since using an Entity seems mostly unrelated and overkill, maybe something like this would make it clear that the format is decided by the writer, and it's not intended to be valid JSON on its own?

// add 2 things since that's common in practice + makes it no longer a single valid json thing
blobstoreWriter.Add("one")
blobstoreWriter.Add("two")

// verify the bytes are as expected
assert.Equal(t, resp.Blob.Body, []byte(`"one"\r\n"two"`)

[agautam478]

this one was failing with some mismatch in bytes error.

Expected :[]byte{0x22, 0x74, 0x65, 0x73, 0x74, 0x2d, 0x64, 0x61, 0x74, 0x61, 0x22, 0xd, 0xa}
Actual :[]byte{0x74, 0x65, 0x73, 0x74, 0x2d, 0x64, 0x61, 0x74, 0x61}

[Groxx]

that'd depend on what you're comparing / where you got those bytes. the 0x22 one looks like what I expect in body though

[Groxx]

Minor assert-vs-request-vs-t.Error fuzziness to clean up and I do think it's worth not implying "produces valid json" (because this writer does not). but otherwise looks good, should be an easy 👍

[Groxx]

for a table-test you'd probably put this in the table (as it's likely to change with each test), but eh. the test is reasonably simple, will be easy to adjust when we change it for future stuff.