refactor: phpunit test #1
Conversation
WalkthroughAdded many PHPUnit tests (integration, form, and DTO unit tests), modernized existing tests (final classes, typed properties, DataProvider attributes), introduced setters on DTOs, changed many Configuration properties from private to public, removed an old Satis test file, and applied minor doc and php-cs-fixer config edits. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (4)
tests/Integration/BuildCommandTest.php (3)
93-93: Unused kernel boot result.
self::bootKernel()->getContainer()is called but the container is not used. If the kernel is not needed for this test, consider removing this line. If it is needed for side effects (e.g., loading services), the intent should be clarified with a comment.- self::bootKernel()->getContainer(); + // Boot kernel not needed for this test as we manually construct dependenciesOr if the kernel is required:
- self::bootKernel()->getContainer(); + self::bootKernel(); // Required for ... (add reason)
113-120: Consider cleaner debugging approach.The
echostatements help debug failures but bypass PHPUnit's output handling. Consider including the debug information in the assertion message or re-throwing with additional context:try { $exitCode = $application->run($input, $output); - self::assertSame(0, $exitCode, 'Expected exit code 0 for default form config build.'); + self::assertSame( + 0, + $exitCode, + \sprintf( + "Expected exit code 0 for default form config build.\nConfig: %s\nOutput: %s", + $configFile->getContent(), + $output->fetch() + ) + ); } catch (AssertionFailedError $error) { - echo $configFile->getContent(); - echo $output->fetch(); throw $error; }
154-172: Duplicated serializer creation logic.The
createSerializer()method duplicates code fromFilePersisterTest::testJsonPersisterNormalizationWorks()(lines 93-110). Consider extracting this to a shared trait or factory to reduce duplication and ensure consistent serializer configuration across tests.tests/Controller/SecurityControllerTest.php (1)
37-39: Redundant redirection assertions.
assertInstanceOf(RedirectResponse::class)already impliesisRedirection()is true. The second assertion is redundant.$response = $client->getResponse(); self::assertInstanceOf(RedirectResponse::class, $response, 'Invalid login submission must redirect.'); - self::assertTrue($response->isRedirection(), 'Response must be a redirection.'); self::assertStringEndsWith('/login', $response->getTargetUrl(), 'Failed login must redirect back to /login.');Same applies to lines 48-49.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
src/Validator/Constraints/Repository.php(0 hunks)tests/Application/ApplicationKernelTest.php(1 hunks)tests/Composer/Satis/Command/BuildCommandTest.php(0 hunks)tests/Controller/AdminControllerTest.php(2 hunks)tests/Controller/SecurityControllerTest.php(1 hunks)tests/Integration/BuildCommandTest.php(1 hunks)tests/Manager/ManagerConfigValidatorTest.php(1 hunks)tests/Persister/FilePersisterTest.php(3 hunks)tests/Traits/SchemaValidatorTrait.php(1 hunks)tests/Traits/VfsTrait.php(1 hunks)tests/app/ApplicationKernelTest.php(0 hunks)
💤 Files with no reviewable changes (3)
- tests/app/ApplicationKernelTest.php
- tests/Composer/Satis/Command/BuildCommandTest.php
- src/Validator/Constraints/Repository.php
🧰 Additional context used
🧬 Code graph analysis (2)
tests/Integration/BuildCommandTest.php (5)
src/DTO/Archive.php (2)
Archive(12-154)setFormat(58-63)src/DTO/Configuration.php (2)
Configuration(8-388)setArchive(240-243)src/Persister/ConfigurationNormalizer.php (1)
ConfigurationNormalizer(16-158)src/Persister/FilePersister.php (1)
FilePersister(9-102)src/Persister/JsonPersister.php (1)
JsonPersister(17-158)
tests/Persister/FilePersisterTest.php (4)
src/Persister/FilePersister.php (1)
FilePersister(9-102)src/Persister/JsonPersister.php (1)
JsonPersister(17-158)src/DTO/Configuration.php (5)
Configuration(8-388)getRequire(218-221)setRepositories(205-213)setRequire(226-232)addMinimumStabilityPerPackage(271-274)src/DTO/PackageConstraint.php (2)
setConstraint(32-37)PackageConstraint(5-38)
🔇 Additional comments (18)
tests/Traits/SchemaValidatorTrait.php (2)
12-12: LGTM: Clean refactor to inline expression.The simplified one-liner removes the unnecessary intermediate variable while maintaining the same functionality.
18-18: The API method change is verified and correct.The JsonSchema library's
Validator::validate()is the current and correct method to use. The web search confirms that there is no publicValidator::check()method in recent releases of the justinrainbow/json-schema library. The code inSchemaValidatorTrait.phpline 18 is already using the proper API with the correct signature:$validator->validate($content, $schema), followed byisValid()andgetErrors()checks that match the documented behavior. No changes are needed.tests/Traits/VfsTrait.php (1)
10-10: LGTM!Good modernization to use PHP 7.4+ typed property declaration. The nullable type is appropriate since
vfsTearDown()sets the property tonull.tests/Persister/FilePersisterTest.php (3)
28-33: LGTM!Good refactoring to use
finalclass declaration and tightening property visibility toprivatewith explicit nullable type. Test properties should be private as they're internal to the test class.
54-83: LGTM!Well-structured test with clear naming, descriptive assertion messages, and proper schema validation. The flow tests both initial flush and truncation scenarios comprehensively.
85-152: LGTM!Comprehensive normalization test that validates the full load-modify-flush-reload cycle. Good use of PHP 8 named arguments for the
ObjectNormalizerconstructor, improving readability.tests/Manager/ManagerConfigValidatorTest.php (2)
18-24: LGTM!Good modernization with
finalclass declaration and PHPUnit 10's#[DataProvider]attribute. The typedvfsStreamFileproperty is appropriately initialized insetUp().
46-67: Test logic appears disconnected from the mocked persister.The test copies a fixture into vfs, creates a mocked
JsonPersisterthat returns a freshConfiguration, then calls$manager->addAll([]). However:
- The mock's
load()returns an emptyConfiguration, not the fixture's content- The mock's
flush()doesn't write to the vfs file- The schema validation (lines 59-60) validates the original fixture content, not the output of
addAll([])If the intent is purely to validate that fixture files are valid Satis schemas, the mock interactions are unnecessary. If testing
RepositoryManagerbehavior, the mock prevents actual persistence.Consider clarifying the test's purpose:
- If testing fixture validity: remove the mock and just validate the fixtures directly
- If testing
RepositoryManager: use a real persister or configure the mock to capture and validate flushed contenttests/Integration/BuildCommandTest.php (3)
30-44: LGTM!Good test setup with vfsStream for filesystem isolation. The nullable typed property with explicit initialization in
setUp()and cleanup intearDown()follows the pattern established inVfsTrait.
46-61: LGTM!Clear negative test case that validates proper error handling when the config file is missing.
63-86: LGTM!Thorough integration test that validates the complete build output including
index.html,packages.json, and theincludedirectory with contents.tests/Application/ApplicationKernelTest.php (1)
19-43: Real filesystem manipulation in test may cause issues.This test renames the actual
public/index.htmlfile, which differs from other tests that usevfsStreamfor isolation. While thefinallyblock provides cleanup, consider these risks:
- If the test process is killed (SIGKILL), the file won't be restored
- Parallel test execution could conflict if multiple tests touch the same file
- CI environments may have the file in unexpected states
Consider whether this test could use vfsStream for full isolation, or add a safeguard in
tearDown()to ensure restoration even if an unexpected error occurs. Alternatively, document why real filesystem access is necessary here.tests/Controller/SecurityControllerTest.php (2)
10-12: LGTM!Good refactoring to
finalclass and more descriptive method name. The test now clearly communicates it tests the complete admin login flow.
16-55: LGTM!Well-organized test with clear phase comments and comprehensive coverage of the authentication flow. The descriptive assertion messages will help diagnose failures quickly.
tests/Controller/AdminControllerTest.php (4)
11-11: LGTM!Appropriate use of
finalclass declaration for the test class.
27-38: LGTM!Good smoke test verifying the admin index page loads successfully and contains the expected navigation links.
40-92: LGTM!Comprehensive CRUD workflow test with clear phase comments. Good use of
disableReboot()to maintain client state across operations, and the helper method provides clean validation of persisted state.
94-115: LGTM!Clean helper method for validating repository persistence. The use of
assertObjectHasPropertyaligns with PHPUnit 10+ best practices.Note: The helper validates the first repository. This works for the current test flow but would need adjustment if testing multiple repositories.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
tests/Form/DeleteFormTypeTest.php (1)
20-56: CSRF option and required-entity tests correctly mirror DeleteFormType behaviorThe three tests accurately cover:
- CSRF being enabled and the
csrf_token_idformat whengetId()returns a value.- The fallback
csrf_token_idprefix when nogetId()exists.- The requirement of the
entityoption viaMissingOptionsException.As an optional improvement, you could add a fourth test for an entity that has
getId()but returnsnull, to explicitly cover that branch of the normalizer as well.tests/Form/LoginFormTest.php (1)
33-47: Valid data submission test looks good.The test correctly verifies form synchronization and data binding for the username and password fields.
Minor observation: Line 38 includes
'login' => truein the submitted data. Sinceloginis a SubmitType field, including data for it is unusual (submit buttons don't typically hold data values). While Symfony handles this gracefully, you might consider omitting it for clarity.Consider adding test coverage for validation scenarios (e.g., missing required fields, invalid formats) if validation constraints are defined on the form or data class.
src/DTO/PackageStability.php (1)
21-26: Consider documenting or reconsidering the silent null-ignore pattern.The setter accepts
?stringbut silently ignoresnullvalues. While this may be intentional for form-data mapping (where empty submissions could passnull), it can lead to confusion—callers might expectsetPackage(null)to either throw or actually set the value.If this is intentional for form compatibility, a brief PHPDoc explaining the behavior would help:
+ /** + * Sets the package name. Null values are ignored to preserve existing data. + */ public function setPackage(?string $package): voidAlternatively, if strict typing is preferred, match
Abandoned::setPackage(string $package)for consistency across DTOs.tests/Form/PackageStabilityTypeTest.php (1)
51-59: Consider adding specific error message assertions for robustness.The test checks error count, which could pass with different validation errors. For more robust testing, consider asserting on the specific error messages:
$this->assertFalse($form->isValid()); $errors = $form->getErrors(true); $this->assertCount(2, $errors); + // Optionally verify specific constraint violations + $messages = array_map(fn($e) => $e->getMessage(), iterator_to_array($errors)); + $this->assertContains('This value should not be blank.', $messages);This is optional—the current test is functional.
src/DTO/Configuration.php (2)
12-96: Reconsider exposing properties publicly while retaining getters/setters.Making these properties public creates redundancy with the existing getter/setter methods and breaks the encapsulation pattern. This dual-access approach can lead to:
- Inconsistent usage patterns across the codebase (direct access vs. methods)
- Difficulty enforcing validation or business logic in setters
- Tighter coupling between tests and implementation details
If the goal is to simplify testing, consider these alternatives:
- Keep properties private and continue using public methods in tests (recommended for maintainability)
- If direct property access is required for serialization frameworks, document this intent and consider whether getters/setters are still needed
- Use PHP 8.1+ readonly properties where applicable to maintain some encapsulation guarantees
49-49: Clarify the visibility inconsistency.These three properties (
stripHosts,providersHistorySize,comment) remain private while most others were made public. The selection criteria isn't clear—all three have getter/setter methods similar to the properties that were made public.Please ensure this inconsistency is intentional and consider documenting the rationale if these properties require different visibility for specific reasons (e.g., validation requirements, internal-only state).
Also applies to: 69-69, 93-93
tests/Form/PackageConstraintTypeTest.php (3)
33-49: Consider asserting form validity in the “valid data” pathYou already assert synchronization; adding an explicit
$this->assertTrue($form->isValid());would also lock in that the NotBlank constraints accept this data, not just that mapping works.
51-59: Make error assertions slightly more explicitCounting two errors works, but it’s a bit brittle if constraint configuration changes (e.g., message nesting or extra constraints). You might instead/assert‑also check that each child field has at least one error, e.g. via
$form->get('package')->getErrors()and$form->get('constraint')->getErrors().
61-71: Clarify intent of the second empty‑submission testThis test nicely covers the “no fields submitted” case and the configured default object. To make its purpose crystal clear relative to
testSubmitEmptyDataIsInvalid(), consider documenting (or asserting) whether you expect this default object to ever be used in valid flows, or if it’s purely a data‑mapping fallback for invalid empty submissions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.php-cs-fixer.dist.php(0 hunks)src/DTO/Abandoned.php(1 hunks)src/DTO/Configuration.php(2 hunks)src/DTO/PackageStability.php(1 hunks)tests/Form/AbandonedTypeTest.php(1 hunks)tests/Form/ArchiveTypeTest.php(1 hunks)tests/Form/DeleteFormTypeTest.php(1 hunks)tests/Form/LoginFormTest.php(1 hunks)tests/Form/PackageConstraintTypeTest.php(1 hunks)tests/Form/PackageStabilityTypeTest.php(1 hunks)tests/Integration/BuildCommandTest.php(1 hunks)
💤 Files with no reviewable changes (1)
- .php-cs-fixer.dist.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Integration/BuildCommandTest.php
🧰 Additional context used
🧬 Code graph analysis (8)
src/DTO/Abandoned.php (1)
src/DTO/PackageStability.php (1)
setPackage(21-26)
tests/Form/ArchiveTypeTest.php (1)
src/DTO/Archive.php (11)
Archive(12-154)getDirectory(41-44)getFormat(53-56)getAbsoluteDirectory(65-68)getPrefixUrl(105-108)isSkipDev(75-78)isChecksum(115-118)isIgnoreFilters(125-128)isOverrideDistType(135-138)isRearchive(145-148)getWhitelist(85-88)
tests/Form/LoginFormTest.php (2)
src/Form/LoginType.php (1)
LoginType(11-21)tests/Form/PackageStabilityTypeTest.php (1)
setUp(17-23)
tests/Form/DeleteFormTypeTest.php (1)
src/Form/DeleteFormType.php (1)
DeleteFormType(12-38)
tests/Form/AbandonedTypeTest.php (2)
src/DTO/Abandoned.php (3)
Abandoned(5-36)getPackage(17-20)getReplacement(22-25)src/Form/AbandonedType.php (1)
AbandonedType(12-52)
src/DTO/PackageStability.php (1)
src/DTO/Abandoned.php (1)
setPackage(27-30)
tests/Form/PackageStabilityTypeTest.php (2)
src/DTO/PackageStability.php (3)
PackageStability(5-39)getPackage(16-19)getStability(28-31)src/Form/PackageStabilityType.php (1)
PackageStabilityType(13-58)
tests/Form/PackageConstraintTypeTest.php (2)
src/DTO/PackageConstraint.php (2)
PackageConstraint(5-38)getConstraint(27-30)src/Form/PackageConstraintType.php (1)
PackageConstraintType(12-55)
🔇 Additional comments (16)
tests/Form/DeleteFormTypeTest.php (1)
15-18: Form factory setup is appropriate and minimalUsing
Forms::createFormFactory()insetUp()keeps the fixture simple and is sufficient for asserting configured options onDeleteFormType. No changes needed here.tests/Form/LoginFormTest.php (3)
12-22: LGTM! Clean test setup.The class structure follows modern PHPUnit best practices with a final class declaration and typed property. The setUp method correctly initializes the form factory with validator support, matching the pattern used in other form tests.
24-31: LGTM! Field presence verification is correct.The test appropriately verifies that all three fields defined in LoginType (username, password, login) are present in the form.
49-57: LGTM! Empty data test is correct.The test appropriately verifies that submitting empty data results in null values for the username and password fields while maintaining form synchronization.
src/DTO/Abandoned.php (1)
26-35: LGTM! Setters align with property types and constructor requirements.The
setPackageaccepts a non-nullablestring(matching theprivate string $packagedeclaration), whilesetReplacementaccepts?string(matchingprivate ?string $replacement). This differs fromPackageStability::setPackagewhich uses a null-guard pattern, but that's appropriate givenPackageStabilityallows nullable input while guarding against assignment, whereas here the constructor enforcesstring $packagefrom the start.tests/Form/PackageStabilityTypeTest.php (3)
13-23: Good test setup with validator extension.The
setUp()properly initializes the form factory withValidatorExtension, enabling constraint validation in tests. This is the correct approach for testing Symfony forms with validation.
25-49: LGTM! Test methods cover field presence and valid data submission.The tests verify form fields exist and that valid data is correctly mapped to the
PackageStabilityDTO.
61-71: LGTM! Correctly tests theempty_dataconfiguration.The test verifies that submitting an empty array creates a default
PackageStabilityobject with empty strings, matching the form'sempty_dataoption.tests/Form/ArchiveTypeTest.php (2)
9-10: Note: NoValidatorExtensionconfigured.Unlike
PackageStabilityTypeTest, this test extendsTypeTestCasewithout addingValidatorExtension. This is fine for testing data mapping, but ifArchiveTypehas validation constraints, you'll need to add validation tests with the extension. If validation is tested elsewhere or not required, this is acceptable.
11-67: LGTM! Comprehensive data mapping tests.Both tests thoroughly cover the
ArchiveDTO's properties:
testSubmitValidData: Verifies all 11 fields are correctly mapped from submitted datatestEmptyData: Confirms default values (empty strings, nulls, booleans, and arrays)tests/Form/AbandonedTypeTest.php (4)
13-18: Good use ofgetExtensions()for test setup.Using
getExtensions()is the idiomaticTypeTestCasepattern for adding theValidatorExtension. This is cleaner than usingsetUp()to manually build the factory.
20-41: LGTM! Comprehensive valid submission test.The test covers form synchronization, submission state, validation, data mapping, and view structure verification.
43-56: LGTM! Correctly tests optional replacement field.The test validates that an empty replacement is allowed (no
NotBlankconstraint), and the result is an empty string due to the form'sempty_dataconfiguration.
58-81: LGTM! Good validation and attribute tests.
testPackageCannotBeBlank: Properly verifies theNotBlankconstraint on the package fieldtestFieldPlaceholders: Ensures form view attributes match expected placeholderstests/Form/PackageConstraintTypeTest.php (2)
17-23: Form factory setup looks goodReusing a single form factory with the validator extension in
setUp()is a clean pattern and keeps the individual tests focused on behavior rather than wiring.
25-31: Field‑presence test is clear and sufficientChecking that both
packageandconstraintfields exist directly reflects the form definition and should catch accidental field removal/rename.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.