test(storage): add mrd test for negative offset and 0 length#13593
test(storage): add mrd test for negative offset and 0 length#13593krishnamd-jkp wants to merge 1 commit intogoogleapis:mainfrom
Conversation
Summary of ChangesHello @krishnamd-jkp, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new integration test to enhance the robustness of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds an integration test for the Multi-Range Downloader (MRD) to verify its behavior with negative offsets and zero-length ranges. The test cases cover positive and negative offsets combined with positive and zero lengths.
My review focuses on improving the test's robustness and maintainability. I've suggested using a unique object name to prevent test conflicts and enhancing the assertions to be more comprehensive and provide better failure diagnostics. Overall, the new test is a valuable addition.
| // Generate random content for testing. | ||
| content := make([]byte, dataSize) | ||
| rand.New(rand.NewSource(0)).Read(content) | ||
| objName := "MRDWithReadHandle" |
There was a problem hiding this comment.
To avoid potential conflicts with other tests, it's good practice to use a unique object name for each test function. The current name MRDWithReadHandle is already used in TestIntegration_MRDWithReadHandle. Using a name specific to this test, like the test function name itself, will prevent flakes if tests are run in parallel against the same bucket.
| objName := "MRDWithReadHandle" | |
| objName := "MRDWithNegativeOffsetsAndZeroLength" |
| var startIdx, endIdx int | ||
| if test.offset < 0 { | ||
| startIdx = max(0, int(test.offset)+dataSize) | ||
| } else { | ||
| startIdx = int(test.offset) | ||
| } | ||
| if test.length == 0 { | ||
| endIdx = dataSize | ||
| } else { | ||
| endIdx = min(dataSize, startIdx+int(test.length)) | ||
| } | ||
| wantContent := content[startIdx:endIdx] | ||
| if !bytes.Equal(res.buf.Bytes(), wantContent) { | ||
| t.Errorf("Downloaded content mismatch. got %v bytes, want %v bytes", res.buf.Bytes(), wantContent) | ||
| } |
There was a problem hiding this comment.
The test's assertions can be improved. It's good practice to also verify the values returned by the onDone callback (offset, limit, and err) to ensure the downloader reports the correct effective range. Additionally, the error message for content mismatch could be more informative by printing the lengths of the byte slices and using %q for a more readable output.
var startIdx, endIdx int
if test.offset < 0 {
startIdx = max(0, int(test.offset)+dataSize)
} else {
startIdx = int(test.offset)
}
if test.length == 0 {
endIdx = dataSize
} else {
endIdx = min(dataSize, startIdx+int(test.length))
}
if res.err != nil {
t.Fatalf("onDone callback returned an error: %v", res.err)
}
if res.offset != int64(startIdx) {
t.Errorf("onDone offset: got %d, want %d", res.offset, startIdx)
}
if res.limit != int64(endIdx) {
t.Errorf("onDone limit: got %d, want %d", res.limit, endIdx)
}
wantContent := content[startIdx:endIdx]
if !bytes.Equal(res.buf.Bytes(), wantContent) {
t.Errorf("Downloaded content mismatch. got len %d, want len %d.\ngot: %q\nwant: %q", res.buf.Len(), len(wantContent), res.buf.Bytes(), wantContent)
}
quartzmo
changed the title
quartzmo
changed the title
| // Generate random content for testing. | ||
| content := make([]byte, dataSize) | ||
| rand.New(rand.NewSource(0)).Read(content) | ||
| objName := "MRDWithReadHandle" |
| { | ||
| name: "negative offset zero length", | ||
| offset: -10, | ||
| length: 0, |
There was a problem hiding this comment.
We take care of negative length right?
product-auto-label
bot
added
the
api: storage
2 participants
No description provided.