proto: Make test run in bazel #3073
Conversation
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @karampok and @oncilla)
go/proto/cereal_test.go, line 51 at r1 (raw file):
wrapped := func() { err := ParseFromRaw(&asEntry{}, asEntry{}.Pack(t)) test.Assertion(t, err)
Is the test.Assertion suppose to be checked after the panic?
go/proto/cereal_test.go, line 92 at r1 (raw file):
type asEntry struct{} func (a asEntry) Pack(t *testing.T) []byte {
There is no reason to be a method right? Can be a function and maybe included within the test
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @oncilla)
go/proto/cereal_test.go, line 31 at r1 (raw file):
tests := map[string]struct { Extractor func(val interface{}, typeID uint64, s capnp.Struct) error Assertion assert.ErrorAssertionFunc
why switch to assert?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @karampok)
go/proto/cereal_test.go, line 31 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
why switch to assert?
Does not really matter.
Undone.
go/proto/cereal_test.go, line 51 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
Is the test.Assertion suppose to be checked after the panic?
Ordering does not matter.
If there is a panic, catching a wrong error does not matter.
If we do not panic. a wrong error is caught.
go/proto/cereal_test.go, line 92 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
There is no reason to be a method right? Can be a function and maybe included within the test
Done.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @karampok and @oncilla)
go/proto/cereal_test.go, line 51 at r1 (raw file):
Previously, Oncilla wrote…
Ordering does not matter.
If there is a panic, catching a wrong error does not matter.
If we do not panic. a wrong error is caught.
agree but for readability I think it is better to have something like
- does not panic
- there is an error so I should check
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @karampok)
go/proto/cereal_test.go, line 51 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
agree but for readability I think it is better to have something like
- does not panic
- there is an error so I should check
I think it is a trade-off.
e.g. then where the error is set becomes less obvious.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @karampok)
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved
This change is