Extract resolver tests to their own crate #7011
Conversation
|
r? @nrc (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
I have 2 open resolver related PRs and hopefully an additional one on the way, all of witch will need to be merged. If this becomes urgent we should land it! If we have some time I'd like to think some more. The resolver test have different dependencies, tooling, and structure. I like that this gives us a place to make that cleare. This also makes it much lower cost to make them even more different. Like adding benchmarks. I have some ergonomics concerns. How easy is it to get Jetbrains/RLS to run the tests in there new location? Does that involve an additional copy of More generally I wonder if this goes far enough. I have for a while thought that we have too many resolver tests. I think a lot of the property based tests are approximations of the new SAT bass test. When we have some experience with the new test, I would strongly consider just removing the other ones. All the example bass test can be made to be of the same form: does some dependencies resolved against a specified index. We could store the outside indexes in a folder of Json or something, and let the SAT framework tell us whether we got the right answer. We could go a step further and just have it output the lock file or error message, and snapshot it. These changes would make it even more true that's a resolver has a different test infrastructure and strategy then everything else. The thing that would make running the resolver tests much more pleasant is making the resolver a separate crate that does not depend on cargo. The only way I see making that happen is making a trate for each type that cargo gives to the resolver. Then making |
|
☔ The latest upstream changes (presumably #7019) made this pull request unmergeable. Please resolve the merge conflicts. |
|
This isn't particularly pressing, no, I was largely just interested in getting updates to rust-lang/rust working again which @ehuss mentioned is blocked on For your questions:
Without being a user of these extensions, I'm not sure!
Currently, yes, because the integration in rust-lang/rust makes using
Presumably yes because there's less code to compile!
Probably not since this is just a few test being extracted.
I believe they should as usual, yes.
Agreed! You're right in that this'll be super tricky, but it may also be possible (and a good idea) to separate out the core types of Cargo like That's certainly a larger undertaking though :) |
| @@ -182,7 +178,7 @@ pub fn resolve_with_config_raw( | |||
|
|
|||
| // The largest test in our suite takes less then 30 sec. | |||
| // So lets fail the test if we have ben running for two long. | |||
| assert!(start.elapsed() < slow_cpu_multiplier(60)); | |||
| assert!(start.elapsed().as_secs() < 60); | |||
There was a problem hiding this comment.
Why remove slow_cpu_multiplier? Do we think that the other people (#6491) running Cargos CI will not run these tests?
There was a problem hiding this comment.
This was just a big chunk of code to migrate that didn't really seem worth it. The workaround can always be re-added if still necessary.
|
To be clear, with an answer to my nit question and a rebase I think this should be accepted at the earliest convenience. If my PRs ever get in good enough shape to be worth merging, I will happily rebase. |
|
Oh sorry I just ran out of time to rebase this and get it through, I will do so now. |
That way when we add more crates we've got a place to put them!
These tests take a good amount of time to run locally and they're also causing a lot of dependencies to get pulled into rust-lang/rust, so let's have a separate crate that we just test on our own CI
alexcrichton
force-pushed
the
resolver-extract
branch
from
4cfd2ad
to
290a727
Compare
|
@bors: r=Eh2406 |
|
📌 Commit 290a727 has been approved by |
bors
added
S-waiting-on-bors
Extract resolver tests to their own crate These tests take a good amount of time to run locally and they're also causing a lot of dependencies to get pulled into rust-lang/rust, so let's have a separate crate that we just test on our own CI
|
☀️ Test successful - checks-travis, status-appveyor |
It has been clear that you have been very busy for an extended period of time, working on many divers projects each important to keeping this Rust experiment going. |
Resolver test/debug cleanup This is several small things salvaged from abandoned PRs and implemented on top of #7011 In working on this I noted that the prop tests are very sensitive to whether backtrace are turned on. Maybe we should set that env to 0 for that builder?
Update cargo 17 commits in 807429e1b6da4e2ec52488ef2f59e77068c31e1f..4c1fa54d10f58d69ac9ff55be68e1b1c25ecb816 2019-06-11 14:06:10 +0000 to 2019-06-24 11:24:18 +0000 - Fix typo in comment (rust-lang/cargo#7066) - travis: enforce formatting of subcrates as well (rust-lang/cargo#7063) - _cargo: Make function style consistent (rust-lang/cargo#7060) - Update some fix comments. (rust-lang/cargo#7061) - Stabilize default-run (rust-lang/cargo#7056) - Fix typo in comment (rust-lang/cargo#7054) - fix(fingerpring): do not touch intermediate artifacts (rust-lang/cargo#7050) - Resolver test/debug cleanup (rust-lang/cargo#7045) - Rename to_url → into_url (rust-lang/cargo#7048) - Remove needless lifetimes (rust-lang/cargo#7047) - Revert test directory cleaning change. (rust-lang/cargo#7042) - cargo book /reference/manifest: fix typo (rust-lang/cargo#7041) - Extract resolver tests to their own crate (rust-lang/cargo#7011) - ci: Do not install addons on rustfmt build jobs (rust-lang/cargo#7038) - Support absolute paths in dep-info files (rust-lang/cargo#7030) - ci: Run cargo fmt on all workspaces (rust-lang/cargo#7033) - Deprecated ONCE_INIT in favor of Once::new() (rust-lang/cargo#7031)
These tests take a good amount of time to run locally and they're also
causing a lot of dependencies to get pulled into rust-lang/rust, so
let's have a separate crate that we just test on our own CI