From b8621c50423ab118d6a7d4ba849b65473062ca31 Mon Sep 17 00:00:00 2001 From: Yehuda Katz Date: Fri, 9 May 2014 16:57:13 -0700 Subject: [PATCH] More tests for error cases in compile Also some refactoring of the error structure. More cleanup work around human-readable and CLI errors is still required. --- src/bin/cargo-compile.rs | 27 ++++------- src/cargo/core/manifest.rs | 7 ++- src/cargo/ops/cargo_read_manifest.rs | 23 +++++---- src/cargo/util/mod.rs | 2 +- src/cargo/util/result.rs | 71 ++++++++++++++++++++++++++-- tests/support.rs | 43 ++++++++++++----- tests/test_cargo_compile.rs | 39 +++++++++++---- 7 files changed, 152 insertions(+), 60 deletions(-) diff --git a/src/bin/cargo-compile.rs b/src/bin/cargo-compile.rs index dca60173c..39bc267d1 100644 --- a/src/bin/cargo-compile.rs +++ b/src/bin/cargo-compile.rs @@ -5,11 +5,11 @@ extern crate cargo; extern crate hammer; extern crate serialize; +use cargo::{execute_main_without_stdin,CLIResult,CLIError,ToResult}; use cargo::ops::cargo_compile::compile; -use cargo::core::errors::{CLIResult,CLIError,ToResult}; use cargo::util::important_paths::find_project; -use hammer::{FlagDecoder,FlagConfig,HammerError}; -use serialize::Decodable; +use cargo::util::ToCLI; +use hammer::FlagConfig; use std::os; #[deriving(Eq,Clone,Decodable,Encodable)] @@ -19,29 +19,18 @@ pub struct Options { impl FlagConfig for Options {} -fn flags>() -> CLIResult { - let mut decoder = FlagDecoder::new::(std::os::args().tail()); - Decodable::decode(&mut decoder).to_result(|e: HammerError| CLIError::new(e.message, None, 1)) +fn main() { + execute_main_without_stdin(execute); } -fn execute() -> CLIResult<()> { - let options = try!(flags::()); - +fn execute(options: Options) -> CLIResult> { let root = match options.manifest_path { Some(path) => Path::new(path), None => try!(find_project(os::getcwd(), "Cargo.toml".to_owned()) .map(|path| path.join("Cargo.toml")) .to_result(|err| - CLIError::new("Could not find Cargo.toml in this directory or any parent directory", Some(err.to_str()), 1))) + CLIError::new("Could not find Cargo.toml in this directory or any parent directory", Some(err.to_str()), 102))) }; - compile(root.as_str().unwrap().as_slice()).to_result(|err| - CLIError::new(format!("Compilation failed: {}", err), None, 1)) -} - -fn main() { - match execute() { - Err(err) => fail!("{}", err), - Ok(_) => return - } + compile(root.as_str().unwrap().as_slice()).map(|v| Some(v)).to_cli(101) } diff --git a/src/cargo/core/manifest.rs b/src/cargo/core/manifest.rs index ffb58d79a..1bdd71d64 100644 --- a/src/cargo/core/manifest.rs +++ b/src/cargo/core/manifest.rs @@ -8,7 +8,6 @@ use core::{ Package, Summary }; -use util::result::CargoResult; #[deriving(Eq,Clone)] pub struct Manifest { @@ -177,7 +176,7 @@ pub struct TomlManifest { } impl TomlManifest { - pub fn to_package(&self, path: &str) -> CargoResult { + pub fn to_package(&self, path: &str) -> Package { // TODO: Convert hte argument to take a Path let path = Path::new(path); @@ -195,12 +194,12 @@ impl TomlManifest { // TODO: https://github.com/mozilla/rust/issues/14049 let root = Path::new(path.dirname()); - Ok(Package::new( + Package::new( &Manifest::new( &Summary::new(&self.project.to_name_ver(), deps.as_slice()), targets.as_slice(), &Path::new("target")), - &root)) + &root) } } diff --git a/src/cargo/ops/cargo_read_manifest.rs b/src/cargo/ops/cargo_read_manifest.rs index 5e7e622bd..1bb26cdf9 100644 --- a/src/cargo/ops/cargo_read_manifest.rs +++ b/src/cargo/ops/cargo_read_manifest.rs @@ -2,23 +2,26 @@ use toml; use toml::from_toml; use core::Package; use core::manifest::{TomlManifest}; -use util::{other_error,CargoResult,CargoError}; +use util::{toml_error,human_error,CargoResult,CargoError}; pub fn read_manifest(path: &str) -> CargoResult { - let root = try!(parse_from_file(path)); - let toml = try!(load_toml(path, root)); - toml.to_package(path) + let root = try!(parse_from_file(path).map_err(|err: CargoError| + human_error(format!("Cargo.toml is not valid Toml"), format!("path={}", path), err))); + + let toml = try!(load_toml(root).map_err(|err: CargoError| + human_error(format!("Cargo.toml is not a valid Cargo manifest"), format!("path={}", path), err))); + + Ok(toml.to_package(path)) } fn parse_from_file(path: &str) -> CargoResult { - toml::parse_from_file(path.clone()).map_err(|err| to_cargo_err(path, err)) + toml::parse_from_file(path.clone()).map_err(to_cargo_err) } -fn load_toml(path: &str, root: toml::Value) -> CargoResult { - from_toml::(root).map_err(|err| to_cargo_err(path, err)) +fn load_toml(root: toml::Value) -> CargoResult { + from_toml::(root).map_err(to_cargo_err) } -fn to_cargo_err(path: &str, err: toml::Error) -> CargoError { - other_error("Cargo.toml is not valid Toml") - .with_detail(format!("path={}; err={}", path, err.to_str())) +fn to_cargo_err(err: toml::Error) -> CargoError { + toml_error("Problem loading manifest", err) } diff --git a/src/cargo/util/mod.rs b/src/cargo/util/mod.rs index f8b9fa906..22c84efc3 100644 --- a/src/cargo/util/mod.rs +++ b/src/cargo/util/mod.rs @@ -1,5 +1,5 @@ pub use self::process_builder::{process,ProcessBuilder}; -pub use self::result::{CargoError,CargoResult,Wrap,Require,other_error}; +pub use self::result::{CargoError,CargoResult,Wrap,Require,ToCLI,other_error,human_error,toml_error}; pub mod graph; pub mod process_builder; diff --git a/src/cargo/util/result.rs b/src/cargo/util/result.rs index 5a3823ebe..2645b0775 100644 --- a/src/cargo/util/result.rs +++ b/src/cargo/util/result.rs @@ -1,4 +1,6 @@ use std::io; +use core::errors::{CLIError,CLIResult}; +use toml; /* * CargoResult should be used in libcargo. CargoCliResult should be used in the @@ -10,7 +12,25 @@ pub type CargoResult = Result; pub fn other_error(desc: &'static str) -> CargoError { CargoError { kind: OtherCargoError, - desc: desc, + desc: StaticDescription(desc), + detail: None, + cause: None + } +} + +pub fn human_error(desc: ~str, detail: ~str, cause: CargoError) -> CargoError { + CargoError { + kind: HumanReadableError, + desc: BoxedDescription(desc), + detail: Some(detail), + cause: Some(box cause) + } +} + +pub fn toml_error(desc: &'static str, error: toml::Error) -> CargoError { + CargoError { + kind: TomlError(error), + desc: StaticDescription(desc), detail: None, cause: None } @@ -19,14 +39,23 @@ pub fn other_error(desc: &'static str) -> CargoError { #[deriving(Show,Clone)] pub struct CargoError { kind: CargoErrorKind, - desc: &'static str, + desc: CargoErrorDescription, detail: Option<~str>, cause: Option> } +#[deriving(Show,Clone)] +enum CargoErrorDescription { + StaticDescription(&'static str), + BoxedDescription(~str) +} + impl CargoError { - pub fn get_desc(&self) -> &'static str { - self.desc + pub fn get_desc<'a>(&'a self) -> &'a str { + match self.desc { + StaticDescription(desc) => desc, + BoxedDescription(ref desc) => desc.as_slice() + } } pub fn get_detail<'a>(&'a self) -> Option<&'a str> { @@ -37,12 +66,31 @@ impl CargoError { self.detail = Some(detail); self } + + pub fn to_cli(self, exit_code: uint) -> CLIError { + match self { + CargoError { kind: HumanReadableError, desc: BoxedDescription(desc), detail: detail, .. } => { + CLIError::new(desc, detail, exit_code) + }, + CargoError { kind: InternalError, desc: StaticDescription(desc), detail: None, .. } => { + CLIError::new("An unexpected error occurred", Some(desc.to_owned()), exit_code) + }, + CargoError { kind: InternalError, desc: StaticDescription(desc), detail: Some(detail), .. } => { + CLIError::new("An unexpected error occurred", Some(format!("{}\n{}", desc, detail)), exit_code) + }, + _ => { + CLIError::new("An unexpected error occurred", None, exit_code) + } + } + } } #[deriving(Show,Clone)] pub enum CargoErrorKind { + HumanReadableError, InternalError, IoError(io::IoError), + TomlError(toml::Error), OtherCargoError } @@ -73,7 +121,7 @@ impl Wrap for Result { Err(e) => { Err(CargoError { kind: e.kind.clone(), - desc: desc, + desc: StaticDescription(desc), detail: None, cause: Some(box e) }) @@ -94,3 +142,16 @@ impl Require for Option { } } } + +pub trait ToCLI { + fn to_cli(self, exit_code: uint) -> CLIResult; +} + +impl ToCLI for Result { + fn to_cli(self, exit_code: uint) -> CLIResult { + match self { + Ok(val) => Ok(val), + Err(err) => Err(err.to_cli(exit_code)) + } + } +} diff --git a/tests/support.rs b/tests/support.rs index cae03d45b..49dc81032 100644 --- a/tests/support.rs +++ b/tests/support.rs @@ -155,9 +155,10 @@ pub fn cargo_dir() -> Path { #[deriving(Clone,Eq)] struct Execs { - expect_stdout: Option<~str>, - expect_stdin: Option<~str>, - expect_exit_code: Option + expect_stdout: Option<~str>, + expect_stdin: Option<~str>, + expect_stderr: Option<~str>, + expect_exit_code: Option } impl Execs { @@ -167,9 +168,20 @@ impl Execs { self } + pub fn with_stderr(mut ~self, expected: &str) -> Box { + self.expect_stderr = Some(expected.to_owned()); + self + } + + pub fn with_status(mut ~self, expected: int) -> Box { + self.expect_exit_code = Some(expected); + self + } + fn match_output(&self, actual: &ProcessOutput) -> ham::MatchResult { self.match_status(actual.status) .and(self.match_stdout(&actual.output)) + .and(self.match_stderr(&actual.error)) } fn match_status(&self, actual: ProcessExit) -> ham::MatchResult { @@ -184,13 +196,21 @@ impl Execs { } fn match_stdout(&self, actual: &Vec) -> ham::MatchResult { - match self.expect_stdout.as_ref().map(|s| s.as_slice()) { + self.match_std(&self.expect_stdout, actual, "stdout") + } + + fn match_stderr(&self, actual: &Vec) -> ham::MatchResult { + self.match_std(&self.expect_stderr, actual, "stderr") + } + + fn match_std(&self, expected: &Option<~str>, actual: &Vec, description: &str) -> ham::MatchResult { + match expected.as_ref().map(|s| s.as_slice()) { None => ham::success(), Some(out) => { match str::from_utf8(actual.as_slice()) { - None => Err("stdout was not utf8 encoded".to_owned()), + None => Err(format!("{} was not utf8 encoded", description)), Some(actual) => { - ham::expect(actual == out, format!("stdout was `{}`", actual)) + ham::expect(actual == out, format!("{} was `{}`", description, actual)) } } } @@ -216,11 +236,12 @@ impl ham::Matcher for Execs { } pub fn execs() -> Box { - box Execs { - expect_stdout: None, - expect_stdin: None, - expect_exit_code: None - } + box Execs { + expect_stdout: None, + expect_stderr: None, + expect_stdin: None, + expect_exit_code: None + } } pub trait ResultTest { diff --git a/tests/test_cargo_compile.rs b/tests/test_cargo_compile.rs index f3e88d94e..a909f802f 100644 --- a/tests/test_cargo_compile.rs +++ b/tests/test_cargo_compile.rs @@ -31,19 +31,24 @@ test!(cargo_compile { execs().with_stdout("i am foo\n")); }) -fn main_file(println: &str, deps: &[&str]) -> ~str { - let mut buf = StrBuf::new(); +test!(cargo_compile_with_invalid_manifest { + let p = project("foo") + .file("Cargo.toml", ""); - for dep in deps.iter() { - buf.push_str(format!("extern crate {};\n", dep)); - } + assert_that(p.cargo_process("cargo-compile"), + execs() + .with_status(101) + .with_stderr("Cargo.toml is not a valid Cargo manifest")); +}) - buf.push_str("fn main() { println!("); - buf.push_str(println); - buf.push_str("); }\n"); +test!(cargo_compile_without_manifest { + let p = project("foo"); - buf.to_owned() -} + assert_that(p.cargo_process("cargo-compile"), + execs() + .with_status(102) + .with_stderr("Could not find Cargo.toml in this directory or any parent directory")); +}) test!(cargo_compile_with_nested_deps { let mut p = project("foo"); @@ -120,4 +125,18 @@ test!(cargo_compile_with_nested_deps { execs().with_stdout("test passed\n")); }) +fn main_file(println: &str, deps: &[&str]) -> ~str { + let mut buf = StrBuf::new(); + + for dep in deps.iter() { + buf.push_str(format!("extern crate {};\n", dep)); + } + + buf.push_str("fn main() { println!("); + buf.push_str(println); + buf.push_str("); }\n"); + + buf.to_owned() +} + // test!(compiling_project_with_invalid_manifest)