From 1317c9b7bf4f38710c8b8e45913ba6476d94ed0f Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Wed, 30 Sep 2026 21:37:02 +0000 Subject: [PATCH 1/3] Collapse the looping end-to-end scenarios to one Run per behavior Each looping test in spec_run, run_notification and release keeps one case end to end and moves the rest to unit tests where the cases differ: the interrupt's signals, the Spec run's Unready labels, the Resend key lookup, and the release scripts' version, summary and bump-PR steps, which the tests now call by sourcing the scripts. The Repair and upstream-move budget tests keep their real rounds, say so, and gain unit tests of the counting. Co-Authored-By: Claude Opus 5.5 --- scripts/release-notes.sh | 5 +- scripts/release.sh | 13 +- src/interrupt.rs | 19 +++ src/resend_key.rs | 64 ++++++++-- src/run.rs | 70 +++++++++++ src/spec_run.rs | 20 ++++ tests/merge_run.rs | 11 +- tests/release.rs | 236 +++++++++++++++++++++++++----------- tests/run_notification.rs | 246 +++++++++++++++++--------------------- tests/spec_run.rs | 66 +++++----- 10 files changed, 495 insertions(+), 255 deletions(-) diff --git a/scripts/release-notes.sh b/scripts/release-notes.sh index e87a712c4..0c977f0f6 100755 --- a/scripts/release-notes.sh +++ b/scripts/release-notes.sh @@ -61,4 +61,7 @@ summary_section() { $0 == "" && !done { inside = 1 }' } -main "$@" +# Only when run, so the tests can source the script and call its functions. +if [ "${BASH_SOURCE[0]}" = "$0" ]; then + main "$@" +fi diff --git a/scripts/release.sh b/scripts/release.sh index 4d902db3b..eb513db28 100755 --- a/scripts/release.sh +++ b/scripts/release.sh @@ -205,7 +205,7 @@ refuse() { # the latest CI run on main is on and succeeded. check_can_release() { local current short=${3:0:7} latest sha status conclusion - if ! [[ $1 =~ ^(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)$ ]]; then + if ! is_plain_version "$1"; then refuse "it is not a plain X.Y.Z version, such as 0.4.0" fi current=$(git show "$3:Cargo.toml" | package_field version) @@ -234,6 +234,12 @@ check_can_release() { fi } +# is_plain_version +# Whether is plain semver X.Y.Z, with no leading zeros. +is_plain_version() { + [[ $1 =~ ^(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)$ ]] +} + # is_higher # Whether plain semver is higher than , part by part. is_higher() { @@ -427,4 +433,7 @@ merge_commit_of() { echo "$merge" } -main "$@" +# Only when run, so the tests can source the script and call its functions. +if [ "${BASH_SOURCE[0]}" = "$0" ]; then + main "$@" +fi diff --git a/src/interrupt.rs b/src/interrupt.rs index 103abd8b0..904dc0f58 100644 --- a/src/interrupt.rs +++ b/src/interrupt.rs @@ -26,3 +26,22 @@ pub fn requested() -> bool { .get() .is_some_and(|flag| flag.load(Ordering::SeqCst)) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn sigint_sigterm_and_sighup_are_each_recorded_as_an_interrupt() { + install().unwrap(); + let flag = REQUESTED.get().unwrap(); + for signal in [SIGINT, SIGTERM, SIGHUP] { + flag.store(false, Ordering::SeqCst); + + signal_hook::low_level::raise(signal).unwrap(); + + assert!(requested(), "signal {signal}"); + } + flag.store(false, Ordering::SeqCst); + } +} diff --git a/src/resend_key.rs b/src/resend_key.rs index 0d42db9df..a96d4111e 100644 --- a/src/resend_key.rs +++ b/src/resend_key.rs @@ -66,13 +66,16 @@ impl ResendKey { /// The key in `RESEND_API_KEY`, if it is set and not empty. fn from_environment() -> Option { - std::env::var("RESEND_API_KEY") - .ok() - .filter(|key| !key.is_empty()) - .map(|secret| ResendKey { - secret, - source: Source::Environment, - }) + environment_key(std::env::var("RESEND_API_KEY").ok()) +} + +/// The key in `RESEND_API_KEY` when it holds `value`: none if it is unset or +/// empty, so the Credentials are read instead. +fn environment_key(value: Option) -> Option { + value.filter(|key| !key.is_empty()).map(|secret| ResendKey { + secret, + source: Source::Environment, + }) } /// The Credentials, as read: the file's text, if there is one, and the key @@ -253,3 +256,50 @@ fn missing(path: &Path) -> String { path.display() ) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn resend_api_key_unset_or_empty_holds_no_key() { + for value in [None, Some(String::new())] { + assert!(environment_key(value.clone()).is_none(), "{value:?}"); + } + let key = environment_key(Some("re_test_123".into())).unwrap(); + assert_eq!(key.secret, "re_test_123"); + assert_eq!(key.source, Source::Environment); + } + + #[test] + fn the_credentials_key_is_read_and_an_empty_one_is_none() { + let file = Source::Credentials(PathBuf::from("/home/me/.thirdshift/credentials.toml")); + + assert_eq!( + parse("[resend]\nkey = \"re_file_456\"\n", &file).unwrap(), + Some("re_file_456".to_string()) + ); + for text in ["", "[resend]\n", "[resend]\nkey = \"\"\n"] { + assert_eq!(parse(text, &file).unwrap(), None, "{text:?}"); + } + } + + #[test] + fn broken_credentials_are_an_error_naming_the_file_and_what_is_wrong() { + let path = "/home/me/.thirdshift/credentials.toml"; + let file = Source::Credentials(PathBuf::from(path)); + for (text, named) in [ + ("[resend\n", "can't parse"), + ("[resend]\nkye = \"re_file_456\"\n", "resend.kye"), + ("[resend]\nkey = true\n", "resend.key"), + ("resend = 1\n", "resend must be the section [resend]"), + ("[email]\n", "unknown section [email]"), + ("key = \"re_file_456\"\n", "unknown key key"), + ] { + let error = format!("{:#}", parse(text, &file).unwrap_err()); + for part in [named, path] { + assert!(error.contains(part), "{text:?}: no {part:?} in {error}"); + } + } + } +} diff --git a/src/run.rs b/src/run.rs index b2c064fe2..7f68f3ef8 100644 --- a/src/run.rs +++ b/src/run.rs @@ -557,3 +557,73 @@ fn ensure_pr_ready_and_mergeable(issue: &IssueUrl, branch: &str) -> Result<()> { ), } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn repairs_of_every_kind_share_one_cap_and_the_one_too_many_names_its_cause() { + let mut budgets = Budgets::default(); + for (n, cause) in ["conflict", "CI red", "CI red", "conflict", "CI red"] + .iter() + .enumerate() + { + assert_eq!( + budgets.next_repair(cause).unwrap(), + format!("repair-{}", n + 1) + ); + } + + for cause in ["new commits on origin/issue-7 to review", "conflict"] { + let error = budgets.next_repair(cause).unwrap_err(); + assert_eq!(error.to_string(), format!("repairs exhausted: {cause}")); + } + assert_eq!(budgets.repairs, MAX_REPAIRS); + } + + #[test] + fn upstream_moves_of_the_base_or_the_issue_branch_share_one_budget() { + let mut budgets = Budgets::default(); + for _ in 0..3 { + budgets.count_base_move("main", "while CI ran").unwrap(); + } + for _ in 0..2 { + budgets + .count_upstream_move("origin/issue-7", format_args!("new commits")) + .unwrap(); + } + + let error = budgets.count_base_move("main", "while CI ran").unwrap_err(); + assert_eq!( + error.to_string(), + "origin/main kept moving: merged it again 5 times" + ); + let error = budgets + .count_upstream_move("origin/issue-7", format_args!("new commits")) + .unwrap_err(); + assert_eq!( + error.to_string(), + "origin/issue-7 kept moving: merged it again 5 times" + ); + } + + #[test] + fn the_repair_cap_and_the_upstream_move_budget_are_spent_apart() { + let mut budgets = Budgets::default(); + for _ in 0..MAX_REPAIRS { + budgets.next_repair("CI red").unwrap(); + } + + for _ in 0..MAX_UPSTREAM_MOVES { + budgets.count_base_move("main", "while CI ran").unwrap(); + } + assert_eq!( + budgets, + Budgets { + repairs: MAX_REPAIRS, + upstream_moves: MAX_UPSTREAM_MOVES + } + ); + } +} diff --git a/src/spec_run.rs b/src/spec_run.rs index 5325a15e4..6a4ee6d8e 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -676,6 +676,26 @@ mod tests { assert_eq!(next_ready(&tickets, &outcomes, &HashSet::new()), Some(23)); } + #[test] + fn each_unready_label_keeps_a_ticket_from_running_and_is_named_in_its_line() { + for label in UNREADY_LABELS { + let mut unready = ticket(21, true, &[], &[]); + unready.labels = vec!["ready-for-agent".to_string(), label.to_string()]; + let tickets = [unready, ticket(22, true, &[], &[])]; + let outcomes = BTreeMap::new(); + + assert_eq!( + next_ready(&tickets, &outcomes, &HashSet::new()), + Some(22), + "{label}" + ); + assert_eq!( + summarize(&tickets, &outcomes)[0], + format!("#21 unready: labelled {label}") + ); + } + } + #[test] fn the_checklist_ticks_done_tickets_and_says_where_each_other_stands() { let mut unready = ticket(26, true, &[], &[]); diff --git a/tests/merge_run.rs b/tests/merge_run.rs index 99c09aa7d..3ce625439 100644 --- a/tests/merge_run.rs +++ b/tests/merge_run.rs @@ -178,6 +178,7 @@ fn the_merge_flag_also_starts_a_merge_run() { #[test] fn a_merge_run_gives_the_agent_the_same_implement_prompt_as_a_run() { + // A Run and a Merge run, as the behavior is that their prompts agree. let run = Scenario::new(); run.agent_does(AGENT_OPENS_PR); let merge_run = Scenario::new(); @@ -668,6 +669,8 @@ echo '{"type": "system", "subtype": "task_updated", "task_id": "b1", "patch": {" #[test] fn foreign_commits_that_keep_coming_spend_the_upstream_move_budget() { + // Five real rounds, as the budget runs out only after them. The Run's unit + // tests take the counting. let scenario = Scenario::new(); scenario.agent_does_in_session( 1, @@ -703,7 +706,8 @@ fn foreign_commits_that_keep_coming_spend_the_upstream_move_budget() { fn a_review_repair_counts_against_the_repair_cap() { let scenario = Scenario::new(); // Five CI-fix Repairs leave none for reviewing the Foreign commit pushed - // while CI runs on the fifth fix. + // while CI runs on the fifth fix. They are real Repairs, as the cap runs + // out only after them; the Run's unit tests take the counting. scenario.agent_does_in_session(1, &format!("{AGENT_OPENS_PR}{}", checks_on_head(RED))); for session in 2..=6 { let (checks, then) = if session == 6 { @@ -897,6 +901,8 @@ fn a_merge_that_fails_then_finds_red_ci_on_a_new_head_gets_a_ci_fix_repair_then_ #[test] fn merge_failures_that_keep_moving_the_base_branch_spend_the_base_move_budget() { + // Six real merge attempts, as the budget runs out only after them. The + // Run's unit tests take the counting. let scenario = Scenario::new(); scenario.agent_does(&format!( "{AGENT_OPENS_PR}{}{}", @@ -931,7 +937,8 @@ fn merge_failures_that_keep_moving_the_base_branch_spend_the_base_move_budget() fn a_repair_needed_after_a_failed_merge_counts_against_the_repair_cap() { let scenario = Scenario::new(); // Five CI-fix Repairs before the first merge attempt leave none for the - // conflict the merge then runs into. + // conflict the merge then runs into. They are real Repairs, as the cap + // runs out only after them; the Run's unit tests take the counting. scenario.agent_does(&format!( "{AGENT_OPENS_PR}{}{}", checks_on_head(RED), diff --git a/tests/release.rs b/tests/release.rs index 090ba61ba..012fc3b51 100644 --- a/tests/release.rs +++ b/tests/release.rs @@ -1,7 +1,8 @@ //! The maintainer's release script, `scripts/release.sh`, run as a black box //! against a bare local origin with the fake `gh` and `claude` on PATH and //! the user's git configuration kept out, as the site deploy tests run the -//! publish script. +//! publish script. Where a step has several cases, one goes through the whole +//! script and the rest call the step's function, with the script sourced. //! //! The script, like the fake `gh` it drives here, relies on GNU tools, so its //! tests run on Linux only. @@ -404,6 +405,25 @@ fn stderr(output: &Output) -> String { String::from_utf8_lossy(&output.stderr).into_owned() } +fn stdout(output: &Output) -> String { + String::from_utf8(output.stdout.clone()).unwrap() +} + +/// Sources `script` from `scripts/` without running it, then runs the bash +/// `code`, which calls its functions, in an empty directory. The cases of a +/// step go here, and one case of each through the whole script. +fn with_functions_of(script: &str, code: &str) -> Output { + let dir = TempDir::new().unwrap(); + isolated(&mut Command::new("bash")) + .arg("-c") + .arg(format!("source \"$1\"\n{code}")) + .arg("bash") + .arg(manifest_dir().join("scripts").join(script)) + .current_dir(dir.path()) + .output() + .expect("could not run bash") +} + const GREEN: &str = r#"[{"name": "test", "conclusion": "success", "pending_polls": 2}]"#; const RED: &str = r#"[{"name": "test", "conclusion": "failure"}]"#; const AGENT_SUMMARY: &str = "The headline is the frobnicator (#2)."; @@ -530,37 +550,62 @@ fn the_pr_body_has_the_agents_summary_in_a_marked_section_then_the_version_diff( #[test] #[cfg(target_os = "linux")] -fn when_the_agent_fails_or_prints_nothing_the_summary_is_the_generated_notes() { - for (case, script) in [ - ("fails", "echo 'Not logged in' >&2; exit 1"), - ("prints nothing", "printf '\\n'"), - ] { - let release = Release::new(); - release.ci_reports(GREEN); - release.agent_runs(script); +fn when_the_agent_fails_the_summary_is_the_generated_notes() { + // One way to fail end to end; the summary step's own test takes the rest. + let release = Release::new(); + release.ci_reports(GREEN); + release.agent_runs("echo 'Not logged in' >&2; exit 1"); - let output = release.run_script("0.2.0"); + let output = release.run_script("0.2.0"); - assert!(output.status.success(), "{case}: {}", stderr(&output)); - let err = stderr(&output); - assert!( - err.lines() - .any(|line| line.starts_with("release: warning:") && line.contains("agent summary")), - "{case}: {err}" - ); - let body = release.pr_body("release-0.2.0"); - let (summary, rest) = summary_and_rest(&body); - let (note, notes) = summary.split_once("\n\n").expect(&body); - assert!( - !note.contains('\n') && note.contains("agent summary was unavailable"), - "{case}: {body}" + assert!(output.status.success(), "{}", stderr(&output)); + let err = stderr(&output); + assert!( + err.lines() + .any(|line| line.starts_with("release: warning:") && line.contains("agent summary")), + "{err}" + ); + let body = release.pr_body("release-0.2.0"); + let (summary, rest) = summary_and_rest(&body); + let (note, notes) = summary.split_once("\n\n").expect(&body); + assert!( + !note.contains('\n') && note.contains("agent summary was unavailable"), + "{body}" + ); + assert!(notes.starts_with("## What's Changed\n"), "{body}"); + assert!(notes.contains("Add the frobnicator"), "{body}"); + assert!(!notes.contains("Shipped in 0.1.0"), "{body}"); + assert!(rest.contains("```diff"), "{body}"); + let merge = release.origin(&["rev-parse", "main"]); + assert_eq!(release.origin_tag("v0.2.0"), Some(merge)); +} + +#[test] +#[cfg(target_os = "linux")] +fn the_summary_is_the_agents_unless_it_fails_or_prints_only_blank_space() { + let notes = "_The agent summary was unavailable, so this is GitHub's generated notes._\n\n\ + notes: api repos/{owner}/{repo}/releases/generate-notes \ + -f tag_name=v0.2.0 -f target_commitish=main --jq .body\n"; + for (agent, summary) in [ + ("echo 'Not logged in' >&2; exit 1", notes), + ("printf '\\n'", notes), + ("printf ' \\n\\t\\n'", notes), + ("echo 'The headline.'", "The headline.\n"), + ] { + let output = with_functions_of( + "release.sh", + &format!( + "summary_input() {{ echo input; }}\n\ + gh() {{ echo \"notes: $*\"; }}\n\ + claude() {{ cat >/dev/null; {agent}; }}\n\ + release_summary main v0.2.0 diff\n" + ), ); - assert!(notes.starts_with("## What's Changed\n"), "{case}: {body}"); - assert!(notes.contains("Add the frobnicator"), "{case}: {body}"); - assert!(!notes.contains("Shipped in 0.1.0"), "{case}: {body}"); - assert!(rest.contains("```diff"), "{case}: {body}"); - let merge = release.origin(&["rev-parse", "main"]); - assert_eq!(release.origin_tag("v0.2.0"), Some(merge), "{case}"); + + assert!(output.status.success(), "{agent}: {}", stderr(&output)); + assert_eq!(stdout(&output), summary, "{agent}"); + let warned = stderr(&output).contains("release: warning: the agent summary is unavailable"); + assert_eq!(warned, summary == notes, "{agent}: {}", stderr(&output)); } } @@ -783,37 +828,60 @@ fn the_release_body_is_the_bump_prs_summary_then_the_generated_notes_then_the_in #[test] #[cfg(target_os = "linux")] fn without_a_bump_pr_summary_the_release_body_is_the_generated_notes_and_install_instructions() { + // v0.1.0 is the merge of a PR without a summary section. The bump PR + // step's own test takes the other ways to have no summary. let release = Release::new(); - // A PR with a summary section whose head commit, not its merge, is tagged. - release.merge_pr( - "summarised", - "Summarised", - "\nNot this release.\n", - ); - let contributor = release.contributor(); - git(&contributor, &["tag", "v0.1.2", "origin/summarised"]); - // A commit pushed straight to main, not through a PR. - fs::write(contributor.join("hand.txt"), "by hand").unwrap(); - git(&contributor, &["add", "-A"]); - git(&contributor, &["commit", "-q", "-m", "By hand"]); - git(&contributor, &["push", "-q", "origin", "HEAD:main"]); - git(&contributor, &["tag", "v0.1.3"]); - git(&contributor, &["push", "-q", "origin", "v0.1.2", "v0.1.3"]); - - for (case, tag) in [ - ("the merge of a PR without a summary section", "v0.1.0"), - ("the head of a PR, not its merge", "v0.1.2"), - ("a commit with no PR", "v0.1.3"), + + let output = release.run_notes_script("v0.1.0"); + + assert!(output.status.success(), "{}", stderr(&output)); + let notes = release.generated_notes("v0.1.0"); + assert_eq!(stdout(&output), format!("{notes}\n\n{INSTALL_NOTES}\n")); +} + +#[test] +#[cfg(target_os = "linux")] +fn the_summary_is_only_that_of_a_pr_merged_as_the_tagged_commit() { + const TAGGED: &str = "1111111111111111111111111111111111111111"; + const OTHER: &str = "2222222222222222222222222222222222222222"; + let with_summary = "Intro.\n\nThe headline.\n\ + \nRest."; + for (case, pulls, body, summary) in [ + ( + "the merge of a PR with a summary section", + format!("{OTHER} 4\n{TAGGED} 5"), + with_summary, + "The headline.\n", + ), + ( + "the merge of a PR without a summary section", + format!("{TAGGED} 5"), + "Old news.", + "", + ), + ( + "the head of a PR, not its merge", + format!("{OTHER} 5"), + with_summary, + "", + ), + ("a commit with no PR", String::new(), with_summary, ""), ] { - let output = release.run_notes_script(tag); + let output = with_functions_of( + "release-notes.sh", + &format!( + "gh() {{\n\ + \tcase $1 in\n\ + \tapi) printf '%s' '{pulls}' ;;\n\ + \tpr) [ \"$3\" = 5 ] && printf '%s\\n' '{body}' ;;\n\ + \tesac\n\ + }}\n\ + bump_summary {TAGGED}\n" + ), + ); assert!(output.status.success(), "{case}: {}", stderr(&output)); - let notes = release.generated_notes(tag); - assert_eq!( - String::from_utf8(output.stdout).unwrap(), - format!("{notes}\n\n{INSTALL_NOTES}\n"), - "{case}" - ); + assert_eq!(stdout(&output), summary, "{case}"); } } @@ -868,29 +936,19 @@ fn assert_refused(release: &Release, version: &str, reasons: &[&str]) { #[test] #[cfg(target_os = "linux")] fn a_version_that_is_not_plain_semver_is_refused() { + // One version end to end; the version check's own test takes the rest. let release = Release::new(); - for version in [ - "v0.4.0", - "0.4", - "abc", - "0.4.0-rc.1", - "01.4.0", - "0.4.0.1", - "", - ] { - assert_refused(&release, version, &["not a plain X.Y.Z version"]); - } + assert_refused(&release, "0.4.0-rc.1", &["not a plain X.Y.Z version"]); } #[test] #[cfg(target_os = "linux")] fn a_version_not_higher_than_the_one_on_main_is_refused() { + // One version end to end; the comparison's own test takes the rest. let release = Release::new(); - for version in ["0.1.0", "0.0.9", "0.0.10"] { - assert_refused(&release, version, &["not higher than 0.1.0 on main"]); - } + assert_refused(&release, "0.0.10", &["not higher than 0.1.0 on main"]); } #[test] @@ -905,6 +963,44 @@ fn versions_are_compared_as_numbers_not_text() { assert!(release.origin_tag("v0.10.0").is_some()); } +#[test] +#[cfg(target_os = "linux")] +fn only_plain_x_y_z_versions_with_no_leading_zeros_pass() { + for (version, plain) in [ + ("0.4.0", true), + ("10.0.12", true), + ("v0.4.0", false), + ("0.4", false), + ("abc", false), + ("0.4.0-rc.1", false), + ("01.4.0", false), + ("0.4.0.1", false), + ("", false), + ] { + let output = with_functions_of("release.sh", &format!("is_plain_version '{version}'")); + + assert_eq!(output.status.success(), plain, "{version:?}"); + } +} + +#[test] +#[cfg(target_os = "linux")] +fn a_version_is_higher_only_by_its_parts_as_numbers() { + for (version, higher) in [ + ("0.1.1", true), + ("0.2.0", true), + ("0.10.0", true), + ("1.0.0", true), + ("0.1.0", false), + ("0.0.9", false), + ("0.0.10", false), + ] { + let output = with_functions_of("release.sh", &format!("is_higher {version} 0.1.0")); + + assert_eq!(output.status.success(), higher, "{version}"); + } +} + #[test] #[cfg(target_os = "linux")] fn a_tag_that_exists_locally_is_refused() { diff --git a/tests/run_notification.rs b/tests/run_notification.rs index 8c8098d76..a96f080c6 100644 --- a/tests/run_notification.rs +++ b/tests/run_notification.rs @@ -155,27 +155,27 @@ fn a_failed_run_sends_one_notification_with_the_cause() { #[test] fn an_interrupted_run_sends_one_notification_that_it_was_interrupted() { - for signal in ["INT", "TERM", "HUP"] { - let scenario = Scenario::new(); - let started = scenario.path("agent-started"); - scenario.agent_does(&format!("touch {}\nsleep 30\n", started.display())); - let resend = ResendStandIn::replying(200, ACCEPTED); + // One signal end to end; the interrupt's unit tests take SIGINT, SIGTERM + // and SIGHUP alike. + let scenario = Scenario::new(); + let started = scenario.path("agent-started"); + scenario.agent_does(&format!("touch {}\nsleep 30\n", started.display())); + let resend = ResendStandIn::replying(200, ACCEPTED); - let result = scenario.run_and_signal_with_env( - &[&scenario.issue_url(7), "--email", "me@example.com"], - &env(&resend, Some(KEY)), - "agent-started", - signal, - ); + let result = scenario.run_and_signal_with_env( + &[&scenario.issue_url(7), "--email", "me@example.com"], + &env(&resend, Some(KEY)), + "agent-started", + "TERM", + ); - assert_eq!(result.code, Some(1), "{signal}: {}", result.stderr); - let request = the_one_request(&resend); - assert!( - subject(&request).ends_with(": interrupted"), - "{signal}: {}", - subject(&request) - ); - } + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + let request = the_one_request(&resend); + assert!( + subject(&request).ends_with(": interrupted"), + "{}", + subject(&request) + ); } #[test] @@ -272,21 +272,20 @@ fn with_no_address_known_the_run_stops_before_any_work_and_sends_nothing() { #[test] fn without_resend_api_key_the_run_stops_before_any_work_and_sends_nothing() { - for key in [None, Some("")] { - let scenario = Scenario::new(); - let resend = ResendStandIn::replying(200, ACCEPTED); + // Unset end to end; the key lookup's unit tests take an empty one alike. + let scenario = Scenario::new(); + let resend = ResendStandIn::replying(200, ACCEPTED); - let result = run( - &scenario, - &resend, - &[&scenario.issue_url(7), "email", "me@example.com"], - key, - ); + let result = run( + &scenario, + &resend, + &[&scenario.issue_url(7), "email", "me@example.com"], + None, + ); - scenario.assert_rejected_before_any_work(&result, "RESEND_API_KEY"); - assert!(scenario.gh_calls().is_empty(), "{:?}", scenario.gh_calls()); - assert!(resend.requests().is_empty()); - } + scenario.assert_rejected_before_any_work(&result, "RESEND_API_KEY"); + assert!(scenario.gh_calls().is_empty(), "{:?}", scenario.gh_calls()); + assert!(resend.requests().is_empty()); } #[test] @@ -304,40 +303,32 @@ fn a_run_without_the_flag_sends_nothing() { #[test] fn a_failed_send_is_a_warning_that_changes_neither_the_exit_code_nor_stdout() { - for (status, reply, message) in [ - ( - 401, - r#"{"statusCode":401,"message":"API key is invalid","name":"validation_error"}"#, - "API key is invalid", - ), - (500, "upstream exploded", "upstream exploded"), - ] { - for (agent, code, stdout) in [ - (AGENT_OPENS_PR, 0, format!("{PR_URL}\n")), - ("exit 3", 1, String::new()), - ] { - let scenario = Scenario::new(); - scenario.agent_does(agent); - let resend = ResendStandIn::replying(status, reply); - - let result = run( - &scenario, - &resend, - &[&scenario.issue_url(7), "--email", "me@example.com"], - Some(KEY), - ); - - assert_eq!(result.code, Some(code), "{status}: {}", result.stderr); - assert_eq!(result.stdout, stdout, "{status}"); - assert_eq!(resend.requests().len(), 1, "{status}"); - let warning = result - .stderr - .lines() - .find(|line| line.contains("warning:")) - .unwrap_or_else(|| panic!("no warning in stderr: {}", result.stderr)); - assert_contains(warning, message); - } - } + // One reply end to end; the email's unit tests take the other shapes of + // reply. The exit code and stdout are settled before the send, whatever + // the outcome. + let scenario = Scenario::new(); + scenario.agent_does(AGENT_OPENS_PR); + let resend = ResendStandIn::replying( + 401, + r#"{"statusCode":401,"message":"API key is invalid","name":"validation_error"}"#, + ); + + let result = run( + &scenario, + &resend, + &[&scenario.issue_url(7), "--email", "me@example.com"], + Some(KEY), + ); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{PR_URL}\n")); + assert_eq!(resend.requests().len(), 1); + let warning = result + .stderr + .lines() + .find(|line| line.contains("warning:")) + .unwrap_or_else(|| panic!("no warning in stderr: {}", result.stderr)); + assert_contains(warning, "API key is invalid"); } /// The User config of a machine where every Run sends a Run notification. @@ -397,39 +388,35 @@ fn with_email_always_an_address_after_the_flag_still_wins() { } #[test] -fn email_always_without_an_address_or_a_key_stops_the_run_before_any_work() { - for (config, key, named) in [ - ("[email]\nalways = true\n", Some(KEY), "no email address"), - (EMAIL_ALWAYS, None, "RESEND_API_KEY"), - ] { - let scenario = Scenario::new(); - scenario.user_config_is(config); - let resend = ResendStandIn::replying(200, ACCEPTED); +fn email_always_without_an_address_stops_the_run_before_any_work() { + // Without a key it stops the same way as with the flag, which + // without_resend_api_key_the_run_stops_before_any_work_and_sends_nothing + // shows; the User config's unit tests 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); - let result = run(&scenario, &resend, &[&scenario.issue_url(7)], key); + let result = run(&scenario, &resend, &[&scenario.issue_url(7)], Some(KEY)); - scenario.assert_rejected_before_any_work(&result, named); - assert!(scenario.gh_calls().is_empty(), "{:?}", scenario.gh_calls()); - assert!(resend.requests().is_empty()); - } + scenario.assert_rejected_before_any_work(&result, "no email address"); + assert!(scenario.gh_calls().is_empty(), "{:?}", scenario.gh_calls()); + assert!(resend.requests().is_empty()); } #[test] -fn without_email_always_a_run_without_the_flag_sends_nothing() { - for config in [ - "[email]\nalways = false\nto = \"me@example.com\"\n", - "[email]\nto = \"me@example.com\"\n", - ] { - let scenario = Scenario::new(); - scenario.user_config_is(config); - scenario.agent_does(AGENT_OPENS_PR); - let resend = ResendStandIn::replying(200, ACCEPTED); +fn email_always_false_makes_a_run_without_the_flag_send_nothing() { + // With no email.always, a_run_without_the_flag_sends_nothing shows the + // same; 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)); + let result = run(&scenario, &resend, &[&scenario.issue_url(7)], Some(KEY)); - assert_eq!(result.code, Some(0), "{config}: {}", result.stderr); - assert!(resend.requests().is_empty(), "{config}"); - } + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert!(resend.requests().is_empty()); } /// Credentials holding the key `re_file_456`. @@ -437,30 +424,25 @@ const CREDENTIALS: &str = "[resend]\nkey = \"re_file_456\"\n"; #[test] fn without_resend_api_key_a_run_sends_with_the_key_in_the_credentials() { - for (config, key) in [(None, None), (None, Some("")), (Some(EMAIL_ALWAYS), None)] { - let scenario = Scenario::new(); - if let Some(config) = config { - scenario.user_config_is(config); - } - scenario.credentials_are(CREDENTIALS); - scenario.agent_does(AGENT_OPENS_PR); - let resend = ResendStandIn::replying(200, ACCEPTED); - let url = scenario.issue_url(7); - let args: &[&str] = match config { - Some(_) => &[&url], - None => &[&url, "--email", "me@example.com"], - }; + // Unset end to end; the key lookup's unit tests take an empty one alike, + // and email.always finds the key the same way as the flag. + let scenario = Scenario::new(); + scenario.credentials_are(CREDENTIALS); + scenario.agent_does(AGENT_OPENS_PR); + let resend = ResendStandIn::replying(200, ACCEPTED); - let result = run(&scenario, &resend, args, key); + let result = run( + &scenario, + &resend, + &[&scenario.issue_url(7), "--email", "me@example.com"], + None, + ); - assert_eq!(result.code, Some(0), "{key:?}: {}", result.stderr); - let request = the_one_request(&resend); - assert_eq!( - request.authorization.as_deref(), - Some("Bearer re_file_456"), - "{key:?}" - ); - } + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!( + the_one_request(&resend).authorization.as_deref(), + Some("Bearer re_file_456") + ); } #[test] @@ -511,29 +493,25 @@ fn with_no_key_anywhere_the_run_stops_before_any_work_listing_every_way_to_give_ #[test] fn broken_credentials_stop_a_run_that_asks_for_a_notification_before_any_work() { - for (credentials, named) in [ - ("[resend\n", "can't parse"), - ("[resend]\nkye = \"re_file_456\"\n", "resend.kye"), - ("[resend]\nkey = true\n", "resend.key"), - ] { - let scenario = Scenario::new(); - let path = scenario.credentials_are(credentials); - let resend = ResendStandIn::replying(200, ACCEPTED); + // One way to break them end to end; the key lookup's unit tests take the + // rest. + let scenario = Scenario::new(); + let path = scenario.credentials_are("[resend]\nkye = \"re_file_456\"\n"); + let resend = ResendStandIn::replying(200, ACCEPTED); - let result = run( - &scenario, - &resend, - &[&scenario.issue_url(7), "--email", "me@example.com"], - None, - ); + let result = run( + &scenario, + &resend, + &[&scenario.issue_url(7), "--email", "me@example.com"], + None, + ); - assert_eq!(result.code, Some(1), "{credentials:?}: {}", result.stderr); - for part in [named, &path.display().to_string()] { - scenario.assert_rejected_before_any_work(&result, part); - } - assert!(scenario.gh_calls().is_empty(), "{:?}", scenario.gh_calls()); - assert!(resend.requests().is_empty(), "{credentials:?}"); + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + for part in ["resend.kye", &path.display().to_string()] { + scenario.assert_rejected_before_any_work(&result, part); } + assert!(scenario.gh_calls().is_empty(), "{:?}", scenario.gh_calls()); + assert!(resend.requests().is_empty()); } #[test] diff --git a/tests/spec_run.rs b/tests/spec_run.rs index c4babdbb1..9ad16b022 100644 --- a/tests/spec_run.rs +++ b/tests/spec_run.rs @@ -491,6 +491,8 @@ fn a_policy_refusal_on_the_spec_pr_leaves_it_ready_and_exits_1() { #[test] fn repairs_exhausted_on_the_spec_pr_send_it_back_to_draft_and_exit_1() { + // Five real Repairs, as the cap runs out only after them. The Run's unit + // tests take the counting. let scenario = linear_spec(); scenario.agent_does_for_in_session(SPEC, 1, &checks_on_head(RED)); for session in 2..=6 { @@ -1078,28 +1080,25 @@ fn a_needs_info_ticket_and_what_it_blocks_are_not_run_while_an_independent_ticke } #[test] -fn each_unready_label_keeps_a_ticket_from_running() { - for label in ["ready-for-human", "wontfix", "needs-triage"] { - let scenario = spec_of(&[(21, &[]), (22, &[])]); - scenario.issue_labelled(21, &[label]); - scenario.issue_labelled(22, &["ready-for-agent"]); +fn an_unready_label_keeps_a_ticket_from_running() { + // One label end to end; the Spec run's unit tests take each of them. + let scenario = spec_of(&[(21, &[]), (22, &[])]); + scenario.issue_labelled(21, &["wontfix"]); + scenario.issue_labelled(22, &["ready-for-agent"]); - let result = scenario.run(&[&spec_url(&scenario)]); + let result = scenario.run(&[&spec_url(&scenario)]); - assert_failed_spec_run( - &scenario, - &result, - &format!( - "- [ ] #21 unready: labelled {label}\n\ - - [x] #22 landed with https://github.com/acme/widgets/pull/1\n" - ), - ); - assert_eq!(sessions_by_issue(&scenario), ["22"], "{label}"); - assert_contains( - &result.stderr, - &format!("thirdshift: #21 unready: labelled {label}\n"), - ); - } + assert_failed_spec_run( + &scenario, + &result, + "- [ ] #21 unready: labelled wontfix\n\ + - [x] #22 landed with https://github.com/acme/widgets/pull/1\n", + ); + assert_eq!(sessions_by_issue(&scenario), ["22"]); + assert_contains( + &result.stderr, + "thirdshift: #21 unready: labelled wontfix\n", + ); } #[test] @@ -1237,25 +1236,13 @@ fn help_explains_unready_tickets_and_that_a_spec_run_takes_every_ticket_it_can_r } } +/// SIGTERM to the Spec run's process alone while #21's agent is at work, +/// leaving it half done: #21's Run pushes that work as a failed run, #22 +/// never starts, and the Spec run ends only after, as interrupted. One +/// signal end to end; the interrupt's unit tests take SIGINT, SIGTERM and +/// SIGHUP alike. #[test] -fn sigterm_while_a_ticket_runs_ends_it_through_its_failed_run_and_the_spec_run_as_interrupted() { - assert_interrupt_fails_the_spec_run("TERM"); -} - -#[test] -fn sighup_while_a_ticket_runs_ends_it_through_its_failed_run_and_the_spec_run_as_interrupted() { - assert_interrupt_fails_the_spec_run("HUP"); -} - -#[test] -fn sigint_to_the_spec_run_alone_is_passed_on_to_the_ticket_run() { - assert_interrupt_fails_the_spec_run("INT"); -} - -/// Send `signal` to the Spec run's process alone while #21's agent is at -/// work, leaving it half done: #21's Run pushes that work as a failed run, -/// #22 never starts, and the Spec run ends only after, as interrupted. -fn assert_interrupt_fails_the_spec_run(signal: &str) { +fn a_signal_to_the_spec_run_alone_fails_the_ticket_run_then_ends_the_spec_run_as_interrupted() { let scenario = linear_spec(); let started = scenario.path("agent-started"); scenario.agent_does_for( @@ -1267,7 +1254,7 @@ fn assert_interrupt_fails_the_spec_run(signal: &str) { ); let began = std::time::Instant::now(); - let result = scenario.run_and_signal(&[&spec_url(&scenario)], "agent-started", signal); + let result = scenario.run_and_signal(&[&spec_url(&scenario)], "agent-started", "TERM"); assert!( began.elapsed() < std::time::Duration::from_secs(20), @@ -1664,6 +1651,7 @@ exit 1 #[test] fn rerunning_a_failed_spec_run_continues_the_spec_branch_and_the_failed_tickets_issue_branch() { + // Two Spec runs, as the rerun is the behavior. let scenario = spec_of(&[(21, &[]), (22, &[21])]); scenario.agent_does_for_in_session(22, 1, TICKET_22_STARTS_THEN_FAILS); scenario.agent_does_for_in_session( From d64682ca5d1551ba8176d80b816bfcb3f6e7af4b Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Wed, 30 Sep 2026 21:49:59 +0000 Subject: [PATCH 2/3] Take no-email and parallel spellings in argument-parsing unit tests Co-Authored-By: Claude Opus 5.5 --- src/args.rs | 20 ++++++++++++++++++++ tests/run_notification.rs | 19 +++++++++---------- tests/spec_run.rs | 25 ++++++++++++------------- 3 files changed, 41 insertions(+), 23 deletions(-) diff --git a/src/args.rs b/src/args.rs index 408803c20..5fa3fe0f0 100644 --- a/src/args.rs +++ b/src/args.rs @@ -202,4 +202,24 @@ mod tests { assert_eq!(run_args.goal, goal, "{args:?}"); } } + + #[test] + 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, + Some(NotificationAsk::Skip), + "{flag}" + ); + } + } + + #[test] + 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:?}"); + } + } + } } diff --git a/tests/run_notification.rs b/tests/run_notification.rs index f86b1a674..2dd5ffcc6 100644 --- a/tests/run_notification.rs +++ b/tests/run_notification.rs @@ -345,18 +345,17 @@ fn email_always_makes_a_run_without_the_flag_send_one_notification_to_email_to() #[test] fn no_email_skips_the_notification_email_always_asks_for() { - for no_email in ["no-email", "--no-email"] { - 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); + // 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)); + let result = run(&scenario, &resend, &["--no-email", &url], Some(KEY)); - assert_eq!(result.code, Some(0), "{no_email}: {}", result.stderr); - assert!(resend.requests().is_empty(), "{no_email}"); - } + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert!(resend.requests().is_empty()); } #[test] diff --git a/tests/spec_run.rs b/tests/spec_run.rs index 9ad16b022..f69b7af6c 100644 --- a/tests/spec_run.rs +++ b/tests/spec_run.rs @@ -959,19 +959,18 @@ fn two_tickets_starting_together_in_one_launch_directory_both_get_their_worktree #[test] fn parallel_1_runs_the_tickets_one_at_a_time() { - for args in [["parallel", "1"], ["--parallel", "1"]] { - let scenario = diamond_spec(); - second_ticket_needs_the_first_landed(&scenario); - let url = spec_url(&scenario); - - let result = scenario.run(&[args[0], args[1], &url]); - - assert_eq!(result.code, Some(0), "{args:?} stderr: {}", result.stderr); - assert_eq!(sessions_by_issue(&scenario), ["21", "22", "23", "20"]); - let landed = result.stderr.find("#21 landed\n").unwrap(); - let started = result.stderr.find("starting #22\n").unwrap(); - assert!(landed < started, "stderr: {}", result.stderr); - } + // One spelling end to end; the argument parsing's unit tests take both. + let scenario = diamond_spec(); + second_ticket_needs_the_first_landed(&scenario); + let url = spec_url(&scenario); + + let result = scenario.run(&["--parallel", "1", &url]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(sessions_by_issue(&scenario), ["21", "22", "23", "20"]); + let landed = result.stderr.find("#21 landed\n").unwrap(); + let started = result.stderr.find("starting #22\n").unwrap(); + assert!(landed < started, "stderr: {}", result.stderr); } #[test] From f233fb50738d3eea240511ba9dfe7c4fb940d89a Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Wed, 30 Sep 2026 21:54:26 +0000 Subject: [PATCH 3/3] Address review: say why tests repeat a run, use the budget constants Co-Authored-By: Claude Opus 5.5 --- src/config.rs | 6 +++++- src/run.rs | 25 +++++++++---------------- tests/merge_run.rs | 2 +- tests/release.rs | 13 +++++++------ tests/run_notification.rs | 11 +++++------ tests/spec_run.rs | 11 ++++++----- 6 files changed, 33 insertions(+), 35 deletions(-) diff --git a/src/config.rs b/src/config.rs index 1ccc2e5e8..dc8f1a400 100644 --- a/src/config.rs +++ b/src/config.rs @@ -726,7 +726,11 @@ mod tests { 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"] { + for text in [ + "", + "[email]\nalways = false\n", + "[email]\nto = \"me@example.com\"\n", + ] { let config = parse(text).unwrap(); assert_eq!( config.email.default_ask(), diff --git a/src/run.rs b/src/run.rs index 7f68f3ef8..6395bfbbe 100644 --- a/src/run.rs +++ b/src/run.rs @@ -565,14 +565,9 @@ mod tests { #[test] fn repairs_of_every_kind_share_one_cap_and_the_one_too_many_names_its_cause() { let mut budgets = Budgets::default(); - for (n, cause) in ["conflict", "CI red", "CI red", "conflict", "CI red"] - .iter() - .enumerate() - { - assert_eq!( - budgets.next_repair(cause).unwrap(), - format!("repair-{}", n + 1) - ); + for n in 1..=MAX_REPAIRS { + let cause = if n % 2 == 0 { "conflict" } else { "CI red" }; + assert_eq!(budgets.next_repair(cause).unwrap(), format!("repair-{n}")); } for cause in ["new commits on origin/issue-7 to review", "conflict"] { @@ -585,26 +580,24 @@ mod tests { #[test] fn upstream_moves_of_the_base_or_the_issue_branch_share_one_budget() { let mut budgets = Budgets::default(); - for _ in 0..3 { + for _ in 1..MAX_UPSTREAM_MOVES { budgets.count_base_move("main", "while CI ran").unwrap(); } - for _ in 0..2 { - budgets - .count_upstream_move("origin/issue-7", format_args!("new commits")) - .unwrap(); - } + budgets + .count_upstream_move("origin/issue-7", format_args!("new commits")) + .unwrap(); let error = budgets.count_base_move("main", "while CI ran").unwrap_err(); assert_eq!( error.to_string(), - "origin/main kept moving: merged it again 5 times" + format!("origin/main kept moving: merged it again {MAX_UPSTREAM_MOVES} times") ); let error = budgets .count_upstream_move("origin/issue-7", format_args!("new commits")) .unwrap_err(); assert_eq!( error.to_string(), - "origin/issue-7 kept moving: merged it again 5 times" + format!("origin/issue-7 kept moving: merged it again {MAX_UPSTREAM_MOVES} times") ); } diff --git a/tests/merge_run.rs b/tests/merge_run.rs index 3ce625439..c828a8f7c 100644 --- a/tests/merge_run.rs +++ b/tests/merge_run.rs @@ -178,7 +178,7 @@ fn the_merge_flag_also_starts_a_merge_run() { #[test] fn a_merge_run_gives_the_agent_the_same_implement_prompt_as_a_run() { - // A Run and a Merge run, as the behavior is that their prompts agree. + // Two Runs, one of them a Merge run: the behavior is that they agree. let run = Scenario::new(); run.agent_does(AGENT_OPENS_PR); let merge_run = Scenario::new(); diff --git a/tests/release.rs b/tests/release.rs index 012fc3b51..06fa678fe 100644 --- a/tests/release.rs +++ b/tests/release.rs @@ -805,6 +805,9 @@ fn without_review_the_script_runs_to_the_tag_without_reading_stdin() { #[cfg(target_os = "linux")] fn the_release_body_is_the_bump_prs_summary_then_the_generated_notes_then_the_install_instructions() { + // The release script cuts the release the notes are for, and the notes + // script runs twice, as the behavior includes that a rerun gives the same + // body. let release = Release::new(); release.ci_reports(GREEN); let output = release.run_script("0.2.0"); @@ -814,7 +817,7 @@ fn the_release_body_is_the_bump_prs_summary_then_the_generated_notes_then_the_in let again = release.run_notes_script("v0.2.0"); assert!(first.status.success(), "{}", stderr(&first)); - let body = String::from_utf8(first.stdout).unwrap(); + let body = stdout(&first); let notes = release.generated_notes("v0.2.0"); assert!(notes.contains("Add the frobnicator"), "{notes}"); assert_eq!( @@ -822,7 +825,7 @@ fn the_release_body_is_the_bump_prs_summary_then_the_generated_notes_then_the_in format!("{AGENT_SUMMARY}\n\n{notes}\n\n{INSTALL_NOTES}\n") ); assert!(again.status.success(), "{}", stderr(&again)); - assert_eq!(String::from_utf8(again.stdout).unwrap(), body); + assert_eq!(stdout(&again), body); } #[test] @@ -888,6 +891,7 @@ fn the_summary_is_only_that_of_a_pr_merged_as_the_tagged_commit() { #[test] #[cfg(target_os = "linux")] fn when_the_bump_pr_cannot_be_read_the_release_body_is_the_generated_notes_with_a_warning() { + // The release script only cuts the release the notes are for. let release = Release::new(); release.ci_reports(GREEN); let output = release.run_script("0.2.0"); @@ -909,10 +913,7 @@ fn when_the_bump_pr_cannot_be_read_the_release_body_is_the_generated_notes_with_ "{err}" ); let notes = release.generated_notes("v0.2.0"); - assert_eq!( - String::from_utf8(output.stdout).unwrap(), - format!("{notes}\n\n{INSTALL_NOTES}\n") - ); + assert_eq!(stdout(&output), format!("{notes}\n\n{INSTALL_NOTES}\n")); } /// Runs the script with `version` and asserts it refused before pushing diff --git a/tests/run_notification.rs b/tests/run_notification.rs index 2dd5ffcc6..ddc0405fe 100644 --- a/tests/run_notification.rs +++ b/tests/run_notification.rs @@ -378,10 +378,8 @@ fn with_email_always_an_address_after_the_flag_still_wins() { #[test] fn email_always_without_an_address_stops_the_run_before_any_work() { - // Without a key it stops the same way as with the flag, which - // without_resend_api_key_the_run_stops_before_any_work_and_sends_nothing - // shows; the User config's unit tests show email.always asks for a - // notification. + // 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. let scenario = Scenario::new(); scenario.user_config_is("[email]\nalways = true\n"); let resend = ResendStandIn::replying(200, ACCEPTED); @@ -395,8 +393,8 @@ fn email_always_without_an_address_stops_the_run_before_any_work() { #[test] fn email_always_false_makes_a_run_without_the_flag_send_nothing() { - // With no email.always, a_run_without_the_flag_sends_nothing shows the - // same; the User config's unit tests take both. + // 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); @@ -547,6 +545,7 @@ fn a_run_without_a_notification_ignores_broken_credentials() { #[test] fn help_version_and_update_never_read_the_credentials() { + // One run of each command, as each is a behavior of its own. let scenario = Scenario::new(); scenario.credentials_are("[resend]\nkye = \"re_file_456\"\n"); diff --git a/tests/spec_run.rs b/tests/spec_run.rs index f69b7af6c..bb193d45d 100644 --- a/tests/spec_run.rs +++ b/tests/spec_run.rs @@ -1235,13 +1235,14 @@ fn help_explains_unready_tickets_and_that_a_spec_run_takes_every_ticket_it_can_r } } -/// SIGTERM to the Spec run's process alone while #21's agent is at work, -/// leaving it half done: #21's Run pushes that work as a failed run, #22 -/// never starts, and the Spec run ends only after, as interrupted. One -/// signal end to end; the interrupt's unit tests take SIGINT, SIGTERM and -/// SIGHUP alike. #[test] fn a_signal_to_the_spec_run_alone_fails_the_ticket_run_then_ends_the_spec_run_as_interrupted() { + // SIGTERM to the Spec run's process alone while #21's agent is at work, + // leaving it half done: #21's Run pushes that work as a failed run, #22 + // never starts, and the Spec run ends only after, as interrupted. One + // signal end to end: the interrupt's unit tests show SIGINT, SIGTERM and + // SIGHUP are each recorded alike, and the Spec run passes any of them on + // to the Ticket's Run as SIGTERM. let scenario = linear_spec(); let started = scenario.path("agent-started"); scenario.agent_does_for(