From fdaf393beb32a7e9bbc49d30edc97c199955c3dc Mon Sep 17 00:00:00 2001 From: Devyn Cairns Date: Thu, 6 Jun 2024 16:29:04 -0700 Subject: [PATCH] more tests, and more fixes for tests --- crates/nu-command/src/system/run_external.rs | 45 ++++--- .../nu-command/tests/commands/run_external.rs | 38 ++++++ crates/nu-parser/src/parser.rs | 11 +- crates/nu-parser/tests/test_parser.rs | 111 ++++++++---------- crates/nu-test-support/src/macros.rs | 6 + 5 files changed, 123 insertions(+), 88 deletions(-) diff --git a/crates/nu-command/src/system/run_external.rs b/crates/nu-command/src/system/run_external.rs index 6e0b287ab8..1c7d5d0af5 100644 --- a/crates/nu-command/src/system/run_external.rs +++ b/crates/nu-command/src/system/run_external.rs @@ -1,5 +1,6 @@ use nu_cmd_base::hook::eval_hook; use nu_engine::{command_prelude::*, env_to_strings, get_eval_expression}; +use nu_path::{dots::expand_ndots, expand_tilde}; use nu_protocol::{ ast::{Expr, Expression}, did_you_mean, @@ -9,6 +10,7 @@ use nu_protocol::{ use nu_system::ForegroundChild; use nu_utils::IgnoreCaseExt; use std::{ + ffi::{OsStr, OsString}, io::Write, path::{Path, PathBuf}, process::Stdio, @@ -76,7 +78,7 @@ impl Command for External { let expanded_name = if is_bare_string(name_expr) { expand_tilde(&name.item) } else { - name.item.clone() + Path::new(&name.item).to_owned() }; // Determine the PATH to be used and then use `which` to find it - though this has no @@ -228,10 +230,10 @@ pub fn eval_arguments_from_call( engine_state: &EngineState, stack: &mut Stack, call: &Call, -) -> Result>, ShellError> { +) -> Result>, ShellError> { let ctrlc = &engine_state.ctrlc; let cwd = engine_state.cwd(Some(stack))?; - let mut args: Vec> = vec![]; + let mut args: Vec> = vec![]; for (expr, spread) in call.rest_iter(1) { if is_bare_string(expr) { // If `expr` is a bare string, perform glob expansion. @@ -244,7 +246,7 @@ pub fn eval_arguments_from_call( } } else { for arg in eval_argument(engine_state, stack, expr, spread)? { - args.push(arg.into_spanned(expr.span)); + args.push(OsString::from(arg).into_spanned(expr.span)); } } } @@ -306,12 +308,6 @@ fn is_bare_string(expr: &Expression) -> bool { ) } -/// Performs tilde expansion on `arg`. Returns the original string if `arg` -/// doesn't start with tilde. -fn expand_tilde(arg: &str) -> String { - nu_path::expand_tilde(arg).to_string_lossy().to_string() -} - /// Performs glob expansion on `arg`. If the expansion found no matches or the pattern /// is not a valid glob, then this returns the original string as the expansion result. /// @@ -322,12 +318,14 @@ fn expand_glob( cwd: &Path, span: Span, interrupt: &Option>, -) -> Result, ShellError> { +) -> Result, ShellError> { const GLOB_CHARS: &[char] = &['*', '?', '[']; - // Don't expand something that doesn't include the GLOB_CHARS + // For an argument that doesn't include the GLOB_CHARS, just do the `expand_tilde` + // and `expand_ndots` expansion if !arg.contains(GLOB_CHARS) { - return Ok(vec![arg.into()]); + let path = expand_ndots(expand_tilde(arg)); + return Ok(vec![path.into()]); } // We must use `nu_engine::glob_from` here, in order to ensure we get paths from the correct @@ -365,11 +363,7 @@ fn expand_glob( path } }) - // Convert the paths returned to UTF-8 strings. - // - // FIXME: this fails to return the correct results for non-UTF-8 paths, but we don't support - // those in Nushell yet. - .map(|path| path.to_string_lossy().into_owned()) + .map(OsString::from) // Abandon if ctrl-c is pressed .map(|path| { if !nu_utils::ctrl_c::was_pressed(interrupt) { @@ -378,7 +372,7 @@ fn expand_glob( Err(ShellError::InterruptedByUser { span: Some(span) }) } }) - .collect::, ShellError>>()?; + .collect::, ShellError>>()?; if !paths.is_empty() { Ok(paths) @@ -556,7 +550,7 @@ pub fn command_not_found( /// Note: the `which.rs` crate always uses PATHEXT from the environment. As /// such, changing PATHEXT within Nushell doesn't work without updating the /// actual environment of the Nushell process. -pub fn which(name: &str, paths: &str, cwd: &Path) -> Option { +pub fn which(name: impl AsRef, paths: &str, cwd: &Path) -> Option { #[cfg(windows)] let paths = format!("{};{}", cwd.display(), paths); which::which_in(name, Some(paths), cwd).ok() @@ -678,9 +672,9 @@ mod test { assert_eq!(actual, expected); let actual = expand_glob("./*.txt", cwd, Span::unknown(), &None).unwrap(); - let expected = vec![ - Path::new(".").join("a.txt").to_string_lossy().into_owned(), - Path::new(".").join("b.txt").to_string_lossy().into_owned(), + let expected: Vec = vec![ + Path::new(".").join("a.txt").into(), + Path::new(".").join("b.txt").into(), ]; assert_eq!(actual, expected); @@ -699,6 +693,11 @@ mod test { let actual = expand_glob("[*.txt", cwd, Span::unknown(), &None).unwrap(); let expected = &["[*.txt"]; assert_eq!(actual, expected); + + let actual = expand_glob("~/foo.txt", cwd, Span::unknown(), &None).unwrap(); + let home = dirs_next::home_dir().expect("failed to get home dir"); + let expected: Vec = vec![home.join("foo.txt").into()]; + assert_eq!(actual, expected); }) } diff --git a/crates/nu-command/tests/commands/run_external.rs b/crates/nu-command/tests/commands/run_external.rs index 68689a8576..6a25e8acd2 100644 --- a/crates/nu-command/tests/commands/run_external.rs +++ b/crates/nu-command/tests/commands/run_external.rs @@ -221,6 +221,44 @@ fn external_command_escape_args() { }) } +#[test] +#[cfg_attr( + not(target_os = "linux"), + ignore = "only runs on Linux, where controlling the HOME var is reliable" +)] +fn external_command_expand_tilde() { + Playground::setup("external command expand tilde", |dirs, _| { + // Make a copy of the nu executable that we can use + let mut src = std::fs::File::open(nu_test_support::fs::binaries().join("nu")) + .expect("failed to open nu"); + let mut dst = std::fs::File::create_new(dirs.test().join("test_nu")) + .expect("failed to create test_nu file"); + std::io::copy(&mut src, &mut dst).expect("failed to copy data for nu binary"); + + // Make test_nu have the same permissions so that it's executable + dst.set_permissions( + src.metadata() + .expect("failed to get nu metadata") + .permissions(), + ) + .expect("failed to set permissions on test_nu"); + + // Close the files + drop(dst); + drop(src); + + let actual = nu!( + envs: vec![ + ("HOME".to_string(), dirs.test().to_string_lossy().into_owned()), + ], + r#" + ^~/test_nu --testbin cococo hello + "# + ); + assert_eq!(actual.out, "hello"); + }) +} + #[test] fn external_arg_expand_tilde() { Playground::setup("external arg expand tilde", |dirs, _| { diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 108e2b3341..6a1bb6ebd0 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -237,7 +237,7 @@ fn parse_external_string(working_set: &mut StateWorkingSet, span: Span) -> Expre parse_raw_string(working_set, span) } else if contents .iter() - .any(|b| matches!(b, b'"' | b'\'' | b'`' | b'(' | b')')) + .any(|b| matches!(b, b'"' | b'\'' | b'(' | b')')) { enum State { Bare { @@ -250,6 +250,11 @@ fn parse_external_string(working_set: &mut StateWorkingSet, span: Span) -> Expre depth: i32, }, } + // Find the spans of parts of the string that can be parsed as their own strings for + // concatenation. + // + // By passing each of these parts to `parse_string()`, we can eliminate the quotes and also + // handle string interpolation. let make_span = |from: usize, index: usize| Span { start: span.start + from, end: span.start + index, @@ -259,7 +264,7 @@ fn parse_external_string(working_set: &mut StateWorkingSet, span: Span) -> Expre for (index, &ch) in contents.iter().enumerate() { match &mut state { State::Bare { from } => match ch { - b'"' | b'\'' | b'`' => { + b'"' | b'\'' => { // Push bare string if index != *from { spans.push(make_span(*from, index)); @@ -281,7 +286,7 @@ fn parse_external_string(working_set: &mut StateWorkingSet, span: Span) -> Expre escaped, depth, } => match ch { - b'"' if !*escaped => { + ch if ch == *quote_char && !*escaped => { // Count if there are more than `depth` quotes remaining if contents[index..] .iter() diff --git a/crates/nu-parser/tests/test_parser.rs b/crates/nu-parser/tests/test_parser.rs index 9998a4aeed..7d1a0ef061 100644 --- a/crates/nu-parser/tests/test_parser.rs +++ b/crates/nu-parser/tests/test_parser.rs @@ -713,6 +713,32 @@ pub fn parse_call_missing_req_flag() { r"foo\external-call", "bare word with backslash and caret" )] +#[case("`foo external call`", "foo external call", "backtick quote")] +#[case( + "^`foo external call`", + "foo external call", + "backtick quote with caret" +)] +#[case( + "`foo/external call`", + "foo/external call", + "backtick quote with forward slash" +)] +#[case( + "^`foo/external call`", + "foo/external call", + "backtick quote with forward slash and caret" +)] +#[case( + r"`foo\external call`", + r"foo\external call", + "backtick quote with backslash" +)] +#[case( + r"^`foo\external call`", + r"foo\external call", + "backtick quote with backslash and caret" +)] pub fn test_external_call_head_glob( #[case] input: &str, #[case] expected: &str, @@ -827,32 +853,6 @@ pub fn test_external_call_head_raw_string( r#"foo\external call"#, "double quote with backslash and caret" )] -#[case("`foo external call`", "foo external call", "backtick quote")] -#[case( - "^`foo external call`", - "foo external call", - "backtick quote with caret" -)] -#[case( - "`foo/external call`", - "foo/external call", - "backtick quote with forward slash" -)] -#[case( - "^`foo/external call`", - "foo/external call", - "backtick quote with forward slash and caret" -)] -#[case( - r"`foo\external call`", - r"foo\external call", - "backtick quote with backslash" -)] -#[case( - r"^`foo\external call`", - r"foo\external call", - "backtick quote with backslash and caret" -)] pub fn test_external_call_head_string( #[case] input: &str, #[case] expected: &str, @@ -948,6 +948,21 @@ pub fn test_external_call_head_interpolated_string( r"foo\external-call", "bare word with backslash" )] +#[case( + "^foo `foo external call`", + "foo external call", + "backtick quote with caret" +)] +#[case( + "^foo `foo/external call`", + "foo/external call", + "backtick quote with forward slash" +)] +#[case( + r"^foo `foo\external call`", + r"foo\external call", + "backtick quote with backslash" +)] pub fn test_external_call_arg_glob(#[case] input: &str, #[case] expected: &str, #[case] tag: &str) { let engine_state = EngineState::new(); let mut working_set = StateWorkingSet::new(&engine_state); @@ -1054,55 +1069,27 @@ pub fn test_external_call_arg_raw_string( } #[rstest] -#[case( - "^foo 'foo external call'", - "foo external call", - "single quote with caret" -)] +#[case("^foo 'foo external call'", "foo external call", "single quote")] #[case( "^foo 'foo/external call'", "foo/external call", - "single quote with forward slash and caret" + "single quote with forward slash" )] #[case( r"^foo 'foo\external call'", r"foo\external call", - "single quote with backslash and caret" -)] -#[case( - r#"^foo "foo external call""#, - r#"foo external call"#, - "double quote with caret" + "single quote with backslash" )] +#[case(r#"^foo "foo external call""#, r#"foo external call"#, "double quote")] #[case( r#"^foo "foo/external call""#, r#"foo/external call"#, - "double quote with forward slash and caret" + "double quote with forward slash" )] #[case( r#"^foo "foo\\external call""#, r#"foo\external call"#, - "double quote with backslash and caret" -)] -#[case( - "^foo `foo external call`", - "foo external call", - "backtick quote with caret" -)] -#[case( - "^foo `foo/external call`", - "foo/external call", - "backtick quote with forward slash" -)] -#[case( - "^foo `foo/external call`", - "foo/external call", - "backtick quote with forward slash and caret" -)] -#[case( - r"^foo `foo\external call`", - r"foo\external call", - "backtick quote with backslash and caret" + "double quote with backslash" )] pub fn test_external_call_arg_string( #[case] input: &str, @@ -1138,11 +1125,11 @@ pub fn test_external_call_arg_string( assert_eq!(expected, string, "{tag}: incorrect arg"); } other => { - panic!("Unexpected expression in command arg position: {other:?}") + panic!("{tag}: Unexpected expression in command arg position: {other:?}") } }, other @ ExternalArgument::Spread(..) => { - panic!("Unexpected external spread argument in command arg position: {other:?}") + panic!("{tag}: Unexpected external spread argument in command arg position: {other:?}") } } } diff --git a/crates/nu-test-support/src/macros.rs b/crates/nu-test-support/src/macros.rs index c18dd5ac81..6806fe2d5b 100644 --- a/crates/nu-test-support/src/macros.rs +++ b/crates/nu-test-support/src/macros.rs @@ -245,6 +245,7 @@ use tempfile::tempdir; pub struct NuOpts { pub cwd: Option, pub locale: Option, + pub envs: Option>, pub collapse_output: Option, } @@ -278,6 +279,11 @@ pub fn nu_run_test(opts: NuOpts, commands: impl AsRef, with_std: bool) -> O command .env(nu_utils::locale::LOCALE_OVERRIDE_ENV_VAR, locale) .env(NATIVE_PATH_ENV_VAR, paths_joined); + + if let Some(envs) = opts.envs { + command.envs(envs); + } + // Ensure that the user's config doesn't interfere with the tests command.arg("--no-config-file"); if !with_std {