HP-2818: Fix falling PHPUnit tests during build php-billing package #110
Conversation
WalkthroughThis pull request systematically modernizes PHPUnit test infrastructure across multiple test files by converting from docblock-based Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![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 (1)
tests/unit/price/ProgressivePriceTest.php (1)
43-43: Consider importing the DataProvider attribute for cleaner syntax.The fully qualified class name works, but importing the attribute would reduce verbosity across all test methods.
Add this import at the top of the file with other imports:
use PHPUnit\Framework\Attributes\DataProvider;Then simplify the attribute usage:
- #[\PHPUnit\Framework\Attributes\DataProvider('progressivePriceProvider')] + #[DataProvider('progressivePriceProvider')]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
tests/unit/Money/MultipliedMoneyTest.php(2 hunks)tests/unit/action/UsageIntervalTest.php(7 hunks)tests/unit/charge/modifiers/OnceTest.php(3 hunks)tests/unit/charge/modifiers/addons/DiscountTest.php(1 hunks)tests/unit/formula/FormulaEngineTest.php(2 hunks)tests/unit/price/ProgressivePriceTest.php(11 hunks)tests/unit/price/ProgressivePriceThresholdTest.php(2 hunks)tests/unit/price/ProgressivePriceThresholdsTest.php(1 hunks)tests/unit/price/SumsTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-29T10:44:02.367Z
Learnt from: SilverFire
Repo: hiqdev/php-billing PR: 81
File: src/action/UsageInterval.php:93-106
Timestamp: 2024-10-29T10:44:02.367Z
Learning: Tests should cover invalid parameter values, such as for the `$fractionOfMonth` parameter in `withMonthAndFraction` method.
Applied to files:
tests/unit/charge/modifiers/OnceTest.phptests/unit/action/UsageIntervalTest.php
🧬 Code graph analysis (5)
tests/unit/price/ProgressivePriceThresholdsTest.php (1)
src/price/ProgressivePriceThresholdList.php (1)
ProgressivePriceThresholdList(12-150)
tests/unit/charge/modifiers/addons/DiscountTest.php (3)
src/charge/modifiers/addons/Discount.php (4)
Discount(30-188)getValue(47-50)multiply(106-113)add(115-129)tests/unit/charge/modifiers/DiscountTest.php (1)
setUp(24-28)src/charge/modifiers/Modifier.php (1)
discount(62-65)
tests/unit/price/ProgressivePriceThresholdTest.php (1)
src/price/ProgressivePriceThreshold.php (2)
price(55-58)ProgressivePriceThreshold(15-90)
tests/unit/price/SumsTest.php (5)
tests/unit/Money/MultipliedMoneyTest.php (1)
PHPUnit(21-29)tests/unit/action/UsageIntervalTest.php (4)
PHPUnit(24-43)PHPUnit(46-59)PHPUnit(141-152)PHPUnit(165-175)tests/unit/charge/modifiers/OnceTest.php (2)
PHPUnit(43-50)PHPUnit(65-71)tests/unit/charge/modifiers/addons/DiscountTest.php (2)
PHPUnit(68-73)PHPUnit(97-102)tests/unit/price/ProgressivePriceTest.php (4)
PHPUnit(43-62)PHPUnit(64-86)PHPUnit(88-135)PHPUnit(218-236)
tests/unit/action/UsageIntervalTest.php (5)
tests/unit/charge/modifiers/addons/DiscountTest.php (2)
PHPUnit(68-73)PHPUnit(97-102)tests/unit/formula/FormulaEngineTest.php (2)
PHPUnit(89-93)PHPUnit(95-99)tests/unit/price/ProgressivePriceTest.php (4)
PHPUnit(43-62)PHPUnit(64-86)PHPUnit(88-135)PHPUnit(218-236)tests/unit/price/ProgressivePriceThresholdTest.php (2)
PHPUnit(20-64)PHPUnit(23-42)tests/unit/price/ProgressivePriceThresholdsTest.php (1)
PHPUnit(17-176)
🪛 PHPMD (2.15.0)
tests/unit/price/ProgressivePriceTest.php
91-91: Avoid unused parameters such as '$expectedAmount'. (undefined)
(UnusedFormalParameter)
94-94: Avoid unused parameters such as '$expectedTrace'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (35)
tests/unit/price/ProgressivePriceThresholdsTest.php (1)
17-17: LGTM: CoversClass attribute migration.The migration from docblock
@coversannotation to PHP 8CoversClassattribute is correct and aligns with modern PHPUnit best practices.tests/unit/Money/MultipliedMoneyTest.php (2)
21-21: LGTM: DataProvider attribute migration.The migration to PHP 8
DataProviderattribute is correct.
31-31: LGTM: Data provider converted to static method.Converting the data provider to a static method is required when using PHP 8 attribute-based data provider linking. This change is correct.
tests/unit/price/ProgressivePriceThresholdTest.php (3)
20-20: LGTM: CoversClass attribute migration.Correct migration from docblock annotation to PHP 8 attribute.
23-23: LGTM: DataProvider attribute applied.Correct usage of the PHP 8
DataProviderattribute.
59-59: LGTM: Data provider converted to static.The conversion to a static method is necessary for attribute-based data provider linking.
tests/unit/price/SumsTest.php (2)
40-40: LGTM: DataProvider attribute migration.Correctly migrated to PHP 8 attribute-based data provider linking.
48-48: LGTM: Static data provider with explicit return type.The conversion to a static method with an explicit
arrayreturn type is correct and improves type safety.tests/unit/formula/FormulaEngineTest.php (4)
75-75: LGTM: Data provider converted to static.Correct conversion to support attribute-based linking.
89-89: LGTM: DataProvider attribute applied.Correctly migrated to PHP 8 attribute for data provider linking.
95-95: LGTM: DataProvider attribute applied.Correctly migrated to PHP 8 attribute for data provider linking.
101-101: LGTM: Data provider converted to static.Correct conversion to support attribute-based linking.
tests/unit/charge/modifiers/OnceTest.php (4)
43-43: LGTM: DataProvider attribute applied.Correctly migrated to PHP 8 attribute-based data provider linking.
52-52: LGTM: Data provider converted to static with explicit return type.The conversion to a public static method with an explicit
arrayreturn type is correct.
65-65: LGTM: DataProvider attribute applied.Correctly migrated to PHP 8 attribute-based data provider linking.
73-73: LGTM: Data provider visibility and signature updated.The conversion from
private functiontopublic static functionwith aniterablereturn type is necessary for attribute-based data provider linking. PHPUnit requires data providers to be public and static when using attributes.tests/unit/action/UsageIntervalTest.php (7)
24-24: LGTM: DataProvider attribute applied.Correctly migrated to PHP 8 attribute-based data provider linking.
46-46: LGTM: DataProvider attribute applied.Correctly migrated to PHP 8 attribute-based data provider linking.
61-61: LGTM: Data provider converted to static.Correct conversion to support attribute-based linking.
141-141: LGTM: DataProvider attribute applied.Correctly migrated to PHP 8 attribute-based data provider linking.
154-154: LGTM: Data provider converted to static.Correct conversion to support attribute-based linking.
165-165: LGTM: DataProvider attribute applied.Correctly migrated to PHP 8 attribute-based data provider linking.
177-177: LGTM: Data provider converted to static with explicit return type.The conversion to a static method with an explicit
arrayreturn type is correct and improves type safety.tests/unit/charge/modifiers/addons/DiscountTest.php (8)
21-23: LGTM: Constants introduced for test data.Using constants for test values improves maintainability and makes the tests more readable.
25-27: LGTM: Properties declared with explicit types.Adding typed properties for test fixtures improves type safety and clarity.
29-45: LGTM: setUp refactored to use factory methods.Extracting factory methods
createAbsoluteDiscountandcreateRelativeDiscountreduces duplication and improves test maintainability. The static methods can be reused in data providers.
47-59: LGTM: Test methods updated with void return types and constants.Adding explicit
voidreturn types and using theself::RATEandself::SUMconstants improves type safety and maintainability.
61-73: LGTM: Data provider migration with attribute.The
badMultipliersdata provider is correctly converted to a static method and linked via the PHP 8DataProviderattribute. The explicititerablereturn type andvoidreturn type on the test method are correct.
75-80: LGTM: Test updated with void return type and constants.Correct usage of
voidreturn type and constants for improved type safety and maintainability.
82-102: LGTM: Data provider creates instances inline.Since data providers must be static, the
badAddendsprovider correctly creates its ownDiscountinstances using the static factory methods. This is the proper approach for static data providers that need object instances.
104-109: LGTM: Test method updated with void return type.Adding the explicit
voidreturn type improves type safety.tests/unit/price/ProgressivePriceTest.php (4)
44-62: LGTM!The test method correctly uses the PHP 8 DataProvider attribute, and the parameter renaming is consistent with the data provider output keys.
64-86: LGTM!Correctly modernized with PHP 8 DataProvider attribute and consistent parameter naming.
137-216: LGTM!The data provider is correctly converted to
public staticas required by PHPUnit 10+, and the key naming is consistent across all test cases.
218-248: LGTM!The test and its dedicated data provider are correctly modernized with PHP 8 attributes and static visibility.
| #[\PHPUnit\Framework\Attributes\DataProvider('progressivePriceProvider')] | ||
| public function testProgressivePriceSerialization( | ||
| array $inputThresholdsArray, | ||
| int $expectedAmount, | ||
| string $startPrice, | ||
| string $prepaid = '0' | ||
| string $prepaid = '0', | ||
| array $expectedTrace = [], | ||
| ): void { |
There was a problem hiding this comment.
Unused parameters $expectedAmount and $expectedTrace flagged by static analysis.
These parameters are received from the shared data provider but are not used in this serialization test. While reusing progressivePriceProvider is convenient, the unused parameters add noise.
Options to address:
- Accept as-is – Since this is test code reusing a shared data provider, the trade-off for reduced duplication may be acceptable.
- Create a dedicated provider – Extract a minimal
progressivePriceSerializationProvider()that only yields the requiredinputThresholdsArray,startPrice, andprepaidvalues.
If keeping the current approach, consider adding a brief comment explaining the intentional parameter reuse.
🧰 Tools
🪛 PHPMD (2.15.0)
91-91: Avoid unused parameters such as '$expectedAmount'. (undefined)
(UnusedFormalParameter)
94-94: Avoid unused parameters such as '$expectedTrace'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In tests/unit/price/ProgressivePriceTest.php around lines 88 to 95, the test
method testProgressivePriceSerialization declares parameters $expectedAmount and
$expectedTrace that come from the shared data provider but are unused,
triggering static analysis noise; fix by replacing the shared provider with a
dedicated data provider (e.g., progressivePriceSerializationProvider) that
yields only the needed values (inputThresholdsArray, startPrice, prepaid) and
update the method signature accordingly, or if you prefer to keep the shared
provider, remove the unused parameters from the method signature and add a short
comment explaining the intentional reuse to silence analysis.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.