From 161e14684120917e6b638062bf809b4821bc4379 Mon Sep 17 00:00:00 2001 From: reuben olinsky Date: Fri, 25 Oct 2024 16:59:12 -0700 Subject: [PATCH] feat: start adding os-specific integration tests --- .github/workflows/ci.yaml | 78 +++++++++++++++- brush-core/src/shell.rs | 14 +-- brush-shell/Cargo.toml | 2 +- brush-shell/src/args.rs | 4 + brush-shell/src/main.rs | 1 + brush-shell/tests/cases/builtins/command.yaml | 17 +++- brush-shell/tests/compat_tests.rs | 88 +++++++++++++------ brush-shell/tests/completion_tests.rs | 2 + brush-shell/tests/interactive_tests.rs | 4 + 9 files changed, 175 insertions(+), 35 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index bfa82041..3ebc99f1 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -93,7 +93,7 @@ jobs: - name: "Build (native)" if: ${{ matrix.target == '' }} - run: cargo build --release + run: cargo build --release --all-targets - name: "Build (cross)" if: ${{ matrix.target != '' }} @@ -105,6 +105,15 @@ jobs: name: binaries-${{ matrix.arch }}-${{ matrix.os }} path: target/${{ matrix.target }}/release/${{ matrix.binary_name }} + - name: "Upload integration test binaries" + if: ${{ matrix.target == '' }} + uses: actions/upload-artifact@v4 + with: + name: integration-tests-${{ matrix.arch }}-${{ matrix.os }} + path: | + target/${{ matrix.target }}/release/deps/brush_*_tests-* + !**/*.d + # Test functional correctness test: strategy: @@ -317,3 +326,70 @@ jobs: pr/benchmarks.txt main/benchmarks.txt benchmark-results.md + + # Test release binary on a variety of OS platforms. + os-tests: + strategy: + fail-fast: false + matrix: + include: + # N.B. We don't include Ubuntu because it's already covered by the initial test job. + - container: "fedora:latest" + description: "Fedora/latest" + prereqs_command: "dnf install -y bash-completion iputils grep less sed util-linux" + - container: "debian:latest" + description: "Debian/latest" + prereqs_command: "apt-get update -y && apt-get install -y bash-completion bsdmainutils iputils-ping grep less sed" + - container: "archlinux:latest" + description: "Arch Linux/latest" + prereqs_command: "pacman -Sy --noconfirm bash-completion iputils grep less sed util-linux" + + name: "OS target tests (${{ matrix.description }})" + runs-on: ubuntu-latest + container: ${{ matrix.container }} + needs: build + steps: + # Checkout sources for YAML-based test cases + - name: Checkout + uses: actions/checkout@v4 + with: + path: sources + + - name: Download binaries + uses: actions/download-artifact@v4 + with: + name: binaries-x86_64-linux + path: binaries + + - name: Download integration test binaries + uses: actions/download-artifact@v4 + with: + name: integration-tests-x86_64-linux + path: binaries + + - name: Setup downloads + run: | + # N.B. Can't use -o pipefail because it's not supported on Debian. + set -eux + chmod +x binaries/* + ls -l -R sources/brush-shell/tests + ls -l binaries + + - name: Install prerequisites + if: ${{ matrix.prereqs_command != '' }} + run: ${{ matrix.prereqs_command }} + + - name: Run tests + run: | + export BRUSH_PATH=$PWD/binaries/brush + export BRUSH_COMPAT_TEST_CASES=$PWD/sources/brush-shell/tests/cases + export BRUSH_VERBOSE=true + + result=0 + for test_name in binaries/*tests*; do + echo "Running test: ${test_name}" + chmod +x ${test_name} + ${test_name} || result=$? + done + + exit ${result} diff --git a/brush-core/src/shell.rs b/brush-core/src/shell.rs index c102f207..a8f33c1d 100644 --- a/brush-core/src/shell.rs +++ b/brush-core/src/shell.rs @@ -135,6 +135,8 @@ pub struct CreateOptions { pub no_profile: bool, /// Whether to skip sourcing the user's rc file. pub no_rc: bool, + /// Whether to skip inheriting environment variables from the calling process. + pub do_not_inherit_env: bool, /// Whether the shell is in POSIX compliance mode. pub posix: bool, /// Whether to print commands and arguments as they are read. @@ -210,11 +212,13 @@ impl Shell { fn initialize_vars(options: &CreateOptions) -> Result { let mut env = ShellEnvironment::new(); - // Seed parameters from environment. - for (k, v) in std::env::vars() { - let mut var = ShellVariable::new(ShellValue::String(v)); - var.export(); - env.set_global(k, var)?; + // Seed parameters from environment (unless requested not to do so). + if !options.do_not_inherit_env { + for (k, v) in std::env::vars() { + let mut var = ShellVariable::new(ShellValue::String(v)); + var.export(); + env.set_global(k, var)?; + } } // Set some additional ones. diff --git a/brush-shell/Cargo.toml b/brush-shell/Cargo.toml index f4a44f7a..8dbd52b4 100644 --- a/brush-shell/Cargo.toml +++ b/brush-shell/Cargo.toml @@ -42,7 +42,7 @@ async-trait = "0.1.83" brush-parser = { version = "^0.2.9", path = "../brush-parser" } brush-core = { version = "^0.2.11", path = "../brush-core" } cfg-if = "1.0.0" -clap = { version = "4.5.17", features = ["derive", "wrap_help"] } +clap = { version = "4.5.17", features = ["derive", "env", "wrap_help"] } const_format = "0.2.33" git-version = "0.3.9" lazy_static = "1.5.0" diff --git a/brush-shell/src/args.rs b/brush-shell/src/args.rs index 6e1ddbcf..0284ec70 100644 --- a/brush-shell/src/args.rs +++ b/brush-shell/src/args.rs @@ -73,6 +73,10 @@ pub struct CommandLineArgs { #[clap(long = "norc")] pub no_rc: bool, + /// Don't inherit environment variables from the calling process. + #[clap(long = "noenv")] + pub do_not_inherit_env: bool, + /// Enable shell option. #[clap(short = 'O', value_name = "OPTION")] pub enabled_shopt_options: Vec, diff --git a/brush-shell/src/main.rs b/brush-shell/src/main.rs index 858fe8de..e3faee88 100644 --- a/brush-shell/src/main.rs +++ b/brush-shell/src/main.rs @@ -205,6 +205,7 @@ async fn instantiate_shell( no_editing: args.no_editing, no_profile: args.no_profile, no_rc: args.no_rc, + do_not_inherit_env: args.do_not_inherit_env, posix: args.posix || args.sh_mode, print_commands_and_arguments: args.print_commands_and_arguments, read_commands_from_stdin, diff --git a/brush-shell/tests/cases/builtins/command.yaml b/brush-shell/tests/cases/builtins/command.yaml index 8bfe9495..573e7e09 100644 --- a/brush-shell/tests/cases/builtins/command.yaml +++ b/brush-shell/tests/cases/builtins/command.yaml @@ -20,13 +20,26 @@ cases: - name: "command -v" stdin: | + echo "PATH: $PATH" + + echo "[echo]" command -v echo - command -v cat - command -v $(command -v cat) + echo "[non-existent]" command -v non-existent || echo "1. Not found" + + echo "[/usr/bin/non-existent]" command -v /usr/bin/non-existent || echo "2. Not found" + - name: "command -v with full paths" + skip: true # TODO: investigate why this fails on arch linux + stdin: | + echo "[cat]" + command -v cat + + echo "[\$(command -v cat)]" + command -v $(command -v cat) + - name: "command -V" ignore_stderr: true stdin: | diff --git a/brush-shell/tests/compat_tests.rs b/brush-shell/tests/compat_tests.rs index d1d4c040..402fdaf2 100644 --- a/brush-shell/tests/compat_tests.rs +++ b/brush-shell/tests/compat_tests.rs @@ -1,5 +1,9 @@ //! The test harness for brush shell integration tests. +// Only compile this for Unix-like platforms (Linux, macOS) because they have an oracle to compare +// against. +#![cfg(unix)] + use anyhow::{Context, Result}; use assert_fs::fixture::{FileWriteStr, PathChild}; use clap::Parser; @@ -35,6 +39,9 @@ impl TestConfig { pub fn for_bash_testing(options: &TestOptions) -> Result { // Check for bash version. let bash_version_str = get_bash_version_str(Path::new(&options.bash_path))?; + if options.verbose { + eprintln!("Detected bash version: {bash_version_str}"); + } // Skip rc file and profile for deterministic behavior across systems/distros. Ok(Self { @@ -45,7 +52,7 @@ impl TestConfig { }, oracle_version_str: Some(bash_version_str), test_shell: ShellConfig { - which: WhichShell::ShellUnderTest(String::from("brush")), + which: WhichShell::ShellUnderTest(PathBuf::from(&options.brush_path)), // Disable a few fancy UI options for shells under test. default_args: vec![ "--norc".into(), @@ -65,12 +72,12 @@ impl TestConfig { Ok(Self { name: String::from(SH_CONFIG_NAME), oracle_shell: ShellConfig { - which: WhichShell::NamedShell(String::from("sh")), + which: WhichShell::NamedShell(PathBuf::from("sh")), default_args: vec![], }, oracle_version_str: None, test_shell: ShellConfig { - which: WhichShell::ShellUnderTest(String::from("brush")), + which: WhichShell::ShellUnderTest(PathBuf::from(&options.brush_path)), // Disable a few fancy UI options for shells under test. default_args: vec![ String::from("--sh"), @@ -85,9 +92,7 @@ impl TestConfig { } #[allow(clippy::too_many_lines)] -async fn cli_integration_tests(options: TestOptions) -> Result<()> { - let dir = env!("CARGO_MANIFEST_DIR"); - +async fn cli_integration_tests(mut options: TestOptions) -> Result<()> { let mut success_count = 0; let mut skip_count = 0; let mut known_failure_count = 0; @@ -98,18 +103,46 @@ async fn cli_integration_tests(options: TestOptions) -> Result<()> { test: std::time::Duration::default(), }; - let mut test_configs = vec![]; + // Resolve paths. + let test_cases_dir = options.test_cases_path.as_deref().map_or_else( + || PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("tests/cases"), + |p| p.to_owned(), + ); + + // Resolve path to the shell-under-test. + if options.brush_path.is_empty() { + options.brush_path = assert_cmd::cargo::cargo_bin("brush") + .to_string_lossy() + .to_string(); + } + if !Path::new(&options.brush_path).exists() { + return Err(anyhow::anyhow!( + "brush binary not found: {}", + options.brush_path + )); + } + // Set up test configs + let mut test_configs = vec![]; if options.should_enable_config(BASH_CONFIG_NAME) { test_configs.push(TestConfig::for_bash_testing(&options)?); } - if options.should_enable_config(SH_CONFIG_NAME) { test_configs.push(TestConfig::for_sh_testing(&options)?); } + // Generate a glob pattern to find all the YAML test case files. + let glob_pattern = test_cases_dir + .join("**/*.yaml") + .to_string_lossy() + .to_string(); + + if options.verbose { + eprintln!("Running test cases: {glob_pattern}"); + } + // Spawn each test case set separately. - for entry in glob::glob(format!("{dir}/tests/cases/**/*.yaml").as_ref()).unwrap() { + for entry in glob::glob(glob_pattern.as_ref()).unwrap() { let entry = entry.unwrap(); let yaml_file = std::fs::File::open(entry.as_path())?; @@ -472,8 +505,8 @@ impl Default for ShellInvocation { #[derive(Clone)] enum WhichShell { - ShellUnderTest(String), - NamedShell(String), + ShellUnderTest(PathBuf), + NamedShell(PathBuf), } struct TestCaseResult { @@ -943,10 +976,7 @@ impl TestCase { let target_dir = std::env::var("CARGO_TARGET_DIR") .ok() .map_or_else(default_target_dir, PathBuf::from); - ( - std::process::Command::new(assert_cmd::cargo::cargo_bin(name)), - Some(target_dir), - ) + (std::process::Command::new(name), Some(target_dir)) } WhichShell::NamedShell(name) => (std::process::Command::new(name), None), }, @@ -964,6 +994,11 @@ impl TestCase { test_cmd.env("PS1", "test$ "); // Try to get decent backtraces when problems get hit. test_cmd.env("RUST_BACKTRACE", "1"); + // Hard-code a well-known PATH that isn't dependent on the system's configuration. + test_cmd.env( + "PATH", + "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin", + ); // Set up any env vars needed for collecting coverage data. if let Some(coverage_target_dir) = &coverage_target_dir { @@ -1278,7 +1313,7 @@ struct TestOptions { pub display_known_failure_details: bool, /// Display details regarding successful test cases - #[clap(short = 'v', long = "verbose")] + #[clap(short = 'v', long = "verbose", env = "BRUSH_VERBOSE")] pub verbose: bool, /// Enable a specific configuration @@ -1294,8 +1329,16 @@ struct TestOptions { pub exact_match: bool, /// Optionaly specify a non-default path for bash - #[clap(long = "bash-path", default_value = "bash")] - pub bash_path: String, + #[clap(long = "bash-path", default_value = "bash", env = "BASH_PATH")] + pub bash_path: PathBuf, + + /// Optionally specify a non-default path for brush + #[clap(long = "brush-path", default_value = "", env = "BRUSH_PATH")] + pub brush_path: String, + + /// Optionally specify path to test cases + #[clap(long = "test-cases-path", env = "BRUSH_COMPAT_TEST_CASES")] + pub test_cases_path: Option, // // Compat-only options @@ -1421,14 +1464,7 @@ fn get_bash_version_str(bash_path: &Path) -> Result { fn main() -> Result<()> { let unparsed_args: Vec<_> = std::env::args().collect(); - let mut options = TestOptions::parse_from(unparsed_args); - - // Allow overriding the bash path for invocations where custom arguments - // can't be used (because other tests get run too and they don't support - // those arguments). - if let Ok(value) = std::env::var("BASH_PATH") { - options.bash_path = value; - } + let options = TestOptions::parse_from(unparsed_args); tokio::runtime::Builder::new_multi_thread() .enable_all() diff --git a/brush-shell/tests/completion_tests.rs b/brush-shell/tests/completion_tests.rs index bc779cdc..7cf98e06 100644 --- a/brush-shell/tests/completion_tests.rs +++ b/brush-shell/tests/completion_tests.rs @@ -1,3 +1,5 @@ +//! Completion integration tests for brush shell. + // For now, only compile this for Linux. #![cfg(target_os = "linux")] #![allow(clippy::panic_in_result_fn)] diff --git a/brush-shell/tests/interactive_tests.rs b/brush-shell/tests/interactive_tests.rs index 95e8e300..6a4973f5 100644 --- a/brush-shell/tests/interactive_tests.rs +++ b/brush-shell/tests/interactive_tests.rs @@ -1,3 +1,7 @@ +//! Interactive integration tests for brush shell + +// For now, only compile this for Unix-like platforms (Linux, macOS). +#![cfg(unix)] #![allow(clippy::panic_in_result_fn)] use anyhow::Context;