diff --git a/Cargo.lock b/Cargo.lock index 55d0441f5..9507460c4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1257,6 +1257,18 @@ dependencies = [ "unicode-width", ] +[[package]] +name = "insta" +version = "1.40.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6593a41c7a73841868772495db7dc1e8ecab43bb5c0b6da2059246c4b506ab60" +dependencies = [ + "console", + "lazy_static", + "linked-hash-map", + "similar", +] + [[package]] name = "instant" version = "0.1.12" @@ -1400,6 +1412,12 @@ dependencies = [ "vcpkg", ] +[[package]] +name = "linked-hash-map" +version = "0.5.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0717cef1bc8b636c6e1c1bbdefc09e6322da8a9321966e8928ef80d20f7f770f" + [[package]] name = "linux-raw-sys" version = "0.1.4" @@ -2430,6 +2448,7 @@ dependencies = [ "humansize", "hyper", "inferno", + "insta", "itertools", "jemalloc-ctl", "jemallocator", diff --git a/site/Cargo.toml b/site/Cargo.toml index 30b2994de..a11f90246 100644 --- a/site/Cargo.toml +++ b/site/Cargo.toml @@ -57,3 +57,6 @@ jemalloc-ctl = "0.5" serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } toml = "0.7" + +[dev-dependencies] +insta = "1.40.0" diff --git a/site/README.md b/site/README.md index 305845a28..aece864fc 100644 --- a/site/README.md +++ b/site/README.md @@ -46,3 +46,8 @@ the `PORT` environment variable. ```console $ ./target/release/site ``` + +## Development +We use [insta](https://github.com/mitsuhiko/insta) for snapshot testing. If any tests that use the +`insta` macros fail, you should `cargo install cargo-insta` and then run `cargo insta review` to review +the snapshot changes. diff --git a/site/src/request_handlers/github.rs b/site/src/request_handlers/github.rs index fd7e97a7d..9034bbb79 100644 --- a/site/src/request_handlers/github.rs +++ b/site/src/request_handlers/github.rs @@ -5,17 +5,9 @@ use crate::github::{ }; use crate::load::SiteCtxt; +use hashbrown::HashMap; use std::sync::Arc; -use regex::Regex; - -lazy_static::lazy_static! { - static ref BODY_TIMER_BUILD: Regex = - Regex::new(r"(?:\W|^)@rust-timer\s+build\s+(\w+)(?:\W|$)(?:include=(\S+))?\s*(?:exclude=(\S+))?\s*(?:runs=(\d+))?").unwrap(); - static ref BODY_TIMER_QUEUE: Regex = - Regex::new(r"(?:\W|^)@rust-timer\s+queue(?:\W|$)(?:include=(\S+))?\s*(?:exclude=(\S+))?\s*(?:runs=(\d+))?").unwrap(); -} - pub async fn handle_github( request: github::Request, ctxt: Arc, @@ -118,36 +110,57 @@ async fn handle_rust_timer( return Ok(github::Response); } - if let Some(captures) = BODY_TIMER_QUEUE.captures(&comment.body) { - let include = captures.get(1).map(|v| v.as_str()); - let exclude = captures.get(2).map(|v| v.as_str()); - let runs = captures.get(3).and_then(|v| v.as_str().parse::().ok()); - { - let conn = ctxt.conn().await; - conn.queue_pr(issue.number, include, exclude, runs).await; - } - main_client - .post_comment( - issue.number, + if let Some(queue) = parse_queue_command(&comment.body) { + let msg = match queue { + Ok(cmd) => { + let conn = ctxt.conn().await; + conn.queue_pr( + issue.number, + cmd.params.include, + cmd.params.exclude, + cmd.params.runs, + ) + .await; format!( "Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf {COMMENT_MARK_TEMPORARY}" - ), - ) - .await; + ) + } + Err(error) => { + format!("Error occurred while parsing comment: {error}") + } + }; + main_client.post_comment(issue.number, msg).await; return Ok(github::Response); } - for captures in build_captures(&comment.body).map(|(_, captures)| captures) { - let include = captures.get(2).map(|v| v.as_str()); - let exclude = captures.get(3).map(|v| v.as_str()); - let runs = captures.get(4).and_then(|v| v.as_str().parse::().ok()); - { - let conn = ctxt.conn().await; - conn.queue_pr(issue.number, include, exclude, runs).await; + let build_cmds: Vec<_> = parse_build_commands(&comment.body).collect(); + let mut valid_build_cmds = vec![]; + let mut errors = String::new(); + for cmd in build_cmds { + match cmd { + Ok(cmd) => valid_build_cmds.push(cmd), + Err(error) => errors.push_str(&format!("Cannot parse build command: {error}\n")), + } + } + if !errors.is_empty() { + main_client.post_comment(issue.number, errors).await; + return Ok(github::Response); + } + + { + let conn = ctxt.conn().await; + for command in &valid_build_cmds { + conn.queue_pr( + issue.number, + command.params.include, + command.params.exclude, + command.params.runs, + ) + .await; } } @@ -156,25 +169,121 @@ async fn handle_rust_timer( main_client, ci_client, issue.number, - build_captures(&comment.body).map(|(commit, _)| commit), + valid_build_cmds.iter().map(|c| c.sha), ) .await?; Ok(github::Response) } -/// Run the `@rust-timer build` regex over the comment message extracting the commit and the other captures -fn build_captures(comment_body: &str) -> impl Iterator { - BODY_TIMER_BUILD - .captures_iter(comment_body) - .filter_map(|captures| { - captures.get(1).map(|m| { - let commit = m - .as_str() - .trim_start_matches("https://github.com/rust-lang/rust/commit/"); - (commit, captures) - }) +/// Parses the first occurrence of a `@rust-timer queue ` command +/// in the input string. +fn parse_queue_command(body: &str) -> Option> { + let args = get_command_lines(body, "queue").next()?; + let args = match parse_command_arguments(args) { + Ok(args) => args, + Err(error) => return Some(Err(error)), + }; + let params = match parse_benchmark_parameters(args) { + Ok(params) => params, + Err(error) => return Some(Err(error)), + }; + + Some(Ok(QueueCommand { params })) +} + +/// Parses all occurrences of a `@rust-timer build ` command in the input string. +fn parse_build_commands(body: &str) -> impl Iterator> { + get_command_lines(body, "build").map(|line| { + let mut iter = line.splitn(2, ' '); + let Some(sha) = iter.next().filter(|s| !s.is_empty() && !s.contains('=')) else { + return Err("Missing SHA in build command".to_string()); + }; + + let sha = sha.trim_start_matches("https://github.com/rust-lang/rust/commit/"); + let args = iter.next().unwrap_or(""); + let args = parse_command_arguments(args)?; + let params = parse_benchmark_parameters(args)?; + Ok(BuildCommand { sha, params }) + }) +} + +fn get_command_lines<'a: 'b, 'b>( + body: &'a str, + command: &'b str, +) -> impl Iterator + 'b { + let prefix = "@rust-timer"; + body.lines() + .filter_map(move |line| { + line.find(prefix) + .map(|index| line[index + prefix.len()..].trim()) }) + .filter_map(move |line| line.strip_prefix(command)) + .map(move |l| l.trim_start()) +} + +fn parse_benchmark_parameters<'a>( + mut args: HashMap<&'a str, &'a str>, +) -> Result, String> { + let mut params = BenchmarkParameters { + include: args.remove("include"), + exclude: args.remove("exclude"), + runs: None, + }; + if let Some(runs) = args.remove("runs") { + let Ok(runs) = runs.parse::() else { + return Err(format!("Cannot parse runs {runs} as a number")); + }; + params.runs = Some(runs as i32); + } + + if !args.is_empty() { + Err(format!( + "Unknown command argument(s) `{}`", + args.into_keys().collect::>().join(",") + )) + } else { + Ok(params) + } +} + +/// Parses command arguments from a single line of text. +/// Expects that arguments are separated by whitespace, and each argument +/// has the format `=`. +fn parse_command_arguments(args: &str) -> Result, String> { + let mut argmap = HashMap::new(); + for arg in args.split_whitespace() { + let Some((key, value)) = arg.split_once('=') else { + return Err(format!( + "Invalid command argument `{arg}` (there may be no spaces around the `=` character)" + )); + }; + let key = key.trim(); + let value = value.trim(); + if argmap.insert(key, value).is_some() { + return Err(format!("Duplicate command argument `{key}`")); + } + } + + Ok(argmap) +} + +#[derive(Debug)] +struct QueueCommand<'a> { + params: BenchmarkParameters<'a>, +} + +#[derive(Debug)] +struct BuildCommand<'a> { + sha: &'a str, + params: BenchmarkParameters<'a>, +} + +#[derive(Debug)] +struct BenchmarkParameters<'a> { + include: Option<&'a str>, + exclude: Option<&'a str>, + runs: Option, } pub async fn get_authorized_users() -> Result, String> { @@ -196,23 +305,151 @@ pub async fn get_authorized_users() -> Result, String> { #[cfg(test)] mod tests { use super::*; + + #[test] + fn build_command_missing() { + assert!(get_build_commands("").is_empty()); + } + + #[test] + fn build_unknown_command() { + assert!(get_build_commands("@rust-timer foo").is_empty()); + } + + #[test] + fn build_command_missing_sha() { + insta::assert_compact_debug_snapshot!(get_build_commands("@rust-timer build"), + @r###"[Err("Missing SHA in build command")]"###); + } + + #[test] + fn build_command() { + insta::assert_compact_debug_snapshot!(get_build_commands("@rust-timer build 5832462aa1d9373b24ace96ad2c50b7a18af9952"), + @r###"[Ok(BuildCommand { sha: "5832462aa1d9373b24ace96ad2c50b7a18af9952", params: BenchmarkParameters { include: None, exclude: None, runs: None } })]"###); + } + #[test] - fn captures_all_shas() { - let comment_body = r#" -Going to do perf runs for a few of these: - -@rust-timer build 5832462aa1d9373b24ace96ad2c50b7a18af9952 (https://github.com/rust-lang/rust/pull/100307) -@rust-timer build 23936af287657fa4148aeab40cc2a780810fae52 (https://github.com/rust-lang/rust/pull/100392) - "#; - let shas = build_captures(comment_body) - .map(|(c, _)| c) - .collect::>(); - assert_eq!( - shas, - &[ - "5832462aa1d9373b24ace96ad2c50b7a18af9952", - "23936af287657fa4148aeab40cc2a780810fae52" - ] - ); + fn build_command_multiple() { + insta::assert_compact_debug_snapshot!(get_build_commands(r#" +@rust-timer build 5832462aa1d9373b24ace96ad2c50b7a18af9952 +@rust-timer build 23936af287657fa4148aeab40cc2a780810fae52 +"#), + @r###"[Ok(BuildCommand { sha: "5832462aa1d9373b24ace96ad2c50b7a18af9952", params: BenchmarkParameters { include: None, exclude: None, runs: None } }), Ok(BuildCommand { sha: "23936af287657fa4148aeab40cc2a780810fae52", params: BenchmarkParameters { include: None, exclude: None, runs: None } })]"###); + } + + #[test] + fn build_command_unknown_arg() { + insta::assert_compact_debug_snapshot!(get_build_commands("@rust-timer build foo=bar"), + @r###"[Err("Missing SHA in build command")]"###); + } + + #[test] + fn build_command_complex() { + insta::assert_compact_debug_snapshot!(get_build_commands(" @rust-timer build sha123456 exclude=baz include=foo,bar runs=4"), + @r###"[Ok(BuildCommand { sha: "sha123456", params: BenchmarkParameters { include: Some("foo,bar"), exclude: Some("baz"), runs: Some(4) } })]"###); + } + + #[test] + fn build_command_link() { + insta::assert_compact_debug_snapshot!(get_build_commands(r#" +@rust-timer build https://github.com/rust-lang/rust/commit/323f521bc6d8f2b966ba7838a3f3ee364e760b7e"#), + @r###"[Ok(BuildCommand { sha: "323f521bc6d8f2b966ba7838a3f3ee364e760b7e", params: BenchmarkParameters { include: None, exclude: None, runs: None } })]"###); + } + + #[test] + fn queue_command_missing() { + assert!(parse_queue_command("").is_none()); + } + + #[test] + fn queue_unknown_command() { + assert!(parse_queue_command("@rust-timer foo").is_none()); + } + + #[test] + fn queue_command() { + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue"), + @"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: None } }))"); + } + + #[test] + fn queue_command_unknown_arg() { + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue foo=bar"), + @r###"Some(Err("Unknown command argument(s) `foo`"))"###); + } + + #[test] + fn queue_command_duplicate_arg() { + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue include=a exclude=c include=b"), + @r###"Some(Err("Duplicate command argument `include`"))"###); + } + + #[test] + fn queue_command_include() { + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue include=abcd,feih"), + @r###"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("abcd,feih"), exclude: None, runs: None } }))"###); + } + + #[test] + fn queue_command_exclude() { + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue exclude=foo134,barzbaz41baf"), + @r###"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: Some("foo134,barzbaz41baf"), runs: None } }))"###); + } + + #[test] + fn queue_command_runs() { + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue runs=5"), + @"Some(Ok(QueueCommand { params: BenchmarkParameters { include: None, exclude: None, runs: Some(5) } }))"); + } + + #[test] + fn queue_command_runs_nan() { + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue runs=xxx"), + @r###"Some(Err("Cannot parse runs xxx as a number"))"###); + } + + #[test] + fn queue_command_combination() { + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue include=acda,13asd exclude=c13,DA runs=5"), + @r###"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("acda,13asd"), exclude: Some("c13,DA"), runs: Some(5) } }))"###); + } + + #[test] + fn queue_command_argument_spaces() { + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue include = abcd,das"), + @r###"Some(Err("Invalid command argument `include` (there may be no spaces around the `=` character)"))"###); + } + + #[test] + fn queue_command_spaces() { + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue include=abcd,das "), + @r###"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("abcd,das"), exclude: None, runs: None } }))"###); + } + + #[test] + fn queue_command_with_bors() { + insta::assert_compact_debug_snapshot!(parse_queue_command("@bors try @rust-timer queue include=foo,bar"), + @r###"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("foo,bar"), exclude: None, runs: None } }))"###); + } + + #[test] + fn queue_command_parameter_order() { + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue runs=3 exclude=c,a include=b"), + @r###"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("b"), exclude: Some("c,a"), runs: Some(3) } }))"###); + } + + #[test] + fn queue_command_multiline() { + insta::assert_compact_debug_snapshot!(parse_queue_command(r#"Ok, this looks good now. +Let's do a perf run quickly and then we can merge it. + +@bors try @rust-timer queue include=foo,bar + +Otherwise LGTM."#), + @r###"Some(Ok(QueueCommand { params: BenchmarkParameters { include: Some("foo,bar"), exclude: None, runs: None } }))"###); + } + + fn get_build_commands(body: &str) -> Vec> { + parse_build_commands(body).collect() } }