Extract var_os and use the mock to test

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

@@ -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<OsString> {
+        var_os(name)
+    }
+}
+
 impl<'args, 'w, W: Write + 'w> Exa<'args, 'w, W> {
     pub fn new<I>(args: I, writer: &'w mut W) -> Result<Exa<'args, 'w, W>, Misfire>
     where I: Iterator<Item=&'args OsString> {
-        Options::getopts(args).map(move |(options, args)| {
+        Options::parse(args, LiveVars).map(move |(options, args)| {
             Exa { options, writer, args }
         })
     }

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
     }
 }

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<Item=&'args OsString> {
+    pub fn parse<'args, I, V>(args: I, vars: V) -> Result<(Options, Vec<&'args OsStr>), Misfire>
+    where I: IntoIterator<Item=&'args OsString>,
+          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<Options, Misfire> {
+    fn deduce<V: Vars>(matches: &MatchedFlags, vars: V) -> Result<Options, Misfire> {
         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<OsString>;
+}
+
+
+
 
 #[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<OsString> {
+        fn get(&self, _name: &'static str) -> Option<OsString> {
+            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<OsString> = Vec::new();
-        let outs = Options::getopts(&nothing).unwrap().1;
+        let outs = Options::parse(&nothing, 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))
     }
 }

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())
     }
 }

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<View, Misfire> {
-        let mode = Mode::deduce(matches)?;
+    pub fn deduce<V: Vars>(matches: &MatchedFlags, vars: V) -> Result<View, Misfire> {
+        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<Mode, Misfire> {
+    pub fn deduce<V: Vars>(matches: &MatchedFlags, vars: V) -> Result<Mode, Misfire> {
         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<TerminalWidth, Misfire> {
-        if let Some(columns) = var_os("COLUMNS").and_then(|s| s.into_string().ok()) {
+    fn deduce<V: Vars>(vars: V) -> Result<TerminalWidth, Misfire> {
+        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));
-
-        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));
-
-        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));
-
-        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));
-
-        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));
-
-        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));
+        // Default
+        test!(empty:         Mode <- [], None;            Both => like Ok(Mode::Grid(_)));
+
+        // 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: _ })));
+
+        // Lines views
+        test!(lines:         Mode <- ["--oneline"], None; Both => like Ok(Mode::Lines));
+        test!(prima:         Mode <- ["-1"], None;        Both => like Ok(Mode::Lines));
+
+        // Details views
+        test!(long:          Mode <- ["--long"], None;    Both => like Ok(Mode::Details(_)));
+        test!(ell:           Mode <- ["-l"], None;        Both => like Ok(Mode::Details(_)));
+
+        // Grid-details views
+        test!(lid:           Mode <- ["--long", "--grid"], None;  Both => like Ok(Mode::GridDetails(_, _)));
+        test!(leg:           Mode <- ["-lG"], None;               Both => like Ok(Mode::GridDetails(_, _)));
+
+
+        // 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));
     }
 }