From 803ff73b549cffd5781d46a6f424db87588650f8 Mon Sep 17 00:00:00 2001 From: Ruben De Smet Date: Sun, 7 Jul 2024 11:17:20 +0200 Subject: [PATCH 1/8] Add proptest --- Cargo.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/Cargo.toml b/Cargo.toml index dae3d42..e1c7be5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -41,6 +41,7 @@ criterion = ">=0.4, <=0.5" doc-comment = "0.3" rstest = ">= 0.13, <=0.19" rstest_reuse = "0.6" +proptest = "1.0.0" [[bench]] name = "parsing" From 2f1ae24224794c6a872eae3ae32c1053f0b3c8ce Mon Sep 17 00:00:00 2001 From: Ruben De Smet Date: Sun, 7 Jul 2024 11:23:28 +0200 Subject: [PATCH 2/8] Add simple prop tests --- tests/prop.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 tests/prop.rs diff --git a/tests/prop.rs b/tests/prop.rs new file mode 100644 index 0000000..d41bbcc --- /dev/null +++ b/tests/prop.rs @@ -0,0 +1,15 @@ +use phonenumber::parse; +use proptest::prelude::*; + +proptest! { + #[test] + fn doesnt_crash(s in "\\PC*") { + let _ = parse(None, &s); + } + + #[test] + fn parse_belgian_phonenumbers(s in "\\+32[0-9]{8,9}") { + let parsed = parse(None, &s).expect("valid Belgian number"); + prop_assert_eq!(parsed.country().id(), phonenumber::country::BE.into()); + } +} From 82f2d2ed9e8690d6e54df59a1af269f9f0cddc92 Mon Sep 17 00:00:00 2001 From: Ruben De Smet Date: Sun, 7 Jul 2024 12:28:34 +0200 Subject: [PATCH 3/8] Add test case for advisory 2 --- src/parser/mod.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/parser/mod.rs b/src/parser/mod.rs index 26a3fc2..0e3110f 100644 --- a/src/parser/mod.rs +++ b/src/parser/mod.rs @@ -280,4 +280,10 @@ mod test { let res = parser::parse(None, ".;phone-context="); assert!(res.is_err(), "{res:?}"); } + + #[test] + fn advisory_2() { + let res = parser::parse(None, "+dwPAA;phone-context=AA"); + assert!(res.is_err(), "{res:?}"); + } } From a01a7f1f5c9bde9c8389ed8b3e498ae7a737bf35 Mon Sep 17 00:00:00 2001 From: Ruben De Smet Date: Sun, 7 Jul 2024 12:33:48 +0200 Subject: [PATCH 4/8] Remove panic on TooLong in NationalNumber Fix #69 --- src/national_number.rs | 11 +++++++---- src/parser/mod.rs | 14 +++++++------- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/national_number.rs b/src/national_number.rs index bc2dc7c..301a1b0 100644 --- a/src/national_number.rs +++ b/src/national_number.rs @@ -22,13 +22,16 @@ pub struct NationalNumber { } impl NationalNumber { - pub fn new(value: u64, zeros: u8) -> Self { + pub fn new(value: u64, zeros: u8) -> Result { // E.164 specifies a maximum of 15 decimals, which corresponds to slightly over 48.9 bits. // 56 bits ought to cut it here. - assert!(value < (1 << 56), "number too long"); - Self { - value: ((zeros as u64) << 56) | value, + if value >= (1 << 56) { + return Err(crate::error::Parse::TooLong); } + + Ok(Self { + value: ((zeros as u64) << 56) | value, + }) } /// The number without any leading zeroes. diff --git a/src/parser/mod.rs b/src/parser/mod.rs index 0e3110f..c5914a9 100644 --- a/src/parser/mod.rs +++ b/src/parser/mod.rs @@ -87,7 +87,7 @@ pub fn parse_with>( national: NationalNumber::new( number.national.parse()?, number.national.chars().take_while(|&c| c == '0').count() as u8, - ), + )?, extension: number.extension.map(|s| Extension(s.into_owned())), carrier: number.carrier.map(|s| Carrier(s.into_owned())), @@ -109,7 +109,7 @@ mod test { source: country::Source::Default, }, - national: NationalNumber::new(33316005, 0), + national: NationalNumber::new(33316005, 0).unwrap(), extension: None, carrier: None, @@ -197,7 +197,7 @@ mod test { source: country::Source::Number, }, - national: NationalNumber::new(64123456, 0), + national: NationalNumber::new(64123456, 0).unwrap(), extension: None, carrier: None, @@ -215,7 +215,7 @@ mod test { source: country::Source::Default, }, - national: NationalNumber::new(30123456, 0), + national: NationalNumber::new(30123456, 0).unwrap(), extension: None, carrier: None, @@ -230,7 +230,7 @@ mod test { source: country::Source::Plus, }, - national: NationalNumber::new(2345, 0,), + national: NationalNumber::new(2345, 0,).unwrap(), extension: None, carrier: None, @@ -245,7 +245,7 @@ mod test { source: country::Source::Default, }, - national: NationalNumber::new(12, 0,), + national: NationalNumber::new(12, 0,).unwrap(), extension: None, carrier: None, @@ -260,7 +260,7 @@ mod test { source: country::Source::Default, }, - national: NationalNumber::new(3121286979, 0), + national: NationalNumber::new(3121286979, 0).unwrap(), extension: None, carrier: Some("12".into()), From deefd99f55112a1d0add977c58c532b5d9899ea7 Mon Sep 17 00:00:00 2001 From: Ruben De Smet Date: Sun, 7 Jul 2024 12:35:32 +0200 Subject: [PATCH 5/8] More prop test crash tests --- tests/prop.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/prop.rs b/tests/prop.rs index d41bbcc..58322ad 100644 --- a/tests/prop.rs +++ b/tests/prop.rs @@ -2,11 +2,27 @@ use phonenumber::parse; use proptest::prelude::*; proptest! { + #[test] + fn rfc3966_crash_test(s in "(tel:)?\\PC*;phone-context=\\PC*") { + let _ = parse(None, &s); + } + + #[test] + fn rfc3966_crash_test_2(s in ".;phone-context=\\PC*") { + let _ = parse(None, &s); + } + + #[test] fn doesnt_crash(s in "\\PC*") { let _ = parse(None, &s); } + #[test] + fn doesnt_crash_2(s in "\\+\\PC*") { + let _ = parse(None, &s); + } + #[test] fn parse_belgian_phonenumbers(s in "\\+32[0-9]{8,9}") { let parsed = parse(None, &s).expect("valid Belgian number"); From 4384852e1bb8183a34d40f2a0c8000f1453cb1d1 Mon Sep 17 00:00:00 2001 From: Ruben De Smet Date: Sun, 7 Jul 2024 12:46:26 +0200 Subject: [PATCH 6/8] Simpligfy crash test case to capture both rfc3966 failures --- tests/prop.proptest-regressions | 8 ++++++++ tests/prop.rs | 8 +------- 2 files changed, 9 insertions(+), 7 deletions(-) create mode 100644 tests/prop.proptest-regressions diff --git a/tests/prop.proptest-regressions b/tests/prop.proptest-regressions new file mode 100644 index 0000000..985c14a --- /dev/null +++ b/tests/prop.proptest-regressions @@ -0,0 +1,8 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc b429ed298cc9cdf5ac50cf41cc9bf7819a00f9a8f8f3dab37240e6f4c7f13c88 # shrinks to s = "-;phone-context=Σ" +cc d2e7fadc2dd4bf597d2ba7e5079c5b0e0d53beab84b45fba0b598226514aa3a3 # shrinks to s = "+3a;phone-context=AAa0a" diff --git a/tests/prop.rs b/tests/prop.rs index 58322ad..343492f 100644 --- a/tests/prop.rs +++ b/tests/prop.rs @@ -3,16 +3,10 @@ use proptest::prelude::*; proptest! { #[test] - fn rfc3966_crash_test(s in "(tel:)?\\PC*;phone-context=\\PC*") { + fn rfc3966_crash_test(s in "(tel:)?.*;phone-context=\\PC*") { let _ = parse(None, &s); } - #[test] - fn rfc3966_crash_test_2(s in ".;phone-context=\\PC*") { - let _ = parse(None, &s); - } - - #[test] fn doesnt_crash(s in "\\PC*") { let _ = parse(None, &s); From 5be48f0743c8d08ff23bde04f05b10fd7b764481 Mon Sep 17 00:00:00 2001 From: Ruben De Smet Date: Sun, 7 Jul 2024 13:10:57 +0200 Subject: [PATCH 7/8] More modular rfc3966 test --- tests/prop.proptest-regressions | 4 ++-- tests/prop.rs | 11 ++++++++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/tests/prop.proptest-regressions b/tests/prop.proptest-regressions index 985c14a..84f2751 100644 --- a/tests/prop.proptest-regressions +++ b/tests/prop.proptest-regressions @@ -4,5 +4,5 @@ # # It is recommended to check this file in to source control so that # everyone who runs the test benefits from these saved cases. -cc b429ed298cc9cdf5ac50cf41cc9bf7819a00f9a8f8f3dab37240e6f4c7f13c88 # shrinks to s = "-;phone-context=Σ" -cc d2e7fadc2dd4bf597d2ba7e5079c5b0e0d53beab84b45fba0b598226514aa3a3 # shrinks to s = "+3a;phone-context=AAa0a" +cc f4f1a19cf143c767508ab557480843e4f4093898768fbbea1034d19d4308257d # shrinks to tel_prefix = false, use_plus = true, s = "da", phone_context = Some("A0A0a") +cc 4ea103e574793bd24b0267cc8a80962299ee50746d69332fbd0b85532fb707e2 # shrinks to tel_prefix = false, use_plus = false, s = "0", phone_context = Some("প") diff --git a/tests/prop.rs b/tests/prop.rs index 343492f..48a0d6b 100644 --- a/tests/prop.rs +++ b/tests/prop.rs @@ -3,7 +3,16 @@ use proptest::prelude::*; proptest! { #[test] - fn rfc3966_crash_test(s in "(tel:)?.*;phone-context=\\PC*") { + fn rfc3966_crash_test( + tel_prefix: bool, + use_plus: bool, + s: String, + phone_context: Option, + ) { + let context = if let Some(phone_context) = &phone_context { format!(";phone-context={phone_context}") } else { "".to_string() }; + let tel_prefix = if tel_prefix { "tel:" } else { "" }; + let plus = if use_plus { "+" } else { "" }; + let s = format!("{}{}{}{}", tel_prefix, plus, s, context); let _ = parse(None, &s); } From d2bef6df1aed6880b1ad04d397799b3cfb7e0421 Mon Sep 17 00:00:00 2001 From: Ruben De Smet Date: Sun, 7 Jul 2024 14:02:43 +0200 Subject: [PATCH 8/8] Add proptest to CI on 2^16 cases in release mode --- .github/workflows/build.yml | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 37d44d5..6b4eb6d 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -25,7 +25,13 @@ jobs: toolchain: ["stable", "beta"] coverage: [false] tests: [true] + proptest_max: [false] include: + # We run the proptests with the stable toolchain on more iterations + - toolchain: "stable" + coverage: false + tests: true + proptest_max: true - toolchain: "nightly" coverage: true tests: true @@ -60,11 +66,20 @@ jobs: - name: Run tests uses: actions-rs/cargo@v1 - if: ${{ !matrix.coverage && matrix.tests }} + if: ${{ !matrix.coverage && matrix.tests && !matrix.proptest_max }} with: command: test args: --all-targets --no-fail-fast + - name: Run tests + uses: actions-rs/cargo@v1 + if: ${{ !matrix.coverage && matrix.tests && matrix.proptest_max }} + env: + PROPTEST_CASES: 65536 + with: + command: test + args: --all-targets --release --no-fail-fast + - name: Run tests uses: actions-rs/cargo@v1 if: ${{ matrix.coverage && matrix.tests }}