CodeRabbit Generated Unit Tests: Add JsonConfigTest to validate .mcp-config.json structure #5
Conversation
…config.json structure
WalkthroughAdds a new PHPUnit test class JsonConfigTest that validates a MCP Server JSON configuration. It loads a sample config, checks JSON validity, structure, schema, content, path styles, encoding/decoding behavior, and specific expectations for a laravel-boost server, including command, args, and allowed keys. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks✅ Passed checks (3 passed)
🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/Unit/JsonConfigTest.php (4)
46-53: Strengthen JSON validity test.Encoding a PHP array with
json_encode()will always produce valid JSON (assuming no encoding errors). This test doesn't validate JSON parsing from an external source.If you load from the actual file (per previous comment), this test becomes more valuable. Alternatively, consider testing with a malformed JSON string to ensure error handling works:
public function testConfigIsValidJson(): void { $jsonString = json_encode($this->config); $decoded = json_decode($jsonString, true); $this->assertNotNull($decoded, 'Configuration should be valid JSON'); $this->assertEquals(JSON_ERROR_NONE, json_last_error(), 'JSON should have no syntax errors'); + + // Test that malformed JSON is detected + $malformedJson = '{"mcpServers": {invalid json}}'; + $invalid = json_decode($malformedJson, true); + $this->assertNull($invalid, 'Malformed JSON should return null'); + $this->assertNotEquals(JSON_ERROR_NONE, json_last_error(), 'Malformed JSON should set error'); }
144-149: PHP executable check is overly permissive.The substring match
'php'would match unintended strings like 'elephant' or 'phosphorus'. Consider a more precise pattern.public function testLaravelBoostCommandIsPhpExecutable(): void { $laravelBoost = $this->config['mcpServers']['laravel-boost']; - $this->assertStringContainsString('php', $laravelBoost['command'], 'Command should point to PHP executable'); + $this->assertMatchesRegularExpression( + '/\bphp\d*(\.\d+)?$/i', + $laravelBoost['command'], + 'Command should point to PHP executable (e.g., php, php8, php8.4)' + ); }
321-342: Consider usingstr_contains()for better readability (PHP 8+).The
strpos() !== falsepattern works butstr_contains()is more readable in PHP 8+. If this project uses PHP 8+, consider refactoring.public function testCommandPathsUseUnixStyle(): void { foreach ($this->config['mcpServers'] as $serverName => $serverConfig) { - if (strpos($serverConfig['command'], '\\') !== false) { + if (str_contains($serverConfig['command'], '\\')) { $this->fail("Server '$serverName' command uses Windows-style backslashes, should use forward slashes"); } } }public function testArtisanPathsUseUnixStyle(): void { foreach ($this->config['mcpServers'] as $serverName => $serverConfig) { foreach ($serverConfig['args'] as $arg) { - if (strpos($arg, 'artisan') !== false && strpos($arg, '\\') !== false) { + if (str_contains($arg, 'artisan') && str_contains($arg, '\\')) { $this->fail("Server '$serverName' artisan path uses Windows-style backslashes"); } } } }
365-372: Redundant assertion: arrays cannot be instances of stdClass.Line 371 checks that an array is not an instance of
stdClass. This is redundant since line 370 already asserts it's an array, and arrays can never be instances of any class.Remove the redundant assertion:
public function testJsonDecodesAsAssociativeArray(): void { $jsonString = json_encode($this->config); $decoded = json_decode($jsonString, true); $this->assertIsArray($decoded, 'Decoded JSON should be an associative array'); - $this->assertNotInstanceOf(\stdClass::class, $decoded, 'Should not decode as stdClass object'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
tests/Unit/JsonConfigTest.php(1 hunks)
🔇 Additional comments (2)
tests/Unit/JsonConfigTest.php (2)
217-226: Keep lowercase-only regex for server namesThe regex
/^[a-z0-9\-]+$/correctly enforces kebab-case (lowercase letters, digits, hyphens) as required by MCP server naming conventions—uppercase letters should remain disallowed.Likely an incorrect or invalid review comment.
136-189: Remove invalid.mcp-config.jsonverification; tests use inline sample config
These assertions operate on the hard-coded$this->configarray defined insetUp()—they never load or validate an external JSON file—so concerns about the real.mcp-config.jsoncontents do not apply.Likely an incorrect or invalid review comment.
| protected function setUp(): void | ||
| { | ||
| parent::setUp(); | ||
|
|
||
| // Locate the JSON config file - adjust path as needed | ||
| $this->configPath = base_path('.mcp-config.json'); | ||
|
|
||
| // For testing purposes, use the sample config if file doesn't exist | ||
| $this->config = [ | ||
| 'mcpServers' => [ | ||
| 'laravel-boost' => [ | ||
| 'command' => '/usr/bin/php8.4', | ||
| 'args' => [ | ||
| '/data/Projects/ivplv2/artisan', | ||
| 'mcp:start', | ||
| 'laravel-boost' | ||
| ] | ||
| ] | ||
| ] | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Tests use hardcoded config and never load the actual .mcp-config.json file.
The comment on line 28 suggests conditional logic ("use the sample config if file doesn't exist"), but the implementation always uses the hardcoded config. The $configPath property (line 26) is set but never used.
This means the tests validate a sample structure but not the actual configuration file in the repository. For unit tests, this is acceptable if the goal is to validate the test logic itself. However, if you want to validate the real .mcp-config.json file, you should add logic to load it.
Consider whether you want to:
- Keep as-is for pure unit testing (validates sample structure only)
- Load and validate the actual file (integration test approach)
If option 2, apply this diff:
protected function setUp(): void
{
parent::setUp();
// Locate the JSON config file - adjust path as needed
$this->configPath = base_path('.mcp-config.json');
- // For testing purposes, use the sample config if file doesn't exist
- $this->config = [
- 'mcpServers' => [
- 'laravel-boost' => [
- 'command' => '/usr/bin/php8.4',
- 'args' => [
- '/data/Projects/ivplv2/artisan',
- 'mcp:start',
- 'laravel-boost'
- ]
- ]
- ]
- ];
+ // Load actual config or use sample if file doesn't exist
+ if (file_exists($this->configPath)) {
+ $jsonContent = file_get_contents($this->configPath);
+ $this->config = json_decode($jsonContent, true);
+ $this->assertNotNull($this->config, 'Failed to parse .mcp-config.json');
+ } else {
+ $this->config = [
+ 'mcpServers' => [
+ 'laravel-boost' => [
+ 'command' => '/usr/bin/php8.4',
+ 'args' => [
+ '/data/Projects/ivplv2/artisan',
+ 'mcp:start',
+ 'laravel-boost'
+ ]
+ ]
+ ]
+ ];
+ }
}Alternatively, verify whether .mcp-config.json exists in the repository:
🏁 Script executed:
#!/bin/bash
# Check if .mcp-config.json exists and show its content
if [ -f ".mcp-config.json" ]; then
echo "=== .mcp-config.json exists ==="
cat .mcp-config.json
else
echo "=== .mcp-config.json does not exist ==="
fiLength of output: 39
Clarify or implement config loading in setUp
In tests/Unit/JsonConfigTest.php setUp() (lines 21–41), you assign $this->configPath but never use it and always load a hardcoded sample config. Either remove the unused property and update the comment to reflect that tests only cover the sample, or add an if (file_exists(...)) block to load and parse .mcp-config.json as described in the comment.
🤖 Prompt for AI Agents
In tests/Unit/JsonConfigTest.php around lines 21 to 41, setUp() assigns
$this->configPath but never uses it and always uses a hardcoded sample config;
either remove $this->configPath and adjust the comment to state the test uses a
sample config only, or implement loading logic: check if
file_exists($this->configPath), read the file, json_decode it into $this->config
with error checking and fall back to the sample config if parsing fails, and
keep the comment in sync with the chosen approach.
| public function testCommandPathsUseUnixStyle(): void | ||
| { | ||
| foreach ($this->config['mcpServers'] as $serverName => $serverConfig) { | ||
| if (strpos($serverConfig['command'], '\\') \!== false) { |
There was a problem hiding this comment.
Syntax issue: unnecessary backslash escape in line 337.
Line 337 has \!== which unnecessarily escapes the exclamation mark. This should be !==.
Apply this diff:
public function testCommandPathsUseUnixStyle(): void
{
foreach ($this->config['mcpServers'] as $serverName => $serverConfig) {
- if (strpos($serverConfig['command'], '\\') \!== false) {
+ if (strpos($serverConfig['command'], '\\') !== false) {
$this->fail("Server '$serverName' command uses Windows-style backslashes, should use forward slashes");
}
}
} public function testArtisanPathsUseUnixStyle(): void
{
foreach ($this->config['mcpServers'] as $serverName => $serverConfig) {
foreach ($serverConfig['args'] as $arg) {
- if (strpos($arg, 'artisan') \!== false && strpos($arg, '\\') \!== false) {
+ if (strpos($arg, 'artisan') !== false && strpos($arg, '\\') !== false) {
$this->fail("Server '$serverName' artisan path uses Windows-style backslashes");
}
}
}
}Also applies to: 337-337
🤖 Prompt for AI Agents
In tests/Unit/JsonConfigTest.php around lines 324 and 337, the condition uses an
escaped exclamation mark `\!==` which is a syntax error; replace the escaped
form with the proper PHP strict-inequality operator `!==` so the line becomes
using `!==` (remove the backslash before `!==`) to correct the comparison and
fix the syntax error.
Unit test generation was requested by @nielsdrost7.
The following files were modified:
tests/Unit/JsonConfigTest.phpSummary by CodeRabbit