From 852df37ffc3761d57bf80bca35443cb7c77dbdfb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jes=C3=BAs=20P=C3=A9rex?= <jpl@jesusperez.com>
Date: Tue, 27 May 2025 00:59:44 +0100
Subject: [PATCH] refactor(tests): improve test logging setup and organization

- Create unit test and integration tests for logs in separate tests
- Centralize temporary log file creation in common.rs
- Remove redundant logging initialization code from common.rs
- Update all test files to handle their own logging initialization
- Add detailed debug logging throughout test execution
- Add log file content display for better test debugging
- Standardize error handling across test files

Changes:
- Create setup_test_log_file() helper in common.rs
- Remove setup_test_logging() and setup_temp_log() functions
- Update test files to use common setup_test_log_file():
  - test_log_detail_macro.rs
  - test_log_timed_macro.rs
  - test_different_log_levels.rs
  - test_init_logging_with_file.rs
  - test_init_logging_append_mode.rs

This change improves test maintainability by:
1. Reducing code duplication
2. Making test behavior more explicit
3. Improving test output for debugging
4. Following single responsibility principle
---
 src/tests.rs                           |  4 +-
 src/tests/directory_processor_test.rs  | 16 ++---
 tests/common.rs                        | 12 ++++
 tests/logging_writer_tests.rs          | 50 ++++++++++++++++
 tests/test_different_log_levels.rs     | 46 +++++++++++++++
 tests/test_init_logging_append_mode.rs | 68 +++++++++++++++++++++
 tests/test_init_logging_with_file.rs   | 37 ++++++++++++
 tests/test_log_detail_macro.rs         | 39 +++++++++++++
 tests/test_log_level_changes.rs        | 81 ++++++++++++++++++++++++++
 tests/test_log_timed_macro.rs          | 42 +++++++++++++
 tests/test_simple_logging.rs           | 37 ++++++++++++
 11 files changed, 422 insertions(+), 10 deletions(-)
 create mode 100644 tests/common.rs
 create mode 100644 tests/logging_writer_tests.rs
 create mode 100644 tests/test_different_log_levels.rs
 create mode 100644 tests/test_init_logging_append_mode.rs
 create mode 100644 tests/test_init_logging_with_file.rs
 create mode 100644 tests/test_log_detail_macro.rs
 create mode 100644 tests/test_log_level_changes.rs
 create mode 100644 tests/test_log_timed_macro.rs
 create mode 100644 tests/test_simple_logging.rs

diff --git a/src/tests.rs b/src/tests.rs
index afb7692..83ae4c6 100644
--- a/src/tests.rs
+++ b/src/tests.rs
@@ -1,3 +1,3 @@
 mod directory_processor_test;
-
-// Add other test modules here as needed 
\ No newline at end of file
+//mod logging_tests;
+// Add other test modules here as needed
diff --git a/src/tests/directory_processor_test.rs b/src/tests/directory_processor_test.rs
index c3edf64..2168a07 100644
--- a/src/tests/directory_processor_test.rs
+++ b/src/tests/directory_processor_test.rs
@@ -1,23 +1,23 @@
-use std::path::PathBuf;
+use dir_odt_to_pdf::directory_processor::DirectoryProcessor;
 use std::fs;
+use std::path::PathBuf;
 use tempfile::TempDir;
-use crate::directory_processor::DirectoryProcessor;
 
 #[test]
-fn test_needs_copy_or_conversion() {
+fn test_needs_processing() {
     let temp_dir = TempDir::new().unwrap();
     let source = temp_dir.path().join("source.txt");
     let dest = temp_dir.path().join("dest.txt");
 
     // Test non-existent destination
-    assert!(DirectoryProcessor::needs_copy_or_conversion(&source, &dest));
+    assert!(DirectoryProcessor::needs_processing(&source, &dest));
 
     // Create files and test modification times
     fs::write(&source, "test").unwrap();
     fs::write(&dest, "test").unwrap();
-    
+
     // Files created very close together should not need copying
-    assert!(!DirectoryProcessor::needs_copy_or_conversion(&source, &dest));
+    assert!(!DirectoryProcessor::needs_processing(&source, &dest));
 }
 
 #[test]
@@ -25,8 +25,8 @@ fn test_new_processor() {
     let source = PathBuf::from("/source");
     let target = PathBuf::from("/target");
     let processor = DirectoryProcessor::new(source.clone(), target.clone());
-    
+
     assert_eq!(processor.source_dir, source);
     assert_eq!(processor.target_dir, target);
     assert!(processor.source_files.is_empty());
-} 
\ No newline at end of file
+}
diff --git a/tests/common.rs b/tests/common.rs
new file mode 100644
index 0000000..3dab1c8
--- /dev/null
+++ b/tests/common.rs
@@ -0,0 +1,12 @@
+use std::path::PathBuf;
+use tempfile::TempDir;
+
+/// Sets up a temporary directory and log file for testing
+/// Returns:
+/// - TempDir: The temporary directory (keep this in scope to prevent cleanup)
+/// - PathBuf: Path to the log file
+pub fn setup_test_log_file() -> (TempDir, PathBuf) {
+    let temp_dir = tempfile::TempDir::new().expect("Failed to create temporary directory for test");
+    let log_path = temp_dir.path().join("test.log");
+    (temp_dir, log_path)
+}
diff --git a/tests/logging_writer_tests.rs b/tests/logging_writer_tests.rs
new file mode 100644
index 0000000..9f885c5
--- /dev/null
+++ b/tests/logging_writer_tests.rs
@@ -0,0 +1,50 @@
+use dir_odt_to_pdf::error::{ProcessError, Result};
+use dir_odt_to_pdf::logging::{LogConfig, LogWriter};
+use log::LevelFilter;
+use std::fs;
+use std::io;
+use tempfile::NamedTempFile;
+
+#[test]
+fn test_log_writer_file_output() -> io::Result<()> {
+    let temp_file = NamedTempFile::new()?;
+    let mut writer = LogWriter::new(Some(temp_file.reopen()?));
+    writer.write_all(b"test message\n")?;
+    writer.flush()?;
+    let content = fs::read_to_string(temp_file.path())?;
+    assert_eq!(content, "test message\n");
+    Ok(())
+}
+
+#[test]
+fn test_log_writer_console_output() {
+    let mut writer = LogWriter::new(None);
+    assert!(writer.write_all(b"test message\n").is_ok());
+    assert!(writer.flush().is_ok());
+}
+
+#[test]
+fn test_log_config_default() {
+    let config = LogConfig::default();
+    assert_eq!(config.log_level, LevelFilter::Info);
+    assert_eq!(config.log_file, None);
+    assert!(!config.append_log);
+}
+
+#[test]
+fn test_log_writer() -> Result<()> {
+    let mut writer = LogWriter::new(None);
+    writer
+        .write_all(b"test message\n")
+        .map_err(ProcessError::Io)?;
+    writer.flush().map_err(ProcessError::Io)?;
+    Ok(())
+}
+
+#[test]
+fn test_log_writer_methods() -> Result<()> {
+    let mut writer = LogWriter::new(None);
+    assert!(writer.write_all(b"test message\n").is_ok());
+    assert!(writer.flush().is_ok());
+    Ok(())
+}
diff --git a/tests/test_different_log_levels.rs b/tests/test_different_log_levels.rs
new file mode 100644
index 0000000..f776245
--- /dev/null
+++ b/tests/test_different_log_levels.rs
@@ -0,0 +1,46 @@
+use dir_odt_to_pdf::error::{ProcessError, Result};
+use dir_odt_to_pdf::logging::{LogConfig, init_logging};
+use log::{LevelFilter, debug, error, info, trace, warn};
+use std::fs;
+mod common;
+
+#[test]
+fn test_different_log_levels() -> Result<()> {
+    // Create a temporary file for logging
+    let (_temp_dir, log_path) = common::setup_test_log_file();
+
+    // Initialize logging with Trace level
+    let config = LogConfig {
+        log_file: Some(log_path.clone()),
+        log_level: LevelFilter::Trace,
+        append_log: false,
+    };
+
+    // Initialize logging
+    init_logging(config)?;
+    debug!("Starting test_different_log_levels test");
+
+    // Test all log levels
+    info!("Testing different log levels...");
+    trace!("trace message");
+    debug!("debug message");
+    info!("info message");
+    warn!("warn message");
+    error!("error message");
+
+    // Read and verify the log file
+    let content = fs::read_to_string(&log_path).map_err(ProcessError::Io)?;
+    println!("=== Log File Contents ===");
+    println!("{}", content);
+    println!("=======================");
+
+    // Verify all messages are present
+    assert!(content.contains("trace message"));
+    assert!(content.contains("debug message"));
+    assert!(content.contains("info message"));
+    assert!(content.contains("warn message"));
+    assert!(content.contains("error message"));
+    debug!("Test completed successfully");
+
+    Ok(())
+}
diff --git a/tests/test_init_logging_append_mode.rs b/tests/test_init_logging_append_mode.rs
new file mode 100644
index 0000000..fc8aeb9
--- /dev/null
+++ b/tests/test_init_logging_append_mode.rs
@@ -0,0 +1,68 @@
+use dir_odt_to_pdf::error::{LogError, ProcessError, Result};
+use dir_odt_to_pdf::logging::{LogConfig, init_logging};
+use log::{LevelFilter, debug, info};
+use std::fs;
+mod common;
+
+#[test]
+fn test_init_logging_append_mode() -> Result<()> {
+    // Create a temporary file for logging
+    let (_temp_dir, log_path) = common::setup_test_log_file();
+    debug!("Starting test_init_logging_append_mode test");
+
+    // First initialization
+    let config = LogConfig {
+        log_file: Some(log_path.clone()),
+        log_level: LevelFilter::Debug,
+        append_log: false,
+    };
+
+    debug!("About to initialize logging for the first time...");
+    init_logging(config)?;
+
+    // Write first message
+    info!("Testing append mode...");
+    info!("first message");
+
+    // Read first message
+    let content = fs::read_to_string(&log_path).map_err(ProcessError::Io)?;
+    println!("=== Initial Log Contents ===");
+    println!("{}", content);
+    println!("=======================");
+    assert!(content.contains("first message"));
+
+    // Try second initialization with append mode
+    debug!("Attempting second initialization with append mode...");
+    let config = LogConfig {
+        log_file: Some(log_path.clone()),
+        log_level: LevelFilter::Debug,
+        append_log: true,
+    };
+
+    // Try to initialize again - this should fail
+    match init_logging(config) {
+        Err(ProcessError::Log(LogError::AlreadyInitialized)) => {
+            debug!("Got expected AlreadyInitialized error");
+            info!("second message");
+
+            // Check final contents
+            let content = fs::read_to_string(&log_path).map_err(ProcessError::Io)?;
+            println!("\n=== Final Log Contents ===");
+            println!("{}", content);
+            println!("=======================");
+
+            assert!(content.contains("first message"));
+            assert!(content.contains("second message"));
+            debug!("Test completed successfully");
+            Ok(())
+        }
+        Ok(_) => {
+            info!("Unexpected success on second initialization");
+            panic!("Expected AlreadyInitialized error")
+        }
+        Err(e) => {
+            info!("Unexpected error: {:?}", e);
+            Err(e)
+        }
+    }
+}
diff --git a/tests/test_init_logging_with_file.rs b/tests/test_init_logging_with_file.rs
new file mode 100644
index 0000000..6575cb0
--- /dev/null
+++ b/tests/test_init_logging_with_file.rs
@@ -0,0 +1,37 @@
+use dir_odt_to_pdf::error::{ProcessError, Result};
+use dir_odt_to_pdf::logging::{LogConfig, init_logging};
+use log::{LevelFilter, debug, info};
+use std::fs;
+mod common;
+
+#[test]
+fn test_init_logging_with_file() -> Result<()> {
+    // Create a temporary file for logging
+    let (_temp_dir, log_path) = common::setup_test_log_file();
+
+    // Initialize logging with Debug level
+    let config = LogConfig {
+        log_file: Some(log_path.clone()),
+        log_level: LevelFilter::Debug,
+        append_log: false,
+    };
+
+    // Initialize logging
+    init_logging(config)?;
+    debug!("Starting test_init_logging_with_file test");
+
+    // Test basic logging
+    info!("Testing basic file logging...");
+    info!("test message");
+
+    // Read and verify the log file
+    let content = fs::read_to_string(&log_path).map_err(ProcessError::Io)?;
+    println!("=== Log File Contents ===");
+    println!("{}", content);
+    println!("=======================");
+
+    assert!(content.contains("test message"));
+    debug!("Test completed successfully");
+
+    Ok(())
+}
diff --git a/tests/test_log_detail_macro.rs b/tests/test_log_detail_macro.rs
new file mode 100644
index 0000000..aa2d6ff
--- /dev/null
+++ b/tests/test_log_detail_macro.rs
@@ -0,0 +1,39 @@
+use dir_odt_to_pdf::error::{ProcessError, Result};
+use dir_odt_to_pdf::log_detail;
+use dir_odt_to_pdf::logging::{LogConfig, init_logging};
+use log::{Level, LevelFilter, debug, info};
+use std::fs;
+mod common;
+
+#[test]
+fn test_log_detail_macro() -> Result<()> {
+    // Create a temporary file for logging
+    let (_temp_dir, log_path) = common::setup_test_log_file();
+
+    // Initialize logging with Debug level
+    let config = LogConfig {
+        log_file: Some(log_path.clone()),
+        log_level: LevelFilter::Debug,
+        append_log: false,
+    };
+
+    // Initialize logging
+    init_logging(config)?;
+    debug!("Starting test_log_detail_macro test");
+
+    // Test the macro
+    info!("Testing log_detail macro...");
+    log_detail!(Level::Info, "test detail message");
+
+    // Read and verify the log file
+    let content = fs::read_to_string(&log_path).map_err(ProcessError::Io)?;
+    println!("=== Log File Contents ===");
+    println!("{}", content);
+    println!("=======================");
+
+    assert!(content.contains("test detail message"));
+    assert!(content.contains(file!()));
+    debug!("Test completed successfully");
+
+    Ok(())
+}
diff --git a/tests/test_log_level_changes.rs b/tests/test_log_level_changes.rs
new file mode 100644
index 0000000..6abb6f0
--- /dev/null
+++ b/tests/test_log_level_changes.rs
@@ -0,0 +1,81 @@
+use dir_odt_to_pdf::error::{LogError, ProcessError, Result};
+use dir_odt_to_pdf::logging::{LogConfig, init_logging};
+use log::{LevelFilter, debug, info};
+use std::fs;
+
+#[test]
+fn test_log_level_changes() -> Result<()> {
+    // Create a temporary file for logging
+    let temp_dir = tempfile::TempDir::new().unwrap();
+    let log_path = temp_dir.path().join("test.log");
+
+    // Initialize logging with Info level first
+    let config = LogConfig {
+        log_file: Some(log_path.clone()),
+        log_level: LevelFilter::Info,
+        append_log: false,
+    };
+
+    // Initialize logging
+    init_logging(config)?;
+
+    // Write some messages at different levels
+    debug!("First debug message"); // Should not appear (we're at Info level)
+    info!("First info message"); // Should appear
+
+    // Read the log file
+    let content = fs::read_to_string(&log_path).map_err(ProcessError::Io)?;
+    println!("=== Initial Log Contents ===");
+    println!("{}", content);
+    println!("=======================");
+
+    // Verify initial messages
+    assert!(
+        !content.contains("First debug message"),
+        "Debug message should not appear at Info level"
+    );
+    assert!(
+        content.contains("First info message"),
+        "Info message should appear"
+    );
+
+    // Try to change log level - this should fail with AlreadyInitialized
+    let config = LogConfig {
+        log_file: Some(log_path.clone()),
+        log_level: LevelFilter::Debug,
+        append_log: true,
+    };
+
+    match init_logging(config) {
+        Err(ProcessError::Log(LogError::AlreadyInitialized)) => {
+            // Expected error
+            info!("Got expected AlreadyInitialized error");
+
+            // Write more messages
+            debug!("Second debug message"); // Still won't appear (still at Info level)
+            info!("Second info message"); // Will appear
+
+            // Check final log contents
+            let content = fs::read_to_string(&log_path).map_err(ProcessError::Io)?;
+            println!("\n=== Final Log Contents ===");
+            println!("{}", content);
+            println!("=======================");
+
+            // Verify final state
+            assert!(
+                !content.contains("Second debug message"),
+                "Debug messages should still not appear"
+            );
+            assert!(
+                content.contains("Second info message"),
+                "Info messages should still appear"
+            );
+
+            Ok(())
+        }
+        Ok(_) => {
+            panic!("Expected AlreadyInitialized error, but logger was reinitialized")
+        }
+        Err(e) => Err(e),
+    }
+}
diff --git a/tests/test_log_timed_macro.rs b/tests/test_log_timed_macro.rs
new file mode 100644
index 0000000..d42e697
--- /dev/null
+++ b/tests/test_log_timed_macro.rs
@@ -0,0 +1,42 @@
+use dir_odt_to_pdf::error::{ProcessError, Result};
+use dir_odt_to_pdf::log_timed;
+use dir_odt_to_pdf::logging::{LogConfig, init_logging};
+use log::{Level, LevelFilter, debug, info};
+use std::fs;
+mod common;
+
+#[test]
+fn test_log_timed_macro() -> Result<()> {
+    // Create a temporary file for logging
+    let (_temp_dir, log_path) = common::setup_test_log_file();
+
+    // Initialize logging with Debug level
+    let config = LogConfig {
+        log_file: Some(log_path.clone()),
+        log_level: LevelFilter::Debug,
+        append_log: false,
+    };
+
+    // Initialize logging
+    init_logging(config)?;
+    debug!("Starting test_log_timed_macro test");
+
+    // Test the macro
+    info!("Testing log_timed macro...");
+    log_timed!(Level::Info, "test operation", {
+        std::thread::sleep(std::time::Duration::from_millis(10));
+    });
+
+    // Read and verify the log file
+    let content = fs::read_to_string(&log_path).map_err(ProcessError::Io)?;
+    println!("=== Log File Contents ===");
+    println!("{}", content);
+    println!("=======================");
+
+    assert!(content.contains("test operation"));
+    assert!(content.contains("completed in"));
+    assert!(content.contains("ms"));
+    debug!("Test completed successfully");
+
+    Ok(())
+}
diff --git a/tests/test_simple_logging.rs b/tests/test_simple_logging.rs
new file mode 100644
index 0000000..816ac43
--- /dev/null
+++ b/tests/test_simple_logging.rs
@@ -0,0 +1,37 @@
+use dir_odt_to_pdf::error::Result;
+use dir_odt_to_pdf::logging::{LogConfig, init_logging};
+use log::{LevelFilter, debug, info};
+use std::fs;
+
+#[test]
+fn test_simple_logging() -> Result<()> {
+    // Create a temporary file for logging
+    let temp_dir = tempfile::TempDir::new().unwrap();
+    let log_path = temp_dir.path().join("test.log");
+
+    // Initialize logging with Debug level
+    let config = LogConfig {
+        log_file: Some(log_path.clone()),
+        log_level: LevelFilter::Debug,
+        append_log: false,
+    };
+
+    // Initialize logging
+    init_logging(config)?;
+
+    // Write some log messages
+    debug!("This is a debug message");
+    info!("This is an info message");
+
+    // Read the log file
+    let content = fs::read_to_string(&log_path).unwrap();
+    println!("=== Log File Contents ===");
+    println!("{}", content);
+    println!("=======================");
+
+    // Verify log messages
+    assert!(content.contains("This is a debug message"));
+    assert!(content.contains("This is an info message"));
+
+    Ok(())
+}