From dbebd60c4e00071fe358da92b0f84458383f67ad Mon Sep 17 00:00:00 2001 From: Benjamin Sago Date: Thu, 10 Aug 2017 17:54:28 +0100 Subject: [PATCH] Extract var_os and use the mock to test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some of the deduce functions used to just blatantly call std::env::var_os and not care, introducing global state into a module that was otherwise nice and functional and self-contained. (Well, almost. There’s still terminal width.) Anyway, this made it hard to test, because we couldn’t test it fully with this global dependency in place. It *is* possible to work around this by actually setting the environment variables in the tests, but this way is more self-documenting. With this in place, we can start to unit test things like deriving the view by passing in what the $COLUMNS environment variable should be, and that’s one of the first things checked. src/options/mod.rs *almost* has all its tests moved to where they should be! --- src/exa.rs | 15 ++++++- src/options/help.rs | 6 +-- src/options/mod.rs | 63 +++++++++++++---------------- src/options/version.rs | 2 +- src/options/view.rs | 92 ++++++++++++++++++++++++++++++------------ 5 files changed, 113 insertions(+), 65 deletions(-) diff --git a/src/exa.rs b/src/exa.rs index cfddf9d..55aa2e9 100644 --- a/src/exa.rs +++ b/src/exa.rs @@ -22,6 +22,7 @@ extern crate term_size; extern crate lazy_static; +use std::env::var_os; use std::ffi::{OsStr, OsString}; use std::io::{stderr, Write, Result as IOResult}; use std::path::{Component, PathBuf}; @@ -29,7 +30,7 @@ use std::path::{Component, PathBuf}; use ansi_term::{ANSIStrings, Style}; use fs::{Dir, File}; -use options::Options; +use options::{Options, Vars}; pub use options::Misfire; use output::{escape, lines, grid, grid_details, details, View, Mode}; @@ -55,10 +56,20 @@ pub struct Exa<'args, 'w, W: Write + 'w> { pub args: Vec<&'args OsStr>, } +/// The “real” environment variables type. +/// Instead of just calling `var_os` from within the options module, +/// the method of looking up environment variables has to be passed in. +struct LiveVars; +impl Vars for LiveVars { + fn get(&self, name: &'static str) -> Option { + var_os(name) + } +} + impl<'args, 'w, W: Write + 'w> Exa<'args, 'w, W> { pub fn new(args: I, writer: &'w mut W) -> Result, Misfire> where I: Iterator { - Options::getopts(args).map(move |(options, args)| { + Options::parse(args, LiveVars).map(move |(options, args)| { Exa { options, writer, args } }) } diff --git a/src/options/help.rs b/src/options/help.rs index 8df31c0..93c2a93 100644 --- a/src/options/help.rs +++ b/src/options/help.rs @@ -130,21 +130,21 @@ mod test { #[test] fn help() { let args = [ os("--help") ]; - let opts = Options::getopts(&args); + let opts = Options::parse(&args, None); assert!(opts.is_err()) } #[test] fn help_with_file() { let args = [ os("--help"), os("me") ]; - let opts = Options::getopts(&args); + let opts = Options::parse(&args, None); assert!(opts.is_err()) } #[test] fn unhelpful() { let args = []; - let opts = Options::getopts(&args); + let opts = Options::parse(&args, None); assert!(opts.is_ok()) // no help when --help isn’t passed } } diff --git a/src/options/mod.rs b/src/options/mod.rs index 9a91908..f4861b7 100644 --- a/src/options/mod.rs +++ b/src/options/mod.rs @@ -112,10 +112,13 @@ pub struct Options { impl Options { - /// Call getopts on the given slice of command-line strings. + /// Parse the given iterator of command-line strings into an Options + /// struct and a list of free filenames, using the environment variables + /// for extra options. #[allow(unused_results)] - pub fn getopts<'args, I>(args: I) -> Result<(Options, Vec<&'args OsStr>), Misfire> - where I: IntoIterator { + pub fn parse<'args, I, V>(args: I, vars: V) -> Result<(Options, Vec<&'args OsStr>), Misfire> + where I: IntoIterator, + V: Vars { use options::parser::{Matches, Strictness}; let Matches { flags, frees } = match flags::ALL_ARGS.parse(args, Strictness::UseLastArguments) { @@ -126,7 +129,7 @@ impl Options { HelpString::deduce(&flags).map_err(Misfire::Help)?; VersionString::deduce(&flags).map_err(Misfire::Version)?; - let options = Options::deduce(&flags)?; + let options = Options::deduce(&flags, vars)?; Ok((options, frees)) } @@ -143,23 +146,36 @@ impl Options { /// Determines the complete set of options based on the given command-line /// arguments, after they’ve been parsed. - fn deduce(matches: &MatchedFlags) -> Result { + fn deduce(matches: &MatchedFlags, vars: V) -> Result { let dir_action = DirAction::deduce(matches)?; let filter = FileFilter::deduce(matches)?; - let view = View::deduce(matches)?; + let view = View::deduce(matches, vars)?; Ok(Options { dir_action, view, filter }) } } +/// Mockable wrapper for `std::env::var_os`. +pub trait Vars { + fn get(&self, name: &'static str) -> Option; +} + + + #[cfg(test)] pub mod test { - use super::{Options, Misfire, flags}; + use super::{Options, Misfire, Vars, flags}; use options::parser::{Arg, MatchedFlags}; use std::ffi::OsString; - use fs::filter::{SortField, SortCase}; + + // Test impl that just returns the value it has. + impl Vars for Option { + fn get(&self, _name: &'static str) -> Option { + self.clone() + } + } #[derive(PartialEq, Debug)] @@ -209,57 +225,36 @@ pub mod test { #[test] fn files() { let args = [ os("this file"), os("that file") ]; - let outs = Options::getopts(&args).unwrap().1; + let outs = Options::parse(&args, None).unwrap().1; assert_eq!(outs, vec![ &os("this file"), &os("that file") ]) } #[test] fn no_args() { let nothing: Vec = Vec::new(); - let outs = Options::getopts(¬hing).unwrap().1; + let outs = Options::parse(¬hing, None).unwrap().1; assert!(outs.is_empty()); // Listing the `.` directory is done in main.rs } #[test] fn long_across() { let args = [ os("--long"), os("--across") ]; - let opts = Options::getopts(&args); + let opts = Options::parse(&args, None); assert_eq!(opts.unwrap_err(), Misfire::Useless(&flags::ACROSS, true, &flags::LONG)) } #[test] fn oneline_across() { let args = [ os("--oneline"), os("--across") ]; - let opts = Options::getopts(&args); + let opts = Options::parse(&args, None); assert_eq!(opts.unwrap_err(), Misfire::Useless(&flags::ACROSS, true, &flags::ONE_LINE)) } - #[test] - fn test_sort_size() { - let args = [ os("--sort=size") ]; - let opts = Options::getopts(&args); - assert_eq!(opts.unwrap().0.filter.sort_field, SortField::Size); - } - - #[test] - fn test_sort_name() { - let args = [ os("--sort=name") ]; - let opts = Options::getopts(&args); - assert_eq!(opts.unwrap().0.filter.sort_field, SortField::Name(SortCase::Sensitive)); - } - - #[test] - fn test_sort_name_lowercase() { - let args = [ os("--sort=Name") ]; - let opts = Options::getopts(&args); - assert_eq!(opts.unwrap().0.filter.sort_field, SortField::Name(SortCase::Insensitive)); - } - #[test] #[cfg(feature="git")] fn just_git() { let args = [ os("--git") ]; - let opts = Options::getopts(&args); + let opts = Options::parse(&args, None); assert_eq!(opts.unwrap_err(), Misfire::Useless(&flags::GIT, false, &flags::LONG)) } } diff --git a/src/options/version.rs b/src/options/version.rs index fb3759e..0cd4c54 100644 --- a/src/options/version.rs +++ b/src/options/version.rs @@ -54,7 +54,7 @@ mod test { #[test] fn help() { let args = [ os("--version") ]; - let opts = Options::getopts(&args); + let opts = Options::parse(&args, None); assert!(opts.is_err()) } } diff --git a/src/options/view.rs b/src/options/view.rs index 927e174..1f3920b 100644 --- a/src/options/view.rs +++ b/src/options/view.rs @@ -1,23 +1,20 @@ -use std::env::var_os; - use output::Colours; use output::{View, Mode, grid, details}; use output::table::{TimeTypes, Environment, SizeFormat, Columns, Options as TableOptions}; use output::file_name::{Classify, FileStyle}; use output::time::TimeFormat; -use options::{flags, Misfire}; +use options::{flags, Misfire, Vars}; use options::parser::MatchedFlags; use fs::feature::xattr; use info::filetype::FileExtensions; - impl View { /// Determine which view to use and all of that view’s arguments. - pub fn deduce(matches: &MatchedFlags) -> Result { - let mode = Mode::deduce(matches)?; + pub fn deduce(matches: &MatchedFlags, vars: V) -> Result { + let mode = Mode::deduce(matches, vars)?; let colours = Colours::deduce(matches)?; let style = FileStyle::deduce(matches)?; Ok(View { mode, colours, style }) @@ -28,7 +25,7 @@ impl View { impl Mode { /// Determine the mode from the command-line arguments. - pub fn deduce(matches: &MatchedFlags) -> Result { + pub fn deduce(matches: &MatchedFlags, vars: V) -> Result { use options::misfire::Misfire::*; let long = || { @@ -67,7 +64,7 @@ impl Mode { }; let other_options_scan = || { - if let Some(width) = TerminalWidth::deduce()?.width() { + if let Some(width) = TerminalWidth::deduce(vars)?.width() { if matches.has(&flags::ONE_LINE)? { if matches.has(&flags::ACROSS)? { Err(Useless(&flags::ACROSS, true, &flags::ONE_LINE)) @@ -153,8 +150,8 @@ impl TerminalWidth { /// Determine a requested terminal width from the command-line arguments. /// /// Returns an error if a requested width doesn’t parse to an integer. - fn deduce() -> Result { - if let Some(columns) = var_os("COLUMNS").and_then(|s| s.into_string().ok()) { + fn deduce(vars: V) -> Result { + if let Some(columns) = vars.get("COLUMNS").and_then(|s| s.into_string().ok()) { match columns.parse() { Ok(width) => Ok(TerminalWidth::Set(width)), Err(e) => Err(Misfire::FailedParse(e)), @@ -435,7 +432,8 @@ mod test { &flags::TIME, &flags::MODIFIED, &flags::CREATED, &flags::ACCESSED, &flags::COLOR, &flags::COLOUR, &flags::HEADER, &flags::GROUP, &flags::INODE, - &flags::LINKS, &flags::BLOCKS, &flags::LONG ]; + &flags::LINKS, &flags::BLOCKS, &flags::LONG, + &flags::GRID, &flags::ACROSS, &flags::ONE_LINE ]; macro_rules! test { @@ -475,6 +473,31 @@ mod test { } } }; + + + ($name:ident: $type:ident <- $inputs:expr, $vars:expr; $stricts:expr => err $result:expr) => { + /// Like above, but with $vars. + #[test] + fn $name() { + for result in parse_for_test($inputs.as_ref(), TEST_ARGS, $stricts, |mf| $type::deduce(mf, $vars)) { + assert_eq!(result.unwrap_err(), $result); + } + } + }; + + ($name:ident: $type:ident <- $inputs:expr, $vars:expr; $stricts:expr => like $pat:pat) => { + /// Like further above, but with $vars. + #[test] + fn $name() { + for result in parse_for_test($inputs.as_ref(), TEST_ARGS, $stricts, |mf| $type::deduce(mf, $vars)) { + println!("Testing {:?}", result); + match result { + $pat => assert!(true), + _ => assert!(false), + } + } + } + }; } @@ -601,26 +624,45 @@ mod test { mod views { use super::*; + use output::grid::Options as GridOptions; - test!(just_header: Mode <- ["--header"]; Last => like Ok(Mode::Grid(_))); - test!(just_header_2: Mode <- ["--header"]; Complain => err Misfire::Useless(&flags::HEADER, false, &flags::LONG)); + // Default + test!(empty: Mode <- [], None; Both => like Ok(Mode::Grid(_))); - test!(just_group: Mode <- ["--group"]; Last => like Ok(Mode::Grid(_))); - test!(just_group_2: Mode <- ["--group"]; Complain => err Misfire::Useless(&flags::GROUP, false, &flags::LONG)); + // Grid views + test!(original_g: Mode <- ["-G"], None; Both => like Ok(Mode::Grid(GridOptions { across: false, console_width: _ }))); + test!(grid: Mode <- ["--grid"], None; Both => like Ok(Mode::Grid(GridOptions { across: false, console_width: _ }))); + test!(across: Mode <- ["--across"], None; Both => like Ok(Mode::Grid(GridOptions { across: true, console_width: _ }))); + test!(gracross: Mode <- ["-xG"], None; Both => like Ok(Mode::Grid(GridOptions { across: true, console_width: _ }))); - test!(just_inode: Mode <- ["--inode"]; Last => like Ok(Mode::Grid(_))); - test!(just_inode_2: Mode <- ["--inode"]; Complain => err Misfire::Useless(&flags::INODE, false, &flags::LONG)); + // Lines views + test!(lines: Mode <- ["--oneline"], None; Both => like Ok(Mode::Lines)); + test!(prima: Mode <- ["-1"], None; Both => like Ok(Mode::Lines)); - test!(just_links: Mode <- ["--links"]; Last => like Ok(Mode::Grid(_))); - test!(just_links_2: Mode <- ["--links"]; Complain => err Misfire::Useless(&flags::LINKS, false, &flags::LONG)); + // Details views + test!(long: Mode <- ["--long"], None; Both => like Ok(Mode::Details(_))); + test!(ell: Mode <- ["-l"], None; Both => like Ok(Mode::Details(_))); - test!(just_blocks: Mode <- ["--blocks"]; Last => like Ok(Mode::Grid(_))); - test!(just_blocks_2: Mode <- ["--blocks"]; Complain => err Misfire::Useless(&flags::BLOCKS, false, &flags::LONG)); + // Grid-details views + test!(lid: Mode <- ["--long", "--grid"], None; Both => like Ok(Mode::GridDetails(_, _))); + test!(leg: Mode <- ["-lG"], None; Both => like Ok(Mode::GridDetails(_, _))); - test!(just_binary: Mode <- ["--binary"]; Last => like Ok(Mode::Grid(_))); - test!(just_binary_2: Mode <- ["--binary"]; Complain => err Misfire::Useless(&flags::BINARY, false, &flags::LONG)); - test!(just_bytes: Mode <- ["--bytes"]; Last => like Ok(Mode::Grid(_))); - test!(just_bytes_2: Mode <- ["--bytes"]; Complain => err Misfire::Useless(&flags::BYTES, false, &flags::LONG)); + // Options that do nothing without --long + test!(just_header: Mode <- ["--header"], None; Last => like Ok(Mode::Grid(_))); + test!(just_group: Mode <- ["--group"], None; Last => like Ok(Mode::Grid(_))); + test!(just_inode: Mode <- ["--inode"], None; Last => like Ok(Mode::Grid(_))); + test!(just_links: Mode <- ["--links"], None; Last => like Ok(Mode::Grid(_))); + test!(just_blocks: Mode <- ["--blocks"], None; Last => like Ok(Mode::Grid(_))); + test!(just_binary: Mode <- ["--binary"], None; Last => like Ok(Mode::Grid(_))); + test!(just_bytes: Mode <- ["--bytes"], None; Last => like Ok(Mode::Grid(_))); + + test!(just_header_2: Mode <- ["--header"], None; Complain => err Misfire::Useless(&flags::HEADER, false, &flags::LONG)); + test!(just_group_2: Mode <- ["--group"], None; Complain => err Misfire::Useless(&flags::GROUP, false, &flags::LONG)); + test!(just_inode_2: Mode <- ["--inode"], None; Complain => err Misfire::Useless(&flags::INODE, false, &flags::LONG)); + test!(just_links_2: Mode <- ["--links"], None; Complain => err Misfire::Useless(&flags::LINKS, false, &flags::LONG)); + test!(just_blocks_2: Mode <- ["--blocks"], None; Complain => err Misfire::Useless(&flags::BLOCKS, false, &flags::LONG)); + test!(just_binary_2: Mode <- ["--binary"], None; Complain => err Misfire::Useless(&flags::BINARY, false, &flags::LONG)); + test!(just_bytes_2: Mode <- ["--bytes"], None; Complain => err Misfire::Useless(&flags::BYTES, false, &flags::LONG)); } }