From 3ef307efc266029dd66e2b77440c6784f32bf138 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Fri, 3 Sep 2021 17:14:23 +0200 Subject: [PATCH 1/4] Refactor missing_commits to be clearer --- site/src/load.rs | 52 +++++++++++++++++++++++++----------------------- 1 file changed, 27 insertions(+), 25 deletions(-) diff --git a/site/src/load.rs b/site/src/load.rs index 4d10429ea..ef3477a21 100644 --- a/site/src/load.rs +++ b/site/src/load.rs @@ -147,27 +147,26 @@ impl SiteCtxt { pub async fn missing_commits(&self) -> Vec<(Commit, MissingReason)> { let conn = self.conn().await; - let (master_commits, queued_commits, in_progress_artifacts) = futures::join!( + let (master_commits, queued_try_commits, in_progress_artifacts) = futures::join!( collector::master_commits(), conn.queued_commits(), conn.in_progress_artifacts() ); - let commits = master_commits + let master_commits = master_commits .map_err(|e| anyhow::anyhow!("{:?}", e)) .context("getting master commit list") .unwrap(); let index = self.index.load(); - let mut have = index + let mut all_commits = index .commits() .iter() .map(|commit| commit.sha.clone()) .collect::>(); let now = Utc::now(); - let mut missing = commits - .iter() - .cloned() + let mut master_commits = master_commits + .into_iter() .filter(|c| now.signed_duration_since(c.time) < Duration::days(29)) .map(|c| { ( @@ -182,9 +181,9 @@ impl SiteCtxt { ) }) .collect::>(); - missing.reverse(); - let mut commits = Vec::new(); - commits.reserve(queued_commits.len() * 2); // Two commits per every try commit + master_commits.reverse(); + + let mut missing = Vec::with_capacity(queued_try_commits.len() * 2 + master_commits.len()); // Two commits per every try commit and all master commits for database::QueuedCommit { sha, parent_sha, @@ -192,15 +191,18 @@ impl SiteCtxt { include, exclude, runs, - } in queued_commits + } in queued_try_commits { // Enqueue the `TryParent` commit before the `TryCommit` itself, so that // all of the `try` run's data is complete when the benchmark results // of that commit are available. - if let Some((commit, _)) = missing.iter().find(|c| c.0.sha == *parent_sha.as_str()) { - commits.push((commit.clone(), MissingReason::TryParent)); + if let Some((try_parent, _)) = master_commits + .iter() + .find(|(m, _)| m.sha == parent_sha.as_str()) + { + missing.push((try_parent.clone(), MissingReason::TryParent)); } - commits.push(( + missing.push(( Commit { sha: sha.to_string(), date: Date::ymd_hms(2001, 01, 01, 0, 0, 0), @@ -213,39 +215,39 @@ impl SiteCtxt { }, )); } - commits.extend(missing); + missing.extend(master_commits); for aid in in_progress_artifacts { match aid { ArtifactId::Commit(c) => { - let previous = commits + let previous = missing .iter() .find(|(i, _)| i.sha == c.sha) .map(|v| Box::new(v.1.clone())); - have.remove(&c.sha); - commits.insert(0, (c, MissingReason::InProgress(previous))); + all_commits.remove(&c.sha); + missing.insert(0, (c, MissingReason::InProgress(previous))); } ArtifactId::Tag(_) => { - // do nothing, for now, though eventually we'll want an artifact - // queue + // do nothing, for now, though eventually we'll want an artifact queue } } } - let mut seen = HashSet::with_capacity(commits.len()); - seen.extend(have); + let mut already_tested = HashSet::with_capacity(all_commits.len()); + already_tested.extend(all_commits); + // Remove commits from missing that have already been tested // FIXME: replace with Vec::drain_filter when it stabilizes let mut i = 0; - while i != commits.len() { - if !seen.insert(commits[i].0.sha.clone()) { - commits.remove(i); + while i != missing.len() { + if !already_tested.insert(missing[i].0.sha.clone()) { + missing.remove(i); } else { i += 1; } } - commits + missing } } From f78ebf6a53234462e5f5366019b326886f51d871 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Fri, 3 Sep 2021 18:42:39 +0200 Subject: [PATCH 2/4] Enqueue master commits in pr_queue --- site/src/github.rs | 141 +++++++++++++---------- site/src/load.rs | 12 ++ site/src/request_handlers/next_commit.rs | 64 +++++----- 3 files changed, 130 insertions(+), 87 deletions(-) diff --git a/site/src/github.rs b/site/src/github.rs index d365d5065..b5828bd99 100644 --- a/site/src/github.rs +++ b/site/src/github.rs @@ -3,7 +3,7 @@ use crate::comparison::{ComparisonSummary, Direction}; use crate::load::{Config, SiteCtxt, TryCommit}; use anyhow::Context as _; -use database::ArtifactId; +use database::{ArtifactId, QueuedCommit}; use hashbrown::HashSet; use reqwest::header::USER_AGENT; use serde::{Deserialize, Serialize}; @@ -509,92 +509,117 @@ pub async fn post_finished(ctxt: &SiteCtxt) { } let conn = ctxt.conn().await; let index = ctxt.index.load(); - let mut commits = index + let mut already_tested_commits = index .commits() .into_iter() .map(|c| c.sha.to_string()) .collect::>(); - let queued = conn.queued_commits().await; + let (master_commits, queued_pr_commits, in_progress_artifacts) = futures::join!( + collector::master_commits(), + conn.queued_commits(), + conn.in_progress_artifacts() + ); + let master_commits = master_commits + .unwrap() + .into_iter() + .map(|c| c.sha) + .collect::>(); - for aid in conn.in_progress_artifacts().await { + for aid in in_progress_artifacts { match aid { ArtifactId::Commit(c) => { - commits.remove(&c.sha); + already_tested_commits.remove(&c.sha); } ArtifactId::Tag(_) => { - // do nothing, for now, though eventually we'll want an artifact - // queue + // do nothing, for now, though eventually we'll want an artifact queue } } } - for commit in queued { - if !commits.contains(&commit.sha) { - continue; - } - - // This commit has been benchmarked. - + for commit in queued_pr_commits + .into_iter() + .filter(|c| already_tested_commits.contains(&c.sha)) + { if let Some(completed) = conn.mark_complete(&commit.sha).await { assert_eq!(completed, commit); - let comparison_url = format!( - "https://perf.rust-lang.org/compare.html?start={}&end={}", - commit.parent_sha, commit.sha - ); - let (summary, direction) = categorize_benchmark(&commit, ctxt).await; - let label = match direction { - Some(Direction::Regression | Direction::Mixed) => "+perf-regression", - Some(Direction::Improvement) | None => "-perf-regression", - }; - let msg = direction - .map(|d| { - format!( - "While you can manually mark this PR as fit \ + let is_master_commit = master_commits.contains(&commit.sha); + post_comparison_comment(commit, ctxt, is_master_commit).await; + } + } +} + +async fn post_comparison_comment(commit: QueuedCommit, ctxt: &SiteCtxt, is_master_commit: bool) { + let comparison_url = format!( + "https://perf.rust-lang.org/compare.html?start={}&end={}", + commit.parent_sha, commit.sha + ); + let (summary, direction) = + categorize_benchmark(commit.sha.clone(), commit.parent_sha, ctxt).await; + let label = match direction { + Some(Direction::Regression | Direction::Mixed) => "+perf-regression", + Some(Direction::Improvement) | None => "-perf-regression", + }; + let next_steps_msg = direction + .map(|d| { + format!( + "{}{}", + if is_master_commit { + "" + } else { + "While you can manually mark this PR as fit \ for rollup, we strongly recommend not doing so since this PR led to changes in \ - compiler perf.{}", - match d { - Direction::Regression | Direction::Mixed => - "\n\n**Next Steps**: If you can justify the \ + compiler perf." + }, + match d { + Direction::Regression | Direction::Mixed => + "\n\n**Next Steps**: If you can justify the \ regressions found in this perf run, please indicate this with \ `@rustbot label: +perf-regression-triaged` along with \ sufficient written justification. If you cannot justify the regressions \ - please fix the regressions and do another perf run. If the next run shows \ - neutral or positive results, the label will be automatically removed.", - Direction::Improvement => "", - } - ) - }) - .unwrap_or(String::new()); - - post_comment( - &ctxt.config, - commit.pr, - format!( - "Finished benchmarking try commit ({}): [comparison url]({}). + please fix the regressions (either in this PR if it's not yet merged or \ + in another PR) and do another perf run.", + Direction::Improvement => "", + } + ) + }) + .unwrap_or(String::new()); + let rollup_msg = if is_master_commit { + "" + } else { + "Benchmarking this pull request likely means that it is \ +perf-sensitive, so we're automatically marking it as not fit \ +for rolling up. " + }; + let bors_msg = if is_master_commit { + "" + } else { + "@bors rollup=never\n" + }; + post_comment( + &ctxt.config, + commit.pr, + format!( + "Finished benchmarking commit ({}): [comparison url]({}). **Summary**: {} -Benchmarking this pull request likely means that it is \ -perf-sensitive, so we're automatically marking it as not fit \ -for rolling up. {} - -@bors rollup=never +{}{} +{} @rustbot label: +S-waiting-on-review -S-waiting-on-perf {}", - commit.sha, comparison_url, summary, msg, label - ), - ) - .await; - } - } + commit.sha, comparison_url, summary, rollup_msg, next_steps_msg, bors_msg, label + ), + ) + .await; } async fn categorize_benchmark( - commit: &database::QueuedCommit, + commit_sha: String, + parent_sha: String, ctxt: &SiteCtxt, ) -> (String, Option) { let comparison = match crate::comparison::compare( - collector::Bound::Commit(commit.parent_sha.clone()), - collector::Bound::Commit(commit.sha.clone()), + collector::Bound::Commit(parent_sha), + collector::Bound::Commit(commit_sha), "instructions:u".to_owned(), ctxt, ) diff --git a/site/src/load.rs b/site/src/load.rs index ef3477a21..756ec20a2 100644 --- a/site/src/load.rs +++ b/site/src/load.rs @@ -23,6 +23,7 @@ pub enum MissingReason { /// This commmit has not yet been benchmarked Master { pr: u32, + parent_sha: String, }, TryParent, Try { @@ -34,6 +35,16 @@ pub enum MissingReason { InProgress(Option>), } +impl MissingReason { + /// If the commit is a master commit get its PR and parent_sha + pub fn master_commit_pr_and_parent(&self) -> Option<(u32, &str)> { + match self { + Self::Master { pr, parent_sha } => Some((*pr, parent_sha)), + _ => None, + } + } +} + #[derive(Clone, Deserialize, Serialize, Debug, PartialEq, Eq)] pub struct TryCommit { pub sha: String, @@ -177,6 +188,7 @@ impl SiteCtxt { // All recent master commits should have an associated PR MissingReason::Master { pr: c.pr.unwrap_or(0), + parent_sha: c.parent_sha, }, ) }) diff --git a/site/src/request_handlers/next_commit.rs b/site/src/request_handlers/next_commit.rs index 3543c6cab..34e645bf2 100644 --- a/site/src/request_handlers/next_commit.rs +++ b/site/src/request_handlers/next_commit.rs @@ -4,41 +4,47 @@ use collector::api::next_commit; use std::sync::Arc; pub async fn handle_next_commit(ctxt: Arc) -> next_commit::Response { - let commit = ctxt - .missing_commits() - .await - .into_iter() - .next() - .map(|(commit, missing_reason)| { - let (include, exclude, runs) = match missing_reason { - crate::load::MissingReason::Try { + let commit = ctxt.missing_commits().await.into_iter().next(); + + let commit = if let Some((commit, missing_reason)) = commit { + // If we're going to run a master commit next, make sure + // it's been enqueued in the pull_request_build table + if let Some((pr, parent_sha)) = missing_reason.master_commit_pr_and_parent() { + let conn = ctxt.conn().await; + conn.queue_pr(pr, None, None, None).await; + conn.pr_attach_commit(pr, &commit.sha, parent_sha).await; + } + let (include, exclude, runs) = match missing_reason { + crate::load::MissingReason::Try { + include, + exclude, + runs, + .. + } => (include, exclude, runs), + crate::load::MissingReason::InProgress(Some(previous)) => { + if let crate::load::MissingReason::Try { include, exclude, runs, .. - } => (include, exclude, runs), - crate::load::MissingReason::InProgress(Some(previous)) => { - if let crate::load::MissingReason::Try { - include, - exclude, - runs, - .. - } = *previous - { - (include, exclude, runs) - } else { - (None, None, None) - } + } = *previous + { + (include, exclude, runs) + } else { + (None, None, None) } - _ => (None, None, None), - }; - next_commit::Commit { - sha: commit.sha, - include, - exclude, - runs, } - }); + _ => (None, None, None), + }; + Some(next_commit::Commit { + sha: commit.sha, + include, + exclude, + runs, + }) + } else { + None + }; next_commit::Response { commit } } From 75d14ff51433d251ba10b0fc1306008bece5d124 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Mon, 6 Sep 2021 11:09:20 +0200 Subject: [PATCH 3/4] Don't return master commits as try commit --- site/src/github.rs | 73 ++++++++++++++---------- site/src/load.rs | 20 +++---- site/src/request_handlers/next_commit.rs | 14 +++-- 3 files changed, 59 insertions(+), 48 deletions(-) diff --git a/site/src/github.rs b/site/src/github.rs index b5828bd99..66a67c7cb 100644 --- a/site/src/github.rs +++ b/site/src/github.rs @@ -501,6 +501,8 @@ pub struct PostComment { pub body: String, } +/// Post messages to GitHub for all queued commits that have +/// not yet been marked as completed. pub async fn post_finished(ctxt: &SiteCtxt) { // If the github token is not configured, do not run this -- we don't want // to mark things as complete without posting the comment. @@ -509,7 +511,7 @@ pub async fn post_finished(ctxt: &SiteCtxt) { } let conn = ctxt.conn().await; let index = ctxt.index.load(); - let mut already_tested_commits = index + let mut known_commits = index .commits() .into_iter() .map(|c| c.sha.to_string()) @@ -519,36 +521,41 @@ pub async fn post_finished(ctxt: &SiteCtxt) { conn.queued_commits(), conn.in_progress_artifacts() ); - let master_commits = master_commits - .unwrap() - .into_iter() - .map(|c| c.sha) - .collect::>(); + let master_commits = if let Ok(mcs) = master_commits { + mcs.into_iter().map(|c| c.sha).collect::>() + } else { + // If we can't fetch master commits, return. + // We'll eventually try again later + return; + }; for aid in in_progress_artifacts { match aid { ArtifactId::Commit(c) => { - already_tested_commits.remove(&c.sha); + known_commits.remove(&c.sha); } ArtifactId::Tag(_) => { // do nothing, for now, though eventually we'll want an artifact queue } } } - for commit in queued_pr_commits + for queued_commit in queued_pr_commits .into_iter() - .filter(|c| already_tested_commits.contains(&c.sha)) + .filter(|c| known_commits.contains(&c.sha)) { - if let Some(completed) = conn.mark_complete(&commit.sha).await { - assert_eq!(completed, commit); + if let Some(completed) = conn.mark_complete(&queued_commit.sha).await { + assert_eq!(completed, queued_commit); - let is_master_commit = master_commits.contains(&commit.sha); - post_comparison_comment(commit, ctxt, is_master_commit).await; + let is_master_commit = master_commits.contains(&queued_commit.sha); + post_comparison_comment(ctxt, queued_commit, is_master_commit).await; } } } -async fn post_comparison_comment(commit: QueuedCommit, ctxt: &SiteCtxt, is_master_commit: bool) { +/// Posts a comment to GitHub summarizing the comparison of the queued commit with its parent +/// +/// `is_master_commit` is used to differentiate messages for try runs and post-merge runs. +async fn post_comparison_comment(ctxt: &SiteCtxt, commit: QueuedCommit, is_master_commit: bool) { let comparison_url = format!( "https://perf.rust-lang.org/compare.html?start={}&end={}", commit.parent_sha, commit.sha @@ -559,6 +566,13 @@ async fn post_comparison_comment(commit: QueuedCommit, ctxt: &SiteCtxt, is_maste Some(Direction::Regression | Direction::Mixed) => "+perf-regression", Some(Direction::Improvement) | None => "-perf-regression", }; + let rollup_msg = if is_master_commit { + "" + } else { + "Benchmarking this pull request likely means that it is \ +perf-sensitive, so we're automatically marking it as not fit \ +for rolling up. " + }; let next_steps_msg = direction .map(|d| { format!( @@ -577,19 +591,12 @@ async fn post_comparison_comment(commit: QueuedCommit, ctxt: &SiteCtxt, is_maste `@rustbot label: +perf-regression-triaged` along with \ sufficient written justification. If you cannot justify the regressions \ please fix the regressions (either in this PR if it's not yet merged or \ - in another PR) and do another perf run.", + in another PR), and then add the `perf-regression-triaged` label to this PR.", Direction::Improvement => "", } ) }) .unwrap_or(String::new()); - let rollup_msg = if is_master_commit { - "" - } else { - "Benchmarking this pull request likely means that it is \ -perf-sensitive, so we're automatically marking it as not fit \ -for rolling up. " - }; let bors_msg = if is_master_commit { "" } else { @@ -599,14 +606,20 @@ for rolling up. " &ctxt.config, commit.pr, format!( - "Finished benchmarking commit ({}): [comparison url]({}). - -**Summary**: {} - -{}{} -{} -@rustbot label: +S-waiting-on-review -S-waiting-on-perf {}", - commit.sha, comparison_url, summary, rollup_msg, next_steps_msg, bors_msg, label + "Finished benchmarking commit ({sha}): [comparison url]({url}). + +**Summary**: {summary} + +{rollup}{next_steps} +{bors} +@rustbot label: +S-waiting-on-review -S-waiting-on-perf {label}", + sha = commit.sha, + url = comparison_url, + summary = summary, + rollup = rollup_msg, + next_steps = next_steps_msg, + bors = bors_msg, + label = label ), ) .await; diff --git a/site/src/load.rs b/site/src/load.rs index 756ec20a2..fb6d2c82b 100644 --- a/site/src/load.rs +++ b/site/src/load.rs @@ -35,16 +35,6 @@ pub enum MissingReason { InProgress(Option>), } -impl MissingReason { - /// If the commit is a master commit get its PR and parent_sha - pub fn master_commit_pr_and_parent(&self) -> Option<(u32, &str)> { - match self { - Self::Master { pr, parent_sha } => Some((*pr, parent_sha)), - _ => None, - } - } -} - #[derive(Clone, Deserialize, Serialize, Debug, PartialEq, Eq)] pub struct TryCommit { pub sha: String, @@ -156,9 +146,10 @@ impl SiteCtxt { self.pool.connection().await } + /// Returns the not yet tested commits pub async fn missing_commits(&self) -> Vec<(Commit, MissingReason)> { let conn = self.conn().await; - let (master_commits, queued_try_commits, in_progress_artifacts) = futures::join!( + let (master_commits, queued_pr_commits, in_progress_artifacts) = futures::join!( collector::master_commits(), conn.queued_commits(), conn.in_progress_artifacts() @@ -195,7 +186,7 @@ impl SiteCtxt { .collect::>(); master_commits.reverse(); - let mut missing = Vec::with_capacity(queued_try_commits.len() * 2 + master_commits.len()); // Two commits per every try commit and all master commits + let mut missing = Vec::with_capacity(queued_pr_commits.len() * 2 + master_commits.len()); // Two commits per every try commit and all master commits for database::QueuedCommit { sha, parent_sha, @@ -203,7 +194,10 @@ impl SiteCtxt { include, exclude, runs, - } in queued_try_commits + } in queued_pr_commits + .into_iter() + // filter out any queued PR master commits (leaving only try commits) + .filter(|c| !master_commits.iter().any(|(mc, _)| mc.sha == c.sha)) { // Enqueue the `TryParent` commit before the `TryCommit` itself, so that // all of the `try` run's data is complete when the benchmark results diff --git a/site/src/request_handlers/next_commit.rs b/site/src/request_handlers/next_commit.rs index 34e645bf2..def4b17e5 100644 --- a/site/src/request_handlers/next_commit.rs +++ b/site/src/request_handlers/next_commit.rs @@ -1,16 +1,18 @@ -use crate::load::SiteCtxt; +use crate::load::{MissingReason, SiteCtxt}; use collector::api::next_commit; use std::sync::Arc; pub async fn handle_next_commit(ctxt: Arc) -> next_commit::Response { - let commit = ctxt.missing_commits().await.into_iter().next(); + let next_commit = ctxt.missing_commits().await.into_iter().next(); - let commit = if let Some((commit, missing_reason)) = commit { + let next_commit = if let Some((commit, missing_reason)) = next_commit { // If we're going to run a master commit next, make sure // it's been enqueued in the pull_request_build table - if let Some((pr, parent_sha)) = missing_reason.master_commit_pr_and_parent() { + if let MissingReason::Master { pr, ref parent_sha } = missing_reason { let conn = ctxt.conn().await; + // TODO: add capability of doing the following in one step + // to avoid possibile illegal inbetween states. conn.queue_pr(pr, None, None, None).await; conn.pr_attach_commit(pr, &commit.sha, parent_sha).await; } @@ -46,5 +48,7 @@ pub async fn handle_next_commit(ctxt: Arc) -> next_commit::Response { None }; - next_commit::Response { commit } + next_commit::Response { + commit: next_commit, + } } From b2383e9c216cbfb96be61cf6794ee8258610c529 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Mon, 6 Sep 2021 11:37:16 +0200 Subject: [PATCH 4/4] Add a test --- site/src/load.rs | 270 +++++++++++++++++++++++++++++++++-------------- 1 file changed, 189 insertions(+), 81 deletions(-) diff --git a/site/src/load.rs b/site/src/load.rs index fb6d2c82b..80d86d55d 100644 --- a/site/src/load.rs +++ b/site/src/load.rs @@ -160,103 +160,211 @@ impl SiteCtxt { .unwrap(); let index = self.index.load(); - let mut all_commits = index + let all_commits = index .commits() .iter() .map(|commit| commit.sha.clone()) .collect::>(); - let now = Utc::now(); - let mut master_commits = master_commits - .into_iter() - .filter(|c| now.signed_duration_since(c.time) < Duration::days(29)) - .map(|c| { - ( - Commit { - sha: c.sha, - date: Date(c.time), - }, - // All recent master commits should have an associated PR - MissingReason::Master { - pr: c.pr.unwrap_or(0), - parent_sha: c.parent_sha, - }, - ) - }) - .collect::>(); - master_commits.reverse(); + calculate_missing( + master_commits, + queued_pr_commits, + in_progress_artifacts, + all_commits, + ) + } +} - let mut missing = Vec::with_capacity(queued_pr_commits.len() * 2 + master_commits.len()); // Two commits per every try commit and all master commits - for database::QueuedCommit { - sha, - parent_sha, - pr, - include, - exclude, - runs, - } in queued_pr_commits - .into_iter() - // filter out any queued PR master commits (leaving only try commits) - .filter(|c| !master_commits.iter().any(|(mc, _)| mc.sha == c.sha)) - { - // Enqueue the `TryParent` commit before the `TryCommit` itself, so that - // all of the `try` run's data is complete when the benchmark results - // of that commit are available. - if let Some((try_parent, _)) = master_commits - .iter() - .find(|(m, _)| m.sha == parent_sha.as_str()) - { - missing.push((try_parent.clone(), MissingReason::TryParent)); - } - missing.push(( +fn calculate_missing( + master_commits: Vec, + queued_pr_commits: Vec, + in_progress_artifacts: Vec, + mut all_commits: HashSet, +) -> Vec<(Commit, MissingReason)> { + let now = Utc::now(); + let mut master_commits = master_commits + .into_iter() + .filter(|c| now.signed_duration_since(c.time) < Duration::days(29)) + .map(|c| { + ( Commit { - sha: sha.to_string(), - date: Date::ymd_hms(2001, 01, 01, 0, 0, 0), + sha: c.sha, + date: Date(c.time), }, - MissingReason::Try { - pr, - include, - exclude, - runs, + // All recent master commits should have an associated PR + MissingReason::Master { + pr: c.pr.unwrap_or(0), + parent_sha: c.parent_sha, }, - )); + ) + }) + .collect::>(); + master_commits.reverse(); + let mut missing = Vec::with_capacity(queued_pr_commits.len() * 2 + master_commits.len()); + for database::QueuedCommit { + sha, + parent_sha, + pr, + include, + exclude, + runs, + } in queued_pr_commits + .into_iter() + // filter out any queued PR master commits (leaving only try commits) + .filter(|c| !master_commits.iter().any(|(mc, _)| mc.sha == c.sha)) + { + // Enqueue the `TryParent` commit before the `TryCommit` itself, so that + // all of the `try` run's data is complete when the benchmark results + // of that commit are available. + if let Some((try_parent, _)) = master_commits + .iter() + .find(|(m, _)| m.sha == parent_sha.as_str()) + { + missing.push((try_parent.clone(), MissingReason::TryParent)); } - missing.extend(master_commits); - - for aid in in_progress_artifacts { - match aid { - ArtifactId::Commit(c) => { - let previous = missing - .iter() - .find(|(i, _)| i.sha == c.sha) - .map(|v| Box::new(v.1.clone())); - all_commits.remove(&c.sha); - missing.insert(0, (c, MissingReason::InProgress(previous))); - } - ArtifactId::Tag(_) => { - // do nothing, for now, though eventually we'll want an artifact queue - } + missing.push(( + Commit { + sha: sha.to_string(), + date: Date::ymd_hms(2001, 01, 01, 0, 0, 0), + }, + MissingReason::Try { + pr, + include, + exclude, + runs, + }, + )); + } + missing.extend(master_commits); + for aid in in_progress_artifacts { + match aid { + ArtifactId::Commit(c) => { + let previous = missing + .iter() + .find(|(i, _)| i.sha == c.sha) + .map(|v| Box::new(v.1.clone())); + all_commits.remove(&c.sha); + missing.insert(0, (c, MissingReason::InProgress(previous))); } - } - - let mut already_tested = HashSet::with_capacity(all_commits.len()); - already_tested.extend(all_commits); - - // Remove commits from missing that have already been tested - // FIXME: replace with Vec::drain_filter when it stabilizes - let mut i = 0; - while i != missing.len() { - if !already_tested.insert(missing[i].0.sha.clone()) { - missing.remove(i); - } else { - i += 1; + ArtifactId::Tag(_) => { + // do nothing, for now, though eventually we'll want an artifact queue } } - - missing } + let mut already_tested = HashSet::with_capacity(all_commits.len()); + already_tested.extend(all_commits); + let mut i = 0; + while i != missing.len() { + if !already_tested.insert(missing[i].0.sha.clone()) { + missing.remove(i); + } else { + i += 1; + } + } + missing } /// One decimal place rounded percent #[derive(Debug, Copy, Clone, PartialEq, Serialize, Deserialize)] pub struct Percent(#[serde(with = "collector::round_float")] pub f64); + +#[cfg(test)] +mod tests { + use std::str::FromStr; + + use collector::MasterCommit; + use database::QueuedCommit; + + use super::*; + #[test] + fn calculates_missing_correct() { + let time = chrono::DateTime::from_str("2021-09-01T00:00:00.000Z").unwrap(); + let master_commits = vec![ + // A not yet tested commit + MasterCommit { + sha: "123".into(), + parent_sha: "345".into(), + pr: Some(11), + time, + }, + // An already tested commit + MasterCommit { + sha: "abc".into(), + parent_sha: "def".into(), + pr: Some(90), + time, + }, + // A queued PR commit + MasterCommit { + sha: "foo".into(), + parent_sha: "bar".into(), + pr: Some(77), + time, + }, + ]; + let queued_pr_commits = vec![ + // A master commit + QueuedCommit { + sha: "foo".into(), + parent_sha: "bar".into(), + pr: 77, + include: None, + exclude: None, + runs: None, + }, + // A try run + QueuedCommit { + sha: "baz".into(), + parent_sha: "foo".into(), + pr: 101, + include: None, + exclude: None, + runs: None, + }, + ]; + let in_progress_artifacts = vec![]; + let mut all_commits = HashSet::new(); + all_commits.insert(master_commits[1].sha.clone()); + + let expected = vec![ + ( + Commit { + sha: "foo".into(), + date: database::Date(time), + }, + MissingReason::TryParent, + ), + ( + Commit { + sha: "baz".into(), + date: database::Date(time), + }, + MissingReason::Try { + pr: 101, + include: None, + exclude: None, + runs: None, + }, + ), + ( + Commit { + sha: "123".into(), + date: database::Date(time), + }, + MissingReason::Master { + pr: 11, + parent_sha: "345".into(), + }, + ), + ]; + assert_eq!( + expected, + calculate_missing( + master_commits, + queued_pr_commits, + in_progress_artifacts, + all_commits + ) + ); + } +}