fix(shardid): use string instead of int64 in id json Marshaler/Unmarshaler #41
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @cnlangzi - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
|
|
||
| bufID, err := json.Marshal(id) | ||
| bufStr, err := json.Marshal(s) |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test case for handling invalid string formats during JSON unmarshalling.
This test should verify the behavior when the string does not represent a valid int64, ensuring the error handling is correct.
| bufStr, err := json.Marshal(s) | |
| bufStr, err := json.Marshal(s) | |
| require.NoError(t, err) | |
| parsedInt, parseErr := strconv.ParseInt(s, 10, 64) | |
| require.Error(t, parseErr, "Expected an error when parsing a non-int64 string") |
|
|
||
| var jsID ID | ||
| err = json.Unmarshal(bufIdInt64, &jsID) | ||
| // Unmarshal id from string json bytes | ||
| err = json.Unmarshal(bufStr, &jsID) |
There was a problem hiding this comment.
suggestion (testing): Add assertions to verify the internal state of 'jsID' after unmarshalling.
It's important to ensure that all fields of 'jsID' are correctly set, not just that it equals 'id'. This helps catch any partial unmarshalling issues.
| err = json.Unmarshal(bufStr, &jsID) | |
| var jsID ID | |
| err = json.Unmarshal(bufStr, &jsID) | |
| require.NoError(t, err) | |
| require.NotNil(t, jsID) | |
| require.Equal(t, id, jsID) |
| err = json.Unmarshal(bufID, &jsIdInt64) | ||
| var jsString string | ||
| // Unmarshal string from ID json bytes | ||
| err = json.Unmarshal(bufID, &jsString) |
There was a problem hiding this comment.
suggestion (testing): Add a test case to verify the behavior when 'bufID' contains unexpected JSON structure.
This test should check how the unmarshalling behaves if the JSON does not match the expected string format, ensuring robust error handling.
| err = json.Unmarshal(bufID, &jsString) | |
| var jsString string | |
| err = json.Unmarshal(bufID, &jsString) | |
| if err != nil { | |
| t.Errorf("Failed to unmarshal JSON: %v", err) | |
| } else if jsString != expectedString { | |
| t.Errorf("Unmarshalled string does not match expected: got %v, want %v", jsString, expectedString) | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #41 +/- ##
==========================================
+ Coverage 75.29% 75.31% +0.01%
==========================================
Files 42 42
Lines 1757 1758 +1
==========================================
+ Hits 1323 1324 +1
Misses 319 319
Partials 115 115
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Here's the code health analysis summary for commits Analysis Summary
|
Fixes