From 9b04c98ddb41993a0ba2669e4e3f8563f9e7dbca Mon Sep 17 00:00:00 2001 From: Sebastian Holgersson Date: Tue, 4 Jan 2022 03:15:35 +0100 Subject: [PATCH 1/8] numfmt: use UResult in more functions This commit replaces generic Results with UResults in some key functions in numfmt. As a result of this, we can provide different exit codes for different errors, which resolves ~70 failing test cases in the GNU numfmt.pl test suite. --- src/uu/numfmt/src/errors.rs | 34 +++++++++++++++++++++++++ src/uu/numfmt/src/numfmt.rs | 48 +++++++++++++++++++++--------------- tests/by-util/test_numfmt.rs | 5 ++++ 3 files changed, 67 insertions(+), 20 deletions(-) create mode 100644 src/uu/numfmt/src/errors.rs diff --git a/src/uu/numfmt/src/errors.rs b/src/uu/numfmt/src/errors.rs new file mode 100644 index 000000000..400fc4619 --- /dev/null +++ b/src/uu/numfmt/src/errors.rs @@ -0,0 +1,34 @@ +use std::{ + error::Error, + fmt::{Debug, Display}, +}; +use uucore::error::UError; + +#[derive(Debug)] +pub enum NumfmtError { + IoError(String), + IllegalArgument(String), + FormattingError(String), +} + +impl UError for NumfmtError { + fn code(&self) -> i32 { + match self { + NumfmtError::IoError(_) => 1, + NumfmtError::IllegalArgument(_) => 1, + NumfmtError::FormattingError(_) => 2, + } + } +} + +impl Error for NumfmtError {} + +impl Display for NumfmtError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + NumfmtError::IoError(s) + | NumfmtError::IllegalArgument(s) + | NumfmtError::FormattingError(s) => write!(f, "{}", s), + } + } +} diff --git a/src/uu/numfmt/src/numfmt.rs b/src/uu/numfmt/src/numfmt.rs index b84b9ab1b..22e7210f1 100644 --- a/src/uu/numfmt/src/numfmt.rs +++ b/src/uu/numfmt/src/numfmt.rs @@ -7,15 +7,17 @@ // spell-checker:ignore N'th M'th +use crate::errors::*; use crate::format::format_and_print; use crate::options::*; use crate::units::{Result, Unit}; use clap::{crate_version, App, AppSettings, Arg, ArgMatches}; use std::io::{BufRead, Write}; use uucore::display::Quotable; -use uucore::error::{UResult, USimpleError}; +use uucore::error::UResult; use uucore::ranges::Range; +pub mod errors; pub mod format; pub mod options; mod units; @@ -53,28 +55,35 @@ fn usage() -> String { format!("{0} [OPTION]... [NUMBER]...", uucore::execution_phrase()) } -fn handle_args<'a>(args: impl Iterator, options: NumfmtOptions) -> Result<()> { +fn handle_args<'a>(args: impl Iterator, options: NumfmtOptions) -> UResult<()> { for l in args { - format_and_print(l, &options)?; + match format_and_print(l, &options) { + Ok(_) => Ok(()), + Err(e) => Err(NumfmtError::FormattingError(e.to_string())), + }?; } Ok(()) } -fn handle_stdin(options: NumfmtOptions) -> Result<()> { +fn handle_stdin(options: NumfmtOptions) -> UResult<()> { let stdin = std::io::stdin(); let locked_stdin = stdin.lock(); let mut lines = locked_stdin.lines(); - for l in lines.by_ref().take(options.header) { - l.map(|s| println!("{}", s)).map_err(|e| e.to_string())?; + for (idx, line) in lines.by_ref().enumerate() { + match line { + Ok(l) if idx < options.header => { + println!("{}", l); + Ok(()) + } + Ok(l) => match format_and_print(&l, &options) { + Ok(_) => Ok(()), + Err(e) => Err(NumfmtError::FormattingError(e.to_string())), + }, + Err(e) => Err(NumfmtError::IoError(e.to_string())), + }?; } - - for l in lines { - l.map_err(|e| e.to_string()) - .and_then(|l| format_and_print(&l, &options))?; - } - Ok(()) } @@ -161,18 +170,17 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let matches = uu_app().usage(&usage[..]).get_matches_from(args); - let result = - parse_options(&matches).and_then(|options| match matches.values_of(options::NUMBER) { - Some(values) => handle_args(values, options), - None => handle_stdin(options), - }); + let options = parse_options(&matches).map_err(NumfmtError::IllegalArgument)?; + + let result = match matches.values_of(options::NUMBER) { + Some(values) => handle_args(values, options), + None => handle_stdin(options), + }; match result { Err(e) => { std::io::stdout().flush().expect("error flushing stdout"); - // TODO Change `handle_args()` and `handle_stdin()` so that - // they return `UResult`. - return Err(USimpleError::new(1, e)); + Err(e) } _ => Ok(()), } diff --git a/tests/by-util/test_numfmt.rs b/tests/by-util/test_numfmt.rs index 596aab6ba..9b1b91ed7 100644 --- a/tests/by-util/test_numfmt.rs +++ b/tests/by-util/test_numfmt.rs @@ -568,3 +568,8 @@ fn test_suffix_with_padding() { .succeeds() .stdout_only(" 1000pad 2000 3000\n"); } + +#[test] +fn test_invalid_number_returns_status_2() { + new_ucmd!().pipe_in("hello").fails().code_is(2); +} From 5cab4e41b3612dd55976f1a3ca9172bb8fa1594d Mon Sep 17 00:00:00 2001 From: sbentmar Date: Sat, 15 Jan 2022 11:15:45 +0100 Subject: [PATCH 2/8] numfmt: add copyright notice --- src/uu/numfmt/src/errors.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/uu/numfmt/src/errors.rs b/src/uu/numfmt/src/errors.rs index 400fc4619..af7de9caa 100644 --- a/src/uu/numfmt/src/errors.rs +++ b/src/uu/numfmt/src/errors.rs @@ -1,3 +1,8 @@ +// * This file is part of the uutils coreutils package. +// * +// * For the full copyright and license information, please view the LICENSE +// * file that was distributed with this source code. + use std::{ error::Error, fmt::{Debug, Display}, From 328cf4d91b53a92e13a2494beaeea25bf4b114ce Mon Sep 17 00:00:00 2001 From: sbentmar Date: Sat, 15 Jan 2022 11:42:48 +0100 Subject: [PATCH 3/8] numfmt: add more negative tests --- tests/by-util/test_numfmt.rs | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/tests/by-util/test_numfmt.rs b/tests/by-util/test_numfmt.rs index 9b1b91ed7..c7dea620c 100644 --- a/tests/by-util/test_numfmt.rs +++ b/tests/by-util/test_numfmt.rs @@ -570,6 +570,25 @@ fn test_suffix_with_padding() { } #[test] -fn test_invalid_number_returns_status_2() { +fn test_invalid_stdin_number_returns_status_2() { new_ucmd!().pipe_in("hello").fails().code_is(2); } + +#[test] +fn test_invalid_stdin_number_in_middle_of_input() { + new_ucmd!().pipe_in("100\nhello\n200").fails().code_is(2); +} + +#[test] +fn test_invalid_argument_number_returns_status_2() { + new_ucmd!().args(&["hello"]).fails().code_is(2); +} + +#[test] +fn test_invalid_argument_returns_status_1() { + new_ucmd!() + .args(&["--header=hello"]) + .pipe_in("53478") + .fails() + .code_is(1); +} From 4a7d313712912c91997cb7a83067d97552513a51 Mon Sep 17 00:00:00 2001 From: sbentmar Date: Sat, 15 Jan 2022 13:30:17 +0100 Subject: [PATCH 4/8] numfmt: add unit test for io error --- src/uu/numfmt/src/numfmt.rs | 54 ++++++++++++++++++++++++++++++++----- 1 file changed, 48 insertions(+), 6 deletions(-) diff --git a/src/uu/numfmt/src/numfmt.rs b/src/uu/numfmt/src/numfmt.rs index 22e7210f1..0b0108453 100644 --- a/src/uu/numfmt/src/numfmt.rs +++ b/src/uu/numfmt/src/numfmt.rs @@ -66,11 +66,11 @@ fn handle_args<'a>(args: impl Iterator, options: NumfmtOptions) Ok(()) } -fn handle_stdin(options: NumfmtOptions) -> UResult<()> { - let stdin = std::io::stdin(); - let locked_stdin = stdin.lock(); - - let mut lines = locked_stdin.lines(); +fn handle_buffer(input: R, options: NumfmtOptions) -> UResult<()> +where + R: BufRead, +{ + let mut lines = input.lines(); for (idx, line) in lines.by_ref().enumerate() { match line { Ok(l) if idx < options.header => { @@ -174,7 +174,11 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let result = match matches.values_of(options::NUMBER) { Some(values) => handle_args(values, options), - None => handle_stdin(options), + None => { + let stdin = std::io::stdin(); + let mut locked_stdin = stdin.lock(); + handle_buffer(&mut locked_stdin, options) + } }; match result { @@ -264,3 +268,41 @@ pub fn uu_app() -> App<'static, 'static> { ) .arg(Arg::with_name(options::NUMBER).hidden(true).multiple(true)) } + +#[cfg(test)] +mod tests { + use super::{handle_buffer, NumfmtOptions, Range, RoundMethod, TransformOptions, Unit}; + use std::io::{BufReader, Error, ErrorKind, Read}; + struct MockBuffer {} + + impl Read for MockBuffer { + fn read(&mut self, _: &mut [u8]) -> Result { + Err(Error::new(ErrorKind::BrokenPipe, "broken pipe")) + } + } + + fn get_options() -> NumfmtOptions { + NumfmtOptions { + transform: TransformOptions { + from: Unit::Auto, + to: Unit::Auto, + }, + padding: 10, + header: 0, + fields: vec![Range { low: 0, high: 1 }], + delimiter: None, + round: RoundMethod::Nearest, + suffix: None, + } + } + + #[test] + fn broken_buffer_returns_io_error() { + let mock_buffer = MockBuffer {}; + let result = handle_buffer(BufReader::new(mock_buffer), get_options()) + .expect_err("returned Ok after receiving IO error"); + let result_debug = format!("{:?}", result); + assert_eq!(result_debug, "IoError(\"broken pipe\")"); + assert_eq!(result.code(), 1); + } +} From 413bff26f83300d9b49f9f6fd4687a097ab63357 Mon Sep 17 00:00:00 2001 From: sbentmar Date: Sat, 15 Jan 2022 20:50:49 +0100 Subject: [PATCH 5/8] numfmt: ignore stdin write error --- tests/by-util/test_numfmt.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/by-util/test_numfmt.rs b/tests/by-util/test_numfmt.rs index c7dea620c..0086c25e1 100644 --- a/tests/by-util/test_numfmt.rs +++ b/tests/by-util/test_numfmt.rs @@ -589,6 +589,7 @@ fn test_invalid_argument_returns_status_1() { new_ucmd!() .args(&["--header=hello"]) .pipe_in("53478") + .ignore_stdin_write_error() .fails() .code_is(1); } From 1287ce378088b40a39293af6b5094b808ae18c92 Mon Sep 17 00:00:00 2001 From: sbentmar Date: Sat, 15 Jan 2022 22:12:21 +0100 Subject: [PATCH 6/8] numfmt: add tests for handle_buffer --- src/uu/numfmt/src/numfmt.rs | 44 +++++++++++++++++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-) diff --git a/src/uu/numfmt/src/numfmt.rs b/src/uu/numfmt/src/numfmt.rs index 0b0108453..eee2f0d2b 100644 --- a/src/uu/numfmt/src/numfmt.rs +++ b/src/uu/numfmt/src/numfmt.rs @@ -281,7 +281,22 @@ mod tests { } } - fn get_options() -> NumfmtOptions { + fn get_valid_options() -> NumfmtOptions { + NumfmtOptions { + transform: TransformOptions { + from: Unit::None, + to: Unit::None, + }, + padding: 10, + header: 0, + fields: vec![Range { low: 0, high: 1 }], + delimiter: None, + round: RoundMethod::Nearest, + suffix: None, + } + } + + fn get_invalid_options() -> NumfmtOptions { NumfmtOptions { transform: TransformOptions { from: Unit::Auto, @@ -299,10 +314,35 @@ mod tests { #[test] fn broken_buffer_returns_io_error() { let mock_buffer = MockBuffer {}; - let result = handle_buffer(BufReader::new(mock_buffer), get_options()) + let result = handle_buffer(BufReader::new(mock_buffer), get_valid_options()) .expect_err("returned Ok after receiving IO error"); let result_debug = format!("{:?}", result); + let result_display = format!("{}", result); assert_eq!(result_debug, "IoError(\"broken pipe\")"); + assert_eq!(result_display, "broken pipe"); assert_eq!(result.code(), 1); } + + #[test] + fn non_numeric_returns_formatting_error() { + let input_value = b"hello"; + let result = handle_buffer(BufReader::new(&input_value[..]), get_valid_options()) + .expect_err("returned Ok after receiving improperly formatted input"); + let result_debug = format!("{:?}", result); + let result_display = format!("{}", result); + assert_eq!( + result_debug, + "FormattingError(\"invalid suffix in input: 'hello'\")" + ); + assert_eq!(result_display, "invalid suffix in input: 'hello'"); + assert_eq!(result.code(), 2); + } + + #[test] + fn valid_input_returns_ok() { + let input_value = b"165"; + let result = handle_buffer(BufReader::new(&input_value[..]), get_valid_options()); + println!("{:?}", result); + assert!(result.is_ok(), "did not return Ok for valid input"); + } } From 635c2d0c31b7fb1e67d37e9f8828f00f9b573d45 Mon Sep 17 00:00:00 2001 From: sbentmar Date: Sat, 15 Jan 2022 22:35:38 +0100 Subject: [PATCH 7/8] numfmt: remove unused function --- src/uu/numfmt/src/numfmt.rs | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/uu/numfmt/src/numfmt.rs b/src/uu/numfmt/src/numfmt.rs index eee2f0d2b..f1b79df00 100644 --- a/src/uu/numfmt/src/numfmt.rs +++ b/src/uu/numfmt/src/numfmt.rs @@ -296,21 +296,6 @@ mod tests { } } - fn get_invalid_options() -> NumfmtOptions { - NumfmtOptions { - transform: TransformOptions { - from: Unit::Auto, - to: Unit::Auto, - }, - padding: 10, - header: 0, - fields: vec![Range { low: 0, high: 1 }], - delimiter: None, - round: RoundMethod::Nearest, - suffix: None, - } - } - #[test] fn broken_buffer_returns_io_error() { let mock_buffer = MockBuffer {}; From b0cf6f9b342b61d63dd47a9f8d4045a5464e5276 Mon Sep 17 00:00:00 2001 From: sbentmar Date: Sun, 16 Jan 2022 12:03:10 +0100 Subject: [PATCH 8/8] numfmt: minor adjustments to test cases --- src/uu/numfmt/src/numfmt.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/uu/numfmt/src/numfmt.rs b/src/uu/numfmt/src/numfmt.rs index f1b79df00..052a5b72a 100644 --- a/src/uu/numfmt/src/numfmt.rs +++ b/src/uu/numfmt/src/numfmt.rs @@ -288,7 +288,7 @@ mod tests { to: Unit::None, }, padding: 10, - header: 0, + header: 1, fields: vec![Range { low: 0, high: 1 }], delimiter: None, round: RoundMethod::Nearest, @@ -310,7 +310,7 @@ mod tests { #[test] fn non_numeric_returns_formatting_error() { - let input_value = b"hello"; + let input_value = b"135\nhello"; let result = handle_buffer(BufReader::new(&input_value[..]), get_valid_options()) .expect_err("returned Ok after receiving improperly formatted input"); let result_debug = format!("{:?}", result); @@ -325,9 +325,8 @@ mod tests { #[test] fn valid_input_returns_ok() { - let input_value = b"165"; + let input_value = b"165\n100\n300\n500"; let result = handle_buffer(BufReader::new(&input_value[..]), get_valid_options()); - println!("{:?}", result); assert!(result.is_ok(), "did not return Ok for valid input"); } }