Focus unit tests on business logic and fix log test state pollution (#53)

Reviewed-on: https://gitea.subcultureofone.org/greg/tkr/pulls/53
Co-authored-by: Greg Sarjeant <greg@subcultureofone.org>
Co-committed-by: Greg Sarjeant <greg@subcultureofone.org>

tests/Controller/AdminController/AdminControllerTest.php

@@ -7,15 +7,9 @@ class AdminControllerTest extends TestCase
     private PDO $mockPdo;
     private ConfigModel $config;
     private UserModel $user;
-    private string $tempLogDir;
 
     protected function setUp(): void
     {
-        // Set up temporary logging
-        $this->tempLogDir = sys_get_temp_dir() . '/tkr_test_logs_' . uniqid();
-        mkdir($this->tempLogDir . '/logs', 0777, true);
-        Log::init($this->tempLogDir . '/logs/tkr.log');
-
         // Create mock PDO
         $this->mockPdo = $this->createMock(PDO::class);
         
@@ -39,29 +33,6 @@ class AdminControllerTest extends TestCase
             'config' => $this->config,
             'user' => $this->user,
         ];
-        
-        // Set log level on config for Log class
-        $this->config->logLevel = 1; // Allow DEBUG level logs
-    }
-
-    protected function tearDown(): void
-    {
-        // Clean up temp directory
-        if (is_dir($this->tempLogDir)) {
-            $this->deleteDirectory($this->tempLogDir);
-        }
-    }
-
-    private function deleteDirectory(string $dir): void
-    {
-        if (!is_dir($dir)) return;
-        
-        $files = array_diff(scandir($dir), ['.', '..']);
-        foreach ($files as $file) {
-            $path = $dir . '/' . $file;
-            is_dir($path) ? $this->deleteDirectory($path) : unlink($path);
-        }
-        rmdir($dir);
     }
 
     public function testGetAdminDataRegularMode(): void
@@ -281,113 +252,4 @@ class AdminControllerTest extends TestCase
         $this->assertContains('Failed to save settings', $result['errors']);
     }
 
-    public function testLoggingOnAdminPageLoad(): void
-    {
-        $controller = new AdminController();
-        $controller->getAdminData(false);
-        
-        // Check that logs were written
-        $logFile = $this->tempLogDir . '/logs/tkr.log';
-        $this->assertFileExists($logFile);
-        
-        $logContent = file_get_contents($logFile);
-        $this->assertStringContainsString('Loading admin page', $logContent);
-    }
-
-    public function testLoggingOnSetupPageLoad(): void
-    {
-        $controller = new AdminController();
-        $controller->getAdminData(true);
-        
-        // Check that logs were written
-        $logFile = $this->tempLogDir . '/logs/tkr.log';
-        $this->assertFileExists($logFile);
-        
-        $logContent = file_get_contents($logFile);
-        $this->assertStringContainsString('Loading admin page (setup mode)', $logContent);
-    }
-
-    public function testLoggingOnValidationErrors(): void
-    {
-        $controller = new AdminController();
-        
-        $postData = [
-            'username' => '',  // Will cause validation error
-            'display_name' => 'Test User',
-            'site_title' => 'Test Site',
-            'base_url' => 'https://example.com',
-            'base_path' => '/tkr',
-            'items_per_page' => 10
-        ];
-        
-        $controller->saveSettings($postData, false);
-        
-        // Check that logs were written
-        $logFile = $this->tempLogDir . '/logs/tkr.log';
-        $this->assertFileExists($logFile);
-        
-        $logContent = file_get_contents($logFile);
-        $this->assertStringContainsString('Settings validation failed', $logContent);
-        $this->assertStringContainsString('Validation error: Username is required', $logContent);
-    }
-
-    public function testLoggingOnSuccessfulSave(): void
-    {
-        // Mock successful database operations
-        $mockStatement = $this->createMock(PDOStatement::class);
-        $mockStatement->method('execute')->willReturn(true);
-        $mockStatement->method('fetchColumn')->willReturn(1);
-        $mockStatement->method('fetch')->willReturnOnConsecutiveCalls(
-            [
-                'site_title' => 'Test Site',
-                'site_description' => 'Test Description',
-                'base_url' => 'https://example.com',
-                'base_path' => '/tkr',
-                'items_per_page' => 10,
-                'css_id' => null,
-                'strict_accessibility' => true,
-                'log_level' => 2
-            ],
-            [
-                'username' => 'testuser',
-                'display_name' => 'Test User',
-                'website' => '',
-                'mood' => ''
-            ]
-        );
-
-        $this->mockPdo->method('prepare')->willReturn($mockStatement);
-        $this->mockPdo->method('query')->willReturn($mockStatement);
-
-        $config = new ConfigModel($this->mockPdo);
-        $user = new UserModel($this->mockPdo);
-        
-        // Update global $app with test models
-        global $app;
-        $app['config'] = $config;
-        $app['user'] = $user;
-        
-        $controller = new AdminController();
-        
-        $postData = [
-            'username' => 'testuser',
-            'display_name' => 'Test User',
-            'site_title' => 'Test Site',
-            'site_description' => 'Test Description',
-            'base_url' => 'https://example.com',
-            'base_path' => '/tkr',
-            'items_per_page' => 10
-        ];
-        
-        $controller->saveSettings($postData, false);
-        
-        // Check that logs were written
-        $logFile = $this->tempLogDir . '/logs/tkr.log';
-        $this->assertFileExists($logFile);
-        
-        $logContent = file_get_contents($logFile);
-        $this->assertStringContainsString('Processing settings for user: testuser', $logContent);
-        $this->assertStringContainsString('Site settings updated', $logContent);
-        $this->assertStringContainsString('User profile updated', $logContent);
-    }
 }

tests/Controller/HomeController/HomeControllerTest.php

@@ -7,14 +7,11 @@ class HomeControllerTest extends TestCase
     private PDOStatement $mockStatement;
     private ConfigModel $mockConfig;
     private UserModel $mockUser;
-    private string $tempLogDir;
 
     protected function setUp(): void
     {
-        // Set up temporary logging
+        // Reset Log state to prevent test pollution
-        $this->tempLogDir = sys_get_temp_dir() . '/tkr_test_logs_' . uniqid();
+        Log::init(sys_get_temp_dir() . '/tkr_controller_test.log');
-        mkdir($this->tempLogDir . '/logs', 0777, true);
-        Log::init($this->tempLogDir . '/logs/tkr.log');
         
         // Create mock PDO and PDOStatement
         $this->mockStatement = $this->createMock(PDOStatement::class);
@@ -37,29 +34,6 @@ class HomeControllerTest extends TestCase
             'config' => $this->mockConfig,
             'user' => $this->mockUser,
         ];
-        
-        // Set log level on config for Log class
-        $this->mockConfig->logLevel = 1; // Allow DEBUG level logs
-    }
-
-    protected function tearDown(): void
-    {
-        // Clean up temp directory
-        if (is_dir($this->tempLogDir)) {
-            $this->deleteDirectory($this->tempLogDir);
-        }
-    }
-
-    private function deleteDirectory(string $dir): void
-    {
-        if (!is_dir($dir)) return;
-        
-        $files = array_diff(scandir($dir), ['.', '..']);
-        foreach ($files as $file) {
-            $path = $dir . '/' . $file;
-            is_dir($path) ? $this->deleteDirectory($path) : unlink($path);
-        }
-        rmdir($dir);
     }
 
     private function setupMockDatabase(array $tickData): void
@@ -246,73 +220,4 @@ class HomeControllerTest extends TestCase
         $this->assertEquals('Failed to save tick', $result['message']);
     }
 
-    public function testLoggingOnHomePageLoad(): void
-    {
-        $testTicks = [
-            ['id' => 1, 'timestamp' => '2025-01-31 12:00:00', 'tick' => 'Test tick']
-        ];
-        $this->setupMockDatabase($testTicks);
-        
-        $controller = new HomeController();
-        $controller->getHomeData(1);
-        
-        // Check that logs were written
-        $logFile = $this->tempLogDir . '/logs/tkr.log';
-        $this->assertFileExists($logFile);
-        
-        $logContent = file_get_contents($logFile);
-        $this->assertStringContainsString('Loading home page 1', $logContent);
-        $this->assertStringContainsString('Home page loaded with 1 ticks', $logContent);
-    }
-
-    public function testLoggingOnTickCreation(): void
-    {
-        $this->setupMockDatabaseForInsert(true);
-        
-        $controller = new HomeController();
-        $postData = ['new_tick' => 'Test tick for logging'];
-        
-        $controller->processTick($postData);
-        
-        // Check that logs were written
-        $logFile = $this->tempLogDir . '/logs/tkr.log';
-        $this->assertFileExists($logFile);
-        
-        $logContent = file_get_contents($logFile);
-        $this->assertStringContainsString('New tick created: Test tick for logging', $logContent);
-    }
-
-    public function testLoggingOnEmptyTick(): void
-    {
-        $controller = new HomeController();
-        $postData = ['new_tick' => ''];
-        
-        $controller->processTick($postData);
-        
-        // Check that logs were written
-        $logFile = $this->tempLogDir . '/logs/tkr.log';
-        
-        // The log file should exist (Log::init creates it) and contain the debug message
-        $this->assertFileExists($logFile);
-        
-        $logContent = file_get_contents($logFile);
-        $this->assertStringContainsString('Empty tick submission ignored', $logContent);
-    }
-
-    public function testLoggingOnDatabaseError(): void
-    {
-        $this->setupMockDatabaseForInsert(false);
-        
-        $controller = new HomeController();
-        $postData = ['new_tick' => 'This will fail'];
-        
-        $controller->processTick($postData);
-        
-        // Check that logs were written
-        $logFile = $this->tempLogDir . '/logs/tkr.log';
-        $this->assertFileExists($logFile);
-        
-        $logContent = file_get_contents($logFile);
-        $this->assertStringContainsString('Failed to save tick: Database error', $logContent);
-    }
 }

tests/Controller/TickController/TickControllerTest.php

@@ -6,18 +6,11 @@ class TickControllerTest extends TestCase
     private $mockPdo;
     private $config;
     private $user;
-    private string $tempLogDir;
-    private string $testLogFile;
 
     protected function setUp(): void
     {
-        // Set up log capture
+        // Reset Log state to prevent test pollution
-        $this->tempLogDir = sys_get_temp_dir() . '/tkr_test_logs_' . uniqid();
+        Log::init(sys_get_temp_dir() . '/tkr_controller_test.log');
-        mkdir($this->tempLogDir, 0777, true);
-        
-        $this->testLogFile = $this->tempLogDir . '/tkr.log';
-        Log::init($this->testLogFile);
-        Log::setRouteContext('GET tick/123');
         
         // Set up mocks
         $this->mockPdo = $this->createMock(PDO::class);
@@ -26,7 +19,6 @@ class TickControllerTest extends TestCase
         $this->config->baseUrl = 'https://example.com';
         $this->config->basePath = '/tkr/';
         $this->config->itemsPerPage = 10;
-        $this->config->logLevel = 1; // DEBUG level for testing
         
         $this->user = new UserModel($this->mockPdo);
 
@@ -39,25 +31,6 @@ class TickControllerTest extends TestCase
         ];
     }
 
-    protected function tearDown(): void
-    {
-        if (is_dir($this->tempLogDir)) {
-            $this->deleteDirectory($this->tempLogDir);
-        }
-    }
-
-    private function deleteDirectory(string $dir): void
-    {
-        if (!is_dir($dir)) return;
-        
-        $files = array_diff(scandir($dir), ['.', '..']);
-        foreach ($files as $file) {
-            $path = $dir . '/' . $file;
-            is_dir($path) ? $this->deleteDirectory($path) : unlink($path);
-        }
-        rmdir($dir);
-    }
-
     public function testIndexWithValidTick(): void
     {
         // Set up mock database response for successful tick retrieval
@@ -98,11 +71,6 @@ class TickControllerTest extends TestCase
         // Should contain the tick content (through the template)
         // Note: We can't easily test the full template rendering without more setup,
         // but we can verify no error occurred
-
-        // Verify logging
-        $logContent = file_get_contents($this->testLogFile);
-        $this->assertStringContainsString('Fetching tick with ID: 123', $logContent);
-        $this->assertStringContainsString('Successfully loaded tick 123: This is a test tick with some content', $logContent);
     }
 
     public function testIndexWithNonexistentTick(): void
@@ -132,11 +100,6 @@ class TickControllerTest extends TestCase
 
         // Should return 404 error
         $this->assertStringContainsString('404 - Tick Not Found', $output);
-
-        // Verify logging
-        $logContent = file_get_contents($this->testLogFile);
-        $this->assertStringContainsString('Fetching tick with ID: 999', $logContent);
-        $this->assertStringContainsString('Tick not found for ID: 999', $logContent);
     }
 
     public function testIndexWithEmptyTickData(): void
@@ -166,10 +129,6 @@ class TickControllerTest extends TestCase
 
         // Should return 404 error for empty data
         $this->assertStringContainsString('404 - Tick Not Found', $output);
-
-        // Verify logging
-        $logContent = file_get_contents($this->testLogFile);
-        $this->assertStringContainsString('Tick not found for ID: 456', $logContent);
     }
 
     public function testIndexWithDatabaseException(): void
@@ -190,84 +149,6 @@ class TickControllerTest extends TestCase
 
         // Should return 500 error
         $this->assertStringContainsString('500 - Internal Server Error', $output);
-
-        // Verify error logging
-        $logContent = file_get_contents($this->testLogFile);
-        $this->assertStringContainsString('Failed to load tick 123: Database connection failed', $logContent);
     }
 
-    public function testIndexWithLongTickContent(): void
-    {
-        // Test logging truncation for long tick content
-        $longContent = str_repeat('This is a very long tick content that should be truncated in the logs. ', 10);
-        
-        $mockStatement = $this->createMock(PDOStatement::class);
-        $mockStatement->expects($this->once())
-                      ->method('execute')
-                      ->with([789]);
-        $mockStatement->expects($this->once())
-                      ->method('fetch')
-                      ->with(PDO::FETCH_ASSOC)
-                      ->willReturn([
-                          'timestamp' => '2025-01-31 15:30:00',
-                          'tick' => $longContent
-                      ]);
-
-        $this->mockPdo->expects($this->once())
-                      ->method('prepare')
-                      ->with('SELECT timestamp, tick FROM tick WHERE id=?')
-                      ->willReturn($mockStatement);
-
-        // Capture output
-        ob_start();
-        
-        $controller = new TickController();
-        $controller->index(789);
-        
-        $output = ob_get_clean();
-
-        // Verify logging shows truncated content with ellipsis
-        $logContent = file_get_contents($this->testLogFile);
-        $this->assertStringContainsString('Successfully loaded tick 789:', $logContent);
-        $this->assertStringContainsString('...', $logContent); // Should be truncated
-        
-        // Verify the log doesn't contain the full long content
-        $this->assertStringNotContainsString($longContent, $logContent);
-    }
-
-    public function testIndexWithShortTickContent(): void
-    {
-        // Test that short content is not truncated in logs
-        $shortContent = 'Short tick';
-        
-        $mockStatement = $this->createMock(PDOStatement::class);
-        $mockStatement->expects($this->once())
-                      ->method('execute')
-                      ->with([100]);
-        $mockStatement->expects($this->once())
-                      ->method('fetch')
-                      ->with(PDO::FETCH_ASSOC)
-                      ->willReturn([
-                          'timestamp' => '2025-01-31 09:15:00',
-                          'tick' => $shortContent
-                      ]);
-
-        $this->mockPdo->expects($this->once())
-                      ->method('prepare')
-                      ->with('SELECT timestamp, tick FROM tick WHERE id=?')
-                      ->willReturn($mockStatement);
-
-        // Capture output
-        ob_start();
-        
-        $controller = new TickController();
-        $controller->index(100);
-        
-        $output = ob_get_clean();
-
-        // Verify logging shows full content without ellipsis
-        $logContent = file_get_contents($this->testLogFile);
-        $this->assertStringContainsString('Successfully loaded tick 100: Short tick', $logContent);
-        $this->assertStringNotContainsString('...', $logContent); // Should NOT be truncated
-    }
 }

tests/Framework/Log/LogTest.php

@@ -31,28 +31,44 @@ class LogTest extends TestCase
     {
         if (!is_dir($dir)) return;
         
-        $files = array_diff(scandir($dir), ['.', '..']);
+        $iterator = new RecursiveIteratorIterator(
-        foreach ($files as $file) {
+            new RecursiveDirectoryIterator($dir, RecursiveDirectoryIterator::SKIP_DOTS),
-            $path = $dir . '/' . $file;
+            RecursiveIteratorIterator::CHILD_FIRST
-            is_dir($path) ? $this->deleteDirectory($path) : unlink($path);
+        );
+        
+        foreach ($iterator as $path) {
+            $path->isDir() ? rmdir($path->getRealPath()) : unlink($path->getRealPath());
         }
         rmdir($dir);
     }
     
+    private function setLogLevel(int $level): void
+    {
+        global $app;
+        $app = ['config' => (object)['logLevel' => $level]];
+    }
+    
+    private function assertLogContains(string $message): void
+    {
+        $this->assertFileExists($this->testLogFile);
+        $logContent = file_get_contents($this->testLogFile);
+        $this->assertStringContainsString($message, $logContent);
+    }
+    
+    private function assertLogDoesNotContain(string $message): void
+    {
+        $this->assertFileExists($this->testLogFile);
+        $logContent = file_get_contents($this->testLogFile);
+        $this->assertStringNotContainsString($message, $logContent);
+    }
+
     public function testSetRouteContext(): void
     {
         Log::setRouteContext('GET /admin');
-        
+        $this->setLogLevel(1); // DEBUG level
-        // Create a mock app config for log level
-        global $app;
-        $app = [
-            'config' => (object)['logLevel' => 1] // DEBUG level
-        ];
         
         Log::debug('Test message');
         
-        $this->assertFileExists($this->testLogFile);
-        
         $logContent = file_get_contents($this->testLogFile);
         $this->assertStringContainsString('[GET /admin]', $logContent);
         $this->assertStringContainsString('Test message', $logContent);
@@ -61,11 +77,7 @@ class LogTest extends TestCase
     public function testEmptyRouteContext(): void
     {
         Log::setRouteContext('');
-        
+        $this->setLogLevel(1);
-        global $app;
-        $app = [
-            'config' => (object)['logLevel' => 1]
-        ];
         
         Log::info('Test without route');
         
@@ -80,32 +92,23 @@ class LogTest extends TestCase
 
     public function testLogLevelFiltering(): void
     {
-        global $app;
+        $this->setLogLevel(3); // WARNING level
-        $app = [
-            'config' => (object)['logLevel' => 3] // WARNING level
-        ];
         
         Log::debug('Debug message');   // Should be filtered out
         Log::info('Info message');     // Should be filtered out  
         Log::warning('Warning message'); // Should be logged
         Log::error('Error message');   // Should be logged
         
-        $logContent = file_get_contents($this->testLogFile);
+        $this->assertLogDoesNotContain('Debug message');
-        
+        $this->assertLogDoesNotContain('Info message');
-        $this->assertStringNotContainsString('Debug message', $logContent);
+        $this->assertLogContains('Warning message');
-        $this->assertStringNotContainsString('Info message', $logContent);
+        $this->assertLogContains('Error message');
-        $this->assertStringContainsString('Warning message', $logContent);
-        $this->assertStringContainsString('Error message', $logContent);
     }
 
     public function testLogMessageFormat(): void
     {
         Log::setRouteContext('POST /admin');
-        
+        $this->setLogLevel(1);
-        global $app;
-        $app = [
-            'config' => (object)['logLevel' => 1]
-        ];
         
         Log::error('Test error message');
         
@@ -129,14 +132,16 @@ class LogTest extends TestCase
         
         // init() should create the directory
         $this->assertDirectoryExists(dirname($newLogFile));
+        
+        // Verify we can actually write to it
+        $this->setLogLevel(1);
+        Log::info('Test directory creation');
+        $this->assertFileExists($newLogFile);
     }
 
     public function testLogRotation(): void
     {
-        global $app;
+        $this->setLogLevel(1);
-        $app = [
-            'config' => (object)['logLevel' => 1]
-        ];
         
         // Create a log file with exactly 1000 lines (the rotation threshold)
         $logLines = str_repeat("[2025-01-31 12:00:00] INFO: 127.0.0.1 - Test line\n", 1000);
@@ -145,32 +150,49 @@ class LogTest extends TestCase
         // This should trigger rotation
         Log::info('This should trigger rotation');
         
-        // Original log should be rotated to .1
+        // Verify rotation happened
         $this->assertFileExists($this->testLogFile . '.1');
+        $this->assertLogContains('This should trigger rotation');
+    }
     
-        // New log should contain the new message
+    public function testLogRotationLimitsFileCount(): void
-        $newLogContent = file_get_contents($this->testLogFile);
+    {
-        $this->assertStringContainsString('This should trigger rotation', $newLogContent);
+        $this->setLogLevel(1);
         
-        // Rotated log should contain old content
+        // Create 5 existing rotated log files (.1 through .5)
-        $rotatedContent = file_get_contents($this->testLogFile . '.1');
+        for ($i = 1; $i <= 5; $i++) {
-        $this->assertStringContainsString('Test line', $rotatedContent);
+            file_put_contents($this->testLogFile . '.' . $i, "Old log file $i\n");
+        }
+        
+        // Create main log file at rotation threshold
+        $logLines = str_repeat("[2025-01-31 12:00:00] INFO: 127.0.0.1 - Test line\n", 1000);
+        file_put_contents($this->testLogFile, $logLines);
+        
+        // This should trigger rotation and delete the oldest file (.5)
+        Log::info('Trigger rotation with max files');
+        
+        // Verify rotation happened and file count is limited
+        $this->assertFileExists($this->testLogFile . '.1'); // New rotated file
+        $this->assertFileExists($this->testLogFile . '.2'); // Old .1 became .2
+        $this->assertFileExists($this->testLogFile . '.3'); // Old .2 became .3
+        $this->assertFileExists($this->testLogFile . '.4'); // Old .3 became .4
+        $this->assertFileExists($this->testLogFile . '.5'); // Old .4 became .5
+        $this->assertFileDoesNotExist($this->testLogFile . '.6'); // Old .5 was deleted
+        
+        $this->assertLogContains('Trigger rotation with max files');
     }
 
     public function testDefaultLogLevelWhenConfigMissing(): void
     {
         // Set up config without logLevel property (simulates missing config value)
         global $app;
-        $app = [
+        $app = ['config' => (object)[]];
-            'config' => (object)[] // Empty config object, no logLevel property
-        ];
         
         // Should not throw errors and should default to INFO level
         Log::debug('Debug message');  // Should be filtered out (default INFO level = 2)
         Log::info('Info message');    // Should be logged
         
-        $logContent = file_get_contents($this->testLogFile);
+        $this->assertLogDoesNotContain('Debug message');
-        $this->assertStringNotContainsString('Debug message', $logContent);
+        $this->assertLogContains('Info message');
-        $this->assertStringContainsString('Info message', $logContent);
     }
 }