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 more tests and run code coverage analysis #53

Merged
merged 15 commits into from
Jun 20, 2023
Merged

Add more tests and run code coverage analysis #53

merged 15 commits into from
Jun 20, 2023

Conversation

rubdos
Copy link
Member

@rubdos rubdos commented Apr 18, 2023

This effectively adds reproducers for the failing #46 and #47 cases.

I have refactored the parsing tests to be more verbose when they fail and easier to dissect. This adds rstest and anyhow as dev-dependencies.

@gferon
Copy link
Contributor

gferon commented Apr 18, 2023

Very nice! Thanks a lot.

@rubdos
Copy link
Member Author

rubdos commented Apr 18, 2023

By trying this:

diff --git a/src/phone_number.rs b/src/phone_number.rs
index fe028ef..8f466e0 100644
--- a/src/phone_number.rs
+++ b/src/phone_number.rs
@@ -294,10 +294,12 @@ mod test {
         };

         let formatted = number.format().mode(mode).to_string();
-        parser::parse(country_hint, &formatted).with_context(|| {
+        let parsed = parser::parse(country_hint, &formatted).with_context(|| {
             format!("parsing {number} after formatting in {mode:?} mode as {formatted}")
         })?;

+        assert_eq!(number, parsed);
+
         Ok(())
     }
 }

I also discovered that the PhoneNumber Eq implementation is a bit wonky in my opinion:

---- phone_number::test::round_trip_parsing::case_2::mode_4_Mode__National stdout ----
thread 'phone_number::test::round_trip_parsing::case_2::mode_4_Mode__National' panicked at 'assertion failed: `(left == right)`
  left: `PhoneNumber { code: Code { value: 32, source: Plus }, national: NationalNumber { value: 474091150, zeros: 0 }, extension: None, carrier: None }`,
 right: `PhoneNumber { code: Code { value: 32, source: Default }, national: NationalNumber { value: 474091150, zeros: 0 }, extension: None, carrier: None }`',
src/phone_number.rs:301:9

I would expect those two numbers to satisfy Eq, but since they are parsed differently, they are not.

Should we reconsider PartialEq/Eq implementations?

@rubdos
Copy link
Member Author

rubdos commented Apr 18, 2023

Maybe we'd also want coverage analysis...

@rubdos rubdos changed the title Add more tests Add more tests and run code coverage analysis May 31, 2023
.github/workflows/build.yml Outdated Show resolved Hide resolved
@rubdos
Copy link
Member Author

rubdos commented May 31, 2023

Seems like coverage analysis works. https://app.codecov.io/github/whisperfish/rust-phonenumber/commit/15165d017b733bc38a27755eb3c0b042f83d447f

Now I'll want to add some feedback in a comment, that's for tomorrow now.

@codecov
Copy link

codecov bot commented May 31, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@03a4b04). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #53   +/-   ##
=======================================
  Coverage        ?   71.01%           
=======================================
  Files           ?       19           
  Lines           ?     1956           
  Branches        ?        0           
=======================================
  Hits            ?     1389           
  Misses          ?      567           
  Partials        ?        0           

@rubdos rubdos requested a review from gferon May 31, 2023 16:12
@rubdos rubdos enabled auto-merge (squash) June 20, 2023 08:23
@rubdos rubdos disabled auto-merge June 20, 2023 09:25
@rubdos rubdos merged commit e83afd7 into main Jun 20, 2023
@rubdos rubdos deleted the tests branch June 20, 2023 09:26
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.

2 participants