whisperfish / rust-phonenumber

Library for parsing, formatting and validating international phone numbers.
Apache License 2.0
162 stars 55 forks source link

Add more tests and run code coverage analysis #53

Closed rubdos closed 1 year ago

rubdos commented 1 year ago

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 commented 1 year ago

Very nice! Thanks a lot.

rubdos commented 1 year ago

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 commented 1 year ago

Maybe we'd also want coverage analysis...

rubdos commented 1 year ago

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[bot] commented 1 year ago

Codecov Report

:exclamation: 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