-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Create "suggested tests" tool in rustbuild
#106249
Changes from all commits
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 | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,80 @@ | ||||||||
#![cfg_attr(feature = "build-metrics", allow(unused))] | ||||||||
|
||||||||
use std::str::FromStr; | ||||||||
|
||||||||
use std::path::PathBuf; | ||||||||
|
||||||||
use crate::{ | ||||||||
builder::{Builder, Kind}, | ||||||||
tool::Tool, | ||||||||
}; | ||||||||
|
||||||||
#[cfg(feature = "build-metrics")] | ||||||||
pub fn suggest(builder: &Builder<'_>, run: bool) { | ||||||||
panic!("`x suggest` is not supported with `build-metrics`") | ||||||||
} | ||||||||
|
||||||||
/// Suggests a list of possible `x.py` commands to run based on modified files in branch. | ||||||||
#[cfg(not(feature = "build-metrics"))] | ||||||||
pub fn suggest(builder: &Builder<'_>, run: bool) { | ||||||||
let suggestions = | ||||||||
builder.tool_cmd(Tool::SuggestTests).output().expect("failed to run `suggest-tests` tool"); | ||||||||
|
||||||||
if !suggestions.status.success() { | ||||||||
println!("failed to run `suggest-tests` tool ({})", suggestions.status); | ||||||||
println!( | ||||||||
"`suggest_tests` stdout:\n{}`suggest_tests` stderr:\n{}", | ||||||||
String::from_utf8(suggestions.stdout).unwrap(), | ||||||||
String::from_utf8(suggestions.stderr).unwrap() | ||||||||
); | ||||||||
panic!("failed to run `suggest-tests`"); | ||||||||
} | ||||||||
|
||||||||
let suggestions = String::from_utf8(suggestions.stdout).unwrap(); | ||||||||
let suggestions = suggestions | ||||||||
.lines() | ||||||||
.map(|line| { | ||||||||
let mut sections = line.split_ascii_whitespace(); | ||||||||
|
||||||||
// this code expects one suggestion per line in the following format: | ||||||||
// <x_subcommand> {some number of flags} [optional stage number] | ||||||||
let cmd = sections.next().unwrap(); | ||||||||
let stage = sections.next_back().map(|s| str::parse(s).ok()).flatten(); | ||||||||
let paths: Vec<PathBuf> = sections.map(|p| PathBuf::from_str(p).unwrap()).collect(); | ||||||||
Ezrashaw marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
|
||||||||
(cmd, stage, paths) | ||||||||
}) | ||||||||
.collect::<Vec<_>>(); | ||||||||
|
||||||||
if !suggestions.is_empty() { | ||||||||
println!("==== SUGGESTIONS ===="); | ||||||||
for sug in &suggestions { | ||||||||
print!("x {} ", sug.0); | ||||||||
if let Some(stage) = sug.1 { | ||||||||
print!("--stage {stage} "); | ||||||||
} | ||||||||
|
||||||||
for path in &sug.2 { | ||||||||
print!("{} ", path.display()); | ||||||||
} | ||||||||
println!(); | ||||||||
} | ||||||||
println!("====================="); | ||||||||
} else { | ||||||||
println!("No suggestions found!"); | ||||||||
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.
Suggested change
Add an early return here to avoid printing the hint about 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. @Ezrashaw looks like this comment was never addressed? 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 it was? Line 57? It's underneath the box, kind of hidden lol. |
||||||||
return; | ||||||||
} | ||||||||
|
||||||||
if run { | ||||||||
for sug in suggestions { | ||||||||
let mut build = builder.build.clone(); | ||||||||
|
||||||||
let builder = | ||||||||
Builder::new_standalone(&mut build, Kind::parse(&sug.0).unwrap(), sug.2, sug.1); | ||||||||
|
||||||||
builder.execute_cli() | ||||||||
} | ||||||||
} else { | ||||||||
println!("help: consider using the `--run` flag to automatically run suggested tests"); | ||||||||
} | ||||||||
} |
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 think you can avoid needing this new function by modifying
build.config.cmd
and usingBuilder::new
instead. You could changesug!
to generate the command for you so it's not too much of a pain: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.
Thanks for the idea, I think I also need to modify
build.config.stage
. Sorry I'm on holiday right now so the progress is slow.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.
No need to apologize! You're responding quite fast actually 😄
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.
Hmm, using
serde_json
means that we can't directly createSubcommand
s. Honestly, the solution is good enough as is for now.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'm not sure I understand the problem, sorry - how is serde_json related here?
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.
@Ezrashaw I don't think I ever got an answer here - how is serde related?
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.
Sorry about that. I have no idea what I was thinking lol. I think that this isn't that relevant anymore since suggestions are implemented in code.