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

Tests for sentences/utils.rs #88

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gflow33
Copy link
Collaborator

@gflow33 gflow33 commented Apr 18, 2023

Additional tests for sentences/utils.rs, tests are meant to fail but are written in different ways. Please review and tell me the best way to proceed

@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (b203556) 79.60% compared to head (44ce869) 79.60%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #88   +/-   ##
=======================================
  Coverage   79.60%   79.60%           
=======================================
  Files          30       30           
  Lines        1123     1123           
=======================================
  Hits          894      894           
  Misses        229      229           
Impacted Files Coverage Δ
src/sentences/utils.rs 100.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -200,6 +200,79 @@ mod tests {
assert_eq!(time.nanosecond(), 500_000_000);
}

#[test]
#[should_panic]
Copy link
Member

Choose a reason for hiding this comment

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

Which of the 2 tests here should panic? Maybe you can split them if they both need to panic as this will only check the first one

Copy link
Member

Choose a reason for hiding this comment

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

A good approach would be to check the actual error that is returned that just expect a panic. A panic might be occurring for something different that you are expecting in a specific test case so try to assert a specific error on each test.

This will make sure that your test is failing of the right reasons that you expect it to.

@elpiel
Copy link
Member

elpiel commented Oct 27, 2023

@gflow33 Do you have any progress on these tests?

@elpiel
Copy link
Member

elpiel commented Feb 13, 2024

@gflow33 have you made any progress on this PR?

@trkohler
Copy link
Contributor

connect: #25

Comment on lines +234 to +238
let (_, time) = parse_hms("237050,").unwrap();
assert_eq!(time.hour(), 23);
assert_eq!(time.minute(), 70);
assert_eq!(time.second(), 50);
assert_eq!(time.nanosecond(), 0);
Copy link
Member

Choose a reason for hiding this comment

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

If calling parse_hms(...).unwrap() will panic the rest of the assertions will not be executed.
It would be best to assert the expected error instead of unwraping in this case.
This way you can be sure that if minutes is out of bound, then you get an minutes error not something else.

Right now, you cannot be sure if the minutes are the culprit of the error.

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