use proptest to fuzz the resolver #5921

Eh2406 · 2018-08-21T18:54:22Z

This has been a long time goal. This uses proptest to generate random registry indexes and throws them at the resolver.

It would be simple to generate a registry by,

make a list of name and version number each picked at random
for each pick a list of dependencies by making a list of name and version requirements at random.

Unfortunately, it would be extremely unlikely to generate any interesting cases, as the chance that the random name you depend on was also generated as the name of a crate is vanishingly small. So this implementation works very hard to ensure that it only generates valid dependency requirements.

This is still a WIP as it has many problems:

The current strategy is very convoluted. It is hard to see that it is correct, and harder to see how it can be expanded. Thanks to @Centril for working with me on IRC to get this far. Do you have advice for improving it?
It is slow as molasses when run without release. I looked with a profilere and we seem to spend 2/3 of the time in to_url. Maybe we can special case example.com for test, like we do for crates.io or something? Edit: Done. lazy_static did its magic.
proptest does not yet work with minimal-versions, a taste of my own medicine.
I have not verified that, if I remove the fixes for other test that this regenerates them.

The current strategy does not:

generate interesting version numbers, it just dose 1.0.0, 2.0.0 ...
guarantee that the version requirements are possible to meet by the crate named.
generate features.
generate dev-dependencies.
build deep dependency trees, it seems to prefer to generate crates with 0 or 1 dependents so that on average the tree is 1 or 2 layers deep.

And last but not least, there are no interesting properties being tested. Like:

If resolution was successful, then all the transitive requirements are met.
If resolution was successful, then unpublishing a version of a crate that was not selected should not change that.
If resolution was unsuccessful, then it should stay unsuccessful even if any version of a crate is unpublished.
@maurer suggested testing for consistency. Same registry, same cargo version, same lockfile, every time.
@maurer suggested a pareto optimality property (if all else stays the same, but new package versions are released, we don't get a new lockfile where every version is <= the old one, and at least one is < the old one)

rust-highfive · 2018-08-21T18:54:26Z

r? @matklad

(rust_highfive has picked a reviewer for you, use r? to override)

Centril · 2018-08-22T10:33:53Z

cc @AltSysrq wrt. advice.

alexcrichton · 2018-08-22T17:06:10Z

Sounds like a great idea to me! I don't have many thoughts in terms of how best to approach this or how to solve the open issues, but when you're comfortable merging I'm game for that :)

Eh2406 · 2018-08-22T17:31:17Z

When the core functionality is working, I may decide to merge before all the things are checked off transferring the remainder to new issues. The code is far to inscrutable for it to be worth doing now, but before merge it needs comments so that other programers can understand what it is doing. I will need help finding the confusing parts, and making me explain them.

AltSysrq

Added a couple suggestions as requested.

In general, I recommend trying to avoid prop_flat_map when reasonably possible since it does cause shrinking to be a lot slower and less likely to find a minimal case.

Readability-wise, my only suggestion would be to split the big chain of combinators into smaller functions so that the strategy parts have names. It also makes it easier to reuse parts later, e.g. should you find a need to just generate a list of crate names in isolation.

AltSysrq · 2018-08-24T01:33:18Z

tests/testsuite/resolve.rs

+    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))


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

AltSysrq · 2018-08-24T01:34:56Z

tests/testsuite/resolve.rs

+        let data_len = data.len();
+        (
+            Just(data),
+            vec(subsequence(names, 0..names_len), data_len..=data_len),


There's impl From<usize> for SizeRange so you should be able to just write data_len by itself instead of data_len..=data_len.

AltSysrq · 2018-08-24T01:40:38Z

tests/testsuite/resolve.rs

+        (
+            Just(data),
+            Just(deps),
+            vec(


I think you could lift this out to a top-level strategy by just generating a vector of size MAX_CRATES*MAX_CRATES since the code below only looks at the elements it needs. That would remove another layer of flat mapping.

AltSysrq · 2018-08-24T01:43:59Z

tests/testsuite/resolve.rs

+/// 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_-]*";


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.

Eh2406 · 2018-08-24T03:20:16Z

@AltSysrq Thanks for the suggestions! Locally, that seems to have made a big difference. Thanks for the pointer on prop_flat_map, I had somehow got in my head that it was the major form of composition. I will endeavor to dial its use back. :-)

The current performance on my computer is almost eceptibal, at least for the trivial tests I have been working with so far. If the test passes, like the one committed, then even in debug it runs in just a few seconds. If I write a test that fails (like, prop_assert!(res.len() <= 4)) then it can shrink in ~10 min with release, after @AltSysrq suggestions.

Eh2406 · 2018-08-25T21:17:43Z

I was working on generating only version requirements that are possible to meet by the named crate. I was having difficulties with the limitations of subsequence, and it is adding another layer of prop_flat_map.

When I thought of another Implementation, but I don't know if it will be shrinking frently. Every time the proposed strategy uses subsequence to ensure it gets a valid entry, (a dependency name that is in the index and smaller the the crate, and a version range end that are actually versions in the index) the new implementation would just generate a u16::any() then in the last prop_map I can use that u16 as an index into the vec of the corresponding thing. Like names[index as usize % names.len()].

@AltSysrq, @Centril what do you think of the heavy use of % to remove the use of prop_flat_map?

AltSysrq · 2018-08-27T00:55:31Z

Cargo.toml

@@ -93,6 +93,8 @@ features = [

 [dev-dependencies]
 bufstream = "0.1"
+proptest = "0.8.4"
+wait-timeout = "0.1.4" # required only for minimal-versions. brought in by proptest.


I believe this particular issue should be fixed in proptest 0.8.6

AltSysrq · 2018-08-27T00:58:02Z

generate a u16::any() then in the last prop_map I can use that u16 as an index into the vec

I think the Index type added in proptest 0.8.6 would be usable for this purpose.

Eh2406 · 2018-08-27T01:12:24Z

@AltSysrq I was just reading through the changes in 0.8.6 one commit at a time. Each commit was more perfectly what I needed then the last! I nearly fell out of my seat with excitement for several of them! Thank you! Thank you! Thank you!

…ncy names

…meet by the crate named.

Eh2406 · 2018-09-21T23:10:09Z

What is going on with appveyor!? That one timed out on cargo check!?

Eh2406 · 2018-09-22T19:01:19Z

@bors: retry

bors · 2018-09-22T19:01:28Z

⌛ Testing commit 3e7192e with merge 702df0e...

@Centril

use proptest to fuzz the resolver This has been a long time goal. This uses proptest to generate random registry indexes and throws them at the resolver. It would be simple to generate a registry by, 1. make a list of name and version number each picked at random 2. for each pick a list of dependencies by making a list of name and version requirements at random. Unfortunately, it would be extremely unlikely to generate any interesting cases, as the chance that the random name you depend on was also generated as the name of a crate is vanishingly small. So this implementation works very hard to ensure that it only generates valid dependency requirements. This is still a WIP as it has many problems: - [x] The current strategy is very convoluted. It is hard to see that it is correct, and harder to see how it can be expanded. Thanks to @Centril for working with me on IRC to get this far. Do you have advice for improving it? - [X] It is slow as molasses when run without release. I looked with a profilere and we seem to spend 2/3 of the time in `to_url`. Maybe we can special case `example.com` for test, like we do for `crates.io` or something? Edit: Done. `lazy_static` did its magic. - [x] `proptest` does not yet work with `minimal-versions`, a taste of my own medicine. - [x] I have not verified that, if I remove the fixes for other test that this regenerates them. The current strategy does not: - [x] generate interesting version numbers, it just dose 1.0.0, 2.0.0 ... - [x] guarantee that the version requirements are possible to meet by the crate named. - [ ] generate features. - [ ] generate dev-dependencies. - [x] build deep dependency trees, it seems to prefer to generate crates with 0 or 1 dependents so that on average the tree is 1 or 2 layers deep. And last but not least, there are no interesting properties being tested. Like: - [ ] If resolution was successful, then all the transitive requirements are met. - [x] If resolution was successful, then unpublishing a version of a crate that was not selected should not change that. - [x] If resolution was unsuccessful, then it should stay unsuccessful even if any version of a crate is unpublished. - [ ] @maurer suggested testing for consistency. Same registry, same cargo version, same lockfile, every time. - [ ] @maurer suggested a pareto optimality property (if all else stays the same, but new package versions are released, we don't get a new lockfile where every version is <= the old one, and at least one is < the old one)

bors · 2018-09-22T19:42:19Z

💔 Test failed - status-travis

matthiaskrgr · 2018-09-22T19:43:03Z

test resolve::limited_independence_of_irrelevant_alternatives ... test resolve::limited_independence_of_irrelevant_alternatives has been running for over 60 seconds
[....]


No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

The build has been terminated

maybe the test is just too slow in debug mode?

Eh2406 · 2018-09-22T20:09:55Z

It is randomize testing of NP complete code, so it is possible that some random portion of the time it finds an example the take so long to complete that we don't see the error message.

Locally I have had this test running repeatedly in a loop for a couple of days trying to find an example that might be causing this. So far no luck.

I know proptest has some fancy features specifically for dealing with this issue, when I get a chance I will investigate using them.

matthiaskrgr · 2018-09-22T20:37:33Z

The limited_independence_of_irrelevant_alternatives is very slow in debug mode.
It took 16 minutes to complete on my system which I deem to be faster than the average travis virtual env.
So if it is launched somewhere at the end, travis will autokill the job because no output has been supplied for 15 minutes.

Eh2406 · 2018-09-22T21:12:53Z

Thanks for the sample! It is randomized so it may take different amounts of time on different runs. Also it is setup to run a smaller number of cases on CI then locally.

Possible solutions include:

Have CI run fewer cases.
Have each case loop fewer times hear and hear
Make the test look at simpler input.
Find ways for the test to run more efficiently.
See if with the new debug the result_cache is worthwhile.
Find the slow casses and mark them as bugs, possibly with the timeout.
Split the test up into more test that each run faster.

I am experimenting.

…with a larger value.

Eh2406 · 2018-09-24T22:23:09Z

So it turn out that the distribution created by the edge list was heavily skewed to unresolvable registries. Srinking the number of edges made everything work faster and better.

Eh2406 · 2018-09-24T22:58:43Z

@bors: r=alexcrichton

Eh2406 · 2018-09-25T00:59:31Z

[35] SSL connect error (schannel: next InitializeSecurityContext failed: Unknown error (0x80092013) - The revocation function was unable to check revocation because the revocation server was offline.)

@bors: retry

bors · 2018-09-25T00:59:39Z

⌛ Testing commit a4da525 with merge 4e09634...

@Centril

use proptest to fuzz the resolver This has been a long time goal. This uses proptest to generate random registry indexes and throws them at the resolver. It would be simple to generate a registry by, 1. make a list of name and version number each picked at random 2. for each pick a list of dependencies by making a list of name and version requirements at random. Unfortunately, it would be extremely unlikely to generate any interesting cases, as the chance that the random name you depend on was also generated as the name of a crate is vanishingly small. So this implementation works very hard to ensure that it only generates valid dependency requirements. This is still a WIP as it has many problems: - [x] The current strategy is very convoluted. It is hard to see that it is correct, and harder to see how it can be expanded. Thanks to @Centril for working with me on IRC to get this far. Do you have advice for improving it? - [X] It is slow as molasses when run without release. I looked with a profilere and we seem to spend 2/3 of the time in `to_url`. Maybe we can special case `example.com` for test, like we do for `crates.io` or something? Edit: Done. `lazy_static` did its magic. - [x] `proptest` does not yet work with `minimal-versions`, a taste of my own medicine. - [x] I have not verified that, if I remove the fixes for other test that this regenerates them. The current strategy does not: - [x] generate interesting version numbers, it just dose 1.0.0, 2.0.0 ... - [x] guarantee that the version requirements are possible to meet by the crate named. - [ ] generate features. - [ ] generate dev-dependencies. - [x] build deep dependency trees, it seems to prefer to generate crates with 0 or 1 dependents so that on average the tree is 1 or 2 layers deep. And last but not least, there are no interesting properties being tested. Like: - [ ] If resolution was successful, then all the transitive requirements are met. - [x] If resolution was successful, then unpublishing a version of a crate that was not selected should not change that. - [x] If resolution was unsuccessful, then it should stay unsuccessful even if any version of a crate is unpublished. - [ ] @maurer suggested testing for consistency. Same registry, same cargo version, same lockfile, every time. - [ ] @maurer suggested a pareto optimality property (if all else stays the same, but new package versions are released, we don't get a new lockfile where every version is <= the old one, and at least one is < the old one)

bors · 2018-09-25T01:41:01Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 4e09634 to master...

killercup · 2018-09-26T19:01:47Z

src/cargo/core/resolver/mod.rs

@@ -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


s/sweet/sweet test suite

(same below)

Proptest/Resolver move test helper functions to support This moves all the code in `tests/testsuite/resolve.rs` that is not tests of cargo to ` tests/testsuite/support/resolve.rs`. (follow up to #5921) This also clears up some inconsistencies in naming between local variables in `activate_deps_loop` and `BacktrackFrame`. (follow up to #6097) This is a true refactoring, nothing about the executable has changed.

a start on using proptest to fuzz the resolver

56a222c

rust-highfive assigned matklad Aug 21, 2018

Eh2406 added 2 commits August 21, 2018 16:21

cache the example url to solve performance problem

1761886

get working with minimal-versions

ce1772c

better generation of version numbers

4e619d8

Eh2406 force-pushed the proptest branch from b7132cb to 4e619d8 Compare August 22, 2018 16:09

Eh2406 added 2 commits August 22, 2018 15:19

small clean up

732aa10

small clean up

c26553a

Eh2406 mentioned this pull request Aug 23, 2018

regex generation tests same case multiple times proptest-rs/proptest#70

Closed

Eh2406 added 2 commits August 23, 2018 13:36

handle "bad" slightly better

b7285e8

small clean up for the cache of the example url

5339e92

Eh2406 mentioned this pull request Aug 23, 2018

impl Strategy for Vec<impl Strategy> proptest-rs/proptest#85

Closed

AltSysrq reviewed Aug 24, 2018

View reviewed changes

incorporate @AltSysrq suggestions

f270dda

AltSysrq reviewed Aug 27, 2018

View reviewed changes

Eh2406 added 7 commits August 26, 2018 22:11

update to the new version of proptest

17fe190

use the new impl Strategy for Vec<S> to only generate valid depende…

85b1976

…ncy names

use the new result_cache

0ef43cb

double down on prop_flat_map to guarantee version requirements are …

0206cec

…meet by the crate named.

stronger assert in the core

1f98871

use args for scale

b0bbb6a

works a LOT better if the simple cases are at the beginning

2628ec0

Eh2406 mentioned this pull request Sep 23, 2018

timeout option giving up? proptest-rs/proptest#95

Closed

Eh2406 added 2 commits September 24, 2018 12:17

proptest 0.8.7 has a better ci_no_shrink

6763ede

In theory shrinkage should be 2, but in practice we get better trees …

a4da525

…with a larger value.

This comment has been minimized.

Sign in to view

bors merged commit a4da525 into rust-lang:master Sep 25, 2018

killercup reviewed Sep 26, 2018

View reviewed changes

Eh2406 mentioned this pull request Sep 26, 2018

Proptest/Resolver move test helper functions to support #6103

Merged

This was referenced Oct 2, 2018

improve the proptest of the resolver. #6120

Open

Abort crate resolution if too many candidates have been tried #4066

Open

ehuss added this to the 1.31.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use proptest to fuzz the resolver #5921

use proptest to fuzz the resolver #5921

Eh2406 commented Aug 21, 2018 •

edited

Loading

rust-highfive commented Aug 21, 2018

Centril commented Aug 22, 2018

alexcrichton commented Aug 22, 2018

Eh2406 commented Aug 22, 2018

AltSysrq left a comment

AltSysrq Aug 24, 2018

AltSysrq Aug 24, 2018

AltSysrq Aug 24, 2018

AltSysrq Aug 24, 2018

Eh2406 commented Aug 24, 2018 •

edited

Loading

Eh2406 commented Aug 25, 2018

AltSysrq Aug 27, 2018

AltSysrq commented Aug 27, 2018

Eh2406 commented Aug 27, 2018

Eh2406 commented Sep 21, 2018

Eh2406 commented Sep 22, 2018

bors commented Sep 22, 2018

bors commented Sep 22, 2018

matthiaskrgr commented Sep 22, 2018

Eh2406 commented Sep 22, 2018

matthiaskrgr commented Sep 22, 2018

Eh2406 commented Sep 22, 2018 •

edited

Loading

Eh2406 commented Sep 24, 2018

Eh2406 commented Sep 24, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Eh2406 commented Sep 25, 2018

bors commented Sep 25, 2018

bors commented Sep 25, 2018

killercup Sep 26, 2018 •

edited

Loading

use proptest to fuzz the resolver #5921

use proptest to fuzz the resolver #5921

Conversation

Eh2406 commented Aug 21, 2018 • edited Loading

rust-highfive commented Aug 21, 2018

Centril commented Aug 22, 2018

alexcrichton commented Aug 22, 2018

Eh2406 commented Aug 22, 2018

AltSysrq left a comment

Choose a reason for hiding this comment

AltSysrq Aug 24, 2018

Choose a reason for hiding this comment

AltSysrq Aug 24, 2018

Choose a reason for hiding this comment

AltSysrq Aug 24, 2018

Choose a reason for hiding this comment

AltSysrq Aug 24, 2018

Choose a reason for hiding this comment

Eh2406 commented Aug 24, 2018 • edited Loading

Eh2406 commented Aug 25, 2018

AltSysrq Aug 27, 2018

Choose a reason for hiding this comment

AltSysrq commented Aug 27, 2018

Eh2406 commented Aug 27, 2018

Eh2406 commented Sep 21, 2018

Eh2406 commented Sep 22, 2018

bors commented Sep 22, 2018

bors commented Sep 22, 2018

matthiaskrgr commented Sep 22, 2018

Eh2406 commented Sep 22, 2018

matthiaskrgr commented Sep 22, 2018

Eh2406 commented Sep 22, 2018 • edited Loading

Eh2406 commented Sep 24, 2018

Eh2406 commented Sep 24, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Eh2406 commented Sep 25, 2018

bors commented Sep 25, 2018

bors commented Sep 25, 2018

killercup Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

Eh2406 commented Aug 21, 2018 •

edited

Loading

Eh2406 commented Aug 24, 2018 •

edited

Loading

Eh2406 commented Sep 22, 2018 •

edited

Loading

killercup Sep 26, 2018 •

edited

Loading