Conversation
📝 WalkthroughWalkthroughDependency bumps in composer.json and test/tooling upgrades; phpunit.xml.dist migrated to PHPUnit 12 format; a PHPUnit BeforeTestHook was replaced by a PHPUnit Extension; tests modernized (PHP 8 attributes for data providers, createMock usage, instance expectation APIs, and related refactors). Changes
Sequence Diagram(s)sequenceDiagram
participant Config as phpunit.xml.dist
participant PHPUnit as PHPUnit Bootstrap
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
Note over PHPUnit,BypassFinals: Tests execute with finals bypassed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
| "dg/bypass-finals": "^1.9", | ||
| "friendsofphp/php-cs-fixer": "^3.89", | ||
| "jangregor/phpstan-prophecy": "^1.0", | ||
| "phpspec/prophecy-phpunit": "^2.3", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n composer.json | head -60Repository: PUGX/badge-poser
Length of output: 2528
🏁 Script executed:
#!/bin/bash
# Search for Prophecy usage in test files
echo "=== Searching for Prophecy usage in tests ==="
find tests -type f -name "*.php" 2>/dev/null | xargs grep -l "prophesize\|Prophecy\|->reveal()" 2>/dev/null | head -20 || echo "No test files with Prophecy found or tests dir not accessible"
echo ""
echo "=== Checking for specific Prophecy imports ==="
rg "use.*Prophecy|prophesize\(|->reveal\(\)" --type php || echo "No Prophecy patterns found"Repository: PUGX/badge-poser
Length of output: 2074
🌐 Web query:
phpspec/prophecy-phpunit version 2.3 PHPUnit 12 compatibility
💡 Result:
phpspec/prophecy-phpunit v2.3.0 is not PHPUnit 12 compatible (its Composer constraint only allows phpunit/phpunit ^9.1 || ^10.1 || ^11.0, not ^12.0). [1]
To use PHPUnit 12, upgrade to v2.4.0+ (adds ^12.0 support) [2] — or v2.5.0 (supports ^12.0 and ^13.0). [3]
Also note PHPUnit 12 itself requires PHP 8.3+. [4]
Sources: [1] https://raw.githubusercontent.com/phpspec/prophecy-phpunit/v2.3.0/composer.json, [2] https://raw.githubusercontent.com/phpspec/prophecy-phpunit/v2.4.0/composer.json, [3] https://raw.githubusercontent.com/phpspec/prophecy-phpunit/v2.5.0/composer.json, [4] https://phpunit.de/announcements/phpunit-12.html
Upgrade phpspec/prophecy-phpunit to v2.4.0+ for PHPUnit 12 compatibility.
The PR upgrades to PHPUnit 12.4 while keeping phpspec/prophecy-phpunit at ^2.3, but v2.3.0 only supports PHPUnit 9.1, 10.1, and 11.0—not 12.0. Tests actively use Prophecy (BadgeSubscriberTest.php, GitLabClientTest.php, CreateComposerLockBadgeTest.php), so this dependency cannot be removed. Upgrade phpspec/prophecy-phpunit to v2.4.0 or higher (v2.5.0 recommended) to resolve the incompatibility and prevent test runtime failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composer.json` at 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.
Summary by CodeRabbit
Chores
Tests