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

Merged
merged 3 commits into from
May 28, 2025

Conversation

AbeZbm
Copy link
Contributor

@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
Contributor 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
Contributor 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
Contributor 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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @Darksonn 👋, thank you for the review!
I’ve updated the code based on your suggestions. If there are any further changes needed, please let me know!😊

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.

Thank you.

@Darksonn Darksonn merged commit f29e939 into tokio-rs:master May 28, 2025
18 checks passed
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