From 708703b9ecea8524d349b2aa538a097c19fde23f Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Sat, 25 Oct 2025 16:46:47 +0200 Subject: [PATCH] Reimplement should_suppress_stderr_for_tests() for some tests "cargo test" captures stdout by default but not stderr. So it's probably still useful to suppress test output like in function 'recursive1' in function 'recursive2' [repeats many times] This was done by should_suppress_stderr_for_tests() which has been broken. Fix that, but only for the relevant cases instead of setting a global. --- src/bin/fish.rs | 2 +- src/builtins/eval.rs | 8 +++++- src/builtins/test.rs | 12 ++++----- src/common.rs | 13 ---------- src/exec.rs | 12 ++++++--- src/parse_execution.rs | 10 +++++--- src/parser.rs | 55 ++++++++++++++++++++++++++++++++++-------- 7 files changed, 74 insertions(+), 38 deletions(-) diff --git a/src/bin/fish.rs b/src/bin/fish.rs index 05cbbb6f1..a8a8aa8d8 100644 --- a/src/bin/fish.rs +++ b/src/bin/fish.rs @@ -228,7 +228,7 @@ fn run_command_list(parser: &Parser, cmds: &[OsString]) -> Result<(), libc::c_in if !errored { // Construct a parsed source ref. let ps = Arc::new(ParsedSource::new(cmd_wcs, ast)); - let _ = parser.eval_parsed_source(&ps, &IoChain::new(), None, BlockType::top); + let _ = parser.eval_parsed_source(&ps, &IoChain::new(), None, BlockType::top, false); retval = Ok(()); } else { let backtrace = parser.get_backtrace(&cmd_wcs, &errors); diff --git a/src/builtins/eval.rs b/src/builtins/eval.rs index 1d9efacde..a8aa84a09 100644 --- a/src/builtins/eval.rs +++ b/src/builtins/eval.rs @@ -52,7 +52,13 @@ pub fn eval(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr]) -> Bui } } - let res = parser.eval_with(&new_cmd, &ios, streams.job_group.as_ref(), BlockType::top); + let res = parser.eval_with( + &new_cmd, + &ios, + streams.job_group.as_ref(), + BlockType::top, + false, + ); let status = if res.was_empty { // Issue #5692, in particular, to catch `eval ""`, `eval "begin; end;"`, etc. // where we have an argument but nothing is executed. diff --git a/src/builtins/test.rs b/src/builtins/test.rs index d842ca985..9d8a544be 100644 --- a/src/builtins/test.rs +++ b/src/builtins/test.rs @@ -1072,14 +1072,12 @@ pub fn test(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Bui let mut eval_errors = Vec::new(); let result = expr.evaluate(streams, &mut eval_errors); if !eval_errors.is_empty() { - if !common::should_suppress_stderr_for_tests() { - for eval_error in eval_errors { - streams.err.appendln(&eval_error); - } - // Add a backtrace but not the "see help" message - // because this isn't about passing the wrong options. - streams.err.append(parser.current_line()); + for eval_error in eval_errors { + streams.err.appendln(&eval_error); } + // Add a backtrace but not the "see help" message + // because this isn't about passing the wrong options. + streams.err.append(parser.current_line()); return Err(STATUS_INVALID_ARGS); } diff --git a/src/common.rs b/src/common.rs index 26fd88836..90d2b7c9d 100644 --- a/src/common.rs +++ b/src/common.rs @@ -1219,19 +1219,6 @@ pub fn wcs2bytes_appending(output: &mut Vec, input: &wstr) { }); } -// Check if we are running in the test mode, where we should suppress error output -pub const TESTS_PROGRAM_NAME: &wstr = L!("(ignore)"); - -/// Hack to not print error messages in the tests. Do not call this from functions in this module -/// like `debug()`. It is only intended to suppress diagnostic noise from testing things like the -/// fish parser where we expect a lot of diagnostic messages due to testing error conditions. -pub fn should_suppress_stderr_for_tests() -> bool { - PROGRAM_NAME - .get() - .map(|p| p == TESTS_PROGRAM_NAME) - .unwrap_or_default() -} - /// Stored in blocks to reference the file which created the block. pub type FilenameRef = Arc; diff --git a/src/exec.rs b/src/exec.rs index f1f2ea738..c98cde253 100644 --- a/src/exec.rs +++ b/src/exec.rs @@ -997,7 +997,7 @@ fn get_performer_for_block_node(p: &Process, job: &Job, io_chain: &IoChain) -> B let node = node.clone(); Box::new(move |parser: &Parser, _out, _err| { parser - .eval_node(&node, &io_chain, job_group.as_ref(), BlockType::top) + .eval_node(&node, &io_chain, job_group.as_ref(), BlockType::top, false) .status }) } @@ -1033,7 +1033,13 @@ fn get_performer_for_function( // Pull out the job list from the function. let fb = function_prepare_environment(parser, argv, &props); let body_node = props.func_node.child_ref(|n| &n.jobs); - let mut res = parser.eval_node(&body_node, &io_chain, job_group.as_ref(), BlockType::top); + let mut res = parser.eval_node( + &body_node, + &io_chain, + job_group.as_ref(), + BlockType::top, + false, + ); function_restore_environment(parser, fb); // If the function did not execute anything, treat it as success. @@ -1488,7 +1494,7 @@ fn exec_subshell_internal( let mut io_chain = IoChain::new(); io_chain.push(bufferfill.clone()); - let eval_res = parser.eval_with(cmd, &io_chain, job_group, BlockType::subst); + let eval_res = parser.eval_with(cmd, &io_chain, job_group, BlockType::subst, false); let buffer = IoBufferfill::finish(bufferfill); if buffer.discarded() { *break_expand = true; diff --git a/src/parse_execution.rs b/src/parse_execution.rs index 416b3aa26..c1850d102 100644 --- a/src/parse_execution.rs +++ b/src/parse_execution.rs @@ -10,8 +10,7 @@ builtin_exists, }; use crate::common::{ - ScopeGuard, ScopeGuarding, ScopedRefCell, escape, should_suppress_stderr_for_tests, - truncate_at_nul, valid_var_name, + ScopeGuard, ScopeGuarding, ScopedRefCell, escape, truncate_at_nul, valid_var_name, }; use crate::complete::CompletionList; use crate::env::{EnvMode, EnvStackSetResult, EnvVar, EnvVarFlags, Environment, Statuses}; @@ -90,6 +89,9 @@ pub struct ExecutionContext<'a> { /// The block IO chain. /// For example, in `begin; foo ; end < file.txt` this would have the 'file.txt' IO. block_io: IoChain, + + /// Hack to supress non-redirectable stderr in some unit tests. + test_only_suppress_stderr: bool, } // Report an error, setting $status to `status`. Always returns @@ -119,12 +121,14 @@ pub fn new( pstree: ParsedSourceRef, block_io: IoChain, line_counter: &'a ScopedRefCell>, + test_only_suppress_stderr: bool, ) -> Self { Self { pstree, cancel_signal: None, line_counter, block_io, + test_only_suppress_stderr, } } @@ -242,7 +246,7 @@ fn report_errors( let backtrace_and_desc = ctx.parser().get_backtrace(&self.pstree().src, error_list); // Print it. - if !should_suppress_stderr_for_tests() { + if !self.test_only_suppress_stderr { eprintf!("%s", backtrace_and_desc); } diff --git a/src/parser.rs b/src/parser.rs index fa11e7da0..3e192a573 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -493,7 +493,7 @@ pub fn is_command_substitution(&self) -> bool { } pub fn eval(&self, cmd: &wstr, io: &IoChain) -> EvalRes { - self.eval_with(cmd, io, None, BlockType::top) + self.eval_with(cmd, io, None, BlockType::top, false) } /// Evaluate the expressions contained in cmd. @@ -510,6 +510,7 @@ pub fn eval_with( io: &IoChain, job_group: Option<&JobGroupRef>, block_type: BlockType, + test_only_suppress_stderr: bool, ) -> EvalRes { // Parse the source into a tree, if we can. let mut error_list = ParseErrorList::new(); @@ -518,14 +519,22 @@ pub fn eval_with( ParseTreeFlags::empty(), Some(&mut error_list), ) { - return self.eval_parsed_source(&ps, io, job_group, block_type); + return self.eval_parsed_source( + &ps, + io, + job_group, + block_type, + test_only_suppress_stderr, + ); } // Get a backtrace. This includes the message. let backtrace_and_desc = self.get_backtrace(cmd, &error_list); - // Print it. - eprintf!("%s\n", backtrace_and_desc); + if !test_only_suppress_stderr { + // Print it. + eprintf!("%s\n", backtrace_and_desc); + } // Set a valid status. self.set_last_statuses(Statuses::just(STATUS_ILLEGAL_CMD)); @@ -545,12 +554,19 @@ pub fn eval_parsed_source( io: &IoChain, job_group: Option<&JobGroupRef>, block_type: BlockType, + test_only_suppress_stderr: bool, ) -> EvalRes { assert!(matches!(block_type, BlockType::top | BlockType::subst)); let job_list = ps.top_job_list(); if !job_list.is_empty() { // Execute the top job list. - self.eval_node(&job_list, io, job_group, block_type) + self.eval_node( + &job_list, + io, + job_group, + block_type, + test_only_suppress_stderr, + ) } else { let status = ProcStatus::from_exit_code(self.get_last_status()); EvalRes { @@ -585,7 +601,7 @@ pub fn eval_wstr( // Construct a parsed source ref. // Be careful to transfer ownership, this could be a very large string. let ps = Arc::new(ParsedSource::new(src, ast)); - Ok(self.eval_parsed_source(&ps, io, job_group, block_type)) + Ok(self.eval_parsed_source(&ps, io, job_group, block_type, false)) } pub fn eval_file_wstr( @@ -616,6 +632,7 @@ pub fn eval_node( block_io: &IoChain, job_group: Option<&JobGroupRef>, block_type: BlockType, + test_only_suppress_stderr: bool, ) -> EvalRes { // Only certain blocks are allowed. assert!( @@ -674,7 +691,12 @@ pub fn eval_node( let restore_line_counter = self.line_counter.scoped_replace(ps.line_counter()); // Create a new execution context. - let mut execution_context = ExecutionContext::new(ps, block_io.clone(), &self.line_counter); + let mut execution_context = ExecutionContext::new( + ps, + block_io.clone(), + &self.line_counter, + test_only_suppress_stderr, + ); // Check the exec count so we know if anything got executed. let prev_exec_count = self.libdata().exec_count; @@ -1401,6 +1423,7 @@ mod tests { }; use crate::parse_tree::{LineCounter, parse_source}; use crate::parse_util::{parse_util_detect_errors, parse_util_detect_errors_in_argument}; + use crate::parser::BlockType; use crate::reader::{fake_scoped_reader, reader_reset_interrupted}; use crate::signal::{signal_clear_cancel, signal_reset_handlers, signal_set_handlers}; use crate::tests::prelude::*; @@ -2033,12 +2056,15 @@ fn test_eval_recursion_detection() { &IoChain::new(), ); - parser.eval( + parser.eval_with( L!(concat!( "function recursive1 ; recursive2 ; end ; ", "function recursive2 ; recursive1 ; end ; recursive1; ", )), &IoChain::new(), + None, + BlockType::top, + /*test_only_suppress_stderr=*/ true, ); } @@ -2049,7 +2075,13 @@ fn test_eval_illegal_exit_code() { let parser = TestParser::new(); macro_rules! validate { ($cmd:expr, $result:expr) => { - parser.eval($cmd, &IoChain::new()); + parser.eval_with( + $cmd, + &IoChain::new(), + None, + BlockType::top, + /*test_only_suppress_stderr=*/ true, + ); let exit_status = parser.get_last_status(); assert_eq!(exit_status, parser.get_last_status()); }; @@ -2073,9 +2105,12 @@ macro_rules! validate { fn test_eval_empty_function_name() { let _cleanup = test_init(); let parser = TestParser::new(); - parser.eval( + parser.eval_with( L!("function '' ; echo fail; exit 42 ; end ; ''"), &IoChain::new(), + None, + BlockType::top, + /*test_only_suppress_stderr=*/ true, ); }