Skip to content
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

Merged
merged 1 commit into from
Oct 19, 2022
Merged

Add tests for author.rs (#700) #829

merged 1 commit into from
Oct 19, 2022

Conversation

gallottino
Copy link
Contributor

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]

Copy link
Collaborator

@spenserblack spenserblack left a 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
}

Comment on lines 139 to 201
#[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");
}
Copy link
Collaborator

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.

Comment on lines 197 to 199

println!("{}", authors_info);
println!("{}", authors_info_2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
println!("{}", authors_info);
println!("{}", authors_info_2);

Let's keep these print statements out of the tests.

Comment on lines 201 to 204
assert_eq!(
authors_info.value(),
"\u{1b}[38;2;255;0;0m75% John Doe <[email protected]> 1500\u{1b}[39m"
);
Copy link
Collaborator

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.

Copy link
Owner

@o2sh o2sh Oct 18, 2022

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()
  );

Copy link
Collaborator

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!

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))
Copy link
Collaborator

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]>
@gallottino
Copy link
Contributor Author

Followed your suggestion :)

@gallottino gallottino requested review from spenserblack and removed request for Byron and o2sh October 18, 2022 21:22
Copy link
Collaborator

@spenserblack spenserblack left a 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!

@o2sh o2sh merged commit 9cf8df4 into o2sh:main Oct 19, 2022
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.

None yet

3 participants