added Test Suite for Windows #36
Conversation
WalkthroughThis PR introduces Windows-specific system testing and refines system detection logic. Changes include: (1) reformatting the keywords array in composer.json across multiple lines, (2) adding a Windows test suite configuration to phpunit.xml, (3) updating CPU architecture detection to match AMD64 (case-insensitive) and refactoring Windows CPU core retrieval with improved parsing and error handling in src/System/System.php, and (4) implementing a new Windows-specific test suite in tests/System/SystemTestWindows.php that validates OS information, resource availability, and expected exceptions for unsupported features on Windows. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/System/SystemTestWindows.php (3)
23-32: LGTM with optional cleanup.The
setUpcorrectly skips tests on non-Windows systems. The emptytearDownmethod can be removed unless you plan to add cleanup logic later.
66-71: Consider using a smaller duration value.The duration of 5 seconds is unnecessary since
getCPUUsagethrows the exception before the sleep call for unsupported OSes. UsinggetCPUUsage(1)or evengetCPUUsage(0)would be clearer and wouldn't affect the test outcome.🔎 Proposed fix
public function testUnsupportedMethods(): void { // CPU Usage is not supported on Windows $this->expectException(\Exception::class); - System::getCPUUsage(5); + System::getCPUUsage(1); }
85-111: Consider usingexpectExceptionfor consistency with other exception tests.The
$this->assertTrue(true)assertions are no-ops. If the goal is to accept any exception/throwable, you can simplify usingexpectException(\Throwable::class)which will match both\Exceptionand\Error.🔎 Proposed fix for testGetIOUsageThrowsException
public function testGetIOUsageThrowsException(): void { - try { - System::getIOUsage(); - $this->fail('Expected exception was not thrown'); - } catch (\Exception $e) { - // Expected - method tries to read /proc/diskstats which doesn't exist on Windows - $this->assertTrue(true); - } catch (\Throwable $e) { - // Also acceptable - might throw warnings/errors - $this->assertTrue(true); - } + $this->expectException(\Throwable::class); + System::getIOUsage(); }🔎 Proposed fix for testGetNetworkUsageThrowsException
public function testGetNetworkUsageThrowsException(): void { - try { - System::getNetworkUsage(); - $this->fail('Expected exception was not thrown'); - } catch (\Exception $e) { - // Expected - method tries to access /sys/class/net which doesn't exist on Windows - $this->assertTrue(true); - } catch (\Throwable $e) { - // Also acceptable - might throw warnings/errors - $this->assertTrue(true); - } + $this->expectException(\Throwable::class); + System::getNetworkUsage(); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
composer.json(2 hunks)phpunit.xml(1 hunks)src/System/System.php(2 hunks)tests/System/SystemTestWindows.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/System/SystemTestWindows.php (1)
src/System/System.php (11)
System(7-618)getOS(77-80)getArch(87-90)getArchEnum(99-110)getHostname(117-120)getCPUCores(129-154)getDiskTotal(350-359)getDiskFree(368-377)getIOUsage(427-485)getNetworkUsage(498-536)getEnv(543-546)
🔇 Additional comments (8)
composer.json (1)
5-11: LGTM!The multi-line formatting for the keywords array improves readability with no functional change.
phpunit.xml (1)
30-32: LGTM!The Windows test suite is correctly configured and follows the existing pattern for other architecture-specific test suites.
src/System/System.php (1)
19-19: LGTM!Adding
AMD64with case-insensitive matching correctly handles Windows x86-64 architecture detection.tests/System/SystemTestWindows.php (5)
34-40: LGTM!Good coverage of OS-related methods with appropriate assertions for Windows.
42-47: LGTM!Properly validates that CPU core detection returns a positive integer on Windows.
49-64: LGTM!Good validation of disk space methods including the invariant check that free space should not exceed total space.
73-83: LGTM!Proper use of
expectExceptionto verify that memory-related methods throw on Windows.
113-123: LGTM!Comprehensive coverage of
getEnvincluding existing variables, default values, and null fallback behavior.
| case 'Windows NT': | ||
| $output = shell_exec('wmic cpu get NumberOfCores'); | ||
| if ($output === null) { | ||
| throw new Exception('Unable to get CPU cores on Windows'); | ||
| } | ||
| // Parse output - wmic returns header line and value(s) | ||
| preg_match_all('/\d+/', $output, $matches); | ||
| return !empty($matches[0]) ? intval($matches[0][0]) : 0; |
There was a problem hiding this comment.
Returning 0 cores is misleading; throw an exception instead.
When preg_match_all finds no numeric matches, returning 0 is incorrect—no system has zero CPU cores. Throwing an exception would be consistent with the null-output case and more reliable for error handling.
Additionally, WMIC is deprecated as of Windows 10 version 21H1 and will be removed from Windows 11 version 25H2. Use PowerShell with WMI interfaces instead via shell_exec() for better long-term compatibility.
🔎 Proposed fix
// Parse output - wmic returns header line and value(s)
preg_match_all('/\d+/', $output, $matches);
- return !empty($matches[0]) ? intval($matches[0][0]) : 0;
+ if (empty($matches[0])) {
+ throw new Exception('Unable to parse CPU cores from wmic output');
+ }
+ return intval($matches[0][0]);1 participant
This PR adds test suite for Windows as described in #35
fixes #35
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.