Add unit tests for registering a codec #8244
Conversation
|
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8244 +/- ##
==========================================
+ Coverage 82.05% 82.16% +0.11%
==========================================
Files 412 412
Lines 40477 40477
==========================================
+ Hits 33213 33258 +45
+ Misses 5896 5854 -42
+ Partials 1368 1365 -3 🚀 New features to boost your workflow:
|
purnesh42H
added
the
Area: RPC Features
isaiahvita
force-pushed
the
isaiahvita-encoding-tests
branch
2 times, most recently
from
df01452 to
d1cc013
Compare
encoding/encoding_test.go
Outdated
| // 1. invalid codecs (nil, empty name) should panic | ||
| // 2. same name codecs are valid, they just overwrite previous. | ||
| func TestRegisterCodec(t *testing.T) { | ||
| cases := map[string]struct { |
There was a problem hiding this comment.
you can do something like this instead of having a map
tests := []struct {
name string
origCodec encoding.CodecV2
newCodec encoding.CodecV2
wantPanic bool
}
There was a problem hiding this comment.
example to refer
grpc-go/clientconn_authority_test.go
Line 38 in eb4b687
encoding/encoding_test.go
Outdated
| if !reflect.DeepEqual(c.codec1, actualCodec) { | ||
| t.Errorf("[%v] Expected returned codec to be equal.\n", caseName) | ||
| } | ||
| if c.codec2 == nil { |
There was a problem hiding this comment.
it looks like we have too many conditions in one test. may be should separate the tests into valid and invalid codec cases? For invalid, we just need one codec then.
There was a problem hiding this comment.
hmm im not sure i agree with that. separating into two different tests would only allow the defer to be in its own test, and then well have to duplicate the other equality conditions in both tests.
let me add some comments to make it more readable, and let me know what you think about that. if you still would like to have two separate tests, then i can change it.
There was a problem hiding this comment.
I still think we should have separate test case for failures because there is lot of code which is not applicable to success cases. It is a good enough reason to make a separate test.
isaiahvita
force-pushed
the
isaiahvita-encoding-tests
branch
2 times, most recently
from
1cad815 to
b87ad50
Compare
isaiahvita
force-pushed
the
isaiahvita-encoding-tests
branch
from
b87ad50 to
380623c
Compare
| return m.CodecName | ||
| } | ||
|
|
||
| // TestRegisterCodecV2 tests the existing behavioral assumptions made by |
There was a problem hiding this comment.
| // TestRegisterCodecV2 tests the existing behavioral assumptions made by | |
| // TestRegisterCodecV2 tests verify the following behaviors of RegisterCodeV2: | |
| - invalid codecs (nil, empty name) should panic | |
| - if new codecs has same name as existing, existing codec is overwritten | |
| - if codec with same name does not exist, it is registered |
| expectPanic bool | ||
| }{ | ||
| { | ||
| name: "nil codes, panic", |
There was a problem hiding this comment.
typo: nil codecs, panics
| func TestRegisterCodecV2(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| codec1 encoding.CodecV2 |
There was a problem hiding this comment.
might be good to name codec1 and codec2 as existingCodec and newCodec or add a comment against them
|
2 more things to keep in mind for future reference
|
|
This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed. |
The existing unit tests do not test the ability to test registering a codec via
encoding.RegisterCodecV2.This PR checks that the existing behavior thats currently implemented in the function will continue to work as it currently does.
Additionally, this brings the coverage report of encoding_v2.go from 75% to 100%.
I tested this by running the unit test and seeing it pass.
RELEASE NOTES: None