-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
use proptest to fuzz the resolver #5921
Changes from 4 commits
56a222c
1761886
ce1772c
4e619d8
732aa10
c26553a
b7285e8
5339e92
f270dda
17fe190
85b1976
0ef43cb
0206cec
1f98871
b0bbb6a
2628ec0
509806e
d6258e9
1eaf543
2b1c39f
05c6ce0
856964f
2b16156
c497851
1b15b15
d8e19fb
2015191
7ad9f5e
6bd2540
40d9de4
3e7192e
6763ede
a4da525
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
use std::collections::{BTreeMap, HashSet}; | ||
use std::collections::{BTreeMap, BTreeSet, HashSet}; | ||
|
||
use support::hamcrest::{assert_that, contains, is_not}; | ||
|
||
|
@@ -12,6 +12,10 @@ use support::ChannelChanger; | |
use support::{execs, project}; | ||
use support::registry::Package; | ||
|
||
use proptest::collection::{btree_map, btree_set, vec}; | ||
use proptest::prelude::*; | ||
use proptest::sample::subsequence; | ||
|
||
fn resolve( | ||
pkg: &PackageId, | ||
deps: Vec<Dependency>, | ||
|
@@ -56,10 +60,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() | ||
} | ||
} | ||
|
@@ -111,8 +118,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 { | ||
|
@@ -155,8 +161,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() | ||
} | ||
|
||
|
@@ -185,6 +190,97 @@ fn loc_names(names: &[(&'static str, &'static str)]) -> Vec<PackageId> { | |
.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<Value=Vec<Summary>> { | ||
const VALID_NAME_STRATEGY: &str = "[A-Za-z_-][A-Za-z0-9_-]*"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since it sounds like runtime performance is a problem right now, I'd suggest precompiling the regex so it doesn't get reparsed every time. |
||
const MAX_CRATES: usize = 10; | ||
const MAX_VERSIONS: usize = 10; | ||
|
||
fn range(max: usize) -> impl Strategy<Value = (usize, usize)> { | ||
(0..max).prop_flat_map(move |low| (Just(low), low..=max)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe you could rewrite this as (0..max, 0..max).prop_map(|(a, b)| min(a, b)..=max(a,b)) which could substantially improve shrinking performance |
||
} | ||
|
||
fn ver(max: usize) -> impl Strategy<Value=(usize, usize, usize)> { | ||
(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::<Vec<String>>(), | ||
b.iter() | ||
.flat_map(|(name, vers)| vers.into_iter().map(move |v| (name.clone(), v.clone()))) | ||
.collect::<Vec<_>>(), | ||
) | ||
}).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), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's |
||
) | ||
}).prop_flat_map(|(data, deps)| { | ||
let len = deps.iter().map(|x| x.len()).sum(); | ||
( | ||
Just(data), | ||
Just(deps), | ||
vec( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you could lift this out to a top-level strategy by just generating a vector of size |
||
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<Dependency> = 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::<String, Vec<String>>::new(), link, false).unwrap() | ||
}) | ||
.collect() | ||
}) | ||
} | ||
|
||
proptest! { | ||
#[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 { | ||
let _res = resolve( | ||
&pkg_id("root"), | ||
vec![dep_req(&this.name(), &format!("={}", this.version()))], | ||
®, | ||
); | ||
} | ||
} | ||
} | ||
|
||
#[test] | ||
#[should_panic(expected = "assertion failed: !name.is_empty()")] | ||
fn test_dependency_with_empty_name() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this particular issue should be fixed in proptest 0.8.6