-
Notifications
You must be signed in to change notification settings - Fork 310
Add some tests for Limit, BytesMut and Reader #785
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
tests/test_bytes.rs
Outdated
@@ -186,6 +186,13 @@ fn split_off_oob() { | |||
let _ = hello.split_off(44); | |||
} | |||
|
|||
#[test] | |||
#[should_panic] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the panic message here?
#[should_panic] | |
#[should_panic = "whatever the error is"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
tests/test_limit.rs
Outdated
|
||
#[test] | ||
#[should_panic] | ||
fn limit_advanve_mut_panic_2() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fn limit_advanve_mut_panic_2() { | |
fn limit_advance_mut_panic_2() { |
tests/test_limit.rs
Outdated
|
||
#[test] | ||
#[should_panic] | ||
fn limit_advanve_mut_panic_1() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fn limit_advanve_mut_panic_1() { | |
fn limit_advance_mut_panic_1() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
…rror for limit_advance_mut_panic
tests/test_limit.rs
Outdated
} | ||
|
||
#[test] | ||
#[should_panic] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add messages to all of them? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your correction and thanks for your time to review my PR!
Hi,
Thanks for your time to review this PR.
By examing the existing code, we found that some tests can be added to improve the repo's overall test coverage. The tests we submitted have been carefully curated by us to ensure their behavior and effectiveness.
The code region we covered is:
limit.rs:
limit.rs
reader.rs:
bytes/src/buf/reader.rs
Lines 40 to 42 in d9d5c59
bytes_mut.rs:
bytes/src/bytes_mut.rs
Lines 323 to 325 in d9d5c59
And while testing the Limit implementation, I noticed that when both Limit and BytesMut are introduced, calling the limit method on a Limit<&mut [u8]> object would by default invoke the limit method from BytesMut. This behavior seems somewhat counterintuitive to me. I'm not certain whether this needs to be addressed, but I thought I'd mention it for awareness.
link
Thanks again for reviewing.