Tests for #206

[liamsi]

• compare to proto3 behaviour
• simple tests for pointers to empty struct and nil pointer

The intended behaviour is achieved by your PR @jaekwon 👍 Currently, the test fails with:

TestProto3CompatPtrs: (Click to expand)

=== RUN   TestProto3CompatPtrs
--- FAIL: TestProto3CompatPtrs (0.00s)
	proto3_compat_test.go:126: 
			Error Trace:	proto3_compat_test.go:126
			Error:      	Not equal: 
			            	expected: []byte(nil)
			            	actual  : []byte{}
			            	
			            	Diff:
			            	--- Expected
			            	+++ Actual
			            	@@ -1,2 +1,3 @@
			            	-([]uint8) <nil>
			            	+([]uint8) {
			            	+}
			            	 
			Test:       	TestProto3CompatPtrs
	proto3_compat_test.go:128: []
	proto3_compat_test.go:140: [10 0]
FAIL

---

The problem here seems orthogonal to what we were trying to do in the initial PR: If we skip (nil-pointer case) in amino we end up with []byte(nil). In protobuf we end up with []byte{}.

This is fixed by e90abf1

[jaekwon]

I don't think this is necessary, fyi. From the docs,
"The zero value for Buffer is an empty buffer ready to use."

[liamsi]

True, it is ready to use but the returned bytes will differ from what go-protobuf returns ([]byte(nil) vs []byte{}). Alternatively, we can make the tests treat these cases as equal instead of modifying it here.

[jaekwon]

The test TestEncodeAminoDecodeProto should be cleaned up and either split into multiple tests or at least separated w/ t.Run(name, func(){}).

Maybe it makes sense to also fold the other test into the new framework...

I think the tests should ideally unmarshal as well and test for equality before and after... I noticed that we lack proper fuzz tests here in go-amino, so I modified the test cases there, but given how we missed this the first time (e.g. the new test cases would fail w/ the WriteEmpty changes before #206) and that the fuzz tests may still not be complete, I think we should also add unmarshal+and+test+for+equality here as well.

#206 (comment)

[liamsi]

OK, I've split up the tests and removed the remaining one which didn't really test a separate concern / unit of code. We can still improve them. I would suggest to add table driven test (as you usually do). we But let's get this merged and tested and switch to table-driven tests in a separate PR?

I'll see why the EmptyStruct test fails while fuzzing (I think I know), run the fuzzer for a while, and we should be good to merge and create a release.

[liamsi]

For the failing fuzzer test: In protobuf this (repeated field containing a nil would not even be encoded with error'ing:
https://github.com/golang/protobuf/blob/f5983d50c82d70eaa88c17080245cc871558081f/proto/table_marshal.go#L277-L279
as nil is not allowed in repeated fields (arrays / slices). For the sake of compatibility, I think we should disallow that in repeated fields, too.

Context (failing fuzzer test):

	Diff:
    			            	--- Expected
    			            	+++ Actual
    			            	@@ -117,4 +117,3 @@
    			            	   (*tests.EmptyStruct)(<nil>),
    			            	-  (*tests.EmptyStruct)({
    			            	-  })
    			            	+  (*tests.EmptyStruct)(<nil>)
    			            	  }
    			Test:       	TestCodecStruct/PointerSlicesStruct:binary

If we look at protobuf's behaviour here:

message EmptyMessage {}

// this will generate (part of) the same go-lang code as PointerSlicesStruct
message PtrSliceEmptyMsg {
    repeated EmptyMessage ptr = 1;
}

when trying to encode what causes the problem on amino's side:

psl := p3.PtrSliceEmptyMsg{Ptr: []*p3.EmptyMessage{nil, &p3.EmptyMessage{}}}
pb, err := proto.Marshal(&psl)

	Error Trace:	proto3_compat_test.go:187
			Error:      	Received unexpected error:
			            	proto: repeated field Ptr has nil element
			Test:       	TestEmptyMessageRoundtrip