From a79857c2bbca8ab43c6094560030f627e1903684 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 2 Oct 2026 12:14:06 -0400 Subject: [PATCH 1/3] One module for what a Run is asked: the flags as given, and the asks resolved, for each kind of run Co-Authored-By: Claude Opus 5.5 --- src/args.rs | 286 ++++++++---------------- src/asks.rs | 579 ++++++++++++++++++++++++++++++++++++++++++++++++ src/config.rs | 53 ----- src/main.rs | 125 +++-------- src/run.rs | 54 ++--- src/spec_run.rs | 19 -- 6 files changed, 714 insertions(+), 402 deletions(-) create mode 100644 src/asks.rs diff --git a/src/args.rs b/src/args.rs index 4dcfc7eca..63e90bcd1 100644 --- a/src/args.rs +++ b/src/args.rs @@ -7,6 +7,7 @@ use std::num::NonZeroUsize; use anyhow::{Context, Result, bail}; +use crate::asks::Flags; use crate::base_fix::BaseFixAsk; use crate::child_run::Kind; use crate::issue::IssueUrl; @@ -39,12 +40,12 @@ pub struct ArchitectArgs { /// The Base branch `base ` named, if given; without it, the /// branch checked out in the Launch directory is the Base branch. pub base: Option, - /// What `email` or `no-email` asked for, if either was given; without - /// one, the User config decides. - pub email: Option, - /// The flags for the Spec run or Run the plan is dispatched as, or none - /// with [`PLAN_ONLY`], which dispatches nothing. - pub dispatch: Option, + /// The flags it shares with a Run: `email` and `no-email` for its own + /// Run notification, the rest for the Spec run or Run the plan is + /// dispatched as, which are never given with [`PLAN_ONLY`]. + pub flags: Flags, + /// Whether [`PLAN_ONLY`] was given: the plan is dispatched as nothing. + pub plan_only: bool, } /// A Pickup run's arguments. @@ -53,20 +54,10 @@ pub struct PickupArgs { /// The Base branch `base ` named, if given; without it, the /// branch checked out in the Launch directory is the Base branch. pub base: Option, - /// What `email` or `no-email` asked for, if either was given; without - /// one, the User config decides. - pub email: Option, - /// The flags for the Spec run or Run the Ready issue is dispatched as. - pub dispatch: DispatchArgs, -} - -/// What the flags of an Architect run or a Pickup run ask of the Spec run or -/// Run it dispatches, each meaning what it does in [`RunArgs`]. -#[derive(Debug, Default, PartialEq, Eq)] -pub struct DispatchArgs { - pub goal: Option, - pub parallel: Option, - pub base_fix: Option, + /// The flags it shares with a Run: `email` and `no-email` for its own + /// Run notification, the rest for the Spec run or Run the Ready issue is + /// dispatched as. + pub flags: Flags, } /// The word that lets a Run start a Base fix, which a Spec run passes on to @@ -90,18 +81,9 @@ pub const BASE_FIX_INTO: &str = "--base-fix-into"; /// A Run's arguments. pub struct RunArgs { pub issue: IssueUrl, - /// The goal `merge` or `no-merge` asked for, if either was given; without - /// one, the User config decides. - pub goal: Option, - /// What `email` or `no-email` asked for, if either was given; without - /// one, the User config decides. - pub email: Option, - /// How many Tickets a Spec run runs at once, if `parallel ` was - /// given; without it, the User config decides. - pub parallel: Option, - /// What `base-fix` or `no-base-fix` asked for, if either was given; - /// without one, the User config decides. - pub base_fix: Option, + /// Its flags, as given: for each one the command left out, the User + /// config decides. + pub flags: Flags, /// What the Run is, if another thirdshift started it, given with /// [`SPEC_BRANCH`] or [`BASE_FIX_INTO`]. pub child: Option, @@ -138,11 +120,11 @@ pub fn parse(args: &[String]) -> Result { _ => {} } let mut issue = None; - let mut flags = RunFlags::default(); + let mut flags = Flags::default(); let mut child = None; let mut args = args.iter().peekable(); while let Some(arg) = args.next() { - if flags.take(arg, &mut args)? { + if take_flag(&mut flags, arg, &mut args)? { continue; } match arg.as_str() { @@ -175,41 +157,11 @@ pub fn parse(args: &[String]) -> Result { }; Ok(Command::Run(RunArgs { issue, - goal: flags.goal, - email: flags.email, - parallel: flags.parallel, - base_fix: flags.base_fix, + flags, child, })) } -/// The command that starts the Run on `issue` again as its command asked for -/// it, with `goal`, `email` and `parallel` as [`RunArgs`] has them, and with -/// `base-fix` added. -pub fn retry_with_base_fix( - issue: &IssueUrl, - goal: Option, - email: Option<&NotificationAsk>, - parallel: Option, -) -> String { - let mut command = format!("thirdshift {}", issue.url); - match goal { - Some(Goal::Merged) => command += " merge", - Some(Goal::ReadyForReview) => command += " --no-merge", - None => {} - } - match email { - Some(NotificationAsk::Send(Some(to))) => command += &format!(" --email {to}"), - Some(NotificationAsk::Send(None)) => command += " --email", - Some(NotificationAsk::Skip) => command += " --no-email", - None => {} - } - if let Some(parallel) = parallel { - command += &format!(" parallel {parallel}"); - } - command + " " + BASE_FIX -} - /// Parse the arguments after `architect`: at most one focus, and its flags, /// each at most once, in any order. `base` must be followed by the Base /// branch. [`PLAN_ONLY`] dispatches nothing, so the @@ -223,10 +175,10 @@ fn parse_architect(args: &[String]) -> Result { let mut focus = None; let mut base = None; let mut plan_only = false; - let mut flags = RunFlags::default(); + let mut flags = Flags::default(); let mut args = args.iter().peekable(); while let Some(arg) = args.next() { - if flags.take(arg, &mut args)? { + if take_flag(&mut flags, arg, &mut args)? { continue; } match arg.as_str() { @@ -243,8 +195,8 @@ fn parse_architect(args: &[String]) -> Result { _ => focus = Some(arg.clone()), } } - let (email, dispatch) = flags.for_dispatch(); - if plan_only && dispatch != DispatchArgs::default() { + let for_dispatch = flags.goal.is_some() || flags.parallel.is_some() || flags.base_fix.is_some(); + if plan_only && for_dispatch { bail!( "merge, no-merge, parallel, base-fix and no-base-fix can't be used with \ {PLAN_ONLY}: it dispatches no run for them to apply to" @@ -253,8 +205,8 @@ fn parse_architect(args: &[String]) -> Result { Ok(ArchitectArgs { focus, base, - email, - dispatch: (!plan_only).then_some(dispatch), + flags, + plan_only, }) } @@ -262,10 +214,10 @@ fn parse_architect(args: &[String]) -> Result { /// order, and nothing else. `base` must be followed by the Base branch. fn parse_pickup(args: &[String]) -> Result { let mut base = None; - let mut flags = RunFlags::default(); + let mut flags = Flags::default(); let mut args = args.iter().peekable(); while let Some(arg) = args.next() { - if flags.take(arg, &mut args)? { + if take_flag(&mut flags, arg, &mut args)? { continue; } match arg.as_str() { @@ -273,69 +225,45 @@ fn parse_pickup(args: &[String]) -> Result { _ => bail!("unexpected argument after pickup: {arg}"), } } - let (email, dispatch) = flags.for_dispatch(); - Ok(PickupArgs { - base, - email, - dispatch, - }) -} - -/// The flags a Run, an Architect run and a Pickup run all take, as given so -/// far: each field is what [`RunArgs`] says of the one it becomes. -#[derive(Default)] -struct RunFlags { - goal: Option, - email: Option, - parallel: Option, - base_fix: Option, + Ok(PickupArgs { base, flags }) } -impl RunFlags { - /// The flags as an Architect run or a Pickup run takes them: what was - /// asked about its Run notification, and the flags for the run it - /// dispatches. - fn for_dispatch(self) -> (Option, DispatchArgs) { - let dispatch = DispatchArgs { - goal: self.goal, - parallel: self.parallel, - base_fix: self.base_fix, - }; - (self.email, dispatch) - } - - /// Record what `arg` asks for, if it is one of these flags, with or - /// without its dashes, taking from `rest` the address after `email`, if - /// one is there, and the number after `parallel`. False, taking nothing, - /// if `arg` is none of them. - fn take<'a>( - &mut self, - arg: &str, - rest: &mut Peekable>, - ) -> Result { - match arg { - "merge" | "--merge" => ask_once(&mut self.goal, Goal::Merged, arg, MERGE_FLAGS)?, - "no-merge" | "--no-merge" => { - ask_once(&mut self.goal, Goal::ReadyForReview, arg, MERGE_FLAGS)? - } - "email" | "--email" => { - let to = rest.next_if(|next| is_address(next)).cloned(); - ask_once(&mut self.email, NotificationAsk::Send(to), arg, EMAIL_FLAGS)? - } - "no-email" | "--no-email" => { - ask_once(&mut self.email, NotificationAsk::Skip, arg, EMAIL_FLAGS)? - } - "parallel" | "--parallel" => ask_parallel(&mut self.parallel, arg, rest.next())?, - BASE_FIX | "--base-fix" => { - ask_once(&mut self.base_fix, BaseFixAsk::Allow, arg, BASE_FIX_FLAGS)? - } - "no-base-fix" | "--no-base-fix" => { - ask_once(&mut self.base_fix, BaseFixAsk::Forbid, arg, BASE_FIX_FLAGS)? - } - _ => return Ok(false), - } - Ok(true) - } +/// Record in `flags` what `arg` asks for, if it is one of the flags a Run, +/// an Architect run and a Pickup run all take, with or without its dashes, +/// taking from `rest` the address after `email`, if one is there, and the +/// number after `parallel`. False, taking nothing, if `arg` is none of them. +fn take_flag<'a>( + flags: &mut Flags, + arg: &str, + rest: &mut Peekable>, +) -> Result { + match arg { + "merge" | "--merge" => ask_once(&mut flags.goal, Goal::Merged, arg, MERGE_FLAGS)?, + "no-merge" | "--no-merge" => { + ask_once(&mut flags.goal, Goal::ReadyForReview, arg, MERGE_FLAGS)? + } + "email" | "--email" => { + let to = rest.next_if(|next| is_address(next)).cloned(); + ask_once( + &mut flags.email, + NotificationAsk::Send(to), + arg, + EMAIL_FLAGS, + )? + } + "no-email" | "--no-email" => { + ask_once(&mut flags.email, NotificationAsk::Skip, arg, EMAIL_FLAGS)? + } + "parallel" | "--parallel" => ask_parallel(&mut flags.parallel, arg, rest.next())?, + BASE_FIX | "--base-fix" => { + ask_once(&mut flags.base_fix, BaseFixAsk::Allow, arg, BASE_FIX_FLAGS)? + } + "no-base-fix" | "--no-base-fix" => { + ask_once(&mut flags.base_fix, BaseFixAsk::Forbid, arg, BASE_FIX_FLAGS)? + } + _ => return Ok(false), + } + Ok(true) } const MERGE_FLAGS: &str = "merge and no-merge"; @@ -440,7 +368,7 @@ mod tests { ] { let architect_args = architect_args(&args); assert_eq!(architect_args.focus, expected, "{args:?}"); - assert_eq!(architect_args.dispatch, None, "{args:?}"); + assert!(architect_args.plan_only, "{args:?}"); } } @@ -455,11 +383,8 @@ mod tests { ] { let architect_args = architect_args(&args); assert_eq!(architect_args.focus, focus, "{args:?}"); - assert_eq!( - architect_args.dispatch, - Some(DispatchArgs::default()), - "{args:?}" - ); + assert!(!architect_args.plan_only, "{args:?}"); + assert_eq!(architect_args.flags, Flags::default(), "{args:?}"); } } @@ -487,12 +412,12 @@ mod tests { ), ] { assert_eq!( - architect_args(&args).dispatch, - Some(DispatchArgs { + architect_args(&args).flags, + Flags { goal, parallel, - base_fix: None - }), + ..Flags::default() + }, "{args:?}" ); } @@ -513,8 +438,7 @@ mod tests { ), ] { let architect_args = architect_args(&args); - let dispatch = architect_args.dispatch.expect("a dispatch"); - assert_eq!(dispatch.base_fix, base_fix, "{args:?}"); + assert_eq!(architect_args.flags.base_fix, base_fix, "{args:?}"); let focus = args.contains(&"the Spec run").then_some("the Spec run"); assert_eq!(architect_args.focus.as_deref(), focus, "{args:?}"); } @@ -549,7 +473,7 @@ mod tests { ), ] { let architect_args = architect_args(&args); - assert_eq!(architect_args.email, email, "{args:?}"); + assert_eq!(architect_args.flags.email, email, "{args:?}"); assert_eq!(architect_args.focus, focus, "{args:?}"); } } @@ -670,7 +594,7 @@ mod tests { ] { let architect_args = architect_args(&args); assert_eq!(architect_args.base.as_deref(), Some("develop"), "{args:?}"); - assert_eq!(architect_args.dispatch, None, "{args:?}"); + assert!(architect_args.plan_only, "{args:?}"); } } @@ -779,15 +703,14 @@ mod tests { fn pickup_takes_each_of_its_flags_with_or_without_dashes_in_any_order() { let none = PickupArgs { base: None, - email: None, - dispatch: DispatchArgs::default(), + flags: Flags::default(), }; assert_eq!(pickup_args(&["pickup"]), none); let all = PickupArgs { base: Some("develop".to_string()), - email: Some(NotificationAsk::Send(Some("me@example.com".to_string()))), - dispatch: DispatchArgs { + flags: Flags { goal: Some(Goal::Merged), + email: Some(NotificationAsk::Send(Some("me@example.com".to_string()))), parallel: NonZeroUsize::new(2), base_fix: Some(BaseFixAsk::Allow), }, @@ -820,9 +743,9 @@ mod tests { } let cautious = PickupArgs { base: None, - email: Some(NotificationAsk::Skip), - dispatch: DispatchArgs { + flags: Flags { goal: Some(Goal::ReadyForReview), + email: Some(NotificationAsk::Skip), parallel: None, base_fix: Some(BaseFixAsk::Forbid), }, @@ -834,8 +757,8 @@ mod tests { assert_eq!(pickup_args(&args), cautious, "{args:?}"); } let bare_email = pickup_args(&["pickup", "email", "merge"]); - assert_eq!(bare_email.email, Some(NotificationAsk::Send(None))); - assert_eq!(bare_email.dispatch.goal, Some(Goal::Merged)); + assert_eq!(bare_email.flags.email, Some(NotificationAsk::Send(None))); + assert_eq!(bare_email.flags.goal, Some(Goal::Merged)); } #[test] @@ -947,7 +870,7 @@ mod tests { (vec!["--email", "flag@example.com", URL], to_flag_address()), ] { let run_args = run_args(&args); - assert_eq!(run_args.email, Some(asked), "{args:?}"); + assert_eq!(run_args.flags.email, Some(asked), "{args:?}"); assert_eq!(run_args.issue.url, URL, "{args:?}"); } } @@ -970,8 +893,8 @@ mod tests { ] { let run_args = run_args(&args); assert_eq!(run_args.issue.url, URL, "{args:?}"); - assert_eq!(run_args.email, Some(asked), "{args:?}"); - assert_eq!(run_args.goal, goal, "{args:?}"); + assert_eq!(run_args.flags.email, Some(asked), "{args:?}"); + assert_eq!(run_args.flags.goal, goal, "{args:?}"); } } @@ -979,54 +902,19 @@ mod tests { fn no_email_asks_for_no_notification_with_or_without_dashes() { for flag in ["no-email", "--no-email"] { assert_eq!( - run_args(&[flag, URL]).email, + run_args(&[flag, URL]).flags.email, Some(NotificationAsk::Skip), "{flag}" ); } } - #[test] - fn the_retry_command_is_the_runs_own_flags_and_issue_url_with_base_fix_added() { - for (args, retry) in [ - (vec![URL], format!("thirdshift {URL} base-fix")), - ( - vec!["--merge", URL, "email"], - format!("thirdshift {URL} merge --email base-fix"), - ), - ( - vec!["no-merge", "--no-email", URL, "--parallel", "2"], - format!("thirdshift {URL} --no-merge --no-email parallel 2 base-fix"), - ), - ( - vec![URL, "email", "me@example.com"], - format!("thirdshift {URL} --email me@example.com base-fix"), - ), - ] { - let run = run_args(&args); - assert_eq!( - retry_with_base_fix(&run.issue, run.goal, run.email.as_ref(), run.parallel), - retry, - "{args:?}" - ); - // The command it gives asks for what the Run was asked for. - let words: Vec<&str> = retry.split(' ').skip(1).collect(); - let again = run_args(&words); - assert_eq!(again.base_fix, Some(BaseFixAsk::Allow), "{retry}"); - assert_eq!( - (again.issue.url, again.goal, again.email, again.parallel), - (run.issue.url, run.goal, run.email, run.parallel), - "{retry}" - ); - } - } - #[test] fn a_tickets_run_is_given_the_command_to_offer_a_base_fix_with() { let retry = format!("thirdshift {URL} base-fix"); let run = run_args(&["--spec-branch", "issue-7", "--offer-base-fix", &retry, URL]); - assert_eq!(run.base_fix, Some(BaseFixAsk::Undecided { retry })); + assert_eq!(run.flags.base_fix, Some(BaseFixAsk::Undecided { retry })); assert_eq!( rejection(&[URL, "--offer-base-fix"]), "missing command to offer" @@ -1037,7 +925,11 @@ mod tests { fn parallel_takes_the_number_after_it_with_or_without_dashes() { for flag in ["parallel", "--parallel"] { for args in [[flag, "1", URL], [URL, flag, "1"]] { - assert_eq!(run_args(&args).parallel, NonZeroUsize::new(1), "{args:?}"); + assert_eq!( + run_args(&args).flags.parallel, + NonZeroUsize::new(1), + "{args:?}" + ); } } } diff --git a/src/asks.rs b/src/asks.rs new file mode 100644 index 000000000..4568821cf --- /dev/null +++ b/src/asks.rs @@ -0,0 +1,579 @@ +//! What a run is asked: its command's flags, and, for whatever the command +//! says nothing about, the User config. The argument parser fills in the +//! [`Flags`] as the command gave them; what they come to, for each kind of +//! run, is its [`Asks`]. + +use std::num::NonZeroUsize; + +use crate::args; +use crate::base_fix::BaseFixAsk; +use crate::child_run::Kind; +use crate::config::UserConfig; +use crate::issue::IssueUrl; +use crate::notification::NotificationAsk; +use crate::run::Goal; + +/// The flags a Run, an Architect run and a Pickup run share, as the command +/// gave them: each is none if the command said nothing about it. +#[derive(Debug, Default, Clone, PartialEq, Eq)] +pub struct Flags { + /// The goal `merge` or `no-merge` asked for. + pub goal: Option, + /// What `email` or `no-email` asked for. + pub email: Option, + /// How many Tickets a Spec run runs at once, as `parallel ` gave it. + pub parallel: Option, + /// What `base-fix` or `no-base-fix` asked for, or, for a Ticket's Run, + /// the command its Spec run gave it to offer a Base fix with. + pub base_fix: Option, +} + +/// Everything a Run, or a Spec run, is asked, resolved. +#[derive(Debug, PartialEq, Eq)] +pub struct Asks { + /// Whether it is a Merge run. + pub goal: Goal, + /// What it is asked about its Run notification. + pub notification: NotificationAsk, + /// How many Tickets a Spec run runs at once. + pub tickets_at_once: NonZeroUsize, + /// Whether the command asked for that with `parallel `, rather than + /// the User config deciding: only a Spec can be asked it. + pub parallel_asked: bool, + /// What it is asked about a Base fix. + pub base_fix: BaseFixAsk, + /// Whether it first brings the Launch directory's checkout of the Base + /// branch up to date with origin. + pub launch_pull: bool, +} + +impl Asks { + /// What the Run, or the Spec run, started on `issue`'s URL is asked: by + /// `flags`, else by `config`. + /// + /// Unless another thirdshift started it, as the child Run `child`, a + /// Ticket's Run in a Spec run or a Base fix. A child Run is always a + /// Merge run, sends no Run notification and leaves the Launch directory + /// alone, whatever `flags` and `config` say: those are for what started + /// it. It starts no Base fix either, unless `flags` allow one or give it + /// the command to offer one with. + pub fn of_run( + issue: &IssueUrl, + flags: &Flags, + child: Option<&Kind>, + config: &UserConfig, + ) -> Asks { + let asks = Asks::resolved(issue, flags, config); + match child { + None => asks, + Some(_) => Asks { + goal: Goal::Merged, + notification: NotificationAsk::Skip, + base_fix: flags.base_fix.clone().unwrap_or(BaseFixAsk::Forbid), + launch_pull: false, + ..asks + }, + } + } + + /// What the Spec run or Run an Architect run dispatches its Architect + /// plan `plan` as is asked: what [`Asks::of_run`] would ask a Run started + /// on the plan's URL with the Architect run's `flags`, except that it + /// sends no Run notification of its own. The Architect run sends the + /// one, as [`Flags::notification`] asks. The command a Base fix is + /// offered with retries the plan as a Run of its own: another Architect + /// run would start a new review instead. + pub fn of_architect_plan(plan: &IssueUrl, flags: &Flags, config: &UserConfig) -> Asks { + Asks { + notification: NotificationAsk::Skip, + ..Asks::resolved(plan, flags, config) + } + } + + /// What the Spec run or Run a Pickup run dispatches the Ready issue + /// `issue` as is asked, as [`Asks::of_architect_plan`] has it for a + /// plan, with one difference: `parallel` is ignored for an issue that is + /// not a Spec, as `is_spec` says, since the command can't know which + /// the Pickup run will take. It is left out of the command a Base fix is + /// offered with too, which retries the issue as a Run of its own: + /// another Pickup run never takes an issue that was started. + pub fn of_ready_issue( + issue: &IssueUrl, + is_spec: bool, + flags: &Flags, + config: &UserConfig, + ) -> Asks { + let flags = Flags { + parallel: flags.parallel.filter(|_| is_spec), + ..flags.clone() + }; + Asks::of_architect_plan(issue, &flags, config) + } + + /// Each ask as `flags` gave it, else as `config` says. Where neither + /// decided about a Base fix, the command that retries `issue` is the one + /// on its Issue URL with `flags`. + fn resolved(issue: &IssueUrl, flags: &Flags, config: &UserConfig) -> Asks { + let goal = flags.goal.unwrap_or(if config.merge_always { + Goal::Merged + } else { + Goal::ReadyForReview + }); + let base_fix = flags.base_fix.clone().unwrap_or_else(|| { + if config.base_fix { + BaseFixAsk::Allow + } else { + let retry = retry_with_base_fix(issue, flags); + BaseFixAsk::Undecided { retry } + } + }); + Asks { + goal, + notification: flags.notification(config), + tickets_at_once: flags.parallel.unwrap_or(config.spec_parallel), + parallel_asked: flags.parallel.is_some(), + base_fix, + launch_pull: config.launch_pull, + } + } +} + +impl Flags { + /// What a run with these flags is asked about its own Run notification: + /// what `email` or `no-email` asked for, else what `config` says. It is + /// a Run's ask, and an Architect run's or a Pickup run's for the one + /// notification it sends itself. + pub fn notification(&self, config: &UserConfig) -> NotificationAsk { + self.email.clone().unwrap_or(if config.email.always { + NotificationAsk::Send(None) + } else { + NotificationAsk::Skip + }) + } +} + +/// The command that starts the Run on `issue` again as `flags` asked for it, +/// with `base-fix` added. +fn retry_with_base_fix(issue: &IssueUrl, flags: &Flags) -> String { + let mut command = format!("thirdshift {}", issue.url); + match flags.goal { + Some(Goal::Merged) => command += " merge", + Some(Goal::ReadyForReview) => command += " --no-merge", + None => {} + } + match &flags.email { + Some(NotificationAsk::Send(Some(to))) => command += &format!(" --email {to}"), + Some(NotificationAsk::Send(None)) => command += " --email", + Some(NotificationAsk::Skip) => command += " --no-email", + None => {} + } + if let Some(parallel) = flags.parallel { + command += &format!(" parallel {parallel}"); + } + command + " " + args::BASE_FIX +} + +#[cfg(test)] +mod tests { + use std::path::PathBuf; + + use super::*; + use crate::config::EmailSettings; + + const URL: &str = "https://github.com/acme/widgets/issues/7"; + + fn issue() -> IssueUrl { + IssueUrl::parse(URL).unwrap() + } + + fn n(n: usize) -> NonZeroUsize { + NonZeroUsize::new(n).unwrap() + } + + fn to(address: &str) -> NotificationAsk { + NotificationAsk::Send(Some(address.to_string())) + } + + /// A User config that sets nothing: three Tickets at once is its own + /// default. + fn no_settings() -> UserConfig { + UserConfig { + merge_always: false, + base_fix: false, + launch_pull: false, + logs_dir: PathBuf::from("/home/me/.thirdshift/logs"), + email: EmailSettings::default(), + spec_parallel: n(3), + pickup_limit: n(3), + } + } + + /// A User config that sets every default a Run can be asked by. + fn every_setting() -> UserConfig { + UserConfig { + merge_always: true, + base_fix: true, + launch_pull: true, + email: EmailSettings { + always: true, + to: Some("config@example.com".to_string()), + from: None, + }, + spec_parallel: n(5), + ..no_settings() + } + } + + /// The command that starts the Run on [`URL`] again with a Base fix + /// allowed, `flags` being its other flags as the command would spell + /// them. + fn undecided(flags: &str) -> BaseFixAsk { + BaseFixAsk::Undecided { + retry: format!("thirdshift {URL}{flags} base-fix"), + } + } + + #[test] + fn a_run_with_neither_a_flag_nor_a_setting_does_only_what_a_run_does() { + let asks = Asks::of_run(&issue(), &Flags::default(), None, &no_settings()); + + assert_eq!( + asks, + Asks { + goal: Goal::ReadyForReview, + notification: NotificationAsk::Skip, + tickets_at_once: n(3), + parallel_asked: false, + base_fix: undecided(""), + launch_pull: false, + } + ); + } + + #[test] + fn the_user_config_decides_each_ask_the_command_says_nothing_about() { + let asks = Asks::of_run(&issue(), &Flags::default(), None, &every_setting()); + + assert_eq!( + asks, + Asks { + goal: Goal::Merged, + notification: NotificationAsk::Send(None), + tickets_at_once: n(5), + parallel_asked: false, + base_fix: BaseFixAsk::Allow, + launch_pull: true, + } + ); + } + + #[test] + fn each_flag_decides_its_ask_whatever_the_user_config_says() { + let against = Flags { + goal: Some(Goal::ReadyForReview), + email: Some(NotificationAsk::Skip), + parallel: Some(n(2)), + base_fix: Some(BaseFixAsk::Forbid), + }; + let asks = Asks::of_run(&issue(), &against, None, &every_setting()); + assert_eq!( + asks, + Asks { + goal: Goal::ReadyForReview, + notification: NotificationAsk::Skip, + tickets_at_once: n(2), + parallel_asked: true, + base_fix: BaseFixAsk::Forbid, + // No flag sets it. + launch_pull: true, + } + ); + + let for_it = Flags { + goal: Some(Goal::Merged), + email: Some(to("flag@example.com")), + parallel: Some(n(2)), + base_fix: Some(BaseFixAsk::Allow), + }; + let asks = Asks::of_run(&issue(), &for_it, None, &no_settings()); + assert_eq!( + asks, + Asks { + goal: Goal::Merged, + notification: to("flag@example.com"), + tickets_at_once: n(2), + parallel_asked: true, + base_fix: BaseFixAsk::Allow, + launch_pull: false, + } + ); + } + + fn ticket() -> Kind { + Kind::Ticket { + spec_branch: "issue-20".to_string(), + } + } + + fn base_fix() -> Kind { + Kind::BaseFix { + base: "main".to_string(), + } + } + + #[test] + fn a_child_run_is_a_merge_run_that_sends_nothing_pulls_nothing_and_starts_no_base_fix() { + for child in [ticket(), base_fix()] { + for config in [no_settings(), every_setting()] { + let asks = Asks::of_run(&issue(), &Flags::default(), Some(&child), &config); + + assert_eq!(asks.goal, Goal::Merged, "{child:?}"); + assert_eq!(asks.notification, NotificationAsk::Skip, "{child:?}"); + assert!(!asks.launch_pull, "{child:?}"); + assert_eq!(asks.base_fix, BaseFixAsk::Forbid, "{child:?}"); + } + } + } + + #[test] + fn a_child_run_asks_about_a_base_fix_what_it_was_given() { + let offer = BaseFixAsk::Undecided { + retry: format!("thirdshift {URL} base-fix"), + }; + for given in [BaseFixAsk::Allow, offer] { + let flags = Flags { + base_fix: Some(given.clone()), + ..Flags::default() + }; + + let asks = Asks::of_run(&issue(), &flags, Some(&ticket()), &no_settings()); + + assert_eq!(asks.base_fix, given); + } + } + + #[test] + fn the_flags_a_child_run_cant_use_are_ignored() { + let flags = Flags { + goal: Some(Goal::ReadyForReview), + email: Some(to("flag@example.com")), + ..Flags::default() + }; + + let asks = Asks::of_run(&issue(), &flags, Some(&ticket()), &no_settings()); + + assert_eq!(asks.goal, Goal::Merged); + assert_eq!(asks.notification, NotificationAsk::Skip); + } + + /// Every flag an Architect run or a Pickup run takes for the run it + /// dispatches, with `email` for its own Run notification. + fn dispatch_flags() -> Flags { + Flags { + goal: Some(Goal::Merged), + email: Some(to("flag@example.com")), + parallel: Some(n(2)), + base_fix: None, + } + } + + #[test] + fn a_dispatched_architect_plan_is_asked_by_the_flags_else_the_user_config_and_sends_no_notification() + { + let asks = Asks::of_architect_plan(&issue(), &dispatch_flags(), &no_settings()); + assert_eq!( + asks, + Asks { + goal: Goal::Merged, + notification: NotificationAsk::Skip, + tickets_at_once: n(2), + parallel_asked: true, + base_fix: undecided(" merge --email flag@example.com parallel 2"), + launch_pull: false, + } + ); + + let asks = Asks::of_architect_plan(&issue(), &Flags::default(), &every_setting()); + assert_eq!( + asks, + Asks { + goal: Goal::Merged, + // Whatever `email.always` says: the Architect run sends it. + notification: NotificationAsk::Skip, + tickets_at_once: n(5), + parallel_asked: false, + base_fix: BaseFixAsk::Allow, + launch_pull: true, + } + ); + } + + #[test] + fn a_dispatched_ready_issue_that_is_a_spec_is_asked_what_a_dispatched_architect_plan_is() { + for (flags, config) in [ + (Flags::default(), every_setting()), + (dispatch_flags(), no_settings()), + ] { + let asks = Asks::of_ready_issue(&issue(), true, &flags, &config); + + assert_eq!( + asks, + Asks::of_architect_plan(&issue(), &flags, &config), + "{flags:?}" + ); + assert_eq!(asks.notification, NotificationAsk::Skip, "{flags:?}"); + } + } + + #[test] + fn parallel_is_ignored_for_a_dispatched_ready_issue_that_is_not_a_spec() { + let asks = Asks::of_ready_issue(&issue(), false, &dispatch_flags(), &every_setting()); + + assert!(!asks.parallel_asked); + assert_eq!(asks.tickets_at_once, n(5)); + + // Nor is it in the command that retries the issue, which would fail + // a Run on an issue that is not a Spec. + let asks = Asks::of_ready_issue(&issue(), false, &dispatch_flags(), &no_settings()); + assert_eq!(asks.base_fix, undecided(" merge --email flag@example.com")); + let asks = Asks::of_ready_issue(&issue(), true, &dispatch_flags(), &no_settings()); + assert_eq!( + asks.base_fix, + undecided(" merge --email flag@example.com parallel 2") + ); + } + + /// The Run the command `retry` starts, read by the argument parser. + fn parsed(retry: &str) -> args::RunArgs { + let words: Vec = retry.split(' ').skip(1).map(str::to_string).collect(); + match args::parse(&words) { + Ok(args::Command::Run(run)) => run, + Ok(_) => panic!("{retry}: not a Run"), + Err(error) => panic!("{retry}: {error:#}"), + } + } + + /// The command `asks` offer a Base fix with. + fn retry_of(asks: Asks) -> String { + match asks.base_fix { + BaseFixAsk::Undecided { retry } => retry, + decided => panic!("no command to offer: {decided:?}"), + } + } + + #[test] + fn the_retry_command_is_the_issue_url_and_the_flags_as_given_with_base_fix_added() { + let flags = |goal, email, parallel| Flags { + goal, + email, + parallel, + base_fix: None, + }; + for (flags, retry) in [ + (Flags::default(), format!("thirdshift {URL} base-fix")), + ( + flags(Some(Goal::Merged), Some(NotificationAsk::Send(None)), None), + format!("thirdshift {URL} merge --email base-fix"), + ), + ( + flags( + Some(Goal::ReadyForReview), + Some(NotificationAsk::Skip), + Some(n(2)), + ), + format!("thirdshift {URL} --no-merge --no-email parallel 2 base-fix"), + ), + ( + flags(None, Some(to("me@example.com")), None), + format!("thirdshift {URL} --email me@example.com base-fix"), + ), + ] { + let config = no_settings(); + let allowed = Flags { + base_fix: Some(BaseFixAsk::Allow), + ..flags.clone() + }; + // Each kind of run offers the same command: a Run on the Issue + // URL, whatever dispatched it. + for (kind, asks) in [ + ("a Run", Asks::of_run(&issue(), &flags, None, &config)), + ( + "an Architect plan", + Asks::of_architect_plan(&issue(), &flags, &config), + ), + ( + "a Ready issue that is a Spec", + Asks::of_ready_issue(&issue(), true, &flags, &config), + ), + ] { + let offered = retry_of(asks); + assert_eq!(offered, retry, "{kind}"); + // The command it gives asks for what the run was asked for. + let again = parsed(&offered); + assert_eq!(again.issue.url, URL, "{kind}: {offered}"); + assert_eq!(again.flags, allowed, "{kind}: {offered}"); + assert_eq!(again.child, None, "{kind}: {offered}"); + } + // Without the `parallel` that would fail its Run by hand. + let offered = retry_of(Asks::of_ready_issue(&issue(), false, &flags, &config)); + let again = parsed(&offered); + let without_parallel = Flags { + parallel: None, + ..allowed + }; + assert_eq!(again.flags, without_parallel, "{offered}"); + } + } + + #[test] + fn an_architect_run_or_a_pickup_run_is_asked_for_its_own_notification_as_a_run_is() { + for (email, config, asked) in [ + (None, no_settings(), NotificationAsk::Skip), + (None, every_setting(), NotificationAsk::Send(None)), + ( + Some(NotificationAsk::Skip), + every_setting(), + NotificationAsk::Skip, + ), + ( + Some(NotificationAsk::Send(None)), + no_settings(), + NotificationAsk::Send(None), + ), + ( + Some(to("flag@example.com")), + every_setting(), + to("flag@example.com"), + ), + ] { + let flags = Flags { + email, + ..dispatch_flags() + }; + + assert_eq!(flags.notification(&config), asked, "{flags:?}"); + } + } + + #[test] + fn an_address_after_the_email_flag_wins_over_email_always() { + let flags = Flags { + email: Some(to("flag@example.com")), + ..Flags::default() + }; + + let asks = Asks::of_run(&issue(), &flags, None, &every_setting()); + + assert_eq!(asks.notification, to("flag@example.com")); + } + + #[test] + fn email_to_alone_asks_for_no_notification() { + let mut config = no_settings(); + config.email.to = Some("config@example.com".to_string()); + + let asks = Asks::of_run(&issue(), &Flags::default(), None, &config); + + assert_eq!(asks.notification, NotificationAsk::Skip); + } +} diff --git a/src/config.rs b/src/config.rs index 8d82438f4..af9606581 100644 --- a/src/config.rs +++ b/src/config.rs @@ -11,12 +11,9 @@ use anyhow::{Context, Result, anyhow, bail}; use toml::{Table, Value}; use toml_edit::{DocumentMut, Item}; -use crate::base_fix::BaseFixAsk; use crate::git::Git; -use crate::notification::NotificationAsk; use crate::questions::{self, Answers}; use crate::resend_key::Credentials; -use crate::run::Goal; use crate::{email, github, progress}; /// The settings a User config can hold. Each is what a Run does when its @@ -140,38 +137,6 @@ impl UserConfig { } Ok(config) } - - /// The goal of a Run whose command gave no `merge` or `no-merge`. - pub fn default_goal(&self) -> Goal { - if self.merge_always { - Goal::Merged - } else { - Goal::ReadyForReview - } - } - - /// What a Run whose command gave no `base-fix` or `no-base-fix` asks - /// about a Base fix, `retry` being the command that starts it again - /// with one allowed: without `base.fix`, nobody decided. - pub fn default_base_fix(&self, retry: String) -> BaseFixAsk { - if self.base_fix { - BaseFixAsk::Allow - } else { - BaseFixAsk::Undecided { retry } - } - } -} - -impl EmailSettings { - /// What a Run whose command gave no `email` or `no-email` asks about its - /// Run notification. - pub fn default_ask(&self) -> NotificationAsk { - if self.always { - NotificationAsk::Send(None) - } else { - NotificationAsk::Skip - } - } } /// Setup: write the User config. From a terminal, the Setup questions come @@ -771,24 +736,6 @@ mod tests { assert!(!parse("[email]\nalways = false\n").unwrap().email.always); } - #[test] - fn email_always_asks_for_a_notification_to_email_to() { - let always = parse("[email]\nalways = true\n").unwrap(); - assert_eq!(always.email.default_ask(), NotificationAsk::Send(None)); - for text in [ - "", - "[email]\nalways = false\n", - "[email]\nto = \"me@example.com\"\n", - ] { - let config = parse(text).unwrap(); - assert_eq!( - config.email.default_ask(), - NotificationAsk::Skip, - "{text:?}" - ); - } - } - #[test] fn logs_dir_is_read_with_a_leading_tilde_expanded_and_defaults_under_home() { for (dir, expanded) in [ diff --git a/src/main.rs b/src/main.rs index fbcb53108..03058cb9d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,5 +1,6 @@ mod architect; mod args; +mod asks; mod base_fix; mod branch; mod child_run; @@ -32,17 +33,14 @@ mod spec_run; mod update; mod worktree; -use std::num::NonZeroUsize; use std::process::ExitCode; use architect::{Outcome, Reviewed}; -use args::{ArchitectArgs, Command, DispatchArgs, PickupArgs, RunArgs}; -use base_fix::BaseFixAsk; +use args::{ArchitectArgs, Command, PickupArgs, RunArgs}; +use asks::Asks; use config::UserConfig; -use issue::IssueUrl; use notification::{ArchitectNotification, NotificationAsk, PickupNotification, RunNotification}; -use run::{Ended, Goal, StartedBy}; -use spec_run::Parallel; +use run::StartedBy; const HELP: &str = "\ thirdshift turns a GitHub issue into a ready-for-review pull request, or a merged one, unattended. @@ -283,10 +281,7 @@ fn main() -> ExitCode { let args: Vec = std::env::args().skip(1).collect(); let RunArgs { issue, - goal, - email, - parallel, - base_fix, + flags, child, } = match args::parse(&args) { Ok(Command::Help) => { @@ -313,46 +308,20 @@ fn main() -> ExitCode { Ok(config) => config, Err(failure) => return failure, }; - // A child Run, a Ticket's Run in a Spec run or a Base fix, is always a - // Merge run, and leaves the Run notification, the Launch directory and - // whether it may start a Base fix to what started it. - let (goal, email, launch_pull, base_fix) = match child { - Some(_) => ( - Goal::Merged, - NotificationAsk::Skip, - false, - base_fix.unwrap_or(BaseFixAsk::Forbid), - ), - None => { - let base_fix = base_fix.unwrap_or_else(|| { - let retry = args::retry_with_base_fix(&issue, goal, email.as_ref(), parallel); - config.default_base_fix(retry) - }); - ( - goal.unwrap_or(config.default_goal()), - email.unwrap_or(config.email.default_ask()), - config.launch_pull, - base_fix, - ) - } - }; + let asks = Asks::of_run(&issue, &flags, child.as_ref(), &config); // First, so no interrupt can end the Run once its notification is checked. if let Err(error) = interrupt::install() { return failure(&error); } - let notification = match asked(email, |to| RunNotification::new(to, &config.email, &issue)) { + let checked = asked(&asks.notification, |to| { + RunNotification::new(to, &config.email, &issue) + }); + let notification = match checked { Ok(notification) => notification, Err(error) => return failure(&error), }; - let ended = run::run_to_end( - &issue, - goal, - &config.logs_dir, - launch_pull, - Parallel::new(parallel, config.spec_parallel), - child.as_ref().map_or(StartedBy::Command, StartedBy::Child), - base_fix, - ); + let started_by = child.as_ref().map_or(StartedBy::Command, StartedBy::Child); + let ended = run::run_to_end(&issue, &asks, started_by, &config.logs_dir); let code = run_ending::show(&ended); if let Some(notification) = notification { notification.send(&ended); @@ -385,10 +354,8 @@ fn architect(args: ArchitectArgs) -> ExitCode { if let Err(error) = interrupt::install() { return failure(&error); } - // As the command gave it, for the command a Base fix is offered with. - let email_flag = args.email.clone(); - let email = args.email.unwrap_or(config.email.default_ask()); - let notification = match asked(email, |to| ArchitectNotification::new(to, &config.email)) { + let email = args.flags.notification(&config); + let notification = match asked(&email, |to| ArchitectNotification::new(to, &config.email)) { Ok(notification) => notification, Err(error) => return failure(&error), }; @@ -398,16 +365,15 @@ fn architect(args: ArchitectArgs) -> ExitCode { &config.logs_dir, config.launch_pull, ); - let dispatched = match (&ended, &args.dispatch) { - (Ok(Outcome::Reviewed(Reviewed::PlanReady { plan, base })), Some(dispatch)) => { + let dispatched = match &ended { + Ok(Outcome::Reviewed(Reviewed::PlanReady { plan, base })) if !args.plan_only => { progress::step(format_args!( "dispatching the plan {url}, as thirdshift {url} would", url = plan.url )); - // The plan is retried as a Run of its own: another Architect - // run would start a new review instead. - let (email, parallel) = (email_flag.as_ref(), dispatch.parallel); - Some(dispatched(plan, base, dispatch, email, parallel, &config)) + let asks = Asks::of_architect_plan(plan, &args.flags, &config); + let started_by = StartedBy::Dispatch { base }; + Some(run::run_to_end(plan, &asks, started_by, &config.logs_dir)) } _ => None, }; @@ -443,8 +409,8 @@ fn pickup(args: PickupArgs) -> ExitCode { if let Err(error) = interrupt::install() { return failure(&error); } - let email = args.email.clone().unwrap_or(config.email.default_ask()); - let notification = match asked(email, |to| PickupNotification::new(to, &config.email)) { + let email = args.flags.notification(&config); + let notification = match asked(&email, |to| PickupNotification::new(to, &config.email)) { Ok(notification) => notification, Err(error) => return failure(&error), }; @@ -454,19 +420,9 @@ fn pickup(args: PickupArgs) -> ExitCode { Err(error) => return failure(&error), }; let notification = notification.map(|checked| checked.of_taken(&taken.issue, taken.title)); - // Ignored for an issue that is not a Spec: the command can't know which - // it will take. Left out of the command a Base fix is offered with too, - // as the issue is retried as a Run of its own: another Pickup run never - // takes an issue that was started. - let parallel = args.dispatch.parallel.filter(|_| taken.is_spec); - let ended = dispatched( - &taken.issue, - &taken.base, - &args.dispatch, - args.email.as_ref(), - parallel, - &config, - ); + let asks = Asks::of_ready_issue(&taken.issue, taken.is_spec, &args.flags, &config); + let started_by = StartedBy::Dispatch { base: &taken.base }; + let ended = run::run_to_end(&taken.issue, &asks, started_by, &config.logs_dir); let code = run_ending::show(&ended); if let Some(notification) = notification { notification.send(&ended); @@ -474,43 +430,14 @@ fn pickup(args: PickupArgs) -> ExitCode { code } -/// The end of the Spec run or Run that an Architect run or a Pickup run -/// dispatches `issue` as, on its Base branch `base`: what `thirdshift ` would start, with the flags `dispatch` and the User config `config` -/// for the rest. It runs as many Tickets at once as `parallel` asks, which is -/// `dispatch`'s unless that is to be ignored. The command it offers a Base -/// fix with is that one on `issue`, with those flags and with `email` as the -/// command gave it. -fn dispatched( - issue: &IssueUrl, - base: &str, - dispatch: &DispatchArgs, - email: Option<&NotificationAsk>, - parallel: Option, - config: &UserConfig, -) -> Ended { - run::run_to_end( - issue, - dispatch.goal.unwrap_or(config.default_goal()), - &config.logs_dir, - config.launch_pull, - Parallel::new(parallel, config.spec_parallel), - StartedBy::Dispatch { base }, - dispatch.base_fix.clone().unwrap_or_else(|| { - let retry = args::retry_with_base_fix(issue, dispatch.goal, email, parallel); - config.default_base_fix(retry) - }), - ) -} - /// The Run notification `email` asks for, if it asks for one: what `checked` /// makes of the address it gave, or its error if a check fails. fn asked( - email: NotificationAsk, + email: &NotificationAsk, checked: impl FnOnce(Option) -> anyhow::Result, ) -> anyhow::Result> { match email { - NotificationAsk::Send(to) => checked(to).map(Some), + NotificationAsk::Send(to) => checked(to.clone()).map(Some), NotificationAsk::Skip => Ok(None), } } diff --git a/src/run.rs b/src/run.rs index 9d1253ba8..f2a8e819a 100644 --- a/src/run.rs +++ b/src/run.rs @@ -6,7 +6,8 @@ use std::path::{Path, PathBuf}; use anyhow::{Context, Result, anyhow, bail}; -use crate::base_fix::{Advice, BaseFix, BaseFixAsk}; +use crate::asks::Asks; +use crate::base_fix::{Advice, BaseFix}; use crate::branch::{self, Selection}; use crate::child_run::Kind; use crate::ci::{self, Ci, FailedChecks}; @@ -22,7 +23,7 @@ use crate::preflight; use crate::progress; use crate::prompt; use crate::session::{Logs, Sessions}; -use crate::spec_run::{self, Parallel}; +use crate::spec_run; use crate::worktree::{Merge, Worktree}; /// Where a Run takes its PR: ready for review, or, in a Merge run, merged. @@ -107,27 +108,13 @@ impl<'a> StartedBy<'a> { } } -/// [`run`] the Run on `issue` that `started_by` started and that asked -/// `base_fix` about a Base fix, to its end. -pub fn run_to_end( - issue: &IssueUrl, - goal: Goal, - logs_dir: &Path, - launch_pull: bool, - parallel: Parallel, - started_by: StartedBy, - base_fix: BaseFixAsk, -) -> Ended { - let mut base_fix = BaseFix::new(started_by.child(), base_fix); - let outcome = run( - issue, - goal, - logs_dir, - launch_pull, - parallel, - started_by, - &mut base_fix, - ); +/// [`run`] the Run on `issue` that `started_by` started and that is asked +/// `asks`, to its end, logging its sessions under `logs_dir`. Its Run +/// notification, if `asks` has it send one, is for what started the Run to +/// send, once it has ended. +pub fn run_to_end(issue: &IssueUrl, asks: &Asks, started_by: StartedBy, logs_dir: &Path) -> Ended { + let mut base_fix = BaseFix::new(started_by.child(), asks.base_fix.clone()); + let outcome = run(issue, asks, started_by, logs_dir, &mut base_fix); Ended { outcome, base_fix: base_fix.report(), @@ -140,7 +127,7 @@ pub fn run_to_end( /// Failed run path. The worktree, the local /// Issue branch and the plugin directory are gone when this returns, except /// that a Failed run whose work did not reach origin keeps the worktree and -/// branch. With `launch_pull`, the Launch directory's checkout of the +/// branch. If `asks` say so, the Launch directory's checkout of the /// Base branch, if that is the branch checked out, is first brought up to /// date with origin. /// @@ -150,7 +137,7 @@ pub fn run_to_end( /// Architect run or the Pickup run that dispatched this one. Unless the Run /// is a child Run, an issue with sub-issues is a Spec, taken on by a Spec run /// instead, whose Spec branch is picked like an Issue branch, running as many -/// Tickets at once as `parallel` says. A `parallel` the command asked for on an issue +/// Tickets at once as `asks` say. A `parallel` the command asked for on an issue /// with no sub-issues fails before any work, as does a Spec whose Tickets are /// all closed with no Spec branch to continue. /// @@ -161,14 +148,13 @@ pub fn run_to_end( /// Self-merge has left `issue` closed: see [`claim::Claim`]. /// /// `base_fix` is the one Base fix the Run, or a Spec run for its Spec PR, may -/// start, or wait on, when its only red checks are Inherited failures. +/// start, or wait on, when its only red checks are Inherited failures: what +/// `asks` ask about one is already in it. fn run( issue: &IssueUrl, - goal: Goal, - logs_dir: &Path, - launch_pull: bool, - parallel: Parallel, + asks: &Asks, started_by: StartedBy, + logs_dir: &Path, base_fix: &mut BaseFix, ) -> Result { let timestamp = chrono::Utc::now().format("%Y%m%dT%H%M%SZ").to_string(); @@ -179,7 +165,7 @@ fn run( Some(_) => Vec::new(), None => github::tickets(issue)?, }; - if parallel.asked && tickets.is_empty() { + if asks.parallel_asked && tickets.is_empty() { return Err(anyhow!( "parallel is only for a Spec, and #{} has no sub-issues", issue.number @@ -198,7 +184,7 @@ fn run( } let base = selection.base_branch(started_by.given_base(), checked_out.as_deref())?; preflight::check_base_branch(&launch, &base)?; - if launch_pull { + if asks.launch_pull { pull_base_branch(&launch, checked_out.as_deref(), &base); } @@ -216,10 +202,10 @@ fn run( &launch, &selection, &base, - goal, + asks.goal, base_fix, &logs, - parallel.tickets, + asks.tickets_at_once, ); if let Some(claim) = claim { match &outcome { diff --git a/src/spec_run.rs b/src/spec_run.rs index 6e27c97c6..3109ea684 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -47,25 +47,6 @@ const SPEC_REVIEW: &str = "spec-review"; const CHECKLIST_START: &str = ""; const CHECKLIST_END: &str = ""; -/// How many Tickets a Spec run runs at once. -pub struct Parallel { - pub tickets: NonZeroUsize, - /// Whether the command asked for it with `parallel `, rather than the - /// User config or the default deciding. - pub asked: bool, -} - -impl Parallel { - /// As many Tickets at once as the command `asked` for with - /// `parallel `, else as the User config's `default` says. - pub fn new(asked: Option, default: NonZeroUsize) -> Self { - Parallel { - tickets: asked.unwrap_or(default), - asked: asked.is_some(), - } - } -} - /// Whether there are `tickets`, as a Spec has, and every one is closed. pub fn all_closed(tickets: &[Ticket]) -> bool { !tickets.is_empty() && tickets.iter().all(|ticket| !ticket.is_open) From 2b668837e7f4bf110d5d6de5b33f14683f92c329 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 2 Oct 2026 12:21:43 -0400 Subject: [PATCH 2/3] End-to-end tests keep one case per ask per path: the flag-or-User-config variants are the asks module's unit tests now Co-Authored-By: Claude Opus 5.5 --- tests/architect_run.rs | 151 +++++++-------------------------- tests/base_fix.rs | 19 ----- tests/pickup_run.rs | 150 ++++++--------------------------- tests/run_notification.rs | 85 +------------------ tests/spec_run.rs | 79 ----------------- tests/user_config.rs | 173 +------------------------------------- 6 files changed, 64 insertions(+), 593 deletions(-) diff --git a/tests/architect_run.rs b/tests/architect_run.rs index b6057a8b9..b886225e8 100644 --- a/tests/architect_run.rs +++ b/tests/architect_run.rs @@ -766,25 +766,6 @@ fn merge_merges_the_runs_pull_request_or_the_spec_pr() { } } -#[test] -fn the_user_configs_merge_default_applies_unless_no_merge_is_given() { - for plan in [single_ticket_plan, || spec_plan("")] { - for (args, outcome, state) in [ - (vec!["architect"], "merged", "MERGED"), - (vec!["architect", "--no-merge"], "ready for review", "OPEN"), - ] { - let scenario = plan(); - scenario.user_config_is("[merge]\nalways = true\n"); - - let result = scenario.run(&args); - - let pr = pr_from(&scenario, "issue-8"); - assert_ended_with_pr(&result, &pr, outcome); - assert_eq!(pr["state"], state, "{args:?}"); - } - } -} - #[test] fn parallel_passes_through_to_the_spec_run() { // #10's session only succeeds if #9 landed on the Spec branch before it @@ -904,39 +885,6 @@ fn base_fix_lets_the_dispatched_run_start_a_base_fix() { } } -#[test] -fn the_user_configs_base_fix_applies_to_the_dispatched_run_unless_no_base_fix_is_given() { - let scenario = single_ticket_plan_that_inherits_a_failure(); - scenario.user_config_is("[base]\nfix = true\n"); - - let result = scenario.run(&["architect"]); - - assert_ended_with_pr(&result, &pr_from(&scenario, "issue-8"), "ready for review"); - assert_eq!(pr_from(&scenario, "issue-9")["state"], "MERGED"); - - for flag in ["no-base-fix", "--no-base-fix"] { - let scenario = single_ticket_plan_that_inherits_a_failure(); - scenario.user_config_is("[base]\nfix = true\n"); - let cause = inherited_failure(&scenario); - - let result = scenario.run(&["architect", flag]); - - assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); - assert!( - result.stderr.contains(&format!("thirdshift: {cause}\n")), - "stderr: {}", - result.stderr - ); - assert!( - !result.stderr.contains("Retry with:"), - "stderr: {}", - result.stderr - ); - // Only the plan was created: no Base fix issue. - assert_eq!(scenario.gh_calls_of("issue", "create").len(), 1, "{flag}"); - } -} - #[test] fn without_base_fix_the_dispatched_run_fails_on_an_inherited_failure() { let scenario = single_ticket_plan_that_inherits_a_failure(); @@ -960,52 +908,41 @@ fn without_base_fix_the_dispatched_run_fails_on_an_inherited_failure() { } #[test] -fn base_fix_or_the_user_configs_reaches_each_tickets_run_of_the_dispatched_spec_run() { - for (args, config) in [ - (vec!["architect", "base-fix", "parallel", "1"], None), - ( - vec!["architect", "parallel", "1"], - Some("[base]\nfix = true\n"), +fn base_fix_reaches_each_tickets_run_of_the_dispatched_spec_run() { + // The first Ticket's Run, #9's, meets an Inherited failure from the + // Spec branch. One Ticket at a time, so the Spec branch can't move + // under it, which would have it merged in again instead. The Base + // fix issue is the next after the Tickets, #11. + let scenario = spec_plan(""); + scenario.agent_does_for( + 9, + &format!( + "{}{}{}", + agent_opens_pr(9, "issue-8"), + checks_on_head(RED), + checks_on_origin("issue-8", RED) ), - ] { - // The first Ticket's Run, #9's, meets an Inherited failure from the - // Spec branch. One Ticket at a time, so the Spec branch can't move - // under it, which would have it merged in again instead. The Base - // fix issue is the next after the Tickets, #11. - let scenario = spec_plan(""); - scenario.agent_does_for( - 9, - &format!( - "{}{}{}", - agent_opens_pr(9, "issue-8"), - checks_on_head(RED), - checks_on_origin("issue-8", RED) - ), - ); - scenario.agent_does_for(11, &base_fix_opens_pr(11, "issue-8")); - if let Some(config) = config { - scenario.user_config_is(config); - } + ); + scenario.agent_does_for(11, &base_fix_opens_pr(11, "issue-8")); - let result = scenario.run(&args); + let result = scenario.run(&["architect", "base-fix", "parallel", "1"]); - assert_ended_with_pr(&result, &pr_from(&scenario, "issue-8"), "ready for review"); - assert!( - result.stderr.contains( - "#9: starting Base fix #11 into issue-8: https://github.com/acme/widgets/issues/11\n" - ), - "{args:?}: stderr: {}", - result.stderr - ); - let fix = pr_from(&scenario, "issue-11"); - assert_eq!(fix["base"], "issue-8"); - assert_eq!(fix["state"], "MERGED"); - for ticket in ["issue-9", "issue-10"] { - assert_eq!(pr_from(&scenario, ticket)["state"], "MERGED", "{ticket}"); - } - for file in ["issue-9.txt", "issue-10.txt", "ci-fix.txt"] { - assert!(scenario.origin_file("issue-8", file).is_some(), "{file}"); - } + assert_ended_with_pr(&result, &pr_from(&scenario, "issue-8"), "ready for review"); + assert!( + result.stderr.contains( + "#9: starting Base fix #11 into issue-8: https://github.com/acme/widgets/issues/11\n" + ), + "stderr: {}", + result.stderr + ); + let fix = pr_from(&scenario, "issue-11"); + assert_eq!(fix["base"], "issue-8"); + assert_eq!(fix["state"], "MERGED"); + for ticket in ["issue-9", "issue-10"] { + assert_eq!(pr_from(&scenario, ticket)["state"], "MERGED", "{ticket}"); + } + for file in ["issue-9.txt", "issue-10.txt", "ci-fix.txt"] { + assert!(scenario.origin_file("issue-8", file).is_some(), "{file}"); } } @@ -1542,30 +1479,6 @@ fn a_dispatched_run_that_fails_sends_one_notification_with_its_outcome_and_cause ); } -#[test] -fn no_email_skips_the_notification_email_always_asks_for() { - let scenario = single_ticket_plan(); - scenario.user_config_is(EMAIL_ALWAYS); - let resend = ResendStandIn::replying(200, ACCEPTED); - - let result = run_with_resend(&scenario, &resend, &["architect", "--no-email"]); - - assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); - assert!(resend.requests().is_empty()); -} - -#[test] -fn an_architect_run_that_is_not_asked_for_a_notification_sends_none() { - let scenario = single_ticket_plan(); - scenario.user_config_is("[email]\nto = \"config@example.com\"\n"); - let resend = ResendStandIn::replying(200, ACCEPTED); - - let result = run_with_resend(&scenario, &resend, &["architect"]); - - assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); - assert!(resend.requests().is_empty()); -} - #[test] fn with_no_address_known_the_architect_run_stops_before_any_work_and_sends_nothing() { let scenario = scenario(); diff --git a/tests/base_fix.rs b/tests/base_fix.rs index 404b3f326..082fecbca 100644 --- a/tests/base_fix.rs +++ b/tests/base_fix.rs @@ -726,25 +726,6 @@ fn with_no_base_fix_the_base_branchs_failing_checks_are_linked_and_no_base_fix_i } } -#[test] -fn with_base_fix_in_the_user_config_and_no_base_fix_given_no_base_fix_is_offered() { - let scenario = Scenario::new(); - scenario.user_config_is("[base]\nfix = true\n"); - scenario.agent_does(RUN_OPENS_PR_WITH_INHERITED_FAILURE); - - let result = scenario.run(&[&scenario.issue_url(7), "no-base-fix"]); - - assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); - assert!( - result - .stderr - .contains(&format!("thirdshift: Base check: {BASE_CHECK}\n")), - "stderr: {}", - result.stderr - ); - assert_no_advice(&result.stderr, &["Retry with:", "Or set:"]); -} - #[test] fn a_base_branch_check_with_no_url_is_named_alone() { let scenario = Scenario::new(); diff --git a/tests/pickup_run.rs b/tests/pickup_run.rs index cae2c5a02..a54f4ed5c 100644 --- a/tests/pickup_run.rs +++ b/tests/pickup_run.rs @@ -612,41 +612,14 @@ fn parallel_is_ignored_without_error_for_an_issue_that_is_not_a_spec() { } #[test] -fn merge_and_no_merge_and_the_user_configs_default_reach_the_dispatched_run() { - for ready in [ready_ticket, || ready_spec("")] { - for (config, args, outcome, state) in [ - ("", vec!["pickup"], "ready for review", "OPEN"), - ("", vec!["pickup", "merge"], "merged", "MERGED"), - ("", vec!["pickup", "--merge"], "merged", "MERGED"), - ( - "[merge]\nalways = true\n", - vec!["pickup"], - "merged", - "MERGED", - ), - ( - "[merge]\nalways = true\n", - vec!["pickup", "no-merge"], - "ready for review", - "OPEN", - ), - ( - "[merge]\nalways = true\n", - vec!["pickup", "--no-merge"], - "ready for review", - "OPEN", - ), - ] { - let scenario = ready(); - scenario.user_config_is(config); +fn merge_reaches_the_dispatched_run() { + let scenario = ready_ticket(); - let result = scenario.run(&args); + let result = scenario.run(&["pickup", "merge"]); - let pr = pr_from(&scenario, "issue-7"); - assert_ended_with_pr(&result, &pr, outcome); - assert_eq!(pr["state"], state, "{args:?} with {config:?}"); - } - } + let pr = pr_from(&scenario, "issue-7"); + assert_ended_with_pr(&result, &pr, "merged"); + assert_eq!(pr["state"], "MERGED"); } const RED: &str = r#"[{"name": "test", "conclusion": "failure"}]"#; @@ -685,45 +658,20 @@ gh fake checks "$(git rev-parse HEAD)" '{GREEN}' } #[test] -fn base_fix_and_no_base_fix_and_the_user_configs_default_reach_the_dispatched_run() { - for (config, args, fixed) in [ - ("", vec!["pickup"], false), - ("", vec!["pickup", "base-fix"], true), - ("", vec!["pickup", "--base-fix"], true), - ("[base]\nfix = true\n", vec!["pickup"], true), - ("[base]\nfix = true\n", vec!["pickup", "no-base-fix"], false), - ( - "[base]\nfix = true\n", - vec!["pickup", "--no-base-fix"], - false, - ), - ] { - let scenario = ready_ticket_that_inherits_a_failure(); - scenario.user_config_is(config); - let red_base = scenario.origin_git(&["rev-parse", "main"]); +fn base_fix_reaches_the_dispatched_run() { + let scenario = ready_ticket_that_inherits_a_failure(); - let result = scenario.run(&args); + let result = scenario.run(&["pickup", "base-fix"]); - let started = result.stderr.contains( + assert!( + result.stderr.contains( "thirdshift: starting Base fix #8 into main: https://github.com/acme/widgets/issues/8\n", - ); - assert_eq!( - started, fixed, - "{args:?} with {config:?}: {}", - result.stderr - ); - if fixed { - assert_ended_with_pr(&result, &pr_from(&scenario, "issue-7"), "ready for review"); - assert_eq!(pr_from(&scenario, "issue-8")["state"], "MERGED"); - } else { - assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); - let cause = format!( - "thirdshift: CI red on test, which also fails on main at {}; fix main first\n", - &red_base[..7] - ); - assert!(result.stderr.contains(&cause), "stderr: {}", result.stderr); - } - } + ), + "stderr: {}", + result.stderr + ); + assert_ended_with_pr(&result, &pr_from(&scenario, "issue-7"), "ready for review"); + assert_eq!(pr_from(&scenario, "issue-8")["state"], "MERGED"); } #[test] @@ -989,50 +937,18 @@ fn titled_ready_ticket() -> Scenario { const EMAIL_ALWAYS: &str = "[email]\nalways = true\nto = \"config@example.com\"\n"; #[test] -fn a_pass_that_took_an_issue_sends_one_notification_when_the_command_or_the_user_config_asks() { - for (config, args, to) in [ - ( - "", - vec!["pickup", "email", "me@example.com"], - Some("me@example.com"), - ), - (EMAIL_ALWAYS, vec!["pickup"], Some("config@example.com")), - ( - EMAIL_ALWAYS, - vec!["pickup", "--email", "me@example.com"], - Some("me@example.com"), - ), - ( - "[email]\nto = \"config@example.com\"\n", - vec!["pickup", "email"], - Some("config@example.com"), - ), - (EMAIL_ALWAYS, vec!["pickup", "no-email"], None), - (EMAIL_ALWAYS, vec!["pickup", "--no-email"], None), - ( - "[email]\nto = \"config@example.com\"\n", - vec!["pickup"], - None, - ), - ] { - let scenario = titled_ready_ticket(); - scenario.user_config_is(config); - let resend = ResendStandIn::replying(200, ACCEPTED); +fn a_pass_that_took_an_issue_sends_one_notification_when_asked() { + let scenario = titled_ready_ticket(); + scenario.user_config_is(EMAIL_ALWAYS); + let resend = ResendStandIn::replying(200, ACCEPTED); - let result = run_with_resend(&scenario, &resend, &args); + let result = run_with_resend(&scenario, &resend, &["pickup"]); - assert_ended_with_pr(&result, &pr_from(&scenario, "issue-7"), "ready for review"); - let requests = resend.requests(); - let sent: Vec<_> = requests.iter().map(|request| &request.body).collect(); - match to { - // Exactly one: the dispatched run sends none of its own. - Some(to) => { - assert_eq!(sent.len(), 1, "{args:?} with {config:?}: {sent:?}"); - assert_eq!(sent[0]["to"], to, "{args:?} with {config:?}"); - } - None => assert!(sent.is_empty(), "{args:?} with {config:?}: {sent:?}"), - } - } + assert_ended_with_pr(&result, &pr_from(&scenario, "issue-7"), "ready for review"); + let requests = resend.requests(); + // Exactly one: the dispatched run sends none of its own. + assert_eq!(requests.len(), 1, "{requests:?}"); + assert_eq!(requests[0].body["to"], "config@example.com"); } #[test] @@ -1353,18 +1269,6 @@ fn a_notification_that_cant_be_sent_is_a_warning_that_changes_neither_the_exit_c } } -#[test] -fn the_user_configs_spec_parallel_reaches_a_dispatched_spec_run() { - // As in `parallel_reaches_a_dispatched_spec_run`: one Ticket at a time. - let scenario = ready_spec("test -f issue-8.txt"); - scenario.user_config_is("[spec]\nparallel = 1\n"); - - let result = scenario.run(&["pickup"]); - - assert_ended_with_pr(&result, &pr_from(&scenario, "issue-7"), "ready for review"); - assert_eq!(sessions(&scenario), ["8", "9", "7"]); -} - /// Make each of `issues` open and labelled `in-progress` on the fake GitHub: /// an issue that carries a Claim. fn claimed_issues(scenario: &Scenario, issues: &[u32]) { diff --git a/tests/run_notification.rs b/tests/run_notification.rs index 463b3744e..9818f605d 100644 --- a/tests/run_notification.rs +++ b/tests/run_notification.rs @@ -278,19 +278,6 @@ fn without_resend_api_key_the_run_stops_before_any_work_and_sends_nothing() { assert!(resend.requests().is_empty()); } -#[test] -fn a_run_without_the_flag_sends_nothing() { - let scenario = Scenario::new(); - scenario.user_config_is("[email]\nto = \"me@example.com\"\n"); - scenario.agent_does(AGENT_OPENS_PR); - let resend = ResendStandIn::replying(200, ACCEPTED); - - let result = run(&scenario, &resend, &[&scenario.issue_url(7)], Some(KEY)); - - assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); - assert!(resend.requests().is_empty()); -} - #[test] fn a_failed_send_is_a_warning_that_changes_neither_the_exit_code_nor_stdout() { // One reply end to end; the email's unit tests take the other shapes of @@ -343,65 +330,10 @@ fn the_warning(result: &RunResult) -> &str { .unwrap_or_else(|| panic!("no warning in stderr: {}", result.stderr)) } -/// The User config of a machine where every Run sends a Run notification. -const EMAIL_ALWAYS: &str = "[email]\nalways = true\nto = \"config@example.com\"\n"; - -#[test] -fn email_always_makes_a_run_without_the_flag_send_one_notification_to_email_to() { - let scenario = Scenario::new(); - scenario.user_config_is(EMAIL_ALWAYS); - scenario.agent_does(AGENT_OPENS_PR); - let resend = ResendStandIn::replying(200, ACCEPTED); - - let result = run(&scenario, &resend, &[&scenario.issue_url(7)], Some(KEY)); - - assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); - let request = the_one_request(&resend); - assert_eq!(request.body["to"], "config@example.com"); - assert!( - subject(&request).ends_with(": ready for review"), - "{}", - subject(&request) - ); -} - -#[test] -fn no_email_skips_the_notification_email_always_asks_for() { - // One spelling end to end; the argument parsing's unit tests take both. - let scenario = Scenario::new(); - scenario.user_config_is(EMAIL_ALWAYS); - scenario.agent_does(AGENT_OPENS_PR); - let resend = ResendStandIn::replying(200, ACCEPTED); - let url = scenario.issue_url(7); - - let result = run(&scenario, &resend, &["--no-email", &url], Some(KEY)); - - assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); - assert!(resend.requests().is_empty()); -} - -#[test] -fn with_email_always_an_address_after_the_flag_still_wins() { - let scenario = Scenario::new(); - scenario.user_config_is(EMAIL_ALWAYS); - scenario.agent_does(AGENT_OPENS_PR); - let resend = ResendStandIn::replying(200, ACCEPTED); - - let result = run( - &scenario, - &resend, - &[&scenario.issue_url(7), "--email", "flag@example.com"], - Some(KEY), - ); - - assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); - assert_eq!(the_one_request(&resend).body["to"], "flag@example.com"); -} - #[test] fn email_always_without_an_address_stops_the_run_before_any_work() { // Without a key it stops as a Run with the flag does, shown above; the - // User config's unit tests show email.always asks for a notification. + // unit tests in `asks` show email.always asks for a notification. let scenario = Scenario::new(); scenario.user_config_is("[email]\nalways = true\n"); let resend = ResendStandIn::replying(200, ACCEPTED); @@ -413,21 +345,6 @@ fn email_always_without_an_address_stops_the_run_before_any_work() { assert!(resend.requests().is_empty()); } -#[test] -fn email_always_false_makes_a_run_without_the_flag_send_nothing() { - // With no email.always, a Run without the flag is shown sending nothing - // above; the User config's unit tests take both. - let scenario = Scenario::new(); - scenario.user_config_is("[email]\nalways = false\nto = \"me@example.com\"\n"); - scenario.agent_does(AGENT_OPENS_PR); - let resend = ResendStandIn::replying(200, ACCEPTED); - - let result = run(&scenario, &resend, &[&scenario.issue_url(7)], Some(KEY)); - - assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); - assert!(resend.requests().is_empty()); -} - /// Credentials holding the key `re_file_456`. const CREDENTIALS: &str = "[resend]\nkey = \"re_file_456\"\n"; diff --git a/tests/spec_run.rs b/tests/spec_run.rs index 58a3c091e..38a6fe353 100644 --- a/tests/spec_run.rs +++ b/tests/spec_run.rs @@ -466,16 +466,6 @@ fn merge_on_a_spec_self_merges_the_spec_pr_into_the_base_branch() { assert_spec_pr_merged(&scenario, &result); } -#[test] -fn a_merge_ask_from_the_user_config_self_merges_the_spec_pr() { - let scenario = linear_spec(); - scenario.user_config_is("[merge]\nalways = true\n"); - - let result = scenario.run(&[&spec_url(&scenario)]); - - assert_spec_pr_merged(&scenario, &result); -} - #[test] fn no_merge_on_a_spec_leaves_the_spec_pr_ready_while_its_tickets_still_merge() { let scenario = linear_spec(); @@ -951,40 +941,6 @@ fn a_shared_base_fix_that_fails_fails_the_ticket_that_started_it_and_the_one_wai ); } -#[test] -fn base_fix_in_the_user_config_gives_a_tickets_inherited_failure_a_base_fix() { - let scenario = spec_whose_ticket_inherits_a_failure(); - scenario.user_config_is("[base]\nfix = true\n"); - base_fix_lands_on_the_spec_branch(&scenario, 22); - - let result = scenario.run(&[&spec_url(&scenario)]); - - assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); - let fix = pr_from(&scenario, "issue-22").expect("the Base fix's PR"); - assert_eq!(fix["base"], "issue-20"); - assert_eq!(fix["state"], "MERGED"); - assert_eq!(pr_from(&scenario, "issue-21").unwrap()["state"], "MERGED"); -} - -#[test] -fn no_base_fix_on_a_spec_reaches_its_tickets_whatever_the_user_config_says() { - let scenario = spec_whose_ticket_inherits_a_failure(); - scenario.user_config_is("[base]\nfix = true\n"); - - let result = scenario.run(&[&spec_url(&scenario), "no-base-fix"]); - - assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); - assert_contains( - &result.stderr, - &format!( - "thirdshift: #21 failed: {}", - inherited_failure(&scenario, "issue-20") - ), - ); - assert!(scenario.gh_calls_of("issue", "create").is_empty()); - assert_eq!(pr_from(&scenario, "issue-21").unwrap()["state"], "OPEN"); -} - #[test] fn a_policy_refusal_on_the_spec_pr_leaves_it_ready_and_exits_1() { let scenario = linear_spec(); @@ -1497,29 +1453,6 @@ fn parallel_1_runs_the_tickets_one_at_a_time() { assert!(landed < started, "stderr: {}", result.stderr); } -#[test] -fn spec_parallel_in_the_user_config_sets_how_many_tickets_run_at_once() { - let scenario = diamond_spec(); - second_ticket_needs_the_first_landed(&scenario); - scenario.user_config_is("[spec]\nparallel = 1\n"); - - let result = scenario.run(&[&spec_url(&scenario)]); - - assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); - assert_eq!(sessions_by_issue(&scenario), ["21", "22", "23", "20"]); -} - -#[test] -fn the_parallel_flag_wins_over_spec_parallel() { - let scenario = diamond_spec(); - independent_tickets_wait_for_each_other(&scenario); - scenario.user_config_is("[spec]\nparallel = 1\n"); - - let result = scenario.run(&[&spec_url(&scenario), "parallel", "2"]); - - assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); -} - #[test] fn without_a_limit_at_most_three_tickets_run_at_once() { let scenario = Scenario::new(); @@ -2091,18 +2024,6 @@ fn a_ready_ticket_an_interrupt_kept_from_starting_is_not_started_in_the_notifica assert_contains(&text, "#22 not started\n"); } -#[test] -fn no_email_keeps_email_always_from_sending_for_a_spec_run() { - let scenario = linear_spec(); - scenario.user_config_is("[email]\nalways = true\nto = \"me@example.com\"\n"); - let resend = ResendStandIn::replying(200, r#"{"id":"1"}"#); - - let result = run_emailing(&scenario, &resend, &["--no-email", &spec_url(&scenario)]); - - assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); - assert_eq!(resend.requests().len(), 0); -} - #[test] fn a_missing_resend_api_key_stops_the_spec_run_before_any_ticket_starts() { let scenario = linear_spec(); diff --git a/tests/user_config.rs b/tests/user_config.rs index e03065292..e45ebb140 100644 --- a/tests/user_config.rs +++ b/tests/user_config.rs @@ -1,8 +1,7 @@ -//! The User config, `~/.thirdshift/config.toml`: `merge.always` makes every -//! Run a Merge run unless the command says `no-merge`, `base.fix` lets every -//! Run start a Base fix unless the command says `no-base-fix`, `logs.dir` -//! moves the session logs, and a config thirdshift can't use stops the Run -//! before any work. +//! The User config, `~/.thirdshift/config.toml`: `logs.dir` moves the session +//! logs, and a config thirdshift can't use stops the Run before any work. +//! What each setting asks of a Run that its command says nothing about is +//! covered by the unit tests in `asks`. mod support; @@ -32,86 +31,6 @@ fn assert_merged(scenario: &Scenario, result: &RunResult) { assert_eq!(scenario.gh_state()["prs"][0]["state"], "MERGED"); } -/// Assert the Run ended with PR #1 open and ready for review, never merged. -fn assert_ready_for_review(scenario: &Scenario, result: &RunResult) { - assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); - assert_eq!(result.stdout, format!("{PR_URL}\n")); - assert_eq!( - result.stderr.lines().last(), - Some(format!("thirdshift: PR {PR_URL} is ready for review").as_str()), - "stderr: {}", - result.stderr - ); - assert_eq!(scenario.gh_state()["prs"][0]["state"], "OPEN"); - assert!( - !scenario - .gh_calls() - .iter() - .any(|call| call.starts_with(&["pr".to_string(), "merge".to_string()])), - "thirdshift ran gh pr merge" - ); -} - -#[test] -fn merge_always_makes_a_run_without_the_merge_word_a_merge_run() { - let scenario = Scenario::new(); - scenario.user_config_is("[merge]\nalways = true\n"); - scenario.agent_does(AGENT_OPENS_PR); - - let result = scenario.run(&[&scenario.issue_url(7)]); - - assert_merged(&scenario, &result); -} - -#[test] -fn no_merge_overrides_merge_always_for_one_run() { - for flag in ["no-merge", "--no-merge"] { - for flag_first in [true, false] { - let scenario = Scenario::new(); - scenario.user_config_is("[merge]\nalways = true\n"); - scenario.agent_does(AGENT_OPENS_PR); - let url = scenario.issue_url(7); - let args = if flag_first { - [flag, url.as_str()] - } else { - [url.as_str(), flag] - }; - - let result = scenario.run(&args); - - assert_ready_for_review(&scenario, &result); - } - } -} - -#[test] -fn without_a_config_file_a_run_is_not_a_merge_run() { - let scenario = Scenario::new(); - scenario.agent_does(AGENT_OPENS_PR); - - let result = scenario.run(&[&scenario.issue_url(7)]); - - assert_ready_for_review(&scenario, &result); -} - -#[test] -fn a_config_that_says_nothing_about_merging_leaves_a_run_not_a_merge_run() { - for config in [ - "", - "# nothing set yet\n", - "[merge]\n", - "[merge]\nalways = false\n", - ] { - let scenario = Scenario::new(); - scenario.user_config_is(config); - scenario.agent_does(AGENT_OPENS_PR); - - let result = scenario.run(&[&scenario.issue_url(7)]); - - assert_ready_for_review(&scenario, &result); - } -} - #[test] fn the_merge_word_goes_before_or_after_the_issue_url() { for merge in ["merge", "--merge"] { @@ -331,77 +250,6 @@ gh fake checks "$(git rev-parse HEAD)" '[{"name": "test", "conclusion": "failure gh fake checks "$(git rev-parse origin/main)" '[{"name": "test", "conclusion": "failure"}]' "#; -/// The agent, on the Base fix issue #8, commits a fix and opens its PR into -/// `main`, with `test` green on its head. -const BASE_FIX_OPENS_PR: &str = r#" -echo "fixed" > ci-fix.txt -git add ci-fix.txt -git commit -q -m "Fix CI on main" -gh pr create --base main --head issue-8 --title "Fix CI on main" --body "Closes #8" -gh fake checks "$(git rev-parse HEAD)" '[{"name": "test", "conclusion": "success"}]' -"#; - -/// Assert the Run on #7 started a Base fix on #8 that merged, and ended with -/// its own PR ready for review. -fn assert_base_fix_merged(scenario: &Scenario, result: &RunResult) { - assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); - let gh = scenario.gh_state(); - assert_eq!(gh["titles"]["8"], "CI red on main: test"); - assert_eq!(gh["prs"][1]["base"], "main"); - assert_eq!(gh["prs"][1]["state"], "MERGED"); - assert_eq!(gh["prs"][0]["state"], "OPEN"); - assert_eq!(gh["prs"][0]["isDraft"], false); -} - -/// Assert the Run on #7 failed on its Inherited failure, with no Base fix -/// issue written. -fn assert_failed_with_no_base_fix(scenario: &Scenario, result: &RunResult) { - assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); - assert!( - result - .stderr - .contains("CI red on test, which also fails on main at ") - && result.stderr.contains("; fix main first"), - "stderr: {}", - result.stderr - ); - assert!(scenario.gh_calls_of("issue", "create").is_empty()); - assert_eq!(scenario.gh_state()["issues"].as_object().unwrap().len(), 1); -} - -#[test] -fn base_fix_lets_a_run_without_the_base_fix_word_start_a_base_fix() { - let scenario = Scenario::new(); - scenario.user_config_is("[base]\nfix = true\n"); - scenario.agent_does(RUN_OPENS_PR_WITH_INHERITED_FAILURE); - scenario.agent_does_for(8, BASE_FIX_OPENS_PR); - - let result = scenario.run(&[&scenario.issue_url(7)]); - - assert_base_fix_merged(&scenario, &result); -} - -#[test] -fn no_base_fix_overrides_base_fix_for_one_run() { - for flag in ["no-base-fix", "--no-base-fix"] { - for flag_first in [true, false] { - let scenario = Scenario::new(); - scenario.user_config_is("[base]\nfix = true\n"); - scenario.agent_does(RUN_OPENS_PR_WITH_INHERITED_FAILURE); - let url = scenario.issue_url(7); - let args = if flag_first { - [flag, url.as_str()] - } else { - [url.as_str(), flag] - }; - - let result = scenario.run(&args); - - assert_failed_with_no_base_fix(&scenario, &result); - } - } -} - #[test] fn a_base_fix_setting_thirdshift_cant_use_stops_the_run_naming_it_and_the_file() { for (config, named) in [ @@ -426,19 +274,6 @@ fn a_base_fix_setting_thirdshift_cant_use_stops_the_run_naming_it_and_the_file() } } -#[test] -fn without_base_fix_set_to_true_an_inherited_failure_fails_the_run() { - for config in ["", "[base]\n", "[base]\nfix = false\n"] { - let scenario = Scenario::new(); - scenario.user_config_is(config); - scenario.agent_does(RUN_OPENS_PR_WITH_INHERITED_FAILURE); - - let result = scenario.run(&[&scenario.issue_url(7)]); - - assert_failed_with_no_base_fix(&scenario, &result); - } -} - #[test] fn base_fix_and_no_base_fix_together_are_an_argument_error_before_the_config_is_read() { let scenario = Scenario::new(); From 9e777baece1e5df113817c9dd5cad2ebbd6e0140 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 2 Oct 2026 12:25:36 -0400 Subject: [PATCH 3/3] Address review: the flags for a dispatched run named by the asks module, one home for a dispatched run's asks, two more flag-or-User-config cases gone from a Pickup run's tests Co-Authored-By: Claude Opus 5.5 --- src/args.rs | 3 +- src/asks.rs | 123 +++++++++++++++++++++++++++++++------------ src/run.rs | 9 ++-- tests/pickup_run.rs | 33 ++++-------- tests/user_config.rs | 7 +-- 5 files changed, 110 insertions(+), 65 deletions(-) diff --git a/src/args.rs b/src/args.rs index 63e90bcd1..bb6e5fa3b 100644 --- a/src/args.rs +++ b/src/args.rs @@ -195,8 +195,7 @@ fn parse_architect(args: &[String]) -> Result { _ => focus = Some(arg.clone()), } } - let for_dispatch = flags.goal.is_some() || flags.parallel.is_some() || flags.base_fix.is_some(); - if plan_only && for_dispatch { + if plan_only && flags.any_for_dispatched_run() { bail!( "merge, no-merge, parallel, base-fix and no-base-fix can't be used with \ {PLAN_ONLY}: it dispatches no run for them to apply to" diff --git a/src/asks.rs b/src/asks.rs index 4568821cf..d9ee06af1 100644 --- a/src/asks.rs +++ b/src/asks.rs @@ -49,14 +49,13 @@ pub struct Asks { impl Asks { /// What the Run, or the Spec run, started on `issue`'s URL is asked: by - /// `flags`, else by `config`. + /// `flags`, else by `config`, unless another thirdshift started it as + /// the child Run `child`, a Ticket's Run in a Spec run or a Base fix. /// - /// Unless another thirdshift started it, as the child Run `child`, a - /// Ticket's Run in a Spec run or a Base fix. A child Run is always a - /// Merge run, sends no Run notification and leaves the Launch directory - /// alone, whatever `flags` and `config` say: those are for what started - /// it. It starts no Base fix either, unless `flags` allow one or give it - /// the command to offer one with. + /// A child Run is always a Merge run, sends no Run notification and + /// leaves the Launch directory alone, whatever `flags` and `config` say: + /// those are for what started it. It starts no Base fix either, unless + /// `flags` allow one or give it the command to offer one with. pub fn of_run( issue: &IssueUrl, flags: &Flags, @@ -77,22 +76,17 @@ impl Asks { } /// What the Spec run or Run an Architect run dispatches its Architect - /// plan `plan` as is asked: what [`Asks::of_run`] would ask a Run started - /// on the plan's URL with the Architect run's `flags`, except that it - /// sends no Run notification of its own. The Architect run sends the - /// one, as [`Flags::notification`] asks. The command a Base fix is - /// offered with retries the plan as a Run of its own: another Architect - /// run would start a new review instead. + /// plan `plan` as is asked by the Architect run's `flags`, else by + /// `config`: see [`Asks::dispatched`]. The command a Base fix is offered + /// with retries the plan as a Run of its own: another Architect run + /// would start a new review instead. pub fn of_architect_plan(plan: &IssueUrl, flags: &Flags, config: &UserConfig) -> Asks { - Asks { - notification: NotificationAsk::Skip, - ..Asks::resolved(plan, flags, config) - } + Asks::dispatched(plan, flags, config) } /// What the Spec run or Run a Pickup run dispatches the Ready issue - /// `issue` as is asked, as [`Asks::of_architect_plan`] has it for a - /// plan, with one difference: `parallel` is ignored for an issue that is + /// `issue` as is asked by the Pickup run's `flags`, else by `config`: + /// see [`Asks::dispatched`]. `parallel` is ignored for an issue that is /// not a Spec, as `is_spec` says, since the command can't know which /// the Pickup run will take. It is left out of the command a Base fix is /// offered with too, which retries the issue as a Run of its own: @@ -107,7 +101,19 @@ impl Asks { parallel: flags.parallel.filter(|_| is_spec), ..flags.clone() }; - Asks::of_architect_plan(issue, &flags, config) + Asks::dispatched(issue, &flags, config) + } + + /// What the Spec run or Run that an Architect run or a Pickup run + /// dispatches `issue` as is asked: what [`Asks::of_run`] would ask a Run + /// started on the Issue URL with `flags`, except that it sends no Run + /// notification of its own. The run that dispatched it sends the one, as + /// [`Flags::notification`] asks. + fn dispatched(issue: &IssueUrl, flags: &Flags, config: &UserConfig) -> Asks { + Asks { + notification: NotificationAsk::Skip, + ..Asks::resolved(issue, flags, config) + } } /// Each ask as `flags` gave it, else as `config` says. Where neither @@ -139,6 +145,17 @@ impl Asks { } impl Flags { + /// Whether any flag here is for the Spec run or Run that an Architect + /// run or a Pickup run dispatches: every one but `email` and `no-email`, + /// which ask that run for its own Run notification. + pub fn any_for_dispatched_run(&self) -> bool { + let for_dispatched_run = Flags { + email: None, + ..self.clone() + }; + for_dispatched_run != Flags::default() + } + /// What a run with these flags is asked about its own Run notification: /// what `email` or `no-email` asked for, else what `config` says. It is /// a Run's ask, and an Architect run's or a Pickup run's for the one @@ -378,8 +395,7 @@ mod tests { } #[test] - fn a_dispatched_architect_plan_is_asked_by_the_flags_else_the_user_config_and_sends_no_notification() - { + fn a_dispatched_architect_plan_sends_no_notification_whoever_decides_its_asks() { let asks = Asks::of_architect_plan(&issue(), &dispatch_flags(), &no_settings()); assert_eq!( asks, @@ -409,19 +425,58 @@ mod tests { } #[test] - fn a_dispatched_ready_issue_that_is_a_spec_is_asked_what_a_dispatched_architect_plan_is() { - for (flags, config) in [ - (Flags::default(), every_setting()), - (dispatch_flags(), no_settings()), + fn a_dispatched_ready_issue_that_is_a_spec_sends_no_notification_whoever_decides_its_asks() { + let asks = Asks::of_ready_issue(&issue(), true, &dispatch_flags(), &no_settings()); + assert_eq!( + asks, + Asks { + goal: Goal::Merged, + notification: NotificationAsk::Skip, + tickets_at_once: n(2), + parallel_asked: true, + base_fix: undecided(" merge --email flag@example.com parallel 2"), + launch_pull: false, + } + ); + + let asks = Asks::of_ready_issue(&issue(), true, &Flags::default(), &every_setting()); + assert_eq!( + asks, + Asks { + goal: Goal::Merged, + // Whatever `email.always` says: the Pickup run sends it. + notification: NotificationAsk::Skip, + tickets_at_once: n(5), + parallel_asked: false, + base_fix: BaseFixAsk::Allow, + launch_pull: true, + } + ); + } + + #[test] + fn only_the_email_flags_are_not_for_a_dispatched_run() { + let email_only = Flags { + email: Some(to("flag@example.com")), + ..Flags::default() + }; + assert!(!Flags::default().any_for_dispatched_run()); + assert!(!email_only.any_for_dispatched_run()); + for flags in [ + Flags { + goal: Some(Goal::ReadyForReview), + ..email_only.clone() + }, + Flags { + parallel: Some(n(1)), + ..Flags::default() + }, + Flags { + base_fix: Some(BaseFixAsk::Forbid), + ..Flags::default() + }, ] { - let asks = Asks::of_ready_issue(&issue(), true, &flags, &config); - - assert_eq!( - asks, - Asks::of_architect_plan(&issue(), &flags, &config), - "{flags:?}" - ); - assert_eq!(asks.notification, NotificationAsk::Skip, "{flags:?}"); + assert!(flags.any_for_dispatched_run(), "{flags:?}"); } } diff --git a/src/run.rs b/src/run.rs index f2a8e819a..069598cf7 100644 --- a/src/run.rs +++ b/src/run.rs @@ -127,7 +127,7 @@ pub fn run_to_end(issue: &IssueUrl, asks: &Asks, started_by: StartedBy, logs_dir /// Failed run path. The worktree, the local /// Issue branch and the plugin directory are gone when this returns, except /// that a Failed run whose work did not reach origin keeps the worktree and -/// branch. If `asks` say so, the Launch directory's checkout of the +/// branch. With `asks.launch_pull`, the Launch directory's checkout of the /// Base branch, if that is the branch checked out, is first brought up to /// date with origin. /// @@ -137,9 +137,10 @@ pub fn run_to_end(issue: &IssueUrl, asks: &Asks, started_by: StartedBy, logs_dir /// Architect run or the Pickup run that dispatched this one. Unless the Run /// is a child Run, an issue with sub-issues is a Spec, taken on by a Spec run /// instead, whose Spec branch is picked like an Issue branch, running as many -/// Tickets at once as `asks` say. A `parallel` the command asked for on an issue -/// with no sub-issues fails before any work, as does a Spec whose Tickets are -/// all closed with no Spec branch to continue. +/// Tickets at once as `asks.tickets_at_once` says. A `parallel` the command +/// asked for, as `asks.parallel_asked` says, on an issue with no sub-issues +/// fails before any work, as does a Spec whose Tickets are all closed with +/// no Spec branch to continue. /// /// Once those checks pass, and before the worktree is created, the Run, or /// the Spec run, makes the Claim on `issue`, unless it is a child Run. A Claim diff --git a/tests/pickup_run.rs b/tests/pickup_run.rs index a54f4ed5c..b3b11705a 100644 --- a/tests/pickup_run.rs +++ b/tests/pickup_run.rs @@ -1097,19 +1097,14 @@ fn the_notification_of_a_failed_spec_run_has_each_tickets_outcome() { #[test] fn a_pass_skipped_for_having_no_ready_issue_sends_no_notification() { - for (config, args) in [ - ("", vec!["pickup", "email", "me@example.com"]), - (EMAIL_ALWAYS, vec!["pickup"]), - ] { - let scenario = Scenario::new(); - scenario.user_config_is(config); - let resend = ResendStandIn::replying(200, ACCEPTED); + let scenario = Scenario::new(); + scenario.user_config_is(EMAIL_ALWAYS); + let resend = ResendStandIn::replying(200, ACCEPTED); - let result = run_with_resend(&scenario, &resend, &args); + let result = run_with_resend(&scenario, &resend, &["pickup"]); - assert_skipped(&scenario, &result, NO_READY_ISSUE); - assert!(resend.requests().is_empty(), "{args:?} with {config:?}"); - } + assert_skipped(&scenario, &result, NO_READY_ISSUE); + assert!(resend.requests().is_empty()); } /// Start a Pickup run that takes #7, made a Ready issue here, and holds the @@ -1142,19 +1137,13 @@ fn a_pass_skipped_for_the_lock_sends_no_notification() { #[test] fn asked_for_a_notification_with_no_address_known_the_pass_stops_before_any_work() { - for (config, args) in [ - ("", ["pickup", "--email"]), - ("[email]\nalways = true\n", ["pickup", "merge"]), - ] { - let scenario = ready_ticket(); - scenario.user_config_is(config); - let resend = ResendStandIn::replying(200, ACCEPTED); + let scenario = ready_ticket(); + let resend = ResendStandIn::replying(200, ACCEPTED); - let result = run_with_resend(&scenario, &resend, &args); + let result = run_with_resend(&scenario, &resend, &["pickup", "--email"]); - assert_stopped_by_preflight(&scenario, &result, "no email address"); - assert!(resend.requests().is_empty()); - } + assert_stopped_by_preflight(&scenario, &result, "no email address"); + assert!(resend.requests().is_empty()); } #[test] diff --git a/tests/user_config.rs b/tests/user_config.rs index e45ebb140..c70bde842 100644 --- a/tests/user_config.rs +++ b/tests/user_config.rs @@ -1,7 +1,8 @@ //! The User config, `~/.thirdshift/config.toml`: `logs.dir` moves the session -//! logs, and a config thirdshift can't use stops the Run before any work. -//! What each setting asks of a Run that its command says nothing about is -//! covered by the unit tests in `asks`. +//! logs, a config thirdshift can't use stops the Run before any work, and an +//! argument error comes before the config is read. What each setting asks of +//! a Run that its command says nothing about is covered by the unit tests in +//! `asks`. mod support;