-
Notifications
You must be signed in to change notification settings - Fork 283
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 tests for author.rs
(#700)
#829
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.
Appreciate your interest in making onefetch
a well-tested tool! Here are some notes.
In general, try to split up the tests a bit.
Example for writing tests
fn is_odd(n: u32) -> bool {
if is_even(n) {
unimplemented!();
}
true
}
// good tests
#[test]
fn test_is_odd_true_on_odd_number() {
assert!(is_odd(1));
}
#[test]
fn test_is_odd_false_on_even_number() {
assert!(!is_odd(2));
// when this fails, the first thing we see is that is_odd
// doesn't handle even numbers
}
// Not so good
#[test]
fn is_odd_works() {
assert!(is_odd(1));
assert!(!is_odd(2));
// when this fails, the first thing we see is that is_odd
// doesn't work, but we have to dig into the test results
// to see what isn't working
}
src/info/repo/author.rs
Outdated
#[test] | ||
fn test_author_info_title() { | ||
let author = Author::new( | ||
"John Doe".into(), | ||
"[email protected]".into(), | ||
1500, | ||
2000, | ||
true, | ||
); | ||
|
||
let author2 = Author::new( | ||
"Roberto Berto".into(), | ||
"[email protected]".into(), | ||
240, | ||
300, | ||
false, | ||
); | ||
|
||
let authors_info = AuthorsInfo { | ||
info_color: DynColors::Rgb(255, 0, 0), | ||
authors: vec![author.clone()], | ||
}; | ||
|
||
let authors_info_2 = AuthorsInfo { | ||
info_color: DynColors::Rgb(255, 0, 0), | ||
authors: vec![author2, author], | ||
}; | ||
|
||
assert_eq!(authors_info.title(), "Author"); | ||
assert_eq!(authors_info_2.title(), "Authors"); | ||
} |
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.
If you're testing title()
, you might want to make it 2 tests -- one like test_author_info_singular_title
and another like test_author_info_plural_title
.
src/info/repo/author.rs
Outdated
|
||
println!("{}", authors_info); | ||
println!("{}", authors_info_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.
println!("{}", authors_info); | |
println!("{}", authors_info_2); |
Let's keep these print statements out of the tests.
src/info/repo/author.rs
Outdated
assert_eq!( | ||
authors_info.value(), | ||
"\u{1b}[38;2;255;0;0m75% John Doe <[email protected]> 1500\u{1b}[39m" | ||
); |
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.
See comments like #827 (comment)
Test that the expected plain text is there with .contains
instead of including the color formatting in the expected value.
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.
BTW, if you do want to check the formatting without losing in readability - by adding a sequence of ANSI escape codes - you could just do:
assert_eq!(
authors_info.value(),
"75% John Doe <[email protected]> 1500"
.color(DynColors::Rgb(255, 0, 0))
.to_string()
);
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.
Great idea, thanks for bringing that up!
src/info/repo/author.rs
Outdated
for i in 0..pad { | ||
pad_str.push_str(" "); | ||
} | ||
assert_eq!(authors_info_2.value(), format!("\u{1b}[38;2;255;0;0m75% John Doe <[email protected]> 1500\u{1b}[39m\n{}\u{1b}[38;2;255;0;0m{}% Roberto Berto 240\u{1b}[39m", pad_str, perc)) |
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.
Also use .contains
here.
This test would be more readable if you just hardcoded the expected percentage, too. Because you control the factors (author's name, etc.) it's OK, even preferable, to hardcode the expected values.
So just make perc
a string or a float without any math and test that {perc}%
is contained.
Signed-off-by: gallottino <[email protected]> Co-authored-by: Oniryu95 <[email protected]>
Followed your suggestion :) |
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 tests are much more readable now, thank you!
Hi guys!
Oniryu95 and me want to collaborate with you to develop onefetch. We started from this simple issue #700.
In future we wants to do something more consistent for this project.
Let us know!
Signed-off-by: gallottino [email protected]
Co-authored-by: Oniryu95 [email protected]