From 215586ac5cf0d84d3de72a1369d268dc2084f62d Mon Sep 17 00:00:00 2001 From: Mubashwer Salman Khurshid Date: Wed, 27 Mar 2024 23:52:06 +1100 Subject: [PATCH] fix: return non-zero exit code in expected error cases * When trying to delete coauthor but no coauthor found with given key * When trying to mob with co-author key but no co-author found with given key * When trying to select co-author for mob session but no co-author added Improves names of tests to be more precise and clearer --- src/cli.rs | 19 ++++--------- src/commands/coauthor.rs | 31 ++++++++------------ src/commands/mob.rs | 61 +++++++++++++++------------------------- src/main.rs | 8 ++---- tests/coauthor.rs | 8 ++++-- tests/mob.rs | 36 ++++++++++++++---------- 6 files changed, 68 insertions(+), 95 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index fa0db5c..c8ba9ea 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -45,24 +45,19 @@ enum Commands { Coauthor(Coauthor), } -pub fn run( - coauthor_repo: &impl CoauthorRepo, - out: &mut impl Write, - err: &mut impl Write, -) -> Result<(), Box> { +pub fn run(coauthor_repo: &impl CoauthorRepo, out: &mut impl Write) -> Result<(), Box> { let cli = Cli::parse(); - run_inner(&cli, coauthor_repo, out, err) + run_inner(&cli, coauthor_repo, out) } fn run_inner( cli: &Cli, coauthor_repo: &impl CoauthorRepo, out: &mut impl Write, - err: &mut impl Write, ) -> Result<(), Box> { match &cli.command { - None => cli.mob.handle(coauthor_repo, out, err)?, - Some(Commands::Coauthor(coauthor)) => coauthor.handle(coauthor_repo, out, err)?, + None => cli.mob.handle(coauthor_repo, out)?, + Some(Commands::Coauthor(coauthor)) => coauthor.handle(coauthor_repo, out)?, } Ok(()) } @@ -94,8 +89,7 @@ mod tests { }; let mut out = Vec::new(); - let mut err = Vec::new(); - run_inner(&cli, &mock_coauthor_repo, &mut out, &mut err)?; + run_inner(&cli, &mock_coauthor_repo, &mut out)?; Ok(()) } @@ -130,8 +124,7 @@ mod tests { }; let mut out = Vec::new(); - let mut err = Vec::new(); - run_inner(&cli, &mock_coauthor_repo, &mut out, &mut err)?; + run_inner(&cli, &mock_coauthor_repo, &mut out)?; Ok(()) } diff --git a/src/commands/coauthor.rs b/src/commands/coauthor.rs index b48365c..39acfcb 100644 --- a/src/commands/coauthor.rs +++ b/src/commands/coauthor.rs @@ -27,12 +27,11 @@ impl Coauthor { &self, coauthor_repo: &impl CoauthorRepo, out: &mut impl Write, - err: &mut impl Write, ) -> Result<(), Box> { if let Some(key) = self.delete.as_deref() { match coauthor_repo.get(key)? { Some(_) => coauthor_repo.remove(key)?, - None => writeln!(err, "No co-author found with key: {key}")?, + None => return Err(format!("No co-author found with key: {key}").into()), } } if self.list { @@ -79,15 +78,15 @@ mod tests { }; let mut out = Vec::new(); - let mut err = Vec::new(); - coauthor_cmd.handle(&mock_coauthor_repo, &mut out, &mut err)?; + coauthor_cmd.handle(&mock_coauthor_repo, &mut out)?; + + assert!(out.is_empty()); Ok(()) } #[test] - fn test_error_message_shown_when_trying_to_delete_coauthor_with_non_existing_coauthor_key( - ) -> Result<(), Box> { + fn test_delete_coauthor_when_coauthor_not_found() -> Result<(), Box> { let key = "em"; let mut mock_coauthor_repo = MockCoauthorRepo::new(); mock_coauthor_repo @@ -103,13 +102,10 @@ mod tests { }; let mut out = Vec::new(); - let mut err = Vec::new(); - coauthor_cmd.handle(&mock_coauthor_repo, &mut out, &mut err)?; + let result = coauthor_cmd.handle(&mock_coauthor_repo, &mut out); - assert_eq!( - err, - format!("No co-author found with key: {key}\n").as_bytes() - ); + assert!(result + .is_err_and(|err| err.to_string() == format!("No co-author found with key: {key}"))); Ok(()) } @@ -137,8 +133,7 @@ mod tests { }; let mut out = Vec::new(); - let mut err = Vec::new(); - coauthor_cmd.handle(&mock_coauthor_repo, &mut out, &mut err)?; + coauthor_cmd.handle(&mock_coauthor_repo, &mut out)?; assert_eq!(out, format!("{name} <{email}>\n").as_bytes()); @@ -167,8 +162,7 @@ mod tests { }; let mut out = Vec::new(); - let mut err = Vec::new(); - coauthor_cmd.handle(&mock_coauthor_repo, &mut out, &mut err)?; + coauthor_cmd.handle(&mock_coauthor_repo, &mut out)?; assert_eq!(out, expected_output.as_bytes()); @@ -176,7 +170,7 @@ mod tests { } #[test] - fn test_list_coauthors_when_list_is_empty() -> Result<(), Box> { + fn test_list_coauthors_when_no_coauthors_added() -> Result<(), Box> { let mut mock_coauthor_repo = MockCoauthorRepo::new(); mock_coauthor_repo .expect_list() @@ -190,8 +184,7 @@ mod tests { }; let mut out = Vec::new(); - let mut err = Vec::new(); - coauthor_cmd.handle(&mock_coauthor_repo, &mut out, &mut err)?; + coauthor_cmd.handle(&mock_coauthor_repo, &mut out)?; assert_eq!(out, b""); diff --git a/src/commands/mob.rs b/src/commands/mob.rs index 80f2ac5..2a6c1b5 100644 --- a/src/commands/mob.rs +++ b/src/commands/mob.rs @@ -33,7 +33,6 @@ impl Mob { &self, coauthor_repo: &impl CoauthorRepo, out: &mut impl Write, - err: &mut impl Write, ) -> Result<(), Box> { if self.clear { coauthor_repo.clear_mob()?; @@ -64,11 +63,9 @@ impl Mob { Some([]) => { let coauthors = coauthor_repo.list(false)?; if coauthors.is_empty() { - writeln!( - err, - "No co-author(s) found. At least one co-author must be added" - )?; - return Ok(()); + return Err( + "No co-author(s) found. At least one co-author must be added".into(), + ); } let result = MultiSelect::new("Select active co-author(s):", coauthors) @@ -94,7 +91,7 @@ impl Mob { coauthor_repo.add_to_mob(&coauthor)?; coauthors.push(coauthor); } - None => writeln!(err, "No co-author found with key: {key}")?, + None => return Err(format!("No co-author found with key: {key}").into()), } } @@ -117,7 +114,7 @@ mod tests { use mockall::predicate; #[test] - fn test_clear_mob_session() -> Result<(), Box> { + fn test_clear_mob() -> Result<(), Box> { let mut mock_coauthor_repo = MockCoauthorRepo::new(); mock_coauthor_repo .expect_clear_mob() @@ -132,14 +129,13 @@ mod tests { }; let mut out = Vec::new(); - let mut err = Vec::new(); - mob_cmd.handle(&mock_coauthor_repo, &mut out, &mut err)?; + mob_cmd.handle(&mock_coauthor_repo, &mut out)?; Ok(()) } #[test] - fn test_list_mob_coauthors() -> Result<(), Box> { + fn test_list_mob() -> Result<(), Box> { let coauthors = vec![ "Leo Messi ".to_owned(), "Emi Martinez ".to_owned(), @@ -161,8 +157,7 @@ mod tests { }; let mut out = Vec::new(); - let mut err = Vec::new(); - mob_cmd.handle(&mock_coauthor_repo, &mut out, &mut err)?; + mob_cmd.handle(&mock_coauthor_repo, &mut out)?; assert_eq!(out, expected_output.as_bytes()); @@ -170,7 +165,7 @@ mod tests { } #[test] - fn test_list_mob_coauthors_when_mob_session_is_empty() -> Result<(), Box> { + fn test_list_mob_when_mob_session_is_empty() -> Result<(), Box> { let mut mock_coauthor_repo = MockCoauthorRepo::new(); mock_coauthor_repo .expect_list_mob() @@ -185,8 +180,7 @@ mod tests { }; let mut out = Vec::new(); - let mut err = Vec::new(); - mob_cmd.handle(&mock_coauthor_repo, &mut out, &mut err)?; + mob_cmd.handle(&mock_coauthor_repo, &mut out)?; assert_eq!(out, b""); @@ -214,8 +208,7 @@ mod tests { }; let mut out = Vec::new(); - let mut err = Vec::new(); - mob_cmd.handle(&mock_coauthor_repo, &mut out, &mut err)?; + mob_cmd.handle(&mock_coauthor_repo, &mut out)?; assert_eq!(out, b"Co-authored-by: Leo Messi \nCo-authored-by: Emi Martinez \n"); @@ -238,8 +231,7 @@ mod tests { }; let mut out = Vec::new(); - let mut err = Vec::new(); - mob_cmd.handle(&mock_coauthor_repo, &mut out, &mut err)?; + mob_cmd.handle(&mock_coauthor_repo, &mut out)?; assert_eq!(out, b""); @@ -247,8 +239,7 @@ mod tests { } #[test] - fn test_error_message_shown_when_trying_to_mob_given_coauthors_list_is_empty( - ) -> Result<(), Box> { + fn test_mob_with_given_no_coauthors_added() -> Result<(), Box> { let coauthors = vec![]; let mut mock_coauthor_repo = MockCoauthorRepo::new(); @@ -266,19 +257,16 @@ mod tests { }; let mut out = Vec::new(); - let mut err = Vec::new(); - mob_cmd.handle(&mock_coauthor_repo, &mut out, &mut err)?; + let result = mob_cmd.handle(&mock_coauthor_repo, &mut out); - assert_eq!( - err, - b"No co-author(s) found. At least one co-author must be added\n" - ); + assert!(result.is_err_and(|err| err.to_string() + == "No co-author(s) found. At least one co-author must be added".to_string())); Ok(()) } #[test] - fn test_adding_coauthors_to_mob_session_by_keys() -> Result<(), Box> { + fn test_mob_with_by_keys() -> Result<(), Box> { let keys = vec!["lm".to_owned(), "em".to_owned()]; let coauthors = [ "Leo Messi ", @@ -319,8 +307,7 @@ mod tests { }; let mut out = Vec::new(); - let mut err = Vec::new(); - mob_cmd.handle(&mock_coauthor_repo, &mut out, &mut err)?; + mob_cmd.handle(&mock_coauthor_repo, &mut out)?; assert_eq!(out, format!("{}\n", coauthors.join("\n")).as_bytes()); @@ -328,8 +315,7 @@ mod tests { } #[test] - fn test_error_message_shown_when_trying_to_mob_while_passing_non_existing_coauthor_key( - ) -> Result<(), Box> { + fn test_mob_with_by_key_when_coauthor_not_found() -> Result<(), Box> { let key = "lm"; let mut mock_coauthor_repo = MockCoauthorRepo::new(); @@ -351,13 +337,10 @@ mod tests { }; let mut out = Vec::new(); - let mut err = Vec::new(); - mob_cmd.handle(&mock_coauthor_repo, &mut out, &mut err)?; + let result = mob_cmd.handle(&mock_coauthor_repo, &mut out); - assert_eq!( - err, - format!("No co-author found with key: {key}\n").as_bytes() - ); + assert!(result + .is_err_and(|err| err.to_string() == format!("No co-author found with key: {key}"))); Ok(()) } diff --git a/src/main.rs b/src/main.rs index 5eefa01..7ac7916 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,13 +1,9 @@ use git_mob_tool::{cli, coauthor_repo::GitConfigCoauthorRepo}; -use std::{ - error::Error, - io::{stderr, stdout}, -}; +use std::{error::Error, io::stdout}; fn main() -> Result<(), Box> { let coauthor_repo = GitConfigCoauthorRepo {}; let out = &mut stdout(); - let err = &mut stderr(); - cli::run(&coauthor_repo, out, err)?; + cli::run(&coauthor_repo, out)?; Ok(()) } diff --git a/tests/coauthor.rs b/tests/coauthor.rs index c393113..c492c75 100644 --- a/tests/coauthor.rs +++ b/tests/coauthor.rs @@ -110,12 +110,14 @@ fn test_delete_coauthor(ctx: TestContextCli) -> Result<(), Box> { #[test_context(TestContextCli, skip_teardown)] #[test] -fn test_delete_coauthor_no_coauthor_found(ctx: TestContextCli) -> Result<(), Box> { +fn test_delete_coauthor_when_coauthor_not_found(ctx: TestContextCli) -> Result<(), Box> { ctx.git() .args(["mob", "coauthor", "--delete", "lm"]) .assert() - .success() - .stderr(predicate::str::diff("No co-author found with key: lm\n")); + .failure() + .stderr(predicate::str::diff( + "Error: \"No co-author found with key: lm\"\n", + )); Ok(()) } diff --git a/tests/mob.rs b/tests/mob.rs index 944ed65..5ceb38b 100644 --- a/tests/mob.rs +++ b/tests/mob.rs @@ -36,12 +36,14 @@ fn test_mob_with_by_keys(ctx: TestContextCli) -> Result<(), Box> { #[test_context(TestContextCli, skip_teardown)] #[test] -fn test_mob_with_by_keys_no_coauthor_found(ctx: TestContextCli) -> Result<(), Box> { +fn test_mob_with_by_key_when_coauthor_not_found(ctx: TestContextCli) -> Result<(), Box> { ctx.git() .args(["mob", "--with", "jk"]) .assert() - .success() - .stderr(predicate::str::diff("No co-author found with key: jk\n")); + .failure() + .stderr(predicate::str::diff( + "Error: \"No co-author found with key: jk\"\n", + )); Ok(()) } @@ -55,9 +57,9 @@ fn test_mob_with_multiselect_given_no_coauthors_added( ctx.git() .args(["mob", "--with"]) .assert() - .success() + .failure() .stderr(predicate::str::diff( - "No co-author(s) found. At least one co-author must be added\n", + "Error: \"No co-author(s) found. At least one co-author must be added\"\n", )); Ok(()) @@ -66,7 +68,7 @@ fn test_mob_with_multiselect_given_no_coauthors_added( #[cfg(unix)] #[test_context(TestContextCli, skip_teardown)] #[test] -fn test_mob_with_multiselect_select_none(ctx: TestContextCli) -> Result<(), Box> { +fn test_mob_with_multiselect_when_select_none(ctx: TestContextCli) -> Result<(), Box> { // given a mob session with 2 co-authors is active add_two_coauthors(&ctx)?; ctx.git() @@ -104,7 +106,9 @@ fn test_mob_with_multiselect_select_none(ctx: TestContextCli) -> Result<(), Box< #[cfg(unix)] #[test_context(TestContextCli, skip_teardown)] #[test] -fn test_mob_with_multiselect_select_coauthor(ctx: TestContextCli) -> Result<(), Box> { +fn test_mob_with_multiselect_when_select_coauthor( + ctx: TestContextCli, +) -> Result<(), Box> { add_two_coauthors(&ctx)?; // running command to display mob session multiselect prompt @@ -137,7 +141,7 @@ fn test_mob_with_multiselect_select_coauthor(ctx: TestContextCli) -> Result<(), #[cfg(unix)] #[test_context(TestContextCli, skip_teardown)] #[test] -fn test_mob_with_multiselect_escape(ctx: TestContextCli) -> Result<(), Box> { +fn test_mob_with_multiselect_when_press_escape(ctx: TestContextCli) -> Result<(), Box> { // given a mob session with 2 co-authors is active add_two_coauthors(&ctx)?; ctx.git() @@ -176,7 +180,7 @@ fn test_mob_with_multiselect_escape(ctx: TestContextCli) -> Result<(), Box Result<(), Box> { +fn test_clear_mob(ctx: TestContextCli) -> Result<(), Box> { add_two_coauthors(&ctx)?; // mobbing with both of the co-authors @@ -207,7 +211,7 @@ fn test_mob_clear(ctx: TestContextCli) -> Result<(), Box> { #[test_context(TestContextCli, skip_teardown)] #[test] -fn test_mob_clear_given_empty_mob_session(ctx: TestContextCli) -> Result<(), Box> { +fn test_clear_mob_given_empty_mob_session(ctx: TestContextCli) -> Result<(), Box> { // clearing mob session ctx.git() .args(["mob", "--clear"]) @@ -227,7 +231,7 @@ fn test_mob_clear_given_empty_mob_session(ctx: TestContextCli) -> Result<(), Box #[test_context(TestContextCli, skip_teardown)] #[test] -fn test_mob_trailers(ctx: TestContextCli) -> Result<(), Box> { +fn test_mob_coauthor_trailers(ctx: TestContextCli) -> Result<(), Box> { add_two_coauthors(&ctx)?; // mobbing with both of the co-authors @@ -239,7 +243,7 @@ fn test_mob_trailers(ctx: TestContextCli) -> Result<(), Box> { "Leo Messi \nEmi Martinez \n", )); - // verifying mob trailers show Co-authored-by trailers for both co-authors + // verifying mob co-author trailers show Co-authored-by trailers for both co-authors ctx.git() .args(["mob", "--trailers"]) .assert() @@ -253,8 +257,10 @@ fn test_mob_trailers(ctx: TestContextCli) -> Result<(), Box> { #[test_context(TestContextCli, skip_teardown)] #[test] -fn test_mob_trailers_given_empty_mob_session(ctx: TestContextCli) -> Result<(), Box> { - // verifying mob trailers is empty +fn test_mob_coauthor_trailers_given_empty_mob_session( + ctx: TestContextCli, +) -> Result<(), Box> { + // verifying mob co-author trailers is empty ctx.git() .args(["mob", "--trailers"]) .assert() @@ -266,7 +272,7 @@ fn test_mob_trailers_given_empty_mob_session(ctx: TestContextCli) -> Result<(), #[test_context(TestContextCli, skip_teardown)] #[test] -fn test_mob_list_given_empty_mob_session(ctx: TestContextCli) -> Result<(), Box> { +fn test_list_mob_given_empty_mob_session(ctx: TestContextCli) -> Result<(), Box> { // verifying mob list is empty ctx.git() .args(["mob", "--list"])