Skip to content
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

Add `--all` flag to `cargo test` #3221

Merged
merged 11 commits into from Dec 9, 2016
Merged

Add `--all` flag to `cargo test` #3221

merged 11 commits into from Dec 9, 2016

Conversation

@euclio
Copy link
Contributor

@euclio euclio commented Oct 21, 2016

Work towards #2878.

This flag allows a user to test all members of a workspace by using cargo test --all. The command is also supported when using a virtual workspace manifest.

@rust-highfive
Copy link

@rust-highfive rust-highfive commented Oct 21, 2016

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Oct 25, 2016

Thanks for the PR! Unfortunately I'll be pretty busy this week so it may take awhile to get back to this, but I haven't forgotten it!

Copy link
Member

@alexcrichton alexcrichton left a comment

Ok, looking good! We may need a few cleanups along the way to really land this, but I think this is a great start.

@@ -281,14 +281,15 @@ impl<'a, 'cfg> Encodable for WorkspaceResolve<'a, 'cfg> {

let root = self.ws.members().max_by_key(|member| {
member.name()
}).unwrap().package_id();
}).map(Package::package_id);

This comment has been minimized.

@alexcrichton

alexcrichton Oct 26, 2016
Member

Hm were the changes in this file required to get tests to pass? Or just cleaning things up?

This comment has been minimized.

@euclio

euclio Oct 26, 2016
Author Contributor

Yes, that panic is triggered when running test on a virtual manifest. It is currently prevented by the "error: manifest path ... is a virtual manifest, but this command requires running against an actual package in this workspace" error.

This comment has been minimized.

@alexcrichton

alexcrichton Nov 4, 2016
Member

Ok, sounds good to me

Packages::All => ws.members()
.map(|package| {
let package_id = package.package_id();
PackageIdSpec::from_package_id(package_id).to_string()

This comment has been minimized.

@alexcrichton

alexcrichton Oct 26, 2016
Member

Instead of round-tripping through PackageIdSpec, perhaps the resolution to a list of local packages could be done sooner?

This comment has been minimized.

@euclio

euclio Oct 26, 2016
Author Contributor

Where would be the right place to do that? I didn't want to place it too high up so that future --all commands could share this logic.

This comment has been minimized.

@alexcrichton

alexcrichton Nov 4, 2016
Member

Maybe the concept of "all" could be plumbed farther down as an enum?

This comment has been minimized.

@euclio

euclio Nov 8, 2016
Author Contributor

Plumbed down where? It makes sense to me to pass Packages down through resolve_dependencies but don't you still need to get the PackageIdSpecs of the workspace here to resolve the targets?

This comment has been minimized.

@alexcrichton

alexcrichton Nov 8, 2016
Member

Yeah let's pass this down to resolve_dependencies, and perhaps that can also return a list of specs?

@@ -28,7 +28,7 @@ pub struct Unit<'a> {
pub struct Context<'a, 'cfg: 'a> {
pub config: &'cfg Config,
pub resolve: &'a Resolve,
pub current_package: PackageId,
pub current_package: Option<PackageId>,

This comment has been minimized.

@alexcrichton

alexcrichton Oct 26, 2016
Member

Hm so I think this is a code smell we need to clean up (not your fault, a historical one). The concept of a "current package" is basically no longer valid in today's world of workspaces and such. Could this field get outright removed as part of this change?

This comment has been minimized.

@euclio

euclio Oct 26, 2016
Author Contributor

I can try that, sure.

This comment has been minimized.

@euclio

euclio Oct 27, 2016
Author Contributor

I think I need some clarification. What does "current package" mean in this context? Isn't it the package of the directory that we issue the compile in? Why is that not needed?

This comment has been minimized.

@alexcrichton

alexcrichton Nov 4, 2016
Member

Yeah the intention is there really is not right question to what the "current package" means. It has different interpretations depending on where you are asking it from, so that's why I figure it'd be best to remove and we can answer that question on a case-by-case basis.

This comment has been minimized.

@euclio

euclio Nov 8, 2016
Author Contributor

I see. Digging into this more, I'm noticing that the places that use Context don't have a Workspace available to pull a root package from (if one exists). So, doesn't making this an Option essentially handle this (i.e., a workspace either has a current package, or it's virtual, so it doesn't). Thank you for your patience :)

let root_package = try!(ws.current());
if spec.len() == 0 {
try!(generate_targets(root_package, &profiles, mode, filter, release));
}

This comment has been minimized.

@alexcrichton

alexcrichton Oct 26, 2016
Member

This kinda sent me spinning for a bit. I think this if statement isn't necessary, right? It's implied to always be true given the above condition.

This comment has been minimized.

@euclio

euclio Oct 26, 2016
Author Contributor

Fixed.

doc: Profile::default_doc(),
custom_build: Profile::default_custom_build(),
},
};

This comment has been minimized.

@alexcrichton

alexcrichton Oct 26, 2016
Member

Hm this is somewhat worrisome to me. This is where I think we'll want to learn about the profiles from the workspace root crate instead of whatever the current crate happens to be.

This comment has been minimized.

@euclio

euclio Oct 26, 2016
Author Contributor

Does the workspace have a root crate in a virtual manifest?

This comment has been minimized.

@matklad

matklad Nov 3, 2016
Member

Not exactly yet, see #3249 :)

This comment has been minimized.

@euclio

euclio Nov 4, 2016
Author Contributor

@matklad Awesome! I can rebase on top of your PR.

This comment has been minimized.

@alexcrichton

alexcrichton Nov 4, 2016
Member

Ah yeah #3249 is exactly what I had in mind here

@bors
Copy link
Contributor

@bors bors commented Nov 2, 2016

The latest upstream changes (presumably #3242) made this pull request unmergeable. Please resolve the merge conflicts.

@brson brson assigned alexcrichton and unassigned brson Nov 3, 2016
bors added a commit that referenced this pull request Nov 4, 2016
Use a single profile set per workspace

This aims to close #3206.

I have not figured out how to do this 100% backward compatibly, that's why I want to discuss this separately, although a related PR (#3221) is already in flight.

The problem is this: suppose that you have a workspace with two members, A and B and that A includes a profiles section and B does not. Now, mentally `cd` inside B and run `cargo build`. Today, Cargo will use a default profile. We want it to use a profile from A. So here the silent behavior switch will inevitably occur :( Looks like we can't detect this situation.

So this PR just switches the behavior to always use root profiles, and to print a warning if a non-root package specifies profiles. Feel free to reuse it in any form for #3221 if that's convenient!
@euclio euclio force-pushed the euclio:test-all branch from ea515af to 37af534 Nov 6, 2016
@euclio
Copy link
Contributor Author

@euclio euclio commented Nov 6, 2016

Rebased. Hopefully will get a chance to work on this more later this week.

@@ -92,8 +99,10 @@ pub enum CompileFilter<'a> {

pub fn compile<'a>(ws: &Workspace<'a>, options: &CompileOptions<'a>)
-> CargoResult<ops::Compilation<'a>> {
for key in try!(ws.current()).manifest().warnings().iter() {
try!(options.config.shell().warn(key))
if let Some(root_package) = ws.current_opt() {

This comment has been minimized.

@alexcrichton

alexcrichton Nov 8, 2016
Member

Random drive-by comment, but could this actually iterate over all members of the workspace? That's the intention here, at least

@bors
Copy link
Contributor

@bors bors commented Nov 11, 2016

The latest upstream changes (presumably #3280) made this pull request unmergeable. Please resolve the merge conflicts.

@euclio euclio force-pushed the euclio:test-all branch from 37af534 to 3faca82 Nov 16, 2016
@euclio
Copy link
Contributor Author

@euclio euclio commented Nov 16, 2016

Rebased, and ready for another round of review, @alexcrichton

Copy link
Member

@alexcrichton alexcrichton left a comment

Looking great!

}

Some(encodable_resolve_node(id, self.resolve))

This comment has been minimized.

@alexcrichton

alexcrichton Nov 17, 2016
Member

Hm ok to sanity check this (changes to resolve are very subtle).

I don't think this change is needed, right? The change above (unwrap -> map) is purely for cargo test against an empty virtual manifest, right? If there exists any crates at all (e.g. ids is a non-empty list) then I'd hope there's at least one workspace member.

.collect::<CargoResult<Vec<_>>>()?;

let pair = resolve_dependencies(ws,
let resolve = resolve_dependencies(ws,
source,

This comment has been minimized.

@alexcrichton

alexcrichton Nov 17, 2016
Member

Some indentation to match up here

for p in spec {
pkgids.push(resolve_with_overrides.query(&p)?);
for p in spec.iter() {
pkgids.push(resolve_with_overrides.query(&p.to_string())?);

This comment has been minimized.

@alexcrichton

alexcrichton Nov 17, 2016
Member

I think this can just be:

pkgids.push(p.query(resolve_with_overrides.iter()));

(avoid going through a string)

}
} else {
let root_package = ws.current()?;
generate_targets(root_package, profiles, mode, filter, release)?;

This comment has been minimized.

@alexcrichton

alexcrichton Nov 17, 2016
Member

I think this was special cased above to get these errors before the resolution process happened (which may involve downloads and such). Perhaps that logic could still be special cased?

This comment has been minimized.

@euclio

euclio Dec 2, 2016
Author Contributor

Putting this logic the old way caused my tests to fail, but I'm not sure why.

self.config.extra_verbose()
self.ws.current_opt().map_or(false, |p| *pkg == *p.package_id())
|| pkg.source_id().is_path()
|| self.config.extra_verbose()

This comment has been minimized.

@alexcrichton

alexcrichton Nov 17, 2016
Member

I think this can be updated to just the clause:

pkg.source_id().is_path() || self.config.extra_verbose()

This comment has been minimized.

@alexcrichton

alexcrichton Nov 17, 2016
Member

Well technically actually, we should show warnings for all workspace members, not not all path dependencies.

@@ -417,7 +416,8 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
// we don't want to link it up.
if src_dir.ends_with("deps") {
// Don't lift up library dependencies
if unit.pkg.package_id() != &self.current_package && !unit.target.is_bin() {
if self.ws.current_opt().map_or(false, |p| unit.pkg.package_id() != p.package_id())

This comment has been minimized.

@alexcrichton

alexcrichton Nov 17, 2016
Member

I think here we can actually just test:

if !unit.pkg.package_id().source_id().is_path() && !unit.target.is_bin() {
    // ...
}

That is, all path dependencies should hit the code path of Some below

This comment has been minimized.

@alexcrichton

alexcrichton Nov 17, 2016
Member

er actually, we should replace the current_package check with a check if the crate is in the workspace rather than all path dependencies.

This comment has been minimized.

@euclio

euclio Dec 2, 2016
Author Contributor

What's the correct way to check this?

ws.members().find(|&p| p == unit.pkg).is_some() causes test failures.

This comment has been minimized.

@alexcrichton

alexcrichton Dec 8, 2016
Member

Hm yeah that should work, but is that not working? What do the test failures look like?

This comment has been minimized.

@euclio

euclio Dec 8, 2016
Author Contributor

Hm, on a second look I think I might have just been reversing the conditional. Pushed a fix.

@@ -273,7 +272,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {

/// Returns the appropriate directory layout for either a plugin or not.
pub fn layout(&self, unit: &Unit) -> LayoutProxy {
let primary = unit.pkg.package_id() == &self.current_package;
let primary = self.ws.current_opt().map_or(false, |p| unit.pkg.package_id() == p.package_id());

This comment has been minimized.

@alexcrichton

alexcrichton Nov 17, 2016
Member

I think this can be replaced with:

let primary = is_member_of_current_workspace(self, unit);
let profile = cx.lib_profile(&cx.current_package);
let profile = cx.ws.current_opt().map_or_else(Profile::default, |p| {
cx.lib_profile(p.package_id()).to_owned()
});

This comment has been minimized.

@alexcrichton

alexcrichton Nov 17, 2016
Member

The package argument here to lib_profile is actually ignored (long story), so let's just go ahead and remove that argument and then we can avoid the workaround here.

unit.pkg.package_id() != &cx.current_package);
cx.ws.current_opt().map_or(false, |p| {
*p.package_id() != *unit.pkg.package_id()
}));

This comment has been minimized.

@alexcrichton

alexcrichton Nov 17, 2016
Member

Wow I have absolutely no clue what this clause is doing (the old one).

Let's just replace it though with, like above, a test whether the package is in the current workspace.

This comment has been minimized.

@euclio

euclio Dec 2, 2016
Author Contributor

Same as above.

This comment has been minimized.

@euclio

euclio Dec 8, 2016
Author Contributor

Ditto.

@euclio
Copy link
Contributor Author

@euclio euclio commented Nov 29, 2016

Still working on this, haven't had much time for open source lately! Hoping to finish up by the weekend.

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Nov 30, 2016

Ah no worries, let me know if you need any help getting it over the finish line!

@bors
Copy link
Contributor

@bors bors commented Dec 2, 2016

The latest upstream changes (presumably #3310) made this pull request unmergeable. Please resolve the merge conflicts.

@euclio euclio force-pushed the euclio:test-all branch 2 times, most recently from fc8e469 to 12331db Dec 2, 2016
@euclio
Copy link
Contributor Author

@euclio euclio commented Dec 2, 2016

@alexcrichton Most comments addressed, I commented on the remaining issues.

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Dec 8, 2016

Sorry for taking awhile to get back to this! I just want to dig into the "is a member" issue a bit and then we should be able to land this.

@euclio euclio force-pushed the euclio:test-all branch from 12331db to addbb7e Dec 8, 2016
@euclio
Copy link
Contributor Author

@euclio euclio commented Dec 8, 2016

@alexcrichton, no problem! Everything should be addressed now. There are failing tests, but those are due to #879.

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Dec 8, 2016

@bors: r+

Awesome, thanks!

@bors
Copy link
Contributor

@bors bors commented Dec 8, 2016

📌 Commit addbb7e has been approved by alexcrichton

@bors
Copy link
Contributor

@bors bors commented Dec 8, 2016

Testing commit addbb7e with merge fec03d3...

bors added a commit that referenced this pull request Dec 8, 2016
Add `--all` flag to `cargo test`

Work towards #2878.

This flag allows a user to test all members of a workspace by using `cargo test --all`. The command is also supported when using a virtual workspace manifest.
@bors
Copy link
Contributor

@bors bors commented Dec 9, 2016

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

@bors bors merged commit addbb7e into rust-lang:master Dec 9, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
bors added a commit that referenced this pull request Dec 13, 2016
Add test for --package and virtual manifest

closes #3194

The issue was actually fixed by #3221 (thanks @euclio !), so let's just add a test (a copy of `virtual_works` basically).
bors added a commit that referenced this pull request Dec 13, 2016
Add test for --package and virtual manifest

closes #3194

The issue was actually fixed by #3221 (thanks @euclio !), so let's just add a test (a copy of `virtual_works` basically).
bors added a commit that referenced this pull request Dec 13, 2016
Add test for --package and virtual manifest

closes #3194

The issue was actually fixed by #3221 (thanks @euclio !), so let's just add a test (a copy of `virtual_works` basically).
@khuey
Copy link

@khuey khuey commented Feb 7, 2017

Was it expected that this changed cargo's behavior so that tests of dependent lib crates are no longer dropped in the target/debug folder?

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Feb 7, 2017

@khuey no I think that may have been accidental! Oddly enough #3660 was just reported as well and I believe that bug was fixed in #3478, could you test beta/nightly to see if they're working for you?

@khuey
Copy link

@khuey khuey commented Feb 7, 2017

It's still broken on tip (de2919f)

@khuey
Copy link

@khuey khuey commented Feb 7, 2017

Woops, I was testing the wrong binary. It is indeed fixed on tip. I'll bisect for a fix cset.

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Feb 7, 2017

Ok, thanks for checking!

@khuey
Copy link

@khuey khuey commented Feb 7, 2017

It was indeed fixed by 3cbf141.

The regression is present in the cargo that ships with rustc 1.15. That's not a big deal for me (I just won't use it) but something you may want to be aware of.

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Feb 8, 2017

Sure thing, thanks for the heads up regardless

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants