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

do not compile test for bins flagged as test = false #10305

Merged
merged 8 commits into from
Jan 20, 2022

Conversation

weihanglo
Copy link
Member

@weihanglo weihanglo commented Jan 18, 2022

What does this PR try to resolve?

Fixes #10268

#6683 introduced a behavior that compiles all bin targets, but for bins with test = false they shouldn't be compiled with --test as testbins.

How should we test and review this PR?

In the first commit of this PR, I refines the test test_filtered_excludes_compiling_examples to reflect the current wrong behavior (test passed). The following two commits correct the behavior and the test accordingly. The last few commits encapsulate scattered target selection logic into functions on CompileFilter.

This behavior is not correct should be fixed.
Update this test only for pointing out the current behavior.
Binaries without `test = false` are included by default, so no need to
explicitly compile all binaries.
@rust-highfive
Copy link

r? @alexcrichton

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 18, 2022
@alexcrichton
Copy link
Member

Thanks for this! As I read this though I'm sort of confused and wondering if you can help. Why is this filter reset given a test name to run? I would naively expect that by passing in a test name that doesn't affect the filter of what to or not to build at all and the test name is simply passed to all binaries which are built anyway

@weihanglo
Copy link
Member Author

IIUC, examples are included with the default target filter, but #6683 (comment) tried to avoid build examples if TESTNAME exists.

CompileMode::Test => targets
.iter()
.filter(|t| t.tested() || t.is_example())
.collect(),

I also feel a bit confused and unease when seeing this inconsistency. Though I should admit that it's somehow reasonable since cargo actually doesn't run any unittest in example with default target filter. If one passes in a test name without specifying target, the intent is quite straightforward that they wants to filter tests against default targets. Thus cargo can happily skip compiling examples.

@alexcrichton
Copy link
Member

Perhaps a better way to organize this would be to have a method on CompileFilter called remove_examples or similar which can perform the desired result in a more targeted fashion?

@weihanglo
Copy link
Member Author

Oops. The PR evolves to a refactoring. 🙇🏾‍♂️

Is 9138c35 in a more targeted fashion as you expect?

@alexcrichton
Copy link
Member

@bors: r+

Seems reasonable to me!

@bors
Copy link
Contributor

bors commented Jan 20, 2022

📌 Commit 2f5b303 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 20, 2022
@bors
Copy link
Contributor

bors commented Jan 20, 2022

⌛ Testing commit 2f5b303 with merge 58790d3...

@bors
Copy link
Contributor

bors commented Jan 20, 2022

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing 58790d3 to master...

@bors bors merged commit 58790d3 into rust-lang:master Jan 20, 2022
@weihanglo weihanglo deleted the issue-10268 branch January 20, 2022 16:35
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 26, 2022
Update cargo

9 commits in 95bb3c92bf516017e812e7f1c14c2dea3845b30e..1c034752de0df744fcd7788fcbca158830b8bf85
2022-01-18 17:39:35 +0000 to 2022-01-25 22:36:53 +0000
- Sync toml_edit versions (rust-lang/cargo#10329)
- Check --config for dotted keys only (rust-lang/cargo#10176)
- Remove deprecated --host arg for search and publish cmds (rust-lang/cargo#10327)
- doc: it's valid to use OUT_DIR for intermediate artifacts (rust-lang/cargo#10326)
- Use local git info for version. (rust-lang/cargo#10323)
- Fix documenting with undocumented dependencies. (rust-lang/cargo#10324)
- do not compile test for bins flagged as `test = false` (rust-lang/cargo#10305)
- Port cargo from toml-rs to toml_edit (rust-lang/cargo#10086)
- Fix new::git_default_branch with different default (rust-lang/cargo#10306)
@ehuss ehuss added this to the 1.60.0 milestone Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cargo test ignores test flags when testing specific keyword
5 participants