-
Notifications
You must be signed in to change notification settings - Fork 44
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
Add unit test for uncovered regions #514
Conversation
Thanks for running the CI check, it seems one of them failed due to nightly toolchain issue, is there anything I can do? |
src/refill.rs
Outdated
@@ -223,6 +223,14 @@ mod tests { | |||
assert_eq!(options.line_ending, LineEnding::LF); | |||
} | |||
|
|||
#[test] | |||
fn test_unfill_missing() { | |||
let (text, options) = unfill("í\n*\n/"); |
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.
Could you simplify the test a bit? The í
could probably be a foo
and the final /
could probably be something else, right? Basically, take the fuzzer output and turn it into something a human would write 😄
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.
Hhh, thanks for your comment, I will manually change it for this one and try to add this feature.
src/refill.rs
Outdated
@@ -223,6 +223,14 @@ mod tests { | |||
assert_eq!(options.line_ending, LineEnding::LF); | |||
} | |||
|
|||
#[test] | |||
fn test_unfill_missing() { |
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.
The word missing
is confusing here. I think the test is covering the case where a bullet point list is not actually seen as a list?
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.
I think you are right and I will manually change the name, 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!
I restarted the jobs — you could have done the same by pushing a different commit to the branch. The PR looks good, great job at finding a test case to improve the coverage like this. I left two small comments about making the test case more human-friendly. |
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.
Awesome, thanks so much!
Thanks for your feedback & comments, it's really helpful. |
Hi,
Thanks for your time & patience to review this PR.
We are researchers focusing on Rust unit tests. By examine the existing code, we found a unit test can be added to improve the repo's overall unit test coverage(this project is already been well tested).
We only manually changed our assert statements to make it consistent with existing unit tests and the newly covered code region is:
textwrap/src/refill.rs
Lines 77 to 80 in 872f221
Thanks again for reviewing.