tests:make robustness report id match log id#21228
tests:make robustness report id match log id#21228madhav-murali wants to merge 1 commit intoetcd-io:mainfrom
Conversation
|
Hi @madhav-murali. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
tests/robustness/model/types.go
Outdated
|
|
||
| type MemberID uint64 | ||
|
|
||
| // MarshallJson implements the json.Marshaler interface for MemberID. |
There was a problem hiding this comment.
nit: inconsistent with the func name and not rhyme.
02c34b4 to
decc822
Compare
| func (m MemberID) MarshalJSON() ([]byte, error) { | ||
| return json.Marshal(fmt.Sprintf("%x", m)) | ||
| } |
There was a problem hiding this comment.
I'm not sure if this is correct though. This looks like we're just converting uint64 into a hex value. Have you tested this that the values actually match?
There was a problem hiding this comment.
Yeah i did confirm that the MemberID's generated in the operations.json file appear in the server logs in hexa format.
|
/ok-to-test Please add tests comparing output of model and type.ID |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted filessee 21 files with indirect coverage changes @@ Coverage Diff @@
## main #21228 +/- ##
==========================================
- Coverage 68.41% 68.40% -0.02%
==========================================
Files 429 429
Lines 35288 35288
==========================================
- Hits 24142 24137 -5
- Misses 9751 9755 +4
- Partials 1395 1396 +1 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
decc822 to
47c401a
Compare
|
@serathius, I have added unit test in |
|
/retest |
There was a problem hiding this comment.
Test for types.go struct should be in types_test.go
|
|
||
| tID := types.ID(raw) | ||
| expectedHex := tID.String() | ||
| require.JSONEq(t, "\""+expectedHex+"\"", jsonStr) |
There was a problem hiding this comment.
(⊙ _ ⊙ ) Forgot the Unmarshaler interface. :S
I've added UnmarshalJSON and included a round-trip test in types_test.go to ensure everything parses back correctly.
47c401a to
72f0165
Compare
|
/retest |
tests/robustness/model/history.go
Outdated
| } | ||
| var revision int64 | ||
| var MemberID uint64 | ||
| var memberID uint64 |
There was a problem hiding this comment.
| var memberID uint64 | |
| var memberID MemberID |
tests/robustness/model/history.go
Outdated
| memberID = resp.Header.MemberId | ||
| } | ||
| h.appendSuccessful(request, start, end, putResponseWithMemberID(revision, MemberID)) | ||
| h.appendSuccessful(request, start, end, putResponseWithMemberID(revision, MemberID(memberID))) |
There was a problem hiding this comment.
| h.appendSuccessful(request, start, end, putResponseWithMemberID(revision, MemberID(memberID))) | |
| h.appendSuccessful(request, start, end, putResponseWithMemberID(revision, memberID)) |
| return err | ||
| } | ||
|
|
||
| id, err := types.IDFromString(s) |
There was a problem hiding this comment.
If we are using IDFromString, for unmarshall we should use symmetrical function during Marshaling. If there is none, we should add it. Expect we should use types.Id(m).String() instead of fmt.Sprintf("%x", m)
There was a problem hiding this comment.
Ok, I have corrected the asymmetrical implementations using types.Id(m).String() instead of the current one. I've also use MemberID in types.go for type-safety.
Thanks for the guidance :)
Signed-off-by: Madhav Murali <mdvmrli@gmail.com>
72f0165 to
d6edb51
Compare
| @@ -70,7 +70,7 @@ func (h *AppendableHistory) AppendRange(startKey, endKey string, revision, limit | |||
| respRevision = resp.Header.Revision | |||
| respMemberID = resp.Header.MemberId | |||
There was a problem hiding this comment.
| respMemberID = resp.Header.MemberId | |
| respMemberID = MemberID(resp.Header.MemberId) |
| respMemberID = resp.Header.MemberId | ||
| } | ||
| h.appendSuccessful(request, start, end, rangeResponseWithMemberID(resp.Kvs, resp.Count, respRevision, respMemberID)) | ||
| h.appendSuccessful(request, start, end, rangeResponseWithMemberID(resp.Kvs, resp.Count, respRevision, MemberID(respMemberID))) |
There was a problem hiding this comment.
| h.appendSuccessful(request, start, end, rangeResponseWithMemberID(resp.Kvs, resp.Count, respRevision, MemberID(respMemberID))) | |
| h.appendSuccessful(request, start, end, rangeResponseWithMemberID(resp.Kvs, resp.Count, respRevision, respMemberID)) |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: madhav-murali, serathius The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
Fixes #21211
This PR updates the
MemberIDJSON marshaling to use hexadecimal format (instead of decimal) in the robustness report. This ensures that the member-id inoperations.jsonmatches the format used in logs.I haven't added unit tests, because I wasn't sure. Ran make test-robustness and verified with the
operations.jsonfile to ensure member-id matched.Signed-off-by: Madhav Murali mdvmrli@gmail.com