From 56a222cd30a3de3e169250e6f86ed34dedc2b45d Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Tue, 21 Aug 2018 10:39:18 -0400 Subject: [PATCH 01/31] a start on using proptest to fuzz the resolver --- Cargo.toml | 1 + tests/testsuite/main.rs | 2 + tests/testsuite/resolve.rs | 88 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index 59b7427b8b2..1a942a9335e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -93,6 +93,7 @@ features = [ [dev-dependencies] bufstream = "0.1" +proptest = "0.8.4" [[bin]] name = "cargo" diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs index 7ec164ef00c..280f0afd9f9 100644 --- a/tests/testsuite/main.rs +++ b/tests/testsuite/main.rs @@ -11,6 +11,8 @@ extern crate glob; extern crate hex; extern crate libc; #[macro_use] +extern crate proptest; +#[macro_use] extern crate serde_derive; #[macro_use] extern crate serde_json; diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index 2acaadab037..0660835c0bf 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -12,6 +12,12 @@ use support::ChannelChanger; use support::{execs, project}; use support::registry::Package; +use proptest::collection::btree_map; +use proptest::collection::vec; +use proptest::prelude::*; +use proptest::sample::subsequence; +use std::iter; + fn resolve( pkg: &PackageId, deps: Vec, @@ -185,6 +191,88 @@ fn loc_names(names: &[(&'static str, &'static str)]) -> Vec { .collect() } +/// This generates a random registry index. +/// Unlike vec((Name, Ver, vec((Name, VerRq), ..), ..) +/// This strategy has a high probability of having valid dependencies +fn registry_strategy() -> impl Strategy> { + const VALID_NAME_STRATEGY: &str = "[A-Za-z_-][A-Za-z0-9_-]*"; + const MAX_CRATES: usize = 10; + const MAX_VERSIONS: usize = 10; + + fn range(max: usize) -> impl Strategy { + (0..max).prop_flat_map(move |low| (Just(low), low..=max)) + } + btree_map(VALID_NAME_STRATEGY, 1..=MAX_CRATES, 1..=MAX_VERSIONS) + .prop_map(|mut b| { + // root is the name of the thing being compiled + // so it would be confusing to have it in the index + b.remove("root"); + ( + b.iter().map(|(name, _)| name.clone()).collect::>(), + b.iter() + .flat_map(|(name, &num)| { + iter::repeat(name.clone()) + .take(num) + .enumerate() + .map(|(i, name)| (name, format!("{}.0.0", i + 1))) + }) + .collect::>(), + ) + }) + .prop_flat_map(|(names, data)| { + let names_len = names.len(); + let data_len = data.len(); + ( + Just(data), + vec(subsequence(names, 0..names_len), data_len..=data_len), + ) + }) + .prop_flat_map(|(data, deps)| { + let len = deps.iter().map(|x| x.len()).sum(); + ( + Just(data), + Just(deps), + vec( + range(MAX_VERSIONS).prop_map(|(b, e)| format!(">={}.0.0, <={}.0.0", b, e)), + len..=len, + ), + ) + }) + .prop_map(|(data, deps, vers)| { + let mut i = 0; + data.into_iter() + .zip(deps.into_iter()) + .map(|((name, ver), deps)| { + let d: Vec = deps.into_iter() + .filter(|n| &name < n) + .map(|n| { + i += 1; + dep_req(&n, &vers[i - 1]) + }) + .collect(); + let pkgid = (name.as_str(), ver).to_pkgid(); + let link = if pkgid.name().ends_with("-sys") { Some(pkgid.name().as_str()) } else { None }; + + Summary::new(pkgid, d, &BTreeMap::>::new(), link, false).unwrap() + }) + .collect() + }) +} + +proptest! { + #[test] + fn doesnt_crash(reg in registry_strategy()) { + let this = reg.last().unwrap().name(); + let reg = registry(reg); + + resolve( + &pkg_id("root"), + vec![dep(&this)], + ®, + ).unwrap(); + } +} + #[test] #[should_panic(expected = "assertion failed: !name.is_empty()")] fn test_dependency_with_empty_name() { From 17618869e0b77373b6bba3735452e9ef6efb1e70 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Tue, 21 Aug 2018 16:21:53 -0400 Subject: [PATCH 02/31] cache the example url to solve performance problem --- tests/testsuite/main.rs | 2 ++ tests/testsuite/resolve.rs | 13 +++++++------ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs index 280f0afd9f9..5f47f7b73dd 100644 --- a/tests/testsuite/main.rs +++ b/tests/testsuite/main.rs @@ -9,6 +9,8 @@ extern crate flate2; extern crate git2; extern crate glob; extern crate hex; +#[macro_use] +extern crate lazy_static; extern crate libc; #[macro_use] extern crate proptest; diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index 0660835c0bf..c481a9a788e 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -62,10 +62,13 @@ trait ToDep { fn to_dep(self) -> Dependency; } +lazy_static! { + static ref EXAMPLE_DOT_COM: ::url::Url = "http://example.com".to_url().unwrap(); +} + impl ToDep for &'static str { fn to_dep(self) -> Dependency { - let url = "http://example.com".to_url().unwrap(); - let source_id = SourceId::for_registry(&url).unwrap(); + let source_id = SourceId::for_registry(&EXAMPLE_DOT_COM).unwrap(); Dependency::parse_no_deprecated(self, Some("1.0.0"), &source_id).unwrap() } } @@ -117,8 +120,7 @@ macro_rules! pkg { } fn registry_loc() -> SourceId { - let remote = "http://example.com".to_url().unwrap(); - SourceId::for_registry(&remote).unwrap() + SourceId::for_registry(&EXAMPLE_DOT_COM).unwrap() } fn pkg(name: &str) -> Summary { @@ -161,8 +163,7 @@ fn dep(name: &str) -> Dependency { dep_req(name, "1.0.0") } fn dep_req(name: &str, req: &str) -> Dependency { - let url = "http://example.com".to_url().unwrap(); - let source_id = SourceId::for_registry(&url).unwrap(); + let source_id = SourceId::for_registry(&EXAMPLE_DOT_COM).unwrap(); Dependency::parse_no_deprecated(name, Some(req), &source_id).unwrap() } From ce1772c85a23093e0f4a86a3a2a438192654bc49 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Tue, 21 Aug 2018 16:43:06 -0400 Subject: [PATCH 03/31] get working with minimal-versions --- Cargo.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/Cargo.toml b/Cargo.toml index 1a942a9335e..6ace9e03a93 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -94,6 +94,7 @@ features = [ [dev-dependencies] bufstream = "0.1" proptest = "0.8.4" +wait-timeout = "0.1.4" # required only for minimal-versions. brought in by proptest. [[bin]] name = "cargo" From 4e619d87461287efe808c9a66a5ba62d7e521960 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Wed, 22 Aug 2018 10:36:16 -0400 Subject: [PATCH 04/31] better generation of version numbers --- tests/testsuite/resolve.rs | 143 +++++++++++++++++++------------------ 1 file changed, 75 insertions(+), 68 deletions(-) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index c481a9a788e..aaf55dd7e52 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -1,4 +1,4 @@ -use std::collections::{BTreeMap, HashSet}; +use std::collections::{BTreeMap, BTreeSet, HashSet}; use support::hamcrest::{assert_that, contains, is_not}; @@ -12,11 +12,9 @@ use support::ChannelChanger; use support::{execs, project}; use support::registry::Package; -use proptest::collection::btree_map; -use proptest::collection::vec; +use proptest::collection::{btree_map, btree_set, vec}; use proptest::prelude::*; use proptest::sample::subsequence; -use std::iter; fn resolve( pkg: &PackageId, @@ -195,7 +193,7 @@ fn loc_names(names: &[(&'static str, &'static str)]) -> Vec { /// This generates a random registry index. /// Unlike vec((Name, Ver, vec((Name, VerRq), ..), ..) /// This strategy has a high probability of having valid dependencies -fn registry_strategy() -> impl Strategy> { +fn registry_strategy() -> impl Strategy> { const VALID_NAME_STRATEGY: &str = "[A-Za-z_-][A-Za-z0-9_-]*"; const MAX_CRATES: usize = 10; const MAX_VERSIONS: usize = 10; @@ -203,74 +201,83 @@ fn registry_strategy() -> impl Strategy> { fn range(max: usize) -> impl Strategy { (0..max).prop_flat_map(move |low| (Just(low), low..=max)) } - btree_map(VALID_NAME_STRATEGY, 1..=MAX_CRATES, 1..=MAX_VERSIONS) - .prop_map(|mut b| { - // root is the name of the thing being compiled - // so it would be confusing to have it in the index - b.remove("root"); - ( - b.iter().map(|(name, _)| name.clone()).collect::>(), - b.iter() - .flat_map(|(name, &num)| { - iter::repeat(name.clone()) - .take(num) - .enumerate() - .map(|(i, name)| (name, format!("{}.0.0", i + 1))) + + fn ver(max: usize) -> impl Strategy { + (0..=max, 0..=max, 0..=max) + } + + btree_map( + VALID_NAME_STRATEGY, + btree_set( + ver(MAX_VERSIONS).prop_map(|(a, b, c)| format!("{}.{}.{}", a, b, c)), + 1..=MAX_VERSIONS, + ), + 1..=MAX_CRATES, + ).prop_map(|mut b| { + // root is the name of the thing being compiled + // so it would be confusing to have it in the index + b.remove("root"); + // if randomly pointing to a dependency that does not exist then call it `bad` + b.insert("bad".to_owned(), BTreeSet::new()); + ( + b.iter() + .map(|(name, _)| name.clone()) + .collect::>(), + b.iter() + .flat_map(|(name, vers)| vers.into_iter().map(move |v| (name.clone(), v.clone()))) + .collect::>(), + ) + }).prop_flat_map(|(names, data)| { + let names_len = names.len(); + let data_len = data.len(); + ( + Just(data), + vec(subsequence(names, 0..names_len), data_len..=data_len), + ) + }).prop_flat_map(|(data, deps)| { + let len = deps.iter().map(|x| x.len()).sum(); + ( + Just(data), + Just(deps), + vec( + range(MAX_VERSIONS).prop_map(|(b, e)| format!(">={}.0.0, <={}.0.0", b, e)), + len..=len, + ), + ) + }).prop_map(|(data, deps, vers)| { + let mut i = 0; + data.into_iter() + .zip(deps.into_iter()) + .map(|((name, ver), deps)| { + let d: Vec = deps.into_iter() + .filter(|n| &name < n) + .map(|n| { + i += 1; + dep_req(&n, &vers[i - 1]) }) - .collect::>(), - ) - }) - .prop_flat_map(|(names, data)| { - let names_len = names.len(); - let data_len = data.len(); - ( - Just(data), - vec(subsequence(names, 0..names_len), data_len..=data_len), - ) - }) - .prop_flat_map(|(data, deps)| { - let len = deps.iter().map(|x| x.len()).sum(); - ( - Just(data), - Just(deps), - vec( - range(MAX_VERSIONS).prop_map(|(b, e)| format!(">={}.0.0, <={}.0.0", b, e)), - len..=len, - ), - ) - }) - .prop_map(|(data, deps, vers)| { - let mut i = 0; - data.into_iter() - .zip(deps.into_iter()) - .map(|((name, ver), deps)| { - let d: Vec = deps.into_iter() - .filter(|n| &name < n) - .map(|n| { - i += 1; - dep_req(&n, &vers[i - 1]) - }) - .collect(); - let pkgid = (name.as_str(), ver).to_pkgid(); - let link = if pkgid.name().ends_with("-sys") { Some(pkgid.name().as_str()) } else { None }; - - Summary::new(pkgid, d, &BTreeMap::>::new(), link, false).unwrap() - }) - .collect() - }) + .collect(); + let pkgid = (name.as_str(), &*ver).to_pkgid(); + let link = if pkgid.name().ends_with("-sys") { Some(pkgid.name().as_str()) } else { None }; + + Summary::new(pkgid, d, &BTreeMap::>::new(), link, false).unwrap() + }) + .collect() + }) } proptest! { #[test] - fn doesnt_crash(reg in registry_strategy()) { - let this = reg.last().unwrap().name(); - let reg = registry(reg); - - resolve( - &pkg_id("root"), - vec![dep(&this)], - ®, - ).unwrap(); + fn doesnt_crash(input in registry_strategy()) { + let reg = registry(input.clone()); + // there is only a small chance that eny one + // crate will be interesting. So we try them all. + for this in input { + let _res = resolve( + &pkg_id("root"), + vec![dep_req(&this.name(), &format!("={}", this.version()))], + ®, + ); + } } } From 732aa10a9302b18a85905149028b7c80c3b3c267 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Wed, 22 Aug 2018 15:19:30 -0400 Subject: [PATCH 05/31] small clean up --- tests/testsuite/resolve.rs | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index aaf55dd7e52..8fac2dd41e8 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -213,21 +213,18 @@ fn registry_strategy() -> impl Strategy> { 1..=MAX_VERSIONS, ), 1..=MAX_CRATES, - ).prop_map(|mut b| { + ).prop_flat_map(|mut b| { // root is the name of the thing being compiled // so it would be confusing to have it in the index b.remove("root"); // if randomly pointing to a dependency that does not exist then call it `bad` b.insert("bad".to_owned(), BTreeSet::new()); - ( - b.iter() - .map(|(name, _)| name.clone()) - .collect::>(), - b.iter() - .flat_map(|(name, vers)| vers.into_iter().map(move |v| (name.clone(), v.clone()))) - .collect::>(), - ) - }).prop_flat_map(|(names, data)| { + let names = b.iter() + .map(|(name, _)| name.clone()) + .collect::>(); + let data = b.iter() + .flat_map(|(name, vers)| vers.into_iter().map(move |v| (name.clone(), v.clone()))) + .collect::>(); let names_len = names.len(); let data_len = data.len(); ( From c26553ac03ae1ede59a2bde3c1c9ce4150e4d9f3 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Wed, 22 Aug 2018 16:38:28 -0400 Subject: [PATCH 06/31] small clean up --- tests/testsuite/resolve.rs | 58 ++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 31 deletions(-) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index 8fac2dd41e8..e0949e8a3cf 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -81,39 +81,33 @@ trait ToPkgId { fn to_pkgid(&self) -> PackageId; } -impl<'a> ToPkgId for &'a str { +impl<'a> ToPkgId for PackageId { fn to_pkgid(&self) -> PackageId { - PackageId::new(*self, "1.0.0", ®istry_loc()).unwrap() + self.clone() } } -impl<'a> ToPkgId for (&'a str, &'a str) { +impl<'a> ToPkgId for &'a str { fn to_pkgid(&self) -> PackageId { - let (name, vers) = *self; - PackageId::new(name, vers, ®istry_loc()).unwrap() + PackageId::new(*self, "1.0.0", ®istry_loc()).unwrap() } } -impl<'a> ToPkgId for (&'a str, String) { +impl, U: AsRef> ToPkgId for (T, U) { fn to_pkgid(&self) -> PackageId { - let (name, ref vers) = *self; - PackageId::new(name, vers, ®istry_loc()).unwrap() + let (name, vers) = self; + PackageId::new(name.as_ref(), vers.as_ref(), ®istry_loc()).unwrap() } } macro_rules! pkg { ($pkgid:expr => [$($deps:expr),+]) => ({ let d: Vec = vec![$($deps.to_dep()),+]; - let pkgid = $pkgid.to_pkgid(); - let link = if pkgid.name().ends_with("-sys") {Some(pkgid.name().as_str())} else {None}; - - Summary::new(pkgid, d, &BTreeMap::>::new(), link, false).unwrap() + pkg_dep($pkgid, d) }); ($pkgid:expr) => ({ - let pkgid = $pkgid.to_pkgid(); - let link = if pkgid.name().ends_with("-sys") {Some(pkgid.name().as_str())} else {None}; - Summary::new(pkgid, Vec::new(), &BTreeMap::>::new(), link, false).unwrap() + pkg($pkgid) }) } @@ -121,13 +115,18 @@ fn registry_loc() -> SourceId { SourceId::for_registry(&EXAMPLE_DOT_COM).unwrap() } -fn pkg(name: &str) -> Summary { - let link = if name.ends_with("-sys") { - Some(name) +fn pkg(name: T) -> Summary { + pkg_dep(name, Vec::new()) +} + +fn pkg_dep(name: T, dep: Vec) -> Summary { + let pkgid = name.to_pkgid(); + let link = if pkgid.name().ends_with("-sys") { + Some(pkgid.name().as_str()) } else { None }; - Summary::new(pkg_id(name), Vec::new(), &BTreeMap::>::new(), link, false).unwrap() + Summary::new(name.to_pkgid(), dep, &BTreeMap::>::new(), link, false).unwrap() } fn pkg_id(name: &str) -> PackageId { @@ -209,7 +208,7 @@ fn registry_strategy() -> impl Strategy> { btree_map( VALID_NAME_STRATEGY, btree_set( - ver(MAX_VERSIONS).prop_map(|(a, b, c)| format!("{}.{}.{}", a, b, c)), + ver(MAX_VERSIONS), 1..=MAX_VERSIONS, ), 1..=MAX_CRATES, @@ -220,10 +219,10 @@ fn registry_strategy() -> impl Strategy> { // if randomly pointing to a dependency that does not exist then call it `bad` b.insert("bad".to_owned(), BTreeSet::new()); let names = b.iter() - .map(|(name, _)| name.clone()) - .collect::>(); + .map(|(name, _)| pkg_id(name).name()) + .collect::>(); let data = b.iter() - .flat_map(|(name, vers)| vers.into_iter().map(move |v| (name.clone(), v.clone()))) + .flat_map(|(name, vers)| vers.iter().map(move |(a, b, c)| (name, format!("{}.{}.{}", a, b, c)).to_pkgid())) .collect::>(); let names_len = names.len(); let data_len = data.len(); @@ -245,18 +244,15 @@ fn registry_strategy() -> impl Strategy> { let mut i = 0; data.into_iter() .zip(deps.into_iter()) - .map(|((name, ver), deps)| { + .map(|(pkgid, deps)| { let d: Vec = deps.into_iter() - .filter(|n| &name < n) + .filter(|n| pkgid.name().as_str() < n) .map(|n| { i += 1; dep_req(&n, &vers[i - 1]) }) .collect(); - let pkgid = (name.as_str(), &*ver).to_pkgid(); - let link = if pkgid.name().ends_with("-sys") { Some(pkgid.name().as_str()) } else { None }; - - Summary::new(pkgid, d, &BTreeMap::>::new(), link, false).unwrap() + pkg_dep(pkgid, d) }) .collect() }) @@ -301,14 +297,14 @@ fn assert_same(a: &[PackageId], b: &[PackageId]) { #[test] fn test_resolving_only_package() { - let reg = registry(vec![pkg("foo")]); + let reg = registry(vec![pkg!("foo")]); let res = resolve(&pkg_id("root"), vec![dep("foo")], ®).unwrap(); assert_same(&res, &names(&["root", "foo"])); } #[test] fn test_resolving_one_dep() { - let reg = registry(vec![pkg("foo"), pkg("bar")]); + let reg = registry(vec![pkg!("foo"), pkg!("bar")]); let res = resolve(&pkg_id("root"), vec![dep("foo")], ®).unwrap(); assert_same(&res, &names(&["root", "foo"])); } From b7285e8880fb0e4f65982285d3ba789fefcb0e0f Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 23 Aug 2018 13:36:47 -0400 Subject: [PATCH 07/31] handle "bad" slightly better --- tests/testsuite/resolve.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index e0949e8a3cf..57dae65a0b5 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -1,4 +1,4 @@ -use std::collections::{BTreeMap, BTreeSet, HashSet}; +use std::collections::{BTreeMap, HashSet}; use support::hamcrest::{assert_that, contains, is_not}; @@ -216,10 +216,10 @@ fn registry_strategy() -> impl Strategy> { // root is the name of the thing being compiled // so it would be confusing to have it in the index b.remove("root"); - // if randomly pointing to a dependency that does not exist then call it `bad` - b.insert("bad".to_owned(), BTreeSet::new()); - let names = b.iter() - .map(|(name, _)| pkg_id(name).name()) + let names = Some("bad".to_owned()) // if randomly pointing to a dependency that does not exist then call it `bad` + .iter() + .chain(b.iter().map(|(name, _)| name)) + .map(|name| pkg_id(name).name()) .collect::>(); let data = b.iter() .flat_map(|(name, vers)| vers.iter().map(move |(a, b, c)| (name, format!("{}.{}.{}", a, b, c)).to_pkgid())) @@ -246,7 +246,7 @@ fn registry_strategy() -> impl Strategy> { .zip(deps.into_iter()) .map(|(pkgid, deps)| { let d: Vec = deps.into_iter() - .filter(|n| pkgid.name().as_str() < n) + .filter(|n| pkgid.name() < *n || n.as_str() == "bad") .map(|n| { i += 1; dep_req(&n, &vers[i - 1]) @@ -264,7 +264,7 @@ proptest! { let reg = registry(input.clone()); // there is only a small chance that eny one // crate will be interesting. So we try them all. - for this in input { + for this in input.iter().rev() { let _res = resolve( &pkg_id("root"), vec![dep_req(&this.name(), &format!("={}", this.version()))], From 5339e92088cfa8e11b5d0fe7371a5f588e2daceb Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 23 Aug 2018 13:46:56 -0400 Subject: [PATCH 08/31] small clean up for the cache of the example url --- tests/testsuite/resolve.rs | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index 57dae65a0b5..e6b8740b941 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -60,14 +60,9 @@ trait ToDep { fn to_dep(self) -> Dependency; } -lazy_static! { - static ref EXAMPLE_DOT_COM: ::url::Url = "http://example.com".to_url().unwrap(); -} - impl ToDep for &'static str { fn to_dep(self) -> Dependency { - let source_id = SourceId::for_registry(&EXAMPLE_DOT_COM).unwrap(); - Dependency::parse_no_deprecated(self, Some("1.0.0"), &source_id).unwrap() + Dependency::parse_no_deprecated(self, Some("1.0.0"), ®istry_loc()).unwrap() } } @@ -112,7 +107,10 @@ macro_rules! pkg { } fn registry_loc() -> SourceId { - SourceId::for_registry(&EXAMPLE_DOT_COM).unwrap() + lazy_static! { + static ref EXAMPLE_DOT_COM: SourceId = SourceId::for_registry(&"http://example.com".to_url().unwrap()).unwrap(); + } + EXAMPLE_DOT_COM.clone() } fn pkg(name: T) -> Summary { @@ -160,8 +158,7 @@ fn dep(name: &str) -> Dependency { dep_req(name, "1.0.0") } fn dep_req(name: &str, req: &str) -> Dependency { - let source_id = SourceId::for_registry(&EXAMPLE_DOT_COM).unwrap(); - Dependency::parse_no_deprecated(name, Some(req), &source_id).unwrap() + Dependency::parse_no_deprecated(name, Some(req), ®istry_loc()).unwrap() } fn dep_loc(name: &str, location: &str) -> Dependency { From f270dda73526f3f56cd885ca17edc98693ba7551 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 23 Aug 2018 22:53:48 -0400 Subject: [PATCH 09/31] incorporate @AltSysrq suggestions --- tests/testsuite/resolve.rs | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index e6b8740b941..507343ee66b 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -1,4 +1,5 @@ use std::collections::{BTreeMap, HashSet}; +use std::cmp::{max, min}; use support::hamcrest::{assert_that, contains, is_not}; @@ -15,6 +16,7 @@ use support::registry::Package; use proptest::collection::{btree_map, btree_set, vec}; use proptest::prelude::*; use proptest::sample::subsequence; +use proptest::string::string_regex; fn resolve( pkg: &PackageId, @@ -190,20 +192,21 @@ fn loc_names(names: &[(&'static str, &'static str)]) -> Vec { /// Unlike vec((Name, Ver, vec((Name, VerRq), ..), ..) /// This strategy has a high probability of having valid dependencies fn registry_strategy() -> impl Strategy> { - const VALID_NAME_STRATEGY: &str = "[A-Za-z_-][A-Za-z0-9_-]*"; const MAX_CRATES: usize = 10; const MAX_VERSIONS: usize = 10; - fn range(max: usize) -> impl Strategy { - (0..max).prop_flat_map(move |low| (Just(low), low..=max)) + let valid_name_strategy = string_regex("[A-Za-z_-][A-Za-z0-9_-]*").unwrap(); + + fn range(m: usize) -> impl Strategy { + (..m, ..m).prop_map(|(a, b)| (min(a, b), max(a,b))) } fn ver(max: usize) -> impl Strategy { - (0..=max, 0..=max, 0..=max) + (..=max, ..=max, ..=max) } btree_map( - VALID_NAME_STRATEGY, + valid_name_strategy, btree_set( ver(MAX_VERSIONS), 1..=MAX_VERSIONS, @@ -225,16 +228,10 @@ fn registry_strategy() -> impl Strategy> { let data_len = data.len(); ( Just(data), - vec(subsequence(names, 0..names_len), data_len..=data_len), - ) - }).prop_flat_map(|(data, deps)| { - let len = deps.iter().map(|x| x.len()).sum(); - ( - Just(data), - Just(deps), + vec(subsequence(names, ..names_len), data_len), vec( range(MAX_VERSIONS).prop_map(|(b, e)| format!(">={}.0.0, <={}.0.0", b, e)), - len..=len, + names_len*data_len, ), ) }).prop_map(|(data, deps, vers)| { @@ -262,11 +259,15 @@ proptest! { // there is only a small chance that eny one // crate will be interesting. So we try them all. for this in input.iter().rev() { - let _res = resolve( + let res = resolve( &pkg_id("root"), vec![dep_req(&this.name(), &format!("={}", this.version()))], ®, ); + + if let Ok(r) = res { + prop_assert!(r.len() <= 4) + } } } } From 17fe190aba1e03ff9234ccb98758b9de74062ea7 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Sun, 26 Aug 2018 22:11:20 -0400 Subject: [PATCH 10/31] update to the new version of proptest --- Cargo.toml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 6ace9e03a93..d5df59ef4d4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -93,8 +93,7 @@ features = [ [dev-dependencies] bufstream = "0.1" -proptest = "0.8.4" -wait-timeout = "0.1.4" # required only for minimal-versions. brought in by proptest. +proptest = "0.8.6" [[bin]] name = "cargo" From 85b1976dab0cf47aabef78b9a6da184f765303b3 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Sun, 26 Aug 2018 22:16:41 -0400 Subject: [PATCH 11/31] use the new `impl Strategy for Vec` to only generate valid dependency names --- tests/testsuite/resolve.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index 507343ee66b..2088c2084b7 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -225,10 +225,15 @@ fn registry_strategy() -> impl Strategy> { .flat_map(|(name, vers)| vers.iter().map(move |(a, b, c)| (name, format!("{}.{}.{}", a, b, c)).to_pkgid())) .collect::>(); let names_len = names.len(); + let deps: Vec<_> = data.iter().map(|name| { + let s: Vec<_> = names.iter().cloned().filter(|&n| name.name() < n || n.as_str() == "bad").collect(); + let s_len = s.len(); + subsequence(s, ..s_len) + }).collect(); let data_len = data.len(); ( Just(data), - vec(subsequence(names, ..names_len), data_len), + deps, vec( range(MAX_VERSIONS).prop_map(|(b, e)| format!(">={}.0.0, <={}.0.0", b, e)), names_len*data_len, @@ -240,7 +245,6 @@ fn registry_strategy() -> impl Strategy> { .zip(deps.into_iter()) .map(|(pkgid, deps)| { let d: Vec = deps.into_iter() - .filter(|n| pkgid.name() < *n || n.as_str() == "bad") .map(|n| { i += 1; dep_req(&n, &vers[i - 1]) From 0ef43cb854005ba3b92f3e5ce71987b9ca253960 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Sun, 26 Aug 2018 23:03:20 -0400 Subject: [PATCH 12/31] use the new `result_cache` --- tests/testsuite/resolve.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index 2088c2084b7..a9da6e96a85 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -17,6 +17,7 @@ use proptest::collection::{btree_map, btree_set, vec}; use proptest::prelude::*; use proptest::sample::subsequence; use proptest::string::string_regex; +use proptest::test_runner::basic_result_cache; fn resolve( pkg: &PackageId, @@ -257,12 +258,18 @@ fn registry_strategy() -> impl Strategy> { } proptest! { + #![proptest_config(ProptestConfig { + result_cache: basic_result_cache, + cases: 256, + .. ProptestConfig::default() + })] #[test] fn doesnt_crash(input in registry_strategy()) { let reg = registry(input.clone()); // there is only a small chance that eny one - // crate will be interesting. So we try them all. - for this in input.iter().rev() { + // crate will be interesting. + // So we some of the most complicated. + for this in input.iter().rev().take(100) { let res = resolve( &pkg_id("root"), vec![dep_req(&this.name(), &format!("={}", this.version()))], From 0206cec0a64f35e9b5b3473f00210f5a4ebff436 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Mon, 27 Aug 2018 13:01:27 -0400 Subject: [PATCH 13/31] double down on `prop_flat_map` to guarantee version requirements are meet by the crate named. --- tests/testsuite/resolve.rs | 100 +++++++++++++++++++++---------------- 1 file changed, 57 insertions(+), 43 deletions(-) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index a9da6e96a85..0bf982dd32c 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -1,5 +1,4 @@ use std::collections::{BTreeMap, HashSet}; -use std::cmp::{max, min}; use support::hamcrest::{assert_that, contains, is_not}; @@ -13,7 +12,7 @@ use support::ChannelChanger; use support::{execs, project}; use support::registry::Package; -use proptest::collection::{btree_map, btree_set, vec}; +use proptest::collection::{btree_map, btree_set}; use proptest::prelude::*; use proptest::sample::subsequence; use proptest::string::string_regex; @@ -192,26 +191,19 @@ fn loc_names(names: &[(&'static str, &'static str)]) -> Vec { /// This generates a random registry index. /// Unlike vec((Name, Ver, vec((Name, VerRq), ..), ..) /// This strategy has a high probability of having valid dependencies -fn registry_strategy() -> impl Strategy> { - const MAX_CRATES: usize = 10; +fn registry_strategy() -> impl Strategy> { + const MAX_CRATES: usize = 50; const MAX_VERSIONS: usize = 10; let valid_name_strategy = string_regex("[A-Za-z_-][A-Za-z0-9_-]*").unwrap(); - fn range(m: usize) -> impl Strategy { - (..m, ..m).prop_map(|(a, b)| (min(a, b), max(a,b))) - } - - fn ver(max: usize) -> impl Strategy { + fn ver(max: usize) -> impl Strategy { (..=max, ..=max, ..=max) } btree_map( valid_name_strategy, - btree_set( - ver(MAX_VERSIONS), - 1..=MAX_VERSIONS, - ), + btree_set(ver(MAX_VERSIONS), 1..=MAX_VERSIONS), 1..=MAX_CRATES, ).prop_flat_map(|mut b| { // root is the name of the thing being compiled @@ -222,37 +214,59 @@ fn registry_strategy() -> impl Strategy> { .chain(b.iter().map(|(name, _)| name)) .map(|name| pkg_id(name).name()) .collect::>(); - let data = b.iter() - .flat_map(|(name, vers)| vers.iter().map(move |(a, b, c)| (name, format!("{}.{}.{}", a, b, c)).to_pkgid())) - .collect::>(); - let names_len = names.len(); - let deps: Vec<_> = data.iter().map(|name| { - let s: Vec<_> = names.iter().cloned().filter(|&n| name.name() < n || n.as_str() == "bad").collect(); - let s_len = s.len(); - subsequence(s, ..s_len) - }).collect(); - let data_len = data.len(); - ( - Just(data), - deps, - vec( - range(MAX_VERSIONS).prop_map(|(b, e)| format!(">={}.0.0, <={}.0.0", b, e)), - names_len*data_len, - ), - ) - }).prop_map(|(data, deps, vers)| { - let mut i = 0; + let data = b + .iter() + .flat_map(|(name, vers)| { + vers.iter() + .map(move |(a, b, c)| (name, format!("{}.{}.{}", a, b, c)).to_pkgid()) + }).collect::>(); + let deps: Vec<_> = data + .iter() + .map(|name| { + let s: Vec<_> = names + .iter() + .cloned() + .filter(|n| name.name() < *n || n.as_str() == "bad") + .collect(); + let s_len = s.len(); + let vers = b + .iter() + .map(|(name, ver)| (pkg_id(name).name(), ver.iter().cloned().collect())) + .collect::>>(); + subsequence(s, ..s_len).prop_flat_map(move |deps| { + deps.into_iter() + .map(|name| { + let s = vers.get(name.as_str()).unwrap_or(&vec![]).clone(); + ( + Just(name), + if s.is_empty() { + subsequence(s, 0) + } else if s.len() == 1 { + subsequence(s, 1) + } else { + subsequence(s, 1..=2) + }.prop_map(|v| { + if v.is_empty() { + "=6.6.6".to_string() + } else if v.len() == 1 { + format!("={}.{}.{}", v[0].0, v[0].1, v[0].2) + } else { + format!( + ">={}.{}.{}, <={}.{}.{}", + v[0].0, v[0].1, v[0].2, v[1].0, v[1].1, v[1].2 + ) + } + }), + ) + .prop_map(|(name, ver)| dep_req(&name, &ver)) + }).collect::>() + }) + }).collect(); + (Just(data), deps) + }).prop_map(|(data, deps)| { data.into_iter() .zip(deps.into_iter()) - .map(|(pkgid, deps)| { - let d: Vec = deps.into_iter() - .map(|n| { - i += 1; - dep_req(&n, &vers[i - 1]) - }) - .collect(); - pkg_dep(pkgid, d) - }) + .map(|(pkgid, deps)| pkg_dep(pkgid, deps)) .collect() }) } @@ -277,7 +291,7 @@ proptest! { ); if let Ok(r) = res { - prop_assert!(r.len() <= 4) + prop_assert!(r.len() <= 10) } } } From 1f988711fe4550779ef96de8ee57ae9dc5f19acc Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Sat, 25 Aug 2018 14:41:54 -0400 Subject: [PATCH 14/31] stronger assert in the core --- src/cargo/core/resolver/mod.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 3957976d3cb..4ebdb70bd11 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -200,7 +200,7 @@ fn activate_deps_loop( } } - let mut ticks = 0; + let mut ticks = 0u16; let start = Instant::now(); let time_to_print = Duration::from_millis(500); let mut printed = false; @@ -240,6 +240,18 @@ fn activate_deps_loop( config.shell().status("Resolving", "dependency graph...")?; } } + // The largest test in our sweet takes less then 5000 ticks + // with all the algorithm improvements. + // If any of them are removed then it takes more than I am willing to measure. + // So lets fail the test fast if we have ben running for two long. + debug_assert!(ticks < 50_000); + // The largest test in our sweet takes less then 30 sec + // with all the improvements to how fast a tick can go. + // If any of them are removed then it takes more than I am willing to measure. + // So lets fail the test fast if we have ben running for two long. + if cfg!(debug_assertions) && (ticks % 1000 == 0) { + assert!(start.elapsed() - deps_time < Duration::from_secs(300)); + } let just_here_for_the_error_messages = deps_frame.just_for_error_messages; From b0bbb6a86e9231914d9ddade50de0dedd12cf6d6 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Tue, 28 Aug 2018 14:15:30 -0400 Subject: [PATCH 15/31] use args for scale --- tests/testsuite/resolve.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index 0bf982dd32c..564b5595cf3 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -191,9 +191,7 @@ fn loc_names(names: &[(&'static str, &'static str)]) -> Vec { /// This generates a random registry index. /// Unlike vec((Name, Ver, vec((Name, VerRq), ..), ..) /// This strategy has a high probability of having valid dependencies -fn registry_strategy() -> impl Strategy> { - const MAX_CRATES: usize = 50; - const MAX_VERSIONS: usize = 10; +fn registry_strategy(max_crates: usize, max_versions: usize) -> impl Strategy> { let valid_name_strategy = string_regex("[A-Za-z_-][A-Za-z0-9_-]*").unwrap(); @@ -203,8 +201,8 @@ fn registry_strategy() -> impl Strategy> { btree_map( valid_name_strategy, - btree_set(ver(MAX_VERSIONS), 1..=MAX_VERSIONS), - 1..=MAX_CRATES, + btree_set(ver(max_versions), 1..=max_versions), + 1..=max_crates, ).prop_flat_map(|mut b| { // root is the name of the thing being compiled // so it would be confusing to have it in the index @@ -278,7 +276,7 @@ proptest! { .. ProptestConfig::default() })] #[test] - fn doesnt_crash(input in registry_strategy()) { + fn doesnt_crash(input in registry_strategy(50, 10)) { let reg = registry(input.clone()); // there is only a small chance that eny one // crate will be interesting. From 2628ec0ddd933410b45f3a42946d445806e852b8 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Wed, 29 Aug 2018 16:53:52 -0400 Subject: [PATCH 16/31] works a LOT better if the simple cases are at the beginning --- tests/testsuite/resolve.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index 564b5595cf3..1defd87763f 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -224,7 +224,7 @@ fn registry_strategy(max_crates: usize, max_versions: usize) -> impl Strategy = names .iter() .cloned() - .filter(|n| name.name() < *n || n.as_str() == "bad") + .filter(|n| name.name() > *n || n.as_str() == "bad") .collect(); let s_len = s.len(); let vers = b @@ -273,6 +273,7 @@ proptest! { #![proptest_config(ProptestConfig { result_cache: basic_result_cache, cases: 256, + max_flat_map_regens: 100, // raise this number for slower and better shrinking .. ProptestConfig::default() })] #[test] @@ -280,7 +281,7 @@ proptest! { let reg = registry(input.clone()); // there is only a small chance that eny one // crate will be interesting. - // So we some of the most complicated. + // So we try some of the most complicated. for this in input.iter().rev().take(100) { let res = resolve( &pkg_id("root"), @@ -289,7 +290,7 @@ proptest! { ); if let Ok(r) = res { - prop_assert!(r.len() <= 10) + prop_assert!(r.len() <= 20, "(\"{}\", =\"{}\")", this.name(), this.version()) } } } From 509806e5c462da3a599944a29250a7188e51d5c2 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 30 Aug 2018 13:15:30 -0400 Subject: [PATCH 17/31] small clean up --- tests/testsuite/resolve.rs | 53 +++++++++++++------------------------- 1 file changed, 18 insertions(+), 35 deletions(-) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index 1defd87763f..bf74153190e 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -78,7 +78,7 @@ trait ToPkgId { fn to_pkgid(&self) -> PackageId; } -impl<'a> ToPkgId for PackageId { +impl ToPkgId for PackageId { fn to_pkgid(&self) -> PackageId { self.clone() } @@ -207,34 +207,27 @@ fn registry_strategy(max_crates: usize, max_versions: usize) -> impl Strategy>(); - let data = b + .map(|(name, ver)| (pkg_id(name).name(), ver.iter().map(|(a, b, c)| format!("{}.{}.{}", a, b, c)).collect())) + .collect::>>(); + vers .iter() .flat_map(|(name, vers)| { vers.iter() - .map(move |(a, b, c)| (name, format!("{}.{}.{}", a, b, c)).to_pkgid()) - }).collect::>(); - let deps: Vec<_> = data - .iter() + .map(move |x| (name.as_str(), x).to_pkgid()) + }) .map(|name| { - let s: Vec<_> = names - .iter() - .cloned() - .filter(|n| name.name() > *n || n.as_str() == "bad") - .collect(); + let s: Vec<_> = Some(pkg_id("bad").name()) // if randomly pointing to a dependency that does not exist then call it `bad` + .into_iter() + .chain(vers.keys().cloned().take_while(|&n| name.name() > n)) + .collect::>(); let s_len = s.len(); - let vers = b - .iter() - .map(|(name, ver)| (pkg_id(name).name(), ver.iter().cloned().collect())) - .collect::>>(); + let vers = vers.clone(); subsequence(s, ..s_len).prop_flat_map(move |deps| { deps.into_iter() .map(|name| { - let s = vers.get(name.as_str()).unwrap_or(&vec![]).clone(); + let s = vers.get(&name).unwrap_or(&vec![]).clone(); ( Just(name), if s.is_empty() { @@ -247,25 +240,15 @@ fn registry_strategy(max_crates: usize, max_versions: usize) -> impl Strategy={}.{}.{}, <={}.{}.{}", - v[0].0, v[0].1, v[0].2, v[1].0, v[1].1, v[1].2 - ) + format!(">={}, <={}",v[0], v[1]) } }), - ) - .prop_map(|(name, ver)| dep_req(&name, &ver)) + ).prop_map(|(name, ver)| dep_req(&name, &ver)) }).collect::>() - }) - }).collect(); - (Just(data), deps) - }).prop_map(|(data, deps)| { - data.into_iter() - .zip(deps.into_iter()) - .map(|(pkgid, deps)| pkg_dep(pkgid, deps)) - .collect() + }).prop_map( move |deps| pkg_dep(name.clone(), deps)) + }).collect::>() }) } From 1eaf543ab87adc9c31e2e466a7f5dd558ccb0a7d Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 30 Aug 2018 15:29:10 -0400 Subject: [PATCH 18/31] small clean up --- tests/testsuite/resolve.rs | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index d09fe3381cd..0874f37a3bd 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -247,27 +247,26 @@ fn registry_strategy( subsequence(s, ..s_len) .prop_flat_map(move |deps| { deps.into_iter() - .map(|name| { - let s = vers.get(&name).unwrap_or(&vec![]).clone(); - ( - Just(name), - if s.is_empty() { - subsequence(s, 0) - } else if s.len() == 1 { - subsequence(s, 1) - } else { - subsequence(s, 1..=2) - }.prop_map(|v| { - if v.is_empty() { - "=6.6.6".to_string() + .map(|dep_name| { + let s = vers.get(&dep_name).unwrap_or(&vec![]).clone(); + if s.is_empty() { + subsequence(s, 0) + } else if s.len() == 1 { + subsequence(s, 1) + } else { + subsequence(s, 1..=2) + }.prop_map(move |v| { + dep_req( + &dep_name, + &if v.is_empty() { + "=6.6.6".to_owned() } else if v.len() == 1 { format!("={}", v[0]) } else { format!(">={}, <={}", v[0], v[1]) - } - }), - ) - .prop_map(|(name, ver)| dep_req(&name, &ver)) + }, + ) + }) }).collect::>() }).prop_map(move |deps| pkg_dep(name.clone(), deps)) }).collect::>() From 2b1c39f0802a06209c2ce3d4a3f38c7a2252ea0a Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 30 Aug 2018 17:42:07 -0400 Subject: [PATCH 19/31] more controllable bad dependencies, and use Index for ranges --- tests/testsuite/resolve.rs | 51 +++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index 0874f37a3bd..69836aa3f5a 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -1,4 +1,5 @@ use std::cmp::PartialEq; +use std::cmp::{max, min}; use std::collections::{BTreeMap, HashSet}; use cargo::core::dependency::Kind::{self, Development}; @@ -238,35 +239,35 @@ fn registry_strategy( vers.iter() .flat_map(|(name, vers)| vers.iter().map(move |x| (name.as_str(), x).to_pkgid())) .map(|name| { - let s: Vec<_> = Some(pkg_id("bad").name()) // if randomly pointing to a dependency that does not exist then call it `bad` - .into_iter() - .chain(vers.keys().cloned().take_while(|&n| name.name() > n)) + let s: Vec<_> = vers + .keys() + .cloned() + .take_while(|&n| name.name() > n) .collect::>(); let s_len = s.len(); let vers = vers.clone(); - subsequence(s, ..s_len) + prop::option::weighted(0.95, subsequence(s, ..=s_len)) .prop_flat_map(move |deps| { - deps.into_iter() + deps.unwrap_or_else(|| vec![pkg_id("bad").name()]) + .into_iter() .map(|dep_name| { let s = vers.get(&dep_name).unwrap_or(&vec![]).clone(); - if s.is_empty() { - subsequence(s, 0) - } else if s.len() == 1 { - subsequence(s, 1) - } else { - subsequence(s, 1..=2) - }.prop_map(move |v| { - dep_req( - &dep_name, - &if v.is_empty() { - "=6.6.6".to_owned() - } else if v.len() == 1 { - format!("={}", v[0]) - } else { - format!(">={}, <={}", v[0], v[1]) - }, - ) - }) + (any::(), any::()) + .prop_map(move |(a, b)| { + if s.is_empty() { + return dep_req(&dep_name, "=6.6.6"); + } + let (a, b) = (a.index(s.len()), b.index(s.len())); + let (a, b) = (min(a, b), max(a, b)); + dep_req( + &dep_name, + &if a == b { + format!("={}", s[a]) + } else { + format!(">={}, <={}", s[a], s[b]) + }, + ) + }) }).collect::>() }).prop_map(move |deps| pkg_dep(name.clone(), deps)) }).collect::>() @@ -286,7 +287,7 @@ proptest! { // there is only a small chance that eny one // crate will be interesting. // So we try some of the most complicated. - for this in input.iter().rev().take(100) { + for this in input.iter().rev().take(10) { let res = resolve( &pkg_id("root"), vec![dep_req(&this.name(), &format!("={}", this.version()))], @@ -294,7 +295,7 @@ proptest! { ); if let Ok(r) = res { - prop_assert!(r.len() <= 20, "(\"{}\", =\"{}\")", this.name(), this.version()) + prop_assert!(r.len() <= 30, "(\"{}\", =\"{}\")", this.name(), this.version()) } } } From 05c6ce0edd819eef8b13ba8c03bca891073e8e95 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Fri, 31 Aug 2018 15:13:20 -0400 Subject: [PATCH 20/31] remove a `prop_flat_map` use @AltSysrq suggestion again --- tests/testsuite/resolve.rs | 57 +++++++++++++++++++++----------------- 1 file changed, 32 insertions(+), 25 deletions(-) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index 69836aa3f5a..f95147963a0 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -11,7 +11,7 @@ use cargo::util::{CargoResult, Config, ToUrl}; use support::project; use support::registry::Package; -use proptest::collection::{btree_map, btree_set}; +use proptest::collection::{btree_map, btree_set, vec}; use proptest::prelude::*; use proptest::sample::subsequence; use proptest::string::string_regex; @@ -226,7 +226,7 @@ fn registry_strategy( // root is the name of the thing being compiled // so it would be confusing to have it in the index b.remove("root"); - let vers = b + let vers: BTreeMap<_, Vec> = b .iter() .map(|(name, ver)| { ( @@ -235,7 +235,7 @@ fn registry_strategy( .map(|(a, b, c)| format!("{}.{}.{}", a, b, c)) .collect(), ) - }).collect::>>(); + }).collect(); vers.iter() .flat_map(|(name, vers)| vers.iter().map(move |x| (name.as_str(), x).to_pkgid())) .map(|name| { @@ -246,30 +246,37 @@ fn registry_strategy( .collect::>(); let s_len = s.len(); let vers = vers.clone(); - prop::option::weighted(0.95, subsequence(s, ..=s_len)) - .prop_flat_map(move |deps| { - deps.unwrap_or_else(|| vec![pkg_id("bad").name()]) - .into_iter() - .map(|dep_name| { + prop::option::weighted( + 0.95, + ( + subsequence(s, ..=s_len), + vec( + (any::(), any::()), + s_len, + ), + ), + ).prop_map(move |deps| { + deps.map(|x| { + x.0.into_iter() + .zip(x.1.into_iter()) + .map(|(dep_name, (a, b))| { let s = vers.get(&dep_name).unwrap_or(&vec![]).clone(); - (any::(), any::()) - .prop_map(move |(a, b)| { - if s.is_empty() { - return dep_req(&dep_name, "=6.6.6"); - } - let (a, b) = (a.index(s.len()), b.index(s.len())); - let (a, b) = (min(a, b), max(a, b)); - dep_req( - &dep_name, - &if a == b { - format!("={}", s[a]) - } else { - format!(">={}, <={}", s[a], s[b]) - }, - ) - }) + if s.is_empty() { + return dep_req(&dep_name, "=6.6.6"); + } + let (a, b) = (a.index(s.len()), b.index(s.len())); + let (a, b) = (min(a, b), max(a, b)); + dep_req( + &dep_name, + &if a == b { + format!("={}", s[a]) + } else { + format!(">={}, <={}", s[a], s[b]) + }, + ) }).collect::>() - }).prop_map(move |deps| pkg_dep(name.clone(), deps)) + }).unwrap_or_else(|| vec![dep_req("bad", "=6.6.6")]) + }).prop_map(move |deps| pkg_dep(name.clone(), deps)) }).collect::>() }) } From 856964fcc807be3bfcf474c4f26ff2f84f1b0dd0 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Fri, 31 Aug 2018 17:37:05 -0400 Subject: [PATCH 21/31] fail fast on CI, don't shrink the input Thanks to, @Centril for suggesting this https://github.com/AltSysrq/proptest/issues/91#issuecomment-417778793 --- tests/testsuite/resolve.rs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index f95147963a0..158eec89f97 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -1,6 +1,7 @@ use std::cmp::PartialEq; use std::cmp::{max, min}; use std::collections::{BTreeMap, HashSet}; +use std::env; use cargo::core::dependency::Kind::{self, Development}; use cargo::core::resolver::{self, Method}; @@ -205,6 +206,18 @@ fn loc_names(names: &[(&'static str, &'static str)]) -> Vec { .collect() } +/// This attempts to make sure that CI will fail fast, +/// but that local builds will give a small clear test case. +fn ci_no_shrink( + s: impl Strategy + 'static, +) -> impl Strategy { + if true || env::var("CI").is_ok() { + s.no_shrink().boxed() + } else { + s.boxed() + } +} + /// This generates a random registry index. /// Unlike vec((Name, Ver, vec((Name, VerRq), ..), ..) /// This strategy has a high probability of having valid dependencies @@ -289,7 +302,7 @@ proptest! { .. ProptestConfig::default() })] #[test] - fn doesnt_crash(input in registry_strategy(50, 10)) { + fn doesnt_crash(input in ci_no_shrink(registry_strategy(50, 10))) { let reg = registry(input.clone()); // there is only a small chance that eny one // crate will be interesting. From 2b16156e47c57c61d033c63f79e9f96258179c5a Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Wed, 5 Sep 2018 15:18:46 -0400 Subject: [PATCH 22/31] If resolution was successful, then unpublishing a version of a crate that was not selected should not change that. --- tests/testsuite/resolve.rs | 48 +++++++++++++++++++++++++++++++++++--- 1 file changed, 45 insertions(+), 3 deletions(-) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index 158eec89f97..8d7e76596d3 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -302,7 +302,10 @@ proptest! { .. ProptestConfig::default() })] #[test] - fn doesnt_crash(input in ci_no_shrink(registry_strategy(50, 10))) { + fn limited_independence_of_irrelevant_alternatives( + input in ci_no_shrink(registry_strategy(50, 10)), + index_to_unpublish in any::() + ) { let reg = registry(input.clone()); // there is only a small chance that eny one // crate will be interesting. @@ -314,8 +317,47 @@ proptest! { ®, ); - if let Ok(r) = res { - prop_assert!(r.len() <= 30, "(\"{}\", =\"{}\")", this.name(), this.version()) + match res { + Ok(r) => { + // If resolution was successful, then unpublishing a version of a crate + // that was not selected should not change that. + let not_selected: Vec<_> = input + .iter() + .cloned() + .filter(|x| !r.contains(x.package_id())) + .collect(); + if !not_selected.is_empty() { + let summary_to_unpublish = index_to_unpublish.get(¬_selected); + let new_reg = registry( + input + .iter() + .cloned() + .filter(|x| summary_to_unpublish != x) + .collect(), + ); + + let res = resolve( + &pkg_id("root"), + vec![dep_req(&this.name(), &format!("={}", this.version()))], + &new_reg, + ); + + // Note: that we can not assert that the two `res` are identical + // as the resolver does depend on irrelevant alternatives. + // It uses how constrained a dependency requirement is + // to determine what order to evaluate requirements. + + prop_assert!( + res.is_ok(), + "unpublishing {:?} stopped `{} = \"={}\"` from working", + summary_to_unpublish, + this.name(), + this.version() + ) + } + } + + Err(_) => {} } } } From c4978512bcc82925c0b84226caf91a57102b9762 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Wed, 5 Sep 2018 17:15:19 -0400 Subject: [PATCH 23/31] If resolution was unsuccessful, then it should stay unsuccessful even if any version of a crate is unpublished. --- tests/testsuite/resolve.rs | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index 8d7e76596d3..78642895168 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -357,7 +357,32 @@ proptest! { } } - Err(_) => {} + Err(_) => { + // If resolution was unsuccessful, then it should stay unsuccessful + // even if any version of a crate is unpublished. + let summary_to_unpublish = index_to_unpublish.get(&input); + let new_reg = registry( + input + .iter() + .cloned() + .filter(|x| summary_to_unpublish != x) + .collect(), + ); + + let res = resolve( + &pkg_id("root"), + vec![dep_req(&this.name(), &format!("={}", this.version()))], + &new_reg, + ); + + prop_assert!( + res.is_err(), + "full index did not work for `{} = \"={}\"` but unpublishing {:?} fixed it!", + this.name(), + this.version(), + summary_to_unpublish + ) + } } } } From 1b15b1595234800d534bf0c5ed20d780d4cd8fc9 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Sun, 16 Sep 2018 22:41:15 -0400 Subject: [PATCH 24/31] give intermediate steps names --- tests/testsuite/resolve.rs | 39 ++++++++++++++++---------------------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index 78642895168..ea348355071 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -225,37 +225,30 @@ fn registry_strategy( max_crates: usize, max_versions: usize, ) -> impl Strategy> { - let valid_name_strategy = string_regex("[A-Za-z_-][A-Za-z0-9_-]*").unwrap(); + let name_strategy = string_regex("[A-Za-z_-][A-Za-z0-9_-]*(-sys)?").unwrap(); - fn ver(max: usize) -> impl Strategy { - (..=max, ..=max, ..=max) - } + let raw_version_strategy = [..max_versions; 3]; + let version_from_raw = |v: &[usize; 3]| format!("{}.{}.{}", v[0], v[1], v[2]); + + let list_of_versions_strategy = btree_set(raw_version_strategy, 1..=max_versions) + .prop_map(move |ver| ver.iter().map(version_from_raw).collect::>()); + + let list_of_crates_with_versions = + btree_map(name_strategy, list_of_versions_strategy, 1..=max_crates).prop_map(|mut vers| { + // root is the name of the thing being compiled + // so it would be confusing to have it in the index + vers.remove("root"); + vers + }); - btree_map( - valid_name_strategy, - btree_set(ver(max_versions), 1..=max_versions), - 1..=max_crates, - ).prop_flat_map(|mut b| { - // root is the name of the thing being compiled - // so it would be confusing to have it in the index - b.remove("root"); - let vers: BTreeMap<_, Vec> = b - .iter() - .map(|(name, ver)| { - ( - pkg_id(name).name(), - ver.iter() - .map(|(a, b, c)| format!("{}.{}.{}", a, b, c)) - .collect(), - ) - }).collect(); + list_of_crates_with_versions.prop_flat_map(|vers| { vers.iter() .flat_map(|(name, vers)| vers.iter().map(move |x| (name.as_str(), x).to_pkgid())) .map(|name| { let s: Vec<_> = vers .keys() .cloned() - .take_while(|&n| name.name() > n) + .take_while(|n| name.name().as_str() > n) .collect::>(); let s_len = s.len(); let vers = vers.clone(); From d8e19fb01fd3ec19b5ce64fba856e0e85a0ed0bb Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Mon, 17 Sep 2018 13:51:40 -0400 Subject: [PATCH 25/31] pull the has bad dep before `prop_flat_map` --- tests/testsuite/resolve.rs | 64 ++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 27 deletions(-) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index ea348355071..e7c9768fdf8 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -225,26 +225,41 @@ fn registry_strategy( max_crates: usize, max_versions: usize, ) -> impl Strategy> { - let name_strategy = string_regex("[A-Za-z_-][A-Za-z0-9_-]*(-sys)?").unwrap(); + let name = string_regex("[A-Za-z_-][A-Za-z0-9_-]*(-sys)?").unwrap(); - let raw_version_strategy = [..max_versions; 3]; + let raw_version = [..max_versions; 3]; let version_from_raw = |v: &[usize; 3]| format!("{}.{}.{}", v[0], v[1], v[2]); - let list_of_versions_strategy = btree_set(raw_version_strategy, 1..=max_versions) - .prop_map(move |ver| ver.iter().map(version_from_raw).collect::>()); + // If this is false than the crate will depend on the nonexistent "bad" + // instead of the complex set we generated for it. + let allow_deps = prop::bool::weighted(0.95); + + let list_of_versions = + btree_set((raw_version, allow_deps), 1..=max_versions).prop_map(move |ver| { + ver.iter() + .map(|a| (version_from_raw(&a.0), a.1)) + .collect::>() + }); let list_of_crates_with_versions = - btree_map(name_strategy, list_of_versions_strategy, 1..=max_crates).prop_map(|mut vers| { + btree_map(name, list_of_versions, 1..=max_crates).prop_map(|mut vers| { // root is the name of the thing being compiled // so it would be confusing to have it in the index vers.remove("root"); + // bad is a name reserved for a dep that won't work + vers.remove("bad"); vers }); list_of_crates_with_versions.prop_flat_map(|vers| { vers.iter() - .flat_map(|(name, vers)| vers.iter().map(move |x| (name.as_str(), x).to_pkgid())) - .map(|name| { + .flat_map(|(name, vers)| { + vers.iter() + .map(move |x| ((name.as_str(), &x.0).to_pkgid(), x.1)) + }).map(|(name, allow_deps)| { + if !allow_deps { + return Just(pkg_dep(name.clone(), vec![dep_req("bad", "*")])).boxed(); + } let s: Vec<_> = vers .keys() .cloned() @@ -252,37 +267,32 @@ fn registry_strategy( .collect::>(); let s_len = s.len(); let vers = vers.clone(); - prop::option::weighted( - 0.95, - ( - subsequence(s, ..=s_len), - vec( - (any::(), any::()), - s_len, - ), + ( + subsequence(s, ..=s_len), + vec( + (any::(), any::()), + s_len, ), - ).prop_map(move |deps| { - deps.map(|x| { - x.0.into_iter() - .zip(x.1.into_iter()) + ) + .prop_map(move |deps| { + deps.0 + .into_iter() + .zip(deps.1.into_iter()) .map(|(dep_name, (a, b))| { - let s = vers.get(&dep_name).unwrap_or(&vec![]).clone(); - if s.is_empty() { - return dep_req(&dep_name, "=6.6.6"); - } + let s = vers[&dep_name].clone(); let (a, b) = (a.index(s.len()), b.index(s.len())); let (a, b) = (min(a, b), max(a, b)); dep_req( &dep_name, &if a == b { - format!("={}", s[a]) + format!("={}", s[a].0) } else { - format!(">={}, <={}", s[a], s[b]) + format!(">={}, <={}", s[a].0, s[b].0) }, ) }).collect::>() - }).unwrap_or_else(|| vec![dep_req("bad", "=6.6.6")]) - }).prop_map(move |deps| pkg_dep(name.clone(), deps)) + }).prop_map(move |deps| pkg_dep(name.clone(), deps)) + .boxed() }).collect::>() }) } From 2015191120233268ad79d36cbae0748e6b046788 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Mon, 17 Sep 2018 17:06:28 -0400 Subject: [PATCH 26/31] `edge list` is a much cleaner approach --- tests/testsuite/resolve.rs | 109 +++++++++++++++++++++---------------- 1 file changed, 61 insertions(+), 48 deletions(-) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index e7c9768fdf8..6366ceb7e86 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -14,7 +14,7 @@ use support::registry::Package; use proptest::collection::{btree_map, btree_set, vec}; use proptest::prelude::*; -use proptest::sample::subsequence; +use proptest::sample::Index; use proptest::string::string_regex; use proptest::test_runner::basic_result_cache; @@ -211,7 +211,7 @@ fn loc_names(names: &[(&'static str, &'static str)]) -> Vec { fn ci_no_shrink( s: impl Strategy + 'static, ) -> impl Strategy { - if true || env::var("CI").is_ok() { + if env::var("CI").is_ok() { s.no_shrink().boxed() } else { s.boxed() @@ -251,57 +251,70 @@ fn registry_strategy( vers }); - list_of_crates_with_versions.prop_flat_map(|vers| { - vers.iter() - .flat_map(|(name, vers)| { - vers.iter() - .map(move |x| ((name.as_str(), &x.0).to_pkgid(), x.1)) - }).map(|(name, allow_deps)| { - if !allow_deps { - return Just(pkg_dep(name.clone(), vec![dep_req("bad", "*")])).boxed(); + // each version of each crate can depend on each crate smaller then it + let max_deps = 1 + max_versions * (max_crates * (max_crates - 1)) / 2; + + let raw_version_range = (any::(), any::()); + let raw_dependency = (any::(), any::(), raw_version_range); + + fn order_index(a: Index, b: Index, size: usize) -> (usize, usize) { + let (a, b) = (a.index(size), b.index(size)); + (min(a, b), max(a, b)) + } + + let list_of_raw_dependency = vec(raw_dependency, ..=max_deps); + + (list_of_crates_with_versions, list_of_raw_dependency).prop_map( + |(crate_vers_by_name, raw_dependencies)| { + let list_of_pkgid: Vec<_> = crate_vers_by_name + .iter() + .flat_map(|(name, vers)| vers.iter().map(move |x| ((name.as_str(), &x.0), x.1))) + .collect(); + let len_all_pkgid = list_of_pkgid.len(); + let mut dependency_by_pkgid = vec![vec![]; len_all_pkgid]; + for (a, b, (c, d)) in raw_dependencies { + let (a, b) = order_index(a, b, len_all_pkgid); + let ((dep_name, _), _) = list_of_pkgid[a]; + if (list_of_pkgid[b].0).0 == dep_name { + continue; } - let s: Vec<_> = vers - .keys() - .cloned() - .take_while(|n| name.name().as_str() > n) - .collect::>(); - let s_len = s.len(); - let vers = vers.clone(); - ( - subsequence(s, ..=s_len), - vec( - (any::(), any::()), - s_len, - ), - ) - .prop_map(move |deps| { - deps.0 - .into_iter() - .zip(deps.1.into_iter()) - .map(|(dep_name, (a, b))| { - let s = vers[&dep_name].clone(); - let (a, b) = (a.index(s.len()), b.index(s.len())); - let (a, b) = (min(a, b), max(a, b)); - dep_req( - &dep_name, - &if a == b { - format!("={}", s[a].0) - } else { - format!(">={}, <={}", s[a].0, s[b].0) - }, - ) - }).collect::>() - }).prop_map(move |deps| pkg_dep(name.clone(), deps)) - .boxed() - }).collect::>() - }) + let s = &crate_vers_by_name[dep_name]; + let (c, d) = order_index(c, d, s.len()); + + dependency_by_pkgid[b].push(dep_req( + &dep_name, + &if c == d { + format!("={}", s[c].0) + } else { + format!(">={}, <={}", s[c].0, s[d].0) + }, + )) + } + + list_of_pkgid + .into_iter() + .zip(dependency_by_pkgid.into_iter()) + .map(|(((name, ver), allow_deps), deps)| { + pkg_dep( + (name, ver).to_pkgid(), + if !allow_deps { + vec![dep_req("bad", "*")] + } else { + let mut deps = deps; + deps.sort_by_key(|d| d.name_in_toml()); + deps.dedup_by_key(|d| d.name_in_toml()); + deps + }, + ) + }).collect() + }, + ) } proptest! { #![proptest_config(ProptestConfig { result_cache: basic_result_cache, cases: 256, - max_flat_map_regens: 100, // raise this number for slower and better shrinking .. ProptestConfig::default() })] #[test] @@ -353,7 +366,7 @@ proptest! { prop_assert!( res.is_ok(), "unpublishing {:?} stopped `{} = \"={}\"` from working", - summary_to_unpublish, + summary_to_unpublish.package_id(), this.name(), this.version() ) @@ -383,7 +396,7 @@ proptest! { "full index did not work for `{} = \"={}\"` but unpublishing {:?} fixed it!", this.name(), this.version(), - summary_to_unpublish + summary_to_unpublish.package_id() ) } } From 7ad9f5e964950a25f3df09cc9c841887e37b51ff Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Wed, 19 Sep 2018 14:16:03 -0400 Subject: [PATCH 27/31] add a test to test the tester --- tests/testsuite/resolve.rs | 48 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index 6366ceb7e86..4067569bb80 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -15,7 +15,9 @@ use support::registry::Package; use proptest::collection::{btree_map, btree_set, vec}; use proptest::prelude::*; use proptest::sample::Index; +use proptest::strategy::ValueTree; use proptest::string::string_regex; +use proptest::test_runner::TestRunner; use proptest::test_runner::basic_result_cache; fn resolve( @@ -311,6 +313,52 @@ fn registry_strategy( ) } +/// This test is to test the generator to ensure +/// that it makes registries with large dependency trees +#[test] +fn meta_test_deep_trees_from_strategy() { + let mut seen_an_error = false; + let mut seen_a_deep_tree = false; + + let strategy = ci_no_shrink(registry_strategy(50, 10)); + for _ in 0..256 { + let input = strategy + .new_tree(&mut TestRunner::default()) + .unwrap() + .current(); + let reg = registry(input.clone()); + for this in input.iter().rev().take(10) { + let res = resolve( + &pkg_id("root"), + vec![dep_req(&this.name(), &format!("={}", this.version()))], + ®, + ); + match res { + Ok(r) => { + if r.len() >= 7 { + seen_a_deep_tree = true; + } + } + Err(_) => { + seen_an_error = true; + } + } + if seen_a_deep_tree && seen_an_error { + return; + } + } + } + + assert!( + seen_an_error, + "In 2560 tries we did not see any crates that could not be built!" + ); + assert!( + seen_a_deep_tree, + "In 2560 tries we did not see any crates that had more then 7 pkg in the dependency tree!" + ); +} + proptest! { #![proptest_config(ProptestConfig { result_cache: basic_result_cache, From 6bd2540f53075ecb9d150b42e4285f3d9388a5ac Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Tue, 18 Sep 2018 16:04:53 -0400 Subject: [PATCH 28/31] check each registry more thoroughly --- tests/testsuite/resolve.rs | 89 ++++++++++++++++++++------------------ 1 file changed, 48 insertions(+), 41 deletions(-) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index 4067569bb80..6a9fb5014c6 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -18,7 +18,6 @@ use proptest::sample::Index; use proptest::strategy::ValueTree; use proptest::string::string_regex; use proptest::test_runner::TestRunner; -use proptest::test_runner::basic_result_cache; fn resolve( pkg: &PackageId, @@ -361,14 +360,18 @@ fn meta_test_deep_trees_from_strategy() { proptest! { #![proptest_config(ProptestConfig { - result_cache: basic_result_cache, - cases: 256, + cases: + if env::var("CI").is_ok() { + 256 // we have a lot of builds in CI so one or another of them will find problems + } else { + 1024 // but locally try and find it in the one build + }, .. ProptestConfig::default() })] #[test] fn limited_independence_of_irrelevant_alternatives( input in ci_no_shrink(registry_strategy(50, 10)), - index_to_unpublish in any::() + indexs_to_unpublish in vec(any::(), 10) ) { let reg = registry(input.clone()); // there is only a small chance that eny one @@ -391,7 +394,43 @@ proptest! { .filter(|x| !r.contains(x.package_id())) .collect(); if !not_selected.is_empty() { - let summary_to_unpublish = index_to_unpublish.get(¬_selected); + for index_to_unpublish in &indexs_to_unpublish { + let summary_to_unpublish = index_to_unpublish.get(¬_selected); + let new_reg = registry( + input + .iter() + .cloned() + .filter(|x| summary_to_unpublish != x) + .collect(), + ); + + let res = resolve( + &pkg_id("root"), + vec![dep_req(&this.name(), &format!("={}", this.version()))], + &new_reg, + ); + + // Note: that we can not assert that the two `res` are identical + // as the resolver does depend on irrelevant alternatives. + // It uses how constrained a dependency requirement is + // to determine what order to evaluate requirements. + + prop_assert!( + res.is_ok(), + "unpublishing {:?} stopped `{} = \"={}\"` from working", + summary_to_unpublish.package_id(), + this.name(), + this.version() + ) + } + } + } + + Err(_) => { + // If resolution was unsuccessful, then it should stay unsuccessful + // even if any version of a crate is unpublished. + for index_to_unpublish in &indexs_to_unpublish { + let summary_to_unpublish = index_to_unpublish.get(&input); let new_reg = registry( input .iter() @@ -406,47 +445,15 @@ proptest! { &new_reg, ); - // Note: that we can not assert that the two `res` are identical - // as the resolver does depend on irrelevant alternatives. - // It uses how constrained a dependency requirement is - // to determine what order to evaluate requirements. - prop_assert!( - res.is_ok(), - "unpublishing {:?} stopped `{} = \"={}\"` from working", - summary_to_unpublish.package_id(), + res.is_err(), + "full index did not work for `{} = \"={}\"` but unpublishing {:?} fixed it!", this.name(), - this.version() + this.version(), + summary_to_unpublish.package_id() ) } } - - Err(_) => { - // If resolution was unsuccessful, then it should stay unsuccessful - // even if any version of a crate is unpublished. - let summary_to_unpublish = index_to_unpublish.get(&input); - let new_reg = registry( - input - .iter() - .cloned() - .filter(|x| summary_to_unpublish != x) - .collect(), - ); - - let res = resolve( - &pkg_id("root"), - vec![dep_req(&this.name(), &format!("={}", this.version()))], - &new_reg, - ); - - prop_assert!( - res.is_err(), - "full index did not work for `{} = \"={}\"` but unpublishing {:?} fixed it!", - this.name(), - this.version(), - summary_to_unpublish.package_id() - ) - } } } } From 3e7192ea5d4bb5b99645b59a7d7bbdf77bc64ec3 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Fri, 21 Sep 2018 16:21:38 -0400 Subject: [PATCH 29/31] pretty print the registry made by proptest --- tests/testsuite/resolve.rs | 92 ++++++++++++++++++++++++++++++-------- 1 file changed, 73 insertions(+), 19 deletions(-) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index 5d728193273..1bdf233faea 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -2,6 +2,7 @@ use std::cmp::PartialEq; use std::cmp::{max, min}; use std::collections::{BTreeMap, HashSet}; use std::env; +use std::fmt; use cargo::core::dependency::Kind::{self, Development}; use cargo::core::resolver::{self, Method}; @@ -207,6 +208,57 @@ fn loc_names(names: &[(&'static str, &'static str)]) -> Vec { .collect() } +/// By default `Summary` and `Dependency` have a very verbose `Debug` representation. +/// This replaces with a representation that uses constructors from this file. +/// +/// If `registry_strategy` is improved to modify more fields +/// then this needs to update to display the corresponding constructor. +struct PrettyPrintRegistry(Vec); + +impl fmt::Debug for PrettyPrintRegistry { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "vec![")?; + for s in &self.0 { + if s.dependencies().is_empty() { + write!(f, "pkg!((\"{}\", \"{}\")),", s.name(), s.version())?; + } else { + write!(f, "pkg!((\"{}\", \"{}\") => [", s.name(), s.version())?; + for d in s.dependencies() { + write!( + f, + "dep_req!(\"{}\", \"{}\"),", + d.name_in_toml(), + d.version_req() + )?; + } + write!(f, "]),")?; + } + } + write!(f, "]") + } +} + +#[test] +fn meta_test_deep_pretty_print_registry() { + assert_eq!(&format!("{:?}", PrettyPrintRegistry(vec![ + pkg!(("foo", "1.0.1") => [dep_req("bar", "1")]), + pkg!(("foo", "1.0.0") => [dep_req("bar", "2")]), + pkg!(("bar", "1.0.0") => [dep_req("baz", "=1.0.2"), + dep_req("other", "1")]), + pkg!(("bar", "2.0.0") => [dep_req("baz", "=1.0.1")]), + pkg!(("baz", "1.0.2") => [dep_req("other", "2")]), + pkg!(("baz", "1.0.1")), + pkg!(("dep_req", "1.0.0")), + pkg!(("dep_req", "2.0.0")), + ])), "vec![pkg!((\"foo\", \"1.0.1\") => [dep_req!(\"bar\", \"^1\"),]),\ + pkg!((\"foo\", \"1.0.0\") => [dep_req!(\"bar\", \"^2\"),]),\ + pkg!((\"bar\", \"1.0.0\") => [dep_req!(\"baz\", \"= 1.0.2\"),dep_req!(\"other\", \"^1\"),]),\ + pkg!((\"bar\", \"2.0.0\") => [dep_req!(\"baz\", \"= 1.0.1\"),]),\ + pkg!((\"baz\", \"1.0.2\") => [dep_req!(\"other\", \"^2\"),]),\ + pkg!((\"baz\", \"1.0.1\")),pkg!((\"dep_req\", \"1.0.0\")),\ + pkg!((\"dep_req\", \"2.0.0\")),]") +} + /// This attempts to make sure that CI will fail fast, /// but that local builds will give a small clear test case. fn ci_no_shrink( @@ -225,7 +277,7 @@ fn ci_no_shrink( fn registry_strategy( max_crates: usize, max_versions: usize, -) -> impl Strategy> { +) -> impl Strategy { let name = string_regex("[A-Za-z_-][A-Za-z0-9_-]*(-sys)?").unwrap(); let raw_version = [..max_versions; 3]; @@ -292,22 +344,24 @@ fn registry_strategy( )) } - list_of_pkgid - .into_iter() - .zip(dependency_by_pkgid.into_iter()) - .map(|(((name, ver), allow_deps), deps)| { - pkg_dep( - (name, ver).to_pkgid(), - if !allow_deps { - vec![dep_req("bad", "*")] - } else { - let mut deps = deps; - deps.sort_by_key(|d| d.name_in_toml()); - deps.dedup_by_key(|d| d.name_in_toml()); - deps - }, - ) - }).collect() + PrettyPrintRegistry( + list_of_pkgid + .into_iter() + .zip(dependency_by_pkgid.into_iter()) + .map(|(((name, ver), allow_deps), deps)| { + pkg_dep( + (name, ver).to_pkgid(), + if !allow_deps { + vec![dep_req("bad", "*")] + } else { + let mut deps = deps; + deps.sort_by_key(|d| d.name_in_toml()); + deps.dedup_by_key(|d| d.name_in_toml()); + deps + }, + ) + }).collect(), + ) }, ) } @@ -321,7 +375,7 @@ fn meta_test_deep_trees_from_strategy() { let strategy = ci_no_shrink(registry_strategy(50, 10)); for _ in 0..256 { - let input = strategy + let PrettyPrintRegistry(input) = strategy .new_tree(&mut TestRunner::default()) .unwrap() .current(); @@ -370,7 +424,7 @@ proptest! { })] #[test] fn limited_independence_of_irrelevant_alternatives( - input in ci_no_shrink(registry_strategy(50, 10)), + PrettyPrintRegistry(input) in ci_no_shrink(registry_strategy(50, 10)), indexs_to_unpublish in vec(any::(), 10) ) { let reg = registry(input.clone()); From 6763eded5816ad763f2a774c70fb4bc5f535ded5 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Mon, 24 Sep 2018 12:17:16 -0400 Subject: [PATCH 30/31] proptest 0.8.7 has a better `ci_no_shrink` --- Cargo.toml | 2 +- src/cargo/core/resolver/mod.rs | 2 +- tests/testsuite/resolve.rs | 70 +++++++++++++++++++--------------- 3 files changed, 41 insertions(+), 33 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index adda820915c..556d076f53f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -94,7 +94,7 @@ features = [ [dev-dependencies] bufstream = "0.1" -proptest = "0.8.6" +proptest = "0.8.7" [[bin]] name = "cargo" diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 24ddf57b615..848ba7c01b8 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -250,7 +250,7 @@ fn activate_deps_loop( // If any of them are removed then it takes more than I am willing to measure. // So lets fail the test fast if we have ben running for two long. if cfg!(debug_assertions) && (ticks % 1000 == 0) { - assert!(start.elapsed() - deps_time < Duration::from_secs(300)); + assert!(start.elapsed() - deps_time < Duration::from_secs(90)); } let just_here_for_the_error_messages = deps_frame.just_for_error_messages; diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index 1bdf233faea..b05b91dacc2 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -3,6 +3,7 @@ use std::cmp::{max, min}; use std::collections::{BTreeMap, HashSet}; use std::env; use std::fmt; +use std::time::{Duration, Instant}; use cargo::core::dependency::Kind::{self, Development}; use cargo::core::resolver::{self, Method}; @@ -59,6 +60,7 @@ fn resolve_with_config( false, ).unwrap(); let method = Method::Everything; + let start = Instant::now(); let resolve = resolver::resolve( &[(summary, method)], &[], @@ -67,6 +69,10 @@ fn resolve_with_config( config, false, )?; + + // The largest test in our sweet takes less then 30 sec. + // So lets fail the test if we have ben running for two long. + assert!(start.elapsed() < Duration::from_secs(60)); let res = resolve.iter().cloned().collect(); Ok(res) } @@ -226,7 +232,7 @@ impl fmt::Debug for PrettyPrintRegistry { for d in s.dependencies() { write!( f, - "dep_req!(\"{}\", \"{}\"),", + "dep_req(\"{}\", \"{}\"),", d.name_in_toml(), d.version_req() )?; @@ -240,35 +246,29 @@ impl fmt::Debug for PrettyPrintRegistry { #[test] fn meta_test_deep_pretty_print_registry() { - assert_eq!(&format!("{:?}", PrettyPrintRegistry(vec![ - pkg!(("foo", "1.0.1") => [dep_req("bar", "1")]), - pkg!(("foo", "1.0.0") => [dep_req("bar", "2")]), - pkg!(("bar", "1.0.0") => [dep_req("baz", "=1.0.2"), + assert_eq!( + &format!( + "{:?}", + PrettyPrintRegistry(vec![ + pkg!(("foo", "1.0.1") => [dep_req("bar", "1")]), + pkg!(("foo", "1.0.0") => [dep_req("bar", "2")]), + pkg!(("bar", "1.0.0") => [dep_req("baz", "=1.0.2"), dep_req("other", "1")]), - pkg!(("bar", "2.0.0") => [dep_req("baz", "=1.0.1")]), - pkg!(("baz", "1.0.2") => [dep_req("other", "2")]), - pkg!(("baz", "1.0.1")), - pkg!(("dep_req", "1.0.0")), - pkg!(("dep_req", "2.0.0")), - ])), "vec![pkg!((\"foo\", \"1.0.1\") => [dep_req!(\"bar\", \"^1\"),]),\ - pkg!((\"foo\", \"1.0.0\") => [dep_req!(\"bar\", \"^2\"),]),\ - pkg!((\"bar\", \"1.0.0\") => [dep_req!(\"baz\", \"= 1.0.2\"),dep_req!(\"other\", \"^1\"),]),\ - pkg!((\"bar\", \"2.0.0\") => [dep_req!(\"baz\", \"= 1.0.1\"),]),\ - pkg!((\"baz\", \"1.0.2\") => [dep_req!(\"other\", \"^2\"),]),\ - pkg!((\"baz\", \"1.0.1\")),pkg!((\"dep_req\", \"1.0.0\")),\ - pkg!((\"dep_req\", \"2.0.0\")),]") -} - -/// This attempts to make sure that CI will fail fast, -/// but that local builds will give a small clear test case. -fn ci_no_shrink( - s: impl Strategy + 'static, -) -> impl Strategy { - if env::var("CI").is_ok() { - s.no_shrink().boxed() - } else { - s.boxed() - } + pkg!(("bar", "2.0.0") => [dep_req("baz", "=1.0.1")]), + pkg!(("baz", "1.0.2") => [dep_req("other", "2")]), + pkg!(("baz", "1.0.1")), + pkg!(("dep_req", "1.0.0")), + pkg!(("dep_req", "2.0.0")), + ]) + ), + "vec![pkg!((\"foo\", \"1.0.1\") => [dep_req(\"bar\", \"^1\"),]),\ + pkg!((\"foo\", \"1.0.0\") => [dep_req(\"bar\", \"^2\"),]),\ + pkg!((\"bar\", \"1.0.0\") => [dep_req(\"baz\", \"= 1.0.2\"),dep_req(\"other\", \"^1\"),]),\ + pkg!((\"bar\", \"2.0.0\") => [dep_req(\"baz\", \"= 1.0.1\"),]),\ + pkg!((\"baz\", \"1.0.2\") => [dep_req(\"other\", \"^2\"),]),\ + pkg!((\"baz\", \"1.0.1\")),pkg!((\"dep_req\", \"1.0.0\")),\ + pkg!((\"dep_req\", \"2.0.0\")),]" + ) } /// This generates a random registry index. @@ -373,7 +373,7 @@ fn meta_test_deep_trees_from_strategy() { let mut seen_an_error = false; let mut seen_a_deep_tree = false; - let strategy = ci_no_shrink(registry_strategy(50, 10)); + let strategy = registry_strategy(50, 10); for _ in 0..256 { let PrettyPrintRegistry(input) = strategy .new_tree(&mut TestRunner::default()) @@ -420,11 +420,19 @@ proptest! { } else { 1024 // but locally try and find it in the one build }, + max_shrink_iters: + if env::var("CI").is_ok() { + // This attempts to make sure that CI will fail fast, + 0 + } else { + // but that local builds will give a small clear test case. + ProptestConfig::default().max_shrink_iters + }, .. ProptestConfig::default() })] #[test] fn limited_independence_of_irrelevant_alternatives( - PrettyPrintRegistry(input) in ci_no_shrink(registry_strategy(50, 10)), + PrettyPrintRegistry(input) in registry_strategy(50, 10), indexs_to_unpublish in vec(any::(), 10) ) { let reg = registry(input.clone()); From a4da525b92877fb7405042bc75b74723d83a51a6 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Mon, 24 Sep 2018 18:00:56 -0400 Subject: [PATCH 31/31] In theory shrinkage should be 2, but in practice we get better trees with a larger value. --- tests/testsuite/resolve.rs | 37 +++++++++++++------------------------ 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index b05b91dacc2..ba050f094f0 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -277,6 +277,7 @@ fn meta_test_deep_pretty_print_registry() { fn registry_strategy( max_crates: usize, max_versions: usize, + shrinkage: usize, ) -> impl Strategy { let name = string_regex("[A-Za-z_-][A-Za-z0-9_-]*(-sys)?").unwrap(); @@ -285,7 +286,7 @@ fn registry_strategy( // If this is false than the crate will depend on the nonexistent "bad" // instead of the complex set we generated for it. - let allow_deps = prop::bool::weighted(0.95); + let allow_deps = prop::bool::weighted(0.99); let list_of_versions = btree_set((raw_version, allow_deps), 1..=max_versions).prop_map(move |ver| { @@ -304,8 +305,9 @@ fn registry_strategy( vers }); - // each version of each crate can depend on each crate smaller then it - let max_deps = 1 + max_versions * (max_crates * (max_crates - 1)) / 2; + // each version of each crate can depend on each crate smaller then it. + // In theory shrinkage should be 2, but in practice we get better trees with a larger value. + let max_deps = max_versions * (max_crates * (max_crates - 1)) / shrinkage; let raw_version_range = (any::(), any::()); let raw_dependency = (any::(), any::(), raw_version_range); @@ -370,10 +372,9 @@ fn registry_strategy( /// that it makes registries with large dependency trees #[test] fn meta_test_deep_trees_from_strategy() { - let mut seen_an_error = false; - let mut seen_a_deep_tree = false; + let mut dis = [0; 21]; - let strategy = registry_strategy(50, 10); + let strategy = registry_strategy(50, 10, 50); for _ in 0..256 { let PrettyPrintRegistry(input) = strategy .new_tree(&mut TestRunner::default()) @@ -386,29 +387,17 @@ fn meta_test_deep_trees_from_strategy() { vec![dep_req(&this.name(), &format!("={}", this.version()))], ®, ); - match res { - Ok(r) => { - if r.len() >= 7 { - seen_a_deep_tree = true; - } - } - Err(_) => { - seen_an_error = true; - } - } - if seen_a_deep_tree && seen_an_error { + dis[res.as_ref().map(|x| min(x.len(), dis.len()) - 1).unwrap_or(0)] += 1; + if dis.iter().all(|&x| x > 0) { return; } } } assert!( - seen_an_error, - "In 2560 tries we did not see any crates that could not be built!" - ); - assert!( - seen_a_deep_tree, - "In 2560 tries we did not see any crates that had more then 7 pkg in the dependency tree!" + dis.iter().all(|&x| x > 0), + "In 2560 tries we did not see a wide enough distribution of dependency trees! dis:{:?}", + dis ); } @@ -432,7 +421,7 @@ proptest! { })] #[test] fn limited_independence_of_irrelevant_alternatives( - PrettyPrintRegistry(input) in registry_strategy(50, 10), + PrettyPrintRegistry(input) in registry_strategy(50, 10, 50), indexs_to_unpublish in vec(any::(), 10) ) { let reg = registry(input.clone());