Conversation
WalkthroughThe pull request modernizes the project's testing infrastructure and build pipeline. Changes include restructuring the GitHub Actions workflow with multi-OS and multi-PHP-version matrix testing and a unified setup action, upgrading the PHP parser dependency from v4 to v5, simplifying PHPUnit configuration, refactoring test files with PHP 8 attributes and renamed methods, and bumping the version for Winter v1.3+ support. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
composer.json (1)
35-37:⚠️ Potential issue | 🟠 MajorUpdate
phpunitandphpversion constraints to match the actual project requirements.The
"phpunit/phpunit": "~4.0"constraint is severely outdated and incompatible with the tested PHP versions (8.2, 8.3, 8.4 per CI workflows). Thephpunit.xmlconfiguration already targets PHPUnit 9.3 schema, indicating a disconnect between declared and actual dependencies.Additionally,
"php": ">=7.0"doesn't reflect that CI tests against PHP 8.2+. Consider updating to:
"php": ">=8.2"to match CI requirements"phpunit/phpunit": "^10.5"(or^11.0if targeting PHP 8.3+)
🤖 Fix all issues with AI agents
In `@tests/unit/classes/FilesystemGeneratorTest.php`:
- Around line 18-23: The tearDown() order is wrong: call your cleanup before
tearing down the test parent to avoid using Laravel features after the container
is destroyed. In the FilesystemGeneratorTest class, move the $this->cleanUp()
call to run before parent::tearDown() (mirror the setUp() order), ensuring
cleanup logic that uses File::deleteDirectory() executes while the
application/container is still available.
🧹 Nitpick comments (6)
tests/unit/classes/MigrationFileParserTest.php (1)
5-9: Clean adoption of PHPUnit attributes with proper import.The
useimport forCoversClassand the attribute usage are clean. Note that the other two test files (ModelFileParserTest.phpandModelModelTest.php) use the fully-qualified class name inline instead of importing it. Consider aligning the style across the test suite for consistency.tests/unit/classes/ModelFileParserTest.php (1)
8-8: Inline FQCN for attribute — inconsistent withMigrationFileParserTest.php.
MigrationFileParserTest.phpimportsCoversClassvia ausestatement (Line 5), while this file uses the fully-qualified#[\PHPUnit\Framework\Attributes\CoversClass(...)]inline. Either approach works, but the test suite should be consistent.♻️ Suggested change for consistency
namespace Winter\Builder\Tests\Unit\Classes; +use PHPUnit\Framework\Attributes\CoversClass; use Winter\Builder\Classes\ModelFileParser; use Winter\Builder\Tests\BuilderPluginTestCase; -#[\PHPUnit\Framework\Attributes\CoversClass(ModelFileParser::class)] +#[CoversClass(ModelFileParser::class)] class ModelFileParserTest extends BuilderPluginTestCasetests/unit/classes/ModelModelTest.php (2)
10-10: Same FQCN inconsistency asModelFileParserTest.php.Same suggestion as the other files — consider importing
CoversClassvia ausestatement for consistency withMigrationFileParserTest.php.
48-56: Test silently passes ifSystemExceptionis not thrown.If
getModelFields()stops throwing on an invalid class name, the test won't fail — it'll just fall through to the remaining assertions. Consider using$this->expectException(SystemException::class)and separating the exception case into its own test method, or adding$this->fail()after the call.♻️ Option A: Add a fail() guard
// Invalid Class Name - try { - ModelModel::getModelFields(null, 'myClassName'); - } catch (SystemException $e) { - $this->assertEquals($e->getMessage(), 'Invalid model class name: myClassName'); - return; - } + $this->expectException(SystemException::class); + $this->expectExceptionMessage('Invalid model class name: myClassName'); + ModelModel::getModelFields(null, 'myClassName');This would require moving the remaining scenarios (lines 58–73) into a separate test method since
expectExceptionapplies to the whole test..github/workflows/unit-tests.yaml (2)
30-30: CI pinned to a WIP branch (wip/1.3).
winter-ref: wip/1.3ties CI to an unstable branch that could be rebased, force-pushed, or deleted. This is fine during active development, but should be updated to a stable ref (release tag or release branch) once Winter v1.3 is released, to prevent CI from silently breaking.
6-6: Branch trigger only coversmain.The push trigger is limited to
main. If the intent is to also run tests on the current feature branch (wip/1.3) during development, consider adding it here or relying solely on thepull_requesttrigger.
| public function tearDown(): void | ||
| { | ||
| parent::tearDown(); | ||
|
|
||
| $this->cleanUp(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find tearDown() patterns in test files across the repository
rg -n -B2 -A5 'function tearDown' --type=php --glob '*Test.php'Repository: wintercms/wn-builder-plugin
Length of output: 1142
🏁 Script executed:
#!/bin/bash
# Also search for variations in naming
fd -e php -path '*Test*' | head -20 | xargs rg -l 'tearDown' 2>/dev/null | head -10Repository: wintercms/wn-builder-plugin
Length of output: 341
🏁 Script executed:
#!/bin/bash
# Look at the parent class to understand what it does in tearDown
rg -n 'class FilesystemGeneratorTest' --type=php -A 1Repository: wintercms/wn-builder-plugin
Length of output: 231
🏁 Script executed:
#!/bin/bash
# Find the cleanUp method in the test file to understand dependencies
rg -n 'function cleanUp' --type=php -A 10Repository: wintercms/wn-builder-plugin
Length of output: 539
🏁 Script executed:
#!/bin/bash
# Find the parent class BuilderPluginTestCase
fd -type f -name '*.php' | xargs grep -l 'class BuilderPluginTestCase'Repository: wintercms/wn-builder-plugin
Length of output: 243
🏁 Script executed:
#!/bin/bash
# Search for BuilderPluginTestCase and its tearDown method
rg -n 'class BuilderPluginTestCase' -A 30 --type=phpRepository: wintercms/wn-builder-plugin
Length of output: 408
🏁 Script executed:
#!/bin/bash
# Look for where BuilderPluginTestCase is defined and check parent tearDown
rg -n 'tearDown' tests/ --type=php -B 2 -A 8 | head -60Repository: wintercms/wn-builder-plugin
Length of output: 1878
🏁 Script executed:
#!/bin/bash
# Find PluginTestCase definition
find . -name '*.php' -type f | xargs grep -l 'class PluginTestCase' 2>/dev/nullRepository: wintercms/wn-builder-plugin
Length of output: 55
🏁 Script executed:
#!/bin/bash
# Check if it's in vendor or external packages
rg 'class PluginTestCase' -A 15Repository: wintercms/wn-builder-plugin
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Check composer dependencies to understand where PluginTestCase comes from
cat composer.json | jq '.require' 2>/dev/null || cat composer.json | grep -A 20 '"require"'Repository: wintercms/wn-builder-plugin
Length of output: 154
🏁 Script executed:
#!/bin/bash
# Check the setUp method in FilesystemGeneratorTest to verify the pattern
rg -n 'function setUp' -A 5 tests/unit/classes/FilesystemGeneratorTest.phpRepository: wintercms/wn-builder-plugin
Length of output: 186
🏁 Script executed:
#!/bin/bash
# Check setUp in ModelModelTest too for comparison
rg -n 'function setUp' -A 5 tests/unit/classes/ModelModelTest.phpRepository: wintercms/wn-builder-plugin
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Search for any documentation or comments about tearDown ordering in the codebase
rg -i 'teardown.*parent\|parent.*teardown' -B 2 -A 2Repository: wintercms/wn-builder-plugin
Length of output: 53
Reorder cleanUp() before parent::tearDown() to ensure proper cleanup.
The setUp() method calls parent::setUp() first and then cleanUp(). The tearDown() method should mirror this in reverse order: cleanup first, then parent. Currently, calling parent::tearDown() before cleanUp() risks tearing down the application container before the File facade is used by File::deleteDirectory(), potentially causing the cleanup to silently fail and leave test artifacts on disk.
Proposed fix
public function tearDown(): void
{
- parent::tearDown();
-
$this->cleanUp();
+
+ parent::tearDown();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function tearDown(): void | |
| { | |
| parent::tearDown(); | |
| $this->cleanUp(); | |
| } | |
| public function tearDown(): void | |
| { | |
| $this->cleanUp(); | |
| parent::tearDown(); | |
| } |
🤖 Prompt for AI Agents
In `@tests/unit/classes/FilesystemGeneratorTest.php` around lines 18 - 23, The
tearDown() order is wrong: call your cleanup before tearing down the test parent
to avoid using Laravel features after the container is destroyed. In the
FilesystemGeneratorTest class, move the $this->cleanUp() call to run before
parent::tearDown() (mirror the setUp() order), ensuring cleanup logic that uses
File::deleteDirectory() executes while the application/container is still
available.
2 participants
I've added support for php-parser v5, I tested and created a plugin and made some changes to that plugin and couldn't find anything broken, but anything that extends from our abstract
PhpSourceParsermay run into issues as thelexerproperty has been removed.Summary by CodeRabbit
Tests
Chores