Skip to content

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AbeZbm
Copy link

@AbeZbm AbeZbm commented May 23, 2025

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

pub fn get_mut(&mut self) -> &mut B {
&mut self.buf
}

bytes_mut.rs:

bytes/src/bytes_mut.rs

Lines 323 to 325 in d9d5c59

"split_off out of bounds: {:?} <= {:?}",
at,
self.capacity(),

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.

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@@ -186,6 +186,13 @@ fn split_off_oob() {
let _ = hello.split_off(44);
}

#[test]
#[should_panic]
Copy link
Contributor

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?

Suggested change
#[should_panic]
#[should_panic = "whatever the error is"]

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

Copy link
Contributor

@paolobarbolini paolobarbolini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo


#[test]
#[should_panic]
fn limit_advanve_mut_panic_2() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fn limit_advanve_mut_panic_2() {
fn limit_advance_mut_panic_2() {


#[test]
#[should_panic]
fn limit_advanve_mut_panic_1() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fn limit_advanve_mut_panic_1() {
fn limit_advance_mut_panic_1() {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

}

#[test]
#[should_panic]
Copy link
Contributor

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!

Copy link
Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants