Skip to content

fix: delete some flaky and not-useful tests #4861

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jamadeo merged 1 commit into from
Sep 26, 2025
Merged

fix: delete some flaky and not-useful tests #4861

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
261 changes: 0 additions & 261 deletions crates/goose-cli/src/logging.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,35 +22,6 @@ fn get_log_directory() -> Result<PathBuf> {
goose::logging::get_log_directory("cli", true)
}

/// Internal function that allows specifying a custom date string for testing
#[cfg(test)]
fn get_log_directory_with_date(test_date: Option<String>) -> Result<PathBuf> {
use etcetera::{choose_app_strategy, AppStrategy};
use goose::config::APP_STRATEGY;

let home_dir =
choose_app_strategy(APP_STRATEGY.clone()).context("HOME environment variable not set")?;

let base_log_dir = home_dir
.in_state_dir("logs")
.unwrap_or_else(|| home_dir.in_data_dir("logs"));

let component_dir = base_log_dir.join("cli");

let log_dir = if let Some(date_str) = test_date {
component_dir.join(date_str)
} else {
// Create date-based subdirectory
let now = chrono::Local::now();
component_dir.join(now.format("%Y-%m-%d").to_string())
};

// Ensure log directory exists
std::fs::create_dir_all(&log_dir).context("Failed to create log directory")?;

Ok(log_dir)
}

/// Sets up the logging infrastructure for the application.
/// This includes:
/// - File-based logging with JSON formatting (DEBUG level)
Expand Down Expand Up @@ -193,8 +164,6 @@ fn setup_logging_internal(
#[cfg(test)]
mod tests {
use super::*;
use chrono::TimeZone;
use rand;
use std::env;
use tempfile::TempDir;

Expand Down Expand Up @@ -222,236 +191,6 @@ mod tests {
assert!(path_components.iter().any(|c| c.as_os_str() == "cli"));
}

#[tokio::test]
async fn test_log_file_name_session_with_error_capture() {
do_test_log_file_name(Some("test_session_with_error"), true).await;
}

#[tokio::test]
async fn test_log_file_name_session_without_error_capture() {
do_test_log_file_name(Some("test_session_without_error"), false).await;
}

#[tokio::test]
async fn test_log_file_name_no_session() {
do_test_log_file_name(None, false).await;
}

async fn do_test_log_file_name(session_name: Option<&str>, _with_error_capture: bool) {
use tempfile::TempDir;

// Create a unique prefix to avoid test interference
let test_id = format!(
"{}_{}",
session_name.unwrap_or("no_session"),
rand::random::<u32>()
);

// Create a proper temporary directory that will be automatically cleaned up
let _temp_dir = TempDir::with_prefix(format!("goose_test_{}_", test_id)).unwrap();
let test_dir = _temp_dir.path();

// Set up environment
if cfg!(windows) {
env::set_var("USERPROFILE", test_dir);
} else {
env::set_var("HOME", test_dir);
// Also set TMPDIR to prevent temp directory sharing between tests
env::set_var("TMPDIR", test_dir);
}

// Create error capture if needed - but don't use it in tests to avoid tokio runtime issues
let error_capture = None;

// Get current timestamp before setting up logging
let before_timestamp = chrono::Local::now() - chrono::Duration::seconds(1);
println!("Before timestamp: {}", before_timestamp);

// Get the log directory and clean any existing log files
let random_suffix = rand::random::<u32>() % 100000000;
let log_dir = get_log_directory_with_date(Some(format!("test-{}", random_suffix))).unwrap();
println!("Log directory: {}", log_dir.display());
println!("Test directory: {}", test_dir.display());
if log_dir.exists() {
for entry in std::fs::read_dir(&log_dir).unwrap() {
let entry = entry.unwrap();
if entry.path().extension().is_some_and(|ext| ext == "log") {
std::fs::remove_file(entry.path()).unwrap();
}
}
} else {
std::fs::create_dir_all(&log_dir).unwrap();
}
println!("Log directory created: {}", log_dir.exists());

// Set up logging with force=true to bypass the Once check
setup_logging_internal(session_name, error_capture, true).unwrap();

// Write a test log entry
tracing::info!("Test log entry");
println!("Wrote first test log entry");

// Wait longer for the log file to be created and flushed
std::thread::sleep(std::time::Duration::from_millis(500));

// Write another log entry to ensure it's flushed
tracing::warn!("Another test log entry");
println!("Wrote second test log entry");

// Wait again to ensure it's flushed
std::thread::sleep(std::time::Duration::from_millis(500));

// List all files in log directory
println!("Log directory exists: {}", log_dir.exists());

// Check if there are any log files directly
let all_files = std::fs::read_dir(&log_dir)
.unwrap_or_else(|e| {
println!("Error reading log directory: {}", e);
panic!("Failed to read log directory: {}", e);
})
.collect::<Result<Vec<_>, _>>()
.unwrap();

let log_count = all_files
.iter()
.filter(|e| e.path().extension().is_some_and(|ext| ext == "log"))
.count();

println!("Found {} log files in directory", log_count);

if log_count == 0 {
// If no log files found, manually create one for testing
println!("No log files found, manually creating one for testing");
let timestamp = chrono::Local::now().format("%Y%m%d_%H%M%S").to_string();
let log_filename = if let Some(session) = session_name {
format!("{}-{}.log", timestamp, session)
} else {
format!("{}.log", timestamp)
};
let log_path = log_dir.join(&log_filename);
std::fs::write(&log_path, "Test log content").unwrap();
println!("Created test log file: {}", log_path.display());
}

// Read directory again after potential manual creation
let entries = std::fs::read_dir(&log_dir)
.unwrap_or_else(|e| {
println!("Error reading log directory: {}", e);
panic!("Failed to read log directory: {}", e);
})
.collect::<Result<Vec<_>, _>>()
.unwrap();

// List all log files for debugging
println!("All files in log directory ({}):", log_dir.display());
for entry in &entries {
println!(
" {} (is_file: {})",
entry.file_name().to_string_lossy(),
entry.file_type().map(|ft| ft.is_file()).unwrap_or(false)
);
}

// Verify the file exists and has the correct name
let mut log_files: Vec<_> = entries
.iter()
.filter(|e| {
let path = e.path();
let matches = path.extension().is_some_and(|ext| ext == "log")
&& path.file_name().is_some_and(|name| {
let name = name.to_string_lossy();
if let Some(session) = session_name {
name.ends_with(&format!("{}.log", session))
} else {
// For non-session logs, verify it's a timestamp format and it's after our before_timestamp
if name.len() != 19 {
// YYYYMMDD_HHMMSS.log
println!(" Rejecting {} - wrong length", name);
return false;
}
if name.as_bytes()[8] != b'_' {
println!(" Rejecting {} - no underscore at position 8", name);
return false;
}
let timestamp_str = &name[..15]; // Get YYYYMMDD_HHMMSS part
if !timestamp_str
.chars()
.all(|c| c.is_ascii_digit() || c == '_')
{
println!(" Rejecting {} - invalid characters in timestamp", name);
return false;
}
// Parse the timestamp
if let Ok(file_time) = chrono::NaiveDateTime::parse_from_str(
timestamp_str,
"%Y%m%d_%H%M%S",
) {
// Convert to DateTime<Local>
let local_time =
chrono::Local.from_local_datetime(&file_time).unwrap();
println!(
" File time: {} vs before time: {}",
local_time, before_timestamp
);
// Check if file timestamp is after our before_timestamp
local_time >= before_timestamp
} else {
println!(" Rejecting {} - couldn't parse timestamp", name);
false
}
}
});
println!(
" File {} matches: {}",
e.file_name().to_string_lossy(),
matches
);
matches
})
.collect();

log_files.sort_by_key(|a| a.file_name());
assert_eq!(log_files.len(), 1, "Expected exactly one matching log file");

let log_file_name = log_files[0].file_name().to_string_lossy().into_owned();
println!("Found log file name: {}", log_file_name);

if let Some(name) = session_name {
assert!(
log_file_name.ends_with(&format!("{}.log", name)),
"Log file {} should end with {}.log",
log_file_name,
name
);
} else {
// Extract just the filename without extension for comparison
let name_without_ext = log_file_name.trim_end_matches(".log");
// Verify it's a valid timestamp format
assert_eq!(
name_without_ext.len(),
15,
"Expected 15 characters (YYYYMMDD_HHMMSS)"
);
assert!(
name_without_ext[8..9].contains('_'),
"Expected underscore at position 8"
);
assert!(
name_without_ext
.chars()
.all(|c| c.is_ascii_digit() || c == '_'),
"Expected only digits and underscore"
);
}

// Wait a moment to ensure all files are written
std::thread::sleep(std::time::Duration::from_millis(100));

// Keep _temp_dir alive until the end so it doesn't get cleaned up prematurely
drop(_temp_dir);
}

#[tokio::test]
async fn test_langfuse_layer_creation() {
let _temp_dir = setup_temp_home();
Expand Down
Loading