Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDependency and dev-tool version bumps in composer.json; phpunit.xml.dist migrated to PHPUnit 12 conventions and bootstraps a new PHPUnit Extension; BeforeTestHook removed; tests modernized (PHP 8 DataProvider attributes, createMock usage, updated expectation APIs, and targeted test refactors). Changes
Sequence Diagram(s)sequenceDiagram
participant Config as phpunit.xml.dist
participant PHPUnit as PHPUnit Runner
participant Extension as App\Tests\Extension\BypassFinalExtension
participant BypassFinals as SebastianBergmann\BypassFinals
PHPUnit->>Config: Load configuration (extensions/bootstrap)
Config->>Extension: Instantiate & call bootstrap()
Extension->>BypassFinals: BypassFinals::enable()
BypassFinals->>BypassFinals: Disable final-class enforcement (runtime)
Note over PHPUnit,BypassFinals: Tests execute with final-class bypass enabled
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
garak
force-pushed
the
phpunit-upgrade
branch
from
4bae8f7 to
eb85187
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/Badge/Service/ClientStrategyTest.php (1)
46-54: Consider createMock() for simpler mock creation.While
getMockBuilder()withdisableOriginalConstructor()is still valid in PHPUnit 12, you could simplify mock creation usingcreateMock()if the client constructors can be safely invoked with no arguments or if test doubles don't need constructor bypassing.Example:
-$this->githubClient = $this->getMockBuilder(GithubClient::class) - ->disableOriginalConstructor() - ->getMock(); +$this->githubClient = $this->createMock(GithubClient::class);However, only apply this if the constructors don't require dependencies or cause side effects during test setup.
tests/Badge/Model/UseCase/CreateComposerLockBadgeTest.php (1)
41-42: Consider removing the redundant helper method.The migration to
createMock()is correct. However, thecreateMockWithoutInvokingTheOriginalConstructorhelper method now just delegates tocreateMock(), which already doesn't invoke constructors by default. Additionally, the$methodsparameter is accepted but never used—in PHPUnit 12,createMock()automatically mocks all methods.You could simplify by directly calling
createMock()at all usage sites and removing this helper entirely:- private function createMockWithoutInvokingTheOriginalConstructor(string $classname, array $methods = []): MockObject - { - return $this->createMock($classname); - }Then replace calls like:
- $package = $this->createMockWithoutInvokingTheOriginalConstructor( - AppPackage::class, - ['hasStableVersion', 'getLatestStableVersion', 'getOriginalObject', 'getDefaultBranch'] - ); + $package = $this->createMock(AppPackage::class);Note: If this pattern appears across multiple test files (as suggested by the relevant snippets), you may want to defer this refactoring to a separate PR for consistency.
Also applies to: 55-58
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
composer.lockis excluded by!**/*.locksymfony.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
composer.json(2 hunks)phpunit.xml.dist(2 hunks)tests/Badge/Model/BadgeTest.php(2 hunks)tests/Badge/Model/ParseStabilityTest.php(1 hunks)tests/Badge/Model/UseCase/CreateCircleCiBadgeTest.php(7 hunks)tests/Badge/Model/UseCase/CreateComposerLockBadgeTest.php(5 hunks)tests/Badge/Model/UseCase/CreateDependentsBadgeTest.php(2 hunks)tests/Badge/Model/UseCase/CreateDownloadsBadgeTest.php(2 hunks)tests/Badge/Model/UseCase/CreateLicenseBadgeTest.php(2 hunks)tests/Badge/Model/UseCase/CreateRequireBadgeTest.php(2 hunks)tests/Badge/Model/UseCase/CreateSuggestersBadgeTest.php(2 hunks)tests/Badge/Model/UseCase/CreateVersionBadgeTest.php(4 hunks)tests/Badge/Service/ClientStrategyTest.php(15 hunks)tests/Badge/Service/TextNormalizerTest.php(3 hunks)tests/Badge/ValueObject/RepositoryTest.php(2 hunks)tests/Controller/HomeControllerTest.php(1 hunks)tests/EventListener/BadgeLoggerSubscriberTest.php(2 hunks)tests/Extension/BypassFinalExtension.php(1 hunks)tests/Hook/BypassFinalHook.php(0 hunks)tests/Service/SnippetGeneratorTest.php(1 hunks)
💤 Files with no reviewable changes (1)
- tests/Hook/BypassFinalHook.php
🧰 Additional context used
🧬 Code graph analysis (14)
tests/Badge/Service/TextNormalizerTest.php (3)
tests/Badge/Model/BadgeTest.php (1)
DataProvider(39-46)tests/Badge/Model/ParseStabilityTest.php (1)
DataProvider(12-16)tests/Badge/ValueObject/RepositoryTest.php (1)
DataProvider(81-87)
tests/Badge/Model/UseCase/CreateDownloadsBadgeTest.php (1)
src/Badge/Model/Package.php (1)
Package(21-308)
tests/Badge/ValueObject/RepositoryTest.php (6)
tests/Badge/Model/BadgeTest.php (1)
DataProvider(39-46)tests/Badge/Model/ParseStabilityTest.php (1)
DataProvider(12-16)tests/Badge/Model/UseCase/CreateCircleCiBadgeTest.php (1)
DataProvider(44-76)tests/Badge/Model/UseCase/CreateComposerLockBadgeTest.php (1)
DataProvider(60-103)tests/Badge/Service/TextNormalizerTest.php (2)
DataProvider(27-34)DataProvider(47-52)tests/Controller/HomeControllerTest.php (1)
DataProvider(10-16)
tests/Badge/Model/UseCase/CreateRequireBadgeTest.php (1)
src/Badge/Model/Package.php (1)
Package(21-308)
tests/Badge/Model/UseCase/CreateCircleCiBadgeTest.php (2)
tests/Badge/Model/BadgeTest.php (1)
DataProvider(39-46)tests/Badge/Model/UseCase/CreateComposerLockBadgeTest.php (1)
DataProvider(60-103)
tests/Badge/Model/UseCase/CreateComposerLockBadgeTest.php (3)
tests/Badge/Model/BadgeTest.php (1)
DataProvider(39-46)tests/Badge/Model/ParseStabilityTest.php (1)
DataProvider(12-16)tests/Badge/Model/UseCase/CreateCircleCiBadgeTest.php (2)
DataProvider(44-76)createMockWithoutInvokingTheOriginalConstructor(39-42)
tests/Badge/Model/BadgeTest.php (2)
tests/Badge/Model/ParseStabilityTest.php (1)
DataProvider(12-16)tests/Badge/Service/TextNormalizerTest.php (2)
DataProvider(27-34)DataProvider(47-52)
tests/Controller/HomeControllerTest.php (6)
tests/Badge/Model/BadgeTest.php (1)
DataProvider(39-46)tests/Badge/Model/ParseStabilityTest.php (1)
DataProvider(12-16)tests/Badge/Model/UseCase/CreateCircleCiBadgeTest.php (1)
DataProvider(44-76)tests/Badge/Model/UseCase/CreateComposerLockBadgeTest.php (1)
DataProvider(60-103)tests/Badge/Service/TextNormalizerTest.php (2)
DataProvider(27-34)DataProvider(47-52)tests/Badge/ValueObject/RepositoryTest.php (1)
DataProvider(81-87)
tests/Badge/Model/UseCase/CreateSuggestersBadgeTest.php (1)
src/Badge/Model/Package.php (1)
Package(21-308)
tests/Badge/Model/UseCase/CreateDependentsBadgeTest.php (1)
src/Badge/Model/Package.php (1)
Package(21-308)
phpunit.xml.dist (1)
tests/Hook/BypassFinalHook.php (1)
BypassFinalHook(8-14)
tests/Badge/Model/UseCase/CreateLicenseBadgeTest.php (1)
src/Badge/Model/Package.php (1)
Package(21-308)
tests/Extension/BypassFinalExtension.php (1)
tests/Hook/BypassFinalHook.php (2)
BypassFinalHook(8-14)executeBeforeTest(10-13)
tests/Badge/Model/ParseStabilityTest.php (3)
tests/Badge/Model/BadgeTest.php (1)
DataProvider(39-46)tests/Badge/Model/UseCase/CreateCircleCiBadgeTest.php (1)
DataProvider(44-76)tests/Badge/Service/TextNormalizerTest.php (2)
DataProvider(27-34)DataProvider(47-52)
🪛 PHPMD (2.15.0)
tests/Extension/BypassFinalExtension.php
13-13: Avoid unused parameters such as '$configuration'. (undefined)
(UnusedFormalParameter)
13-13: Avoid unused parameters such as '$facade'. (undefined)
(UnusedFormalParameter)
13-13: Avoid unused parameters such as '$parameters'. (undefined)
(UnusedFormalParameter)
tests/Service/SnippetGeneratorTest.php
172-172: Avoid unused parameters such as '$referenceType'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (8.3, pcov)
- GitHub Check: docker
- GitHub Check: build (8.3, pcov)
🔇 Additional comments (35)
tests/Badge/Service/ClientStrategyTest.php (1)
67-67: LGTM! PHPUnit 12 mock expectation syntax correctly updated.The migration from
self::once()to$this->once()is required for PHPUnit 12 compatibility. All mock expectations have been consistently updated throughout the file.Also applies to: 74-74, 91-91, 103-103, 108-108, 121-121, 148-148, 152-152, 171-171, 178-178, 197-197, 204-204, 223-223, 231-231, 236-236, 263-263, 268-268, 286-286, 296-296, 301-301, 319-319, 331-331, 336-336, 355-355, 371-371
tests/EventListener/BadgeLoggerSubscriberTest.php (2)
43-46: LGTM! Modern PHPUnit expectation syntax.The migration from
self::once()to$this->once()correctly adopts PHPUnit 12's instance-based expectation API.
70-71: LGTM! Consistent expectation syntax.Both expectations correctly use the instance-based
$this->once()method, aligning with PHPUnit 12 best practices.tests/Badge/Model/UseCase/CreateVersionBadgeTest.php (4)
40-42: LGTM! Modern expectation syntax.Correctly migrated to instance-based
$this->once()for PHPUnit 12 compatibility.
77-77: LGTM! Modern exception stubbing.The change from
self::throwException()to$this->throwException()aligns with PHPUnit 12's instance-based API.
91-93: LGTM! Consistent with modern PHPUnit API.Instance-based expectation method correctly used.
128-128: LGTM! Exception stubbing modernized.Instance-based exception method correctly applied.
tests/Badge/Model/UseCase/CreateDownloadsBadgeTest.php (2)
34-38: LGTM! Modern mock creation and expectations.The migration to
createMock()and instance-based$this->once()follows PHPUnit 12 best practices. The simplified mock creation is more concise than the previousgetMockBuilderpattern.
55-55: LGTM! Exception stubbing modernized.Correctly uses instance-based
$this->throwException()for PHPUnit 12.tests/Badge/Service/TextNormalizerTest.php (3)
15-15: LGTM! PHP 8 attribute import added.Correctly imports the DataProvider attribute for modern PHPUnit test configuration.
27-34: LGTM! Modern data provider syntax.Migration from PHPDoc
@dataProviderto PHP 8#[DataProvider()]attribute follows PHPUnit 12 best practices and provides better IDE support.
47-52: LGTM! Consistent attribute usage.Data provider correctly uses the PHP 8 attribute syntax.
tests/Badge/ValueObject/RepositoryTest.php (2)
8-8: LGTM! Attribute import added.Correctly imports DataProvider for PHP 8 attribute usage.
81-87: LGTM! Data provider attribute migration.The transition from PHPDoc annotation to PHP 8
#[DataProvider()]attribute is correctly applied, consistent with the broader modernization effort across the test suite.tests/Badge/Model/UseCase/CreateLicenseBadgeTest.php (2)
34-38: LGTM! Mock creation and expectation modernized.The simplified
createMock()approach and instance-based$this->once()expectation follow PHPUnit 12 best practices.
53-53: LGTM! Exception stubbing updated.Instance-based
$this->throwException()correctly applied for PHPUnit 12.tests/Badge/Model/BadgeTest.php (2)
6-6: LGTM! DataProvider attribute import.Correctly imports the attribute for modern test configuration.
39-46: LGTM! Data provider attribute applied.The PHP 8
#[DataProvider()]attribute correctly replaces the PHPDoc annotation, maintaining consistency across the test suite.phpunit.xml.dist (3)
3-3: LGTM! Documentation reference updated.The configuration header correctly references PHPUnit 12.4 documentation.
46-50: LGTM! Coverage configuration modernized.The migration from
<coverage>to<source>block follows PHPUnit 12's simplified configuration schema. The direct<directory>inclusion without suffix filtering is cleaner and aligns with current best practices.
53-56: LGTM! Extension bootstrap mechanism updated.The replacement of the Hook-based approach with the Extension bootstrap pattern aligns with PHPUnit 12 requirements. Adding
SymfonyExtensionensures proper integration with Symfony's PHPUnit Bridge.tests/Badge/Model/UseCase/CreateRequireBadgeTest.php (1)
25-29: LGTM! PHPUnit 12 migration correctly applied.The migration from
getMockBuildertocreateMock, and from static methods (self::once(),self::throwException()) to instance methods ($this->once(),$this->throwException()) is correct and aligns with PHPUnit 12 best practices.Also applies to: 45-47
tests/Badge/Model/UseCase/CreateDependentsBadgeTest.php (1)
34-38: LGTM! Consistent PHPUnit 12 migration.Mock creation and expectation methods have been correctly updated to use PHPUnit 12 APIs.
Also applies to: 53-55
tests/Badge/Model/UseCase/CreateCircleCiBadgeTest.php (3)
18-18: LGTM! DataProvider attribute migration applied correctly.The migration from docblock
@dataProviderannotation to PHP 8#[DataProvider]attribute is correct and consistent with modern PHPUnit 12 usage.Also applies to: 44-44
31-32: LGTM! Mock creation simplified correctly.The migration to
createMockis correct. Note that thecreateMockWithoutInvokingTheOriginalConstructorhelper now effectively just delegates tocreateMock(which already disables constructors by default in PHPUnit 12), making the$methodsparameter unused. This is acceptable as the method maintains backward compatibility with the existing test structure.Also applies to: 39-42
61-69: LGTM! Expectation methods migrated correctly.All mock expectations have been correctly updated from static (
self::once()) to instance-based ($this->once()) methods, and exception stubbing updated to$this->throwException(), following PHPUnit 12 conventions.Also applies to: 104-110, 136-144, 176-180
tests/Controller/HomeControllerTest.php (1)
5-5: LGTM! Clean DataProvider attribute migration.The migration from docblock annotation to PHP 8 attribute is straightforward and correct.
Also applies to: 10-10
tests/Badge/Model/UseCase/CreateSuggestersBadgeTest.php (1)
34-38: LGTM! PHPUnit 12 migration applied correctly.Mock creation and expectation methods follow the same correct migration pattern observed across the test suite.
Also applies to: 53-55
tests/Extension/BypassFinalExtension.php (1)
1-17: LGTM! Correct PHPUnit 12 Extension implementation.This properly replaces the deprecated
BeforeTestHookwith the modernExtensioninterface required by PHPUnit 12. The bootstrap method signature must match the interface requirements, so the PHPMD warnings about unused parameters are false positives and can be safely ignored—the parameters are mandated by theExtensioninterface contract.tests/Badge/Model/ParseStabilityTest.php (1)
7-7: LGTM! DataProvider attribute migration applied.Clean migration from docblock annotation to PHP 8 attribute, consistent with the test suite modernization.
Also applies to: 12-12
tests/Service/SnippetGeneratorTest.php (2)
150-164: LGTM! Modern mock return value mapping.The use of
createMockandwillReturnMapis a clean approach for mapping multiple route names to their corresponding route objects, replacing the likely more verbose pattern from PHPUnit 9.
172-210: LGTM! Correct migration from deprecated withConsecutive.The
willReturnCallbackapproach correctly replaces the deprecatedwithConsecutive/willReturnOnConsecutiveCallspattern that was removed in PHPUnit 10+. The callback provides flexible conditional logic based on route name and parameters. The unused$referenceTypeparameter is part of theRouterInterface::generatesignature and is expected—the PHPMD warning is a false positive.tests/Badge/Model/UseCase/CreateComposerLockBadgeTest.php (2)
21-21: LGTM! DataProvider migration is correct.The migration from docblock
@dataProviderannotation to PHP 8 attribute syntax is properly implemented for PHPUnit 12 compatibility.Also applies to: 60-60
76-76: LGTM! Mock expectation syntax updated correctly.All changes from static method calls (
self::once(),self::throwException()) to instance method calls ($this->once(),$this->throwException()) are correct for PHPUnit 12 compatibility.Also applies to: 80-80, 84-84, 92-92, 96-96, 120-120, 122-122, 124-124
composer.json (1)
24-56: Dependency versions verified; manually test for breaking changes.All specified versions exist and are available on Packagist. Security advisories found apply only to much older versions (not the versions being upgraded from/to). PHPUnit 9.5→12.4 is a major version upgrade and may have breaking changes; verify compatibility by running your test suite. Symfony 7.1→7.3 is within a minor version bump and unlikely to introduce breaking changes.
garak
force-pushed
the
phpunit-upgrade
branch
from
eb85187 to
dd9915b
Compare
You're right, I forgot that! Updated now |
There was a problem hiding this comment.
Pull request overview
This PR upgrades the project’s PHP tooling and framework dependencies (notably PHPUnit and Symfony), and modernizes the test suite/configuration to align with the newer toolchain.
Changes:
- Bump dependencies (Symfony 7.1 → 7.3, PHPUnit → 12.4, plus various dev tools) and update lockfiles.
- Update PHPUnit configuration (new
<source>section, switch to PHPUnit extension bootstraps). - Modernize tests (DataProvider PHP 8 attributes, updated PHPUnit mocking/assertion APIs, replace per-test hook with a PHPUnit extension for bypassing finals).
Reviewed changes
Copilot reviewed 21 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
composer.json |
Updates Symfony/PHPUnit and dev-tool version constraints. |
composer.lock |
Locks upgraded dependency graph for the new Symfony/PHPUnit versions. |
symfony.lock |
Syncs Symfony Flex lock entries with updated dependencies. |
phpunit.xml.dist |
Updates PHPUnit config format and switches to extension bootstraps. |
.php-cs-fixer.dist.php |
Updates php-cs-fixer preset configuration for the upgraded toolchain. |
tests/Extension/BypassFinalExtension.php |
Adds PHPUnit extension to enable DG\BypassFinals at bootstrap. |
tests/Hook/BypassFinalHook.php |
Removes the old BeforeTestHook implementation. |
tests/Service/SnippetGeneratorTest.php |
Refactors router/route mocks to use maps/callbacks instead of consecutive expectations. |
tests/EventListener/BadgeLoggerSubscriberTest.php |
Updates mock expectation API usage ($this->once() etc.). |
tests/Controller/HomeControllerTest.php |
Migrates data provider annotation to PHP 8 attribute. |
tests/Badge/ValueObject/RepositoryTest.php |
Migrates data provider annotation to PHP 8 attribute. |
tests/Badge/Service/TextNormalizerTest.php |
Migrates data provider annotations to PHP 8 attributes. |
tests/Badge/Service/ClientStrategyTest.php |
Updates mock expectation API usage. |
tests/Badge/Model/UseCase/CreateVersionBadgeTest.php |
Updates exception stubbing/expectations for newer PHPUnit APIs. |
tests/Badge/Model/UseCase/CreateSuggestersBadgeTest.php |
Simplifies mocks and updates expectation API usage. |
tests/Badge/Model/UseCase/CreateRequireBadgeTest.php |
Simplifies mocks and updates exception stubbing. |
tests/Badge/Model/UseCase/CreateLicenseBadgeTest.php |
Simplifies mocks and updates exception stubbing. |
tests/Badge/Model/UseCase/CreateDownloadsBadgeTest.php |
Simplifies mocks and updates exception stubbing. |
tests/Badge/Model/UseCase/CreateDependentsBadgeTest.php |
Simplifies mocks and updates exception stubbing. |
tests/Badge/Model/UseCase/CreateComposerLockBadgeTest.php |
Updates mocks, typing, and migrates provider to attribute. |
tests/Badge/Model/UseCase/CreateCircleCiBadgeTest.php |
Updates mocks and migrates provider to attribute. |
tests/Badge/Model/ParseStabilityTest.php |
Migrates data provider annotation to PHP 8 attribute. |
tests/Badge/Model/BadgeTest.php |
Migrates data provider annotation to PHP 8 attribute. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $router | ||
| ->method('generate') | ||
| ->withConsecutive( | ||
| [ | ||
| 'pugx_badge_packagist', | ||
| ['repository' => 'vendor/package'], | ||
| ], | ||
| [ | ||
| 'pugx_badge_version_latest', | ||
| ['latest' => 'stable', 'repository' => 'vendor/package'], | ||
| RouterInterface::ABSOLUTE_URL, | ||
| ], | ||
| [ | ||
| 'pugx_badge_download', | ||
| ['repository' => 'vendor/package'], | ||
| RouterInterface::ABSOLUTE_URL, | ||
| ], | ||
| [ | ||
| 'pugx_badge_version_latest', | ||
| ['latest' => 'unstable', 'repository' => 'vendor/package'], | ||
| RouterInterface::ABSOLUTE_URL, | ||
| ], | ||
| [ | ||
| 'pugx_badge_license', | ||
| ['repository' => 'vendor/package'], | ||
| RouterInterface::ABSOLUTE_URL, | ||
| ], | ||
| [ | ||
| 'pugx_badge_download_type', | ||
| ['repository' => 'vendor/package', 'type' => 'monthly'], | ||
| RouterInterface::ABSOLUTE_URL, | ||
| ], | ||
| [ | ||
| 'pugx_badge_download_type', | ||
| ['repository' => 'vendor/package', 'type' => 'daily'], | ||
| RouterInterface::ABSOLUTE_URL, | ||
| ], | ||
| [ | ||
| 'pugx_badge_version', | ||
| ['repository' => 'vendor/package'], | ||
| RouterInterface::ABSOLUTE_URL, | ||
| ], | ||
| [ | ||
| 'pugx_badge_require', | ||
| ['repository' => 'vendor/package', 'type' => 'php'], | ||
| RouterInterface::ABSOLUTE_URL, | ||
| ], | ||
| [ | ||
| 'pugx_badge_composerlock', | ||
| ['repository' => 'vendor/package'], | ||
| RouterInterface::ABSOLUTE_URL, | ||
| ], | ||
| [ | ||
| 'pugx_badge_gitattributes', | ||
| ['repository' => 'vendor/package'], | ||
| RouterInterface::ABSOLUTE_URL, | ||
| ], | ||
| [ | ||
| 'pugx_badge_dependents', | ||
| ['repository' => 'vendor/package'], | ||
| RouterInterface::ABSOLUTE_URL, | ||
| ], | ||
| [ | ||
| 'pugx_badge_suggesters', | ||
| ['repository' => 'vendor/package'], | ||
| RouterInterface::ABSOLUTE_URL, | ||
| ], | ||
| [ | ||
| 'pugx_badge_circleci', | ||
| ['repository' => 'vendor/package'], | ||
| RouterInterface::ABSOLUTE_URL, | ||
| ] | ||
| ) | ||
| ->willReturnOnConsecutiveCalls('repo_url', 'img_url0', 'img_url1', 'img_url2', 'img_url3', 'img_url4', 'img_url5', 'img_url6', 'img_url7', 'img_url8', 'img_url9', 'img_url10', 'img_url11', 'img_url12'); | ||
| ->willReturnCallback(function (string $name, array $params = []) { | ||
| if ('pugx_badge_packagist' === $name) { | ||
| return 'repo_url'; | ||
| } | ||
|
|
There was a problem hiding this comment.
The RouterInterface::generate() mock uses willReturnCallback with a closure that only accepts ($name, $params). In SnippetGenerator::generateImg() the real call passes a 3rd argument (RouterInterface::ABSOLUTE_URL), so this callback will be invoked with 3 parameters and can error. Update the callback signature to accept the $referenceType argument (or use a variadic) to match RouterInterface::generate().
| 'pugx_badge_circleci' => 'img_url12', | ||
| ]; | ||
|
|
||
| return $map[$name] ?? 'repo_url'; |
There was a problem hiding this comment.
The generate() stub falls back to returning 'repo_url' for any unknown route name. This can mask regressions if SnippetGenerator starts generating an unexpected route name. Consider throwing an exception (or failing the test) when $name is not one of the expected routes so the test fails loudly on unexpected calls.
| return $map[$name] ?? 'repo_url'; | |
| if (!\array_key_exists($name, $map)) { | |
| throw new \InvalidArgumentException(\sprintf('Unexpected route name "%s" in SnippetGeneratorTest router mock.', $name)); | |
| } | |
| return $map[$name]; |
| "dg/bypass-finals": "^1.9", | ||
| "friendsofphp/php-cs-fixer": "^3.89", | ||
| "jangregor/phpstan-prophecy": "^1.0", | ||
| "phpspec/prophecy-phpunit": "^2.3", | ||
| "phpstan/extension-installer": "^1.2", | ||
| "phpstan/phpstan": "^1.9", | ||
| "phpstan/phpstan": "^1.12", | ||
| "phpstan/phpstan-phpunit": "^1.2", | ||
| "phpstan/phpstan-symfony": "^1.2", | ||
| "phpunit/phpunit": "^9.5", | ||
| "phpunit/phpunit": "^12.4", | ||
| "roave/security-advisories": "dev-latest", |
There was a problem hiding this comment.
composer.json upgrades phpunit/phpunit to ^12.4, but the lockfile resolves jangregor/phpstan-prophecy 1.0.0 which declares a conflict with phpunit/phpunit >=10. This combination is not installable with Composer. Either remove/replace phpstan-prophecy (or Prophecy tooling) or change the PHPUnit upgrade plan so dependency constraints are satisfiable.
| '@PHP8x0Migration:risky' => true, | ||
| '@PHP8x1Migration' => true, | ||
| '@PHPUnit8x4Migration:risky' => true, |
There was a problem hiding this comment.
The php-cs-fixer rule set names '@PHP8x0Migration', '@PHP8x1Migration', and '@PHPUnit8x4Migration' don't match the standard PHP-CS-Fixer preset names (e.g. '@PHP80Migration', '@PHP81Migration', '@PHPUnit84Migration'). If these presets don't exist in the pinned php-cs-fixer version, running php-cs-fixer will fail to load the config. Verify the correct preset names for the version being used and update them accordingly.
| '@PHP8x0Migration:risky' => true, | |
| '@PHP8x1Migration' => true, | |
| '@PHPUnit8x4Migration:risky' => true, | |
| '@PHP80Migration:risky' => true, | |
| '@PHP81Migration' => true, | |
| '@PHPUnit84Migration:risky' => true, |
| <!-- see https://www.tomasvotruba.cz/blog/2019/03/28/how-to-mock-final-classes-in-phpunit/ --> | ||
| <extensions> | ||
| <extension class="App\Tests\Hook\BypassFinalHook"/> | ||
| <bootstrap class="App\Tests\Extension\BypassFinalExtension"/> | ||
| <bootstrap class="Symfony\Bridge\PhpUnit\SymfonyExtension"/> | ||
| </extensions> |
There was a problem hiding this comment.
phpunit.xml.dist is updated for PHPUnit 12.x, but it still sets SYMFONY_PHPUNIT_VERSION=9.5 (earlier in this file). If CI (or local dev) uses Symfony's simple-phpunit, that env var will force PHPUnit 9.5 and clash with the PHPUnit 12 configuration/dependencies. Consider removing the env var or updating it to match the intended PHPUnit major version.
garak
force-pushed
the
phpunit-upgrade
branch
from
dd9915b to
26ad9b4
Compare
|
@copilot could you help me fix the three errors and the failure in phpunit tests? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/Badge/Model/UseCase/CreateCircleCiBadgeTest.php (1)
39-42: Helper method now ignores the$methodsparameter.The
createMockWithoutInvokingTheOriginalConstructormethod accepts a$methodsarray but no longer uses it. Multiple callers (lines 53-56, 91-94, 100-103, etc.) still pass method names that are silently ignored.This is technically correct for PHPUnit 12 since
createMock()allows stubbing any method, but consider either:
- Removing the unused
$methodsparameter and updating call sites, or- Inlining
createMock()calls directly since the helper no longer provides additional value♻️ Option 1: Remove unused parameter
- private function createMockWithoutInvokingTheOriginalConstructor(string $classname, array $methods = []): MockObject + private function createMockWithoutInvokingTheOriginalConstructor(string $classname): MockObject { return $this->createMock($classname); }Then update call sites to remove the method arrays.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Badge/Model/UseCase/CreateCircleCiBadgeTest.php` around lines 39 - 42, The helper createMockWithoutInvokingTheOriginalConstructor currently accepts a $methods parameter but ignores it; remove the unused parameter from the method signature and update all call sites that pass method arrays to stop supplying that argument (or replace those calls with direct createMock(...) if you prefer), ensuring references to createMockWithoutInvokingTheOriginalConstructor across tests are updated accordingly so there are no extra/unused arguments being passed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composer.json`:
- Line 43: Update the phpspec/prophecy-phpunit dependency in composer.json from
"^2.3" to a PHPUnit‑12 compatible version (use "^2.4" or preferably "^2.5") so
tests that use Prophecy (e.g., BadgeSubscriberTest, GitLabClientTest,
CreateComposerLockBadgeTest) work with PHPUnit 12; after changing the version
string for the package name "phpspec/prophecy-phpunit" run composer update
phpspec/prophecy-phpunit (or composer update) and rerun the test suite to
confirm the incompatibility is resolved.
---
Nitpick comments:
In `@tests/Badge/Model/UseCase/CreateCircleCiBadgeTest.php`:
- Around line 39-42: The helper createMockWithoutInvokingTheOriginalConstructor
currently accepts a $methods parameter but ignores it; remove the unused
parameter from the method signature and update all call sites that pass method
arrays to stop supplying that argument (or replace those calls with direct
createMock(...) if you prefer), ensuring references to
createMockWithoutInvokingTheOriginalConstructor across tests are updated
accordingly so there are no extra/unused arguments being passed.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
composer.lockis excluded by!**/*.locksymfony.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
.php-cs-fixer.dist.phpcomposer.jsonphpunit.xml.disttests/Badge/Model/BadgeTest.phptests/Badge/Model/ParseStabilityTest.phptests/Badge/Model/UseCase/CreateCircleCiBadgeTest.phptests/Badge/Model/UseCase/CreateComposerLockBadgeTest.phptests/Badge/Model/UseCase/CreateDependentsBadgeTest.phptests/Badge/Model/UseCase/CreateDownloadsBadgeTest.phptests/Badge/Model/UseCase/CreateLicenseBadgeTest.phptests/Badge/Model/UseCase/CreateRequireBadgeTest.phptests/Badge/Model/UseCase/CreateSuggestersBadgeTest.phptests/Badge/Model/UseCase/CreateVersionBadgeTest.phptests/Badge/Service/ClientStrategyTest.phptests/Badge/Service/TextNormalizerTest.phptests/Badge/ValueObject/RepositoryTest.phptests/Controller/HomeControllerTest.phptests/EventListener/BadgeLoggerSubscriberTest.phptests/Extension/BypassFinalExtension.phptests/Hook/BypassFinalHook.phptests/Service/SnippetGeneratorTest.php
💤 Files with no reviewable changes (1)
- tests/Hook/BypassFinalHook.php
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/Badge/Model/BadgeTest.php
- tests/Badge/ValueObject/RepositoryTest.php
- tests/Badge/Model/UseCase/CreateDownloadsBadgeTest.php
- tests/Badge/Model/UseCase/CreateDependentsBadgeTest.php
- tests/Badge/Model/UseCase/CreateVersionBadgeTest.php
- .php-cs-fixer.dist.php
- tests/Badge/Model/UseCase/CreateSuggestersBadgeTest.php
- tests/Badge/Service/TextNormalizerTest.php
- tests/Badge/Model/UseCase/CreateLicenseBadgeTest.php
- tests/Controller/HomeControllerTest.php
Co-authored-by: garak <179866+garak@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/Service/SnippetGeneratorTest.php (1)
172-214: Preserve argument validation inside the callback.The callback currently validates route names but mostly not required parameters (especially
repository), so incorrectgenerate()arguments can still pass this test. Consider asserting key params per route to keep the old test strictness.Proposed tightening
$router ->method('generate') ->willReturnCallback(function (string $name, array $params = [], int $referenceType = RouterInterface::ABSOLUTE_URL) { + if ('pugx_badge_packagist' !== $name && !isset($params['repository'])) { + throw new \InvalidArgumentException(\sprintf('Missing "repository" for route "%s".', $name)); + } + if ('pugx_badge_packagist' === $name) { return 'repo_url'; } if ('pugx_badge_version_latest' === $name) { + if (!\in_array($params['latest'] ?? 'stable', ['stable', 'unstable'], true)) { + throw new \InvalidArgumentException('Invalid "latest" value.'); + } return ($params['latest'] ?? 'stable') === 'unstable' ? 'img_url2' : 'img_url0'; } if ('pugx_badge_download_type' === $name) { + if (!\in_array($params['type'] ?? 'monthly', ['daily', 'monthly'], true)) { + throw new \InvalidArgumentException('Invalid "type" value for pugx_badge_download_type.'); + } return ($params['type'] ?? 'monthly') === 'daily' ? 'img_url5' : 'img_url4'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Service/SnippetGeneratorTest.php` around lines 172 - 214, The router mock's willReturnCallback closure in SnippetGeneratorTest currently only asserts route names; update that callback to also validate expected parameters per route (e.g., require a non-empty 'repository' param for 'pugx_badge_packagist' and any other repo-based routes, assert the presence/values of 'latest' for 'pugx_badge_version_latest' and 'type' for 'pugx_badge_download_type'), throwing an InvalidArgumentException on missing/invalid params so generate() call arguments are strictly enforced; keep the existing route-name checks (RouterInterface::ABSOLUTE_URL context) and map behavior for the remaining routes.tests/Badge/Model/UseCase/CreateComposerLockBadgeTest.php (1)
35-46: Consider unifying mocking approach.The setUp method mixes PHPUnit's
createMock(lines 37-38) with Prophecy'sprophesize(line 39). Other badge tests in this PR (e.g.,CreateLicenseBadgeTest,CreateRequireBadgeTest) use onlycreateMock. ConvertingClientStrategytocreateMockwould remove theProphecyTraitdependency and make this test consistent with the rest of the suite.♻️ Optional: Convert ClientStrategy to createMock
-use Prophecy\PhpUnit\ProphecyTrait;final class CreateComposerLockBadgeTest extends TestCase { - use ProphecyTrait; private CreateComposerLockBadge $useCase;protected function setUp(): void { $this->repository = $this->createMock(PackageRepositoryInterface::class); $this->client = $this->createMock(ClientInterface::class); - $clientStrategy = $this->prophesize(ClientStrategy::class); + $clientStrategy = $this->createMock(ClientStrategy::class); $repoUrl = 'https://github.com/user/repository'; $repositoryInfo = Repository::createFromRepositoryUrl($repoUrl); - $clientStrategy->getRepositoryPrefix($repositoryInfo, $repoUrl) - ->willReturn(''); - $this->useCase = new CreateComposerLockBadge($this->repository, $this->client, $clientStrategy->reveal()); + $clientStrategy->method('getRepositoryPrefix') + ->with($repositoryInfo, $repoUrl) + ->willReturn(''); + $this->useCase = new CreateComposerLockBadge($this->repository, $this->client, $clientStrategy); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Badge/Model/UseCase/CreateComposerLockBadgeTest.php` around lines 35 - 46, The test setUp mixes Prophecy and PHPUnit mocks which is inconsistent; replace the prophesize usage for ClientStrategy with a PHPUnit mock in the CreateComposerLockBadgeTest::setUp so all collaborators are created via createMock, configure the mock's getRepositoryPrefix($repositoryInfo, $repoUrl) expectation to return '' (matching the prophesize behavior), and pass that mock into new CreateComposerLockBadge($this->repository, $this->client, $clientStrategy) instead of using prophesize()/reveal().tests/Badge/Model/UseCase/CreateCircleCiBadgeTest.php (1)
39-41:$methodsis now unused and makes the helper misleading.
createMockWithoutInvokingTheOriginalConstructor()ignores the$methodsargument, but callers still pass method lists. Either remove the parameter (and simplify callers) or restore behavior that uses it.♻️ Proposed simplification
- /** - * `@param` array<int, mixed> $methods - */ - private function createMockWithoutInvokingTheOriginalConstructor(string $classname, array $methods = []): MockObject + private function createMockWithoutInvokingTheOriginalConstructor(string $classname): MockObject { return $this->createMock($classname); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Badge/Model/UseCase/CreateCircleCiBadgeTest.php` around lines 39 - 41, The helper createMockWithoutInvokingTheOriginalConstructor currently ignores the $methods parameter, making callers misleading; either remove the $methods parameter from createMockWithoutInvokingTheOriginalConstructor and update all callers to stop passing method lists, or restore intended behavior by using the mock builder API inside createMockWithoutInvokingTheOriginalConstructor (e.g., createMockBuilder($classname)->setMethods($methods)->disableOriginalConstructor()->getMock()) so the passed $methods are applied; update references to this function accordingly (search for createMockWithoutInvokingTheOriginalConstructor usages) and run tests to verify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/Badge/Model/UseCase/CreateCircleCiBadgeTest.php`:
- Around line 39-41: The helper createMockWithoutInvokingTheOriginalConstructor
currently ignores the $methods parameter, making callers misleading; either
remove the $methods parameter from
createMockWithoutInvokingTheOriginalConstructor and update all callers to stop
passing method lists, or restore intended behavior by using the mock builder API
inside createMockWithoutInvokingTheOriginalConstructor (e.g.,
createMockBuilder($classname)->setMethods($methods)->disableOriginalConstructor()->getMock())
so the passed $methods are applied; update references to this function
accordingly (search for createMockWithoutInvokingTheOriginalConstructor usages)
and run tests to verify.
In `@tests/Badge/Model/UseCase/CreateComposerLockBadgeTest.php`:
- Around line 35-46: The test setUp mixes Prophecy and PHPUnit mocks which is
inconsistent; replace the prophesize usage for ClientStrategy with a PHPUnit
mock in the CreateComposerLockBadgeTest::setUp so all collaborators are created
via createMock, configure the mock's getRepositoryPrefix($repositoryInfo,
$repoUrl) expectation to return '' (matching the prophesize behavior), and pass
that mock into new CreateComposerLockBadge($this->repository, $this->client,
$clientStrategy) instead of using prophesize()/reveal().
In `@tests/Service/SnippetGeneratorTest.php`:
- Around line 172-214: The router mock's willReturnCallback closure in
SnippetGeneratorTest currently only asserts route names; update that callback to
also validate expected parameters per route (e.g., require a non-empty
'repository' param for 'pugx_badge_packagist' and any other repo-based routes,
assert the presence/values of 'latest' for 'pugx_badge_version_latest' and
'type' for 'pugx_badge_download_type'), throwing an InvalidArgumentException on
missing/invalid params so generate() call arguments are strictly enforced; keep
the existing route-name checks (RouterInterface::ABSOLUTE_URL context) and map
behavior for the remaining routes.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/Badge/Model/UseCase/CreateCircleCiBadgeTest.phptests/Badge/Model/UseCase/CreateComposerLockBadgeTest.phptests/Service/SnippetGeneratorTest.php
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composer.json`:
- Line 119: The composer.json extra.symfony.require is pinned to "7.3.*" while
your Symfony packages (in require and require-dev) use ^7.4; update the
"extra.symfony.require" value to "7.4.*" so Flex selects Symfony 7.4-compatible
recipes — locate the extra.symfony.require entry in composer.json and change its
version constraint from 7.3.* to 7.4.* ensuring it matches the ^7.4 constraints
used elsewhere.
fix: correct PHPUnit mock setups in tests
Summary by CodeRabbit
Chores
Tests