HP-2446: format number as string#98
Conversation
|
""" WalkthroughThe update modifies the calculation logic in the Changes
Poem
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (1)
src/price/ProgressivePrice.php (1)
107-110: Improved numeric precision in monetary calculations.The change to format quantity and multiplier values as fixed-point decimal strings with 14 decimal places will help ensure consistent precision in financial calculations. This addresses potential floating-point precision issues that might occur with direct string casting.
However, note that the
(string)cast aftersprintfis redundant sincesprintfalready returns a string. While this doesn't affect functionality, it could be simplified:$chargedAmount = $price->money() - ->multiply((string)(sprintf('%.14F', $billedUsage->getQuantity()))) - ->divide((string)(sprintf('%.14F', $price->multiplier()))); + ->multiply(sprintf('%.14F', $billedUsage->getQuantity())) + ->divide(sprintf('%.14F', $price->multiplier()));
| ->multiply((string)(sprintf('%.14F', $billedUsage->getQuantity()))) | ||
| ->divide((string)(sprintf('%.14F', $price->multiplier()))); |
There was a problem hiding this comment.
Please, cover this case with a unit test in https://github.com/hiqdev/php-billing/blob/master/tests/unit/price/ProgressivePriceTest.php
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/unit/price/ProgressivePriceTest.php (3)
228-234: Consider adding assertions for the correct unitSince this test specifically focuses on very small usage values (bps), it's important to verify that the unit is preserved or converted correctly throughout the calculation process.
$this->usage = Quantity::bps(6043); $usage = $price->calculateUsage($this->usage); + // Assert that the unit is preserved (or converted as expected) + $this->assertSame('bps', $usage->getUnit()->getName(), 'Usage unit should be preserved'); $this->assertSame($this->usage->getQuantity(), $usage->getQuantity());
241-254: Enhance test coverage with more edge cases for small usage valuesThe current data provider only contains one test case. Consider adding more test cases with varying small usage values to thoroughly test the formatting behavior.
private function progressivePriceProviderSmallUsage(): Generator { yield 'Simple case' => [ 'thresholds' => [ ['price' => '10', 'currency' => 'EUR', 'quantity' => '0', 'unit' => 'gbps'], ], 'money' => 0, 'price' => '1', 'prepaid' => '0', 'trace' => [ '6043bps * 0.00000001 = 0.00', ], ]; + + yield 'Edge case - ultra small usage' => [ + 'thresholds' => [ + ['price' => '10', 'currency' => 'EUR', 'quantity' => '0', 'unit' => 'gbps'], + ], + 'money' => 0, + 'price' => '1', + 'prepaid' => '0', + 'trace' => [ + '1bps * 0.00000001 = 0.00', + ], + ]; + + yield 'Edge case - slightly higher usage' => [ + 'thresholds' => [ + ['price' => '10', 'currency' => 'EUR', 'quantity' => '0', 'unit' => 'gbps'], + ], + 'money' => 1, // Adjust expected amount based on actual calculation + 'price' => '1', + 'prepaid' => '0', + 'trace' => [ + '1000000bps * 0.00000001 = 0.01', + ], + ]; }
238-238: Add trace verification for small usage caseThe test for small usage doesn't verify the calculation trace, unlike the other test methods. Consider adding trace verification to ensure the calculation steps are correctly recorded.
$amount = $price->calculateSum($this->usage); $this->assertEquals($expectedAmount, $amount->getAmount()); + + // Verify calculation trace + $trace = array_map(fn($trace) => $trace->__toString(), $price->getCalculationTraces()); + $this->assertSame($inputThresholdsArray['trace'] ?? [], $trace);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/price/ProgressivePriceTest.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/unit/price/ProgressivePriceTest.php (2)
src/price/PriceFactory.php (1)
createProgressivePrice(95-100)src/price/ProgressivePrice.php (2)
calculateUsage(50-59)calculateSum(84-121)
🪛 GitHub Actions: phpunit-tests
tests/unit/price/ProgressivePriceTest.php
[error] 235-235: PHPUnit test failure: Failed asserting that 0.006043 is identical to 6043 in testProgressivePriceSmallUsage.
| /** | ||
| * @dataProvider progressivePriceProviderSmallUsage | ||
| */ | ||
| public function testProgressivePriceSmallUsage( | ||
| array $inputThresholdsArray, | ||
| int $expectedAmount, | ||
| string $startPrice, | ||
| string $prepaid = '0' | ||
| ): void { | ||
| $price = $this->createProgressivePrice( | ||
| prepaid: $prepaid, | ||
| startPrice: $startPrice, | ||
| thresholdsArray: $inputThresholdsArray | ||
| ); | ||
| $this->usage = Quantity::bps(6043); | ||
| $usage = $price->calculateUsage($this->usage); | ||
| $this->assertSame($this->usage->getQuantity(), $usage->getQuantity()); | ||
|
|
||
| $amount = $price->calculateSum($this->usage); | ||
| $this->assertEquals($expectedAmount, $amount->getAmount()); | ||
| } |
There was a problem hiding this comment.
Fix the assertion for quantity comparison in testProgressivePriceSmallUsage
The test is failing with the error: "Failed asserting that 0.006043 is identical to 6043 in testProgressivePriceSmallUsage." This indicates that when you retrieve the quantity from the usage object, unit conversion is happening (bps to gbps or another unit).
To fix this, you should compare the unit types as well or use a different assertion method that accounts for unit conversion:
- $this->assertSame($this->usage->getQuantity(), $usage->getQuantity());
+ // Option 1: Check that the quantities represent the same value
+ $this->assertEquals($this->usage->getQuantity(), $usage->getQuantity(), '', 0.0001);
+ // Option 2: Or ensure they use the same unit before comparison
+ // $this->assertSame($this->usage->getUnit()->getName(), $usage->getUnit()->getName());
+ // $this->assertSame($this->usage->getQuantity(), $usage->getQuantity());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * @dataProvider progressivePriceProviderSmallUsage | |
| */ | |
| public function testProgressivePriceSmallUsage( | |
| array $inputThresholdsArray, | |
| int $expectedAmount, | |
| string $startPrice, | |
| string $prepaid = '0' | |
| ): void { | |
| $price = $this->createProgressivePrice( | |
| prepaid: $prepaid, | |
| startPrice: $startPrice, | |
| thresholdsArray: $inputThresholdsArray | |
| ); | |
| $this->usage = Quantity::bps(6043); | |
| $usage = $price->calculateUsage($this->usage); | |
| $this->assertSame($this->usage->getQuantity(), $usage->getQuantity()); | |
| $amount = $price->calculateSum($this->usage); | |
| $this->assertEquals($expectedAmount, $amount->getAmount()); | |
| } | |
| /** | |
| * @dataProvider progressivePriceProviderSmallUsage | |
| */ | |
| public function testProgressivePriceSmallUsage( | |
| array $inputThresholdsArray, | |
| int $expectedAmount, | |
| string $startPrice, | |
| string $prepaid = '0' | |
| ): void { | |
| $price = $this->createProgressivePrice( | |
| prepaid: $prepaid, | |
| startPrice: $startPrice, | |
| thresholdsArray: $inputThresholdsArray | |
| ); | |
| $this->usage = Quantity::bps(6043); | |
| $usage = $price->calculateUsage($this->usage); | |
| // Option 1: Check that the quantities represent the same value | |
| $this->assertEquals($this->usage->getQuantity(), $usage->getQuantity(), '', 0.0001); | |
| // Option 2: Or ensure they use the same unit before comparison | |
| // $this->assertSame($this->usage->getUnit()->getName(), $usage->getUnit()->getName()); | |
| // $this->assertSame($this->usage->getQuantity(), $usage->getQuantity()); | |
| $amount = $price->calculateSum($this->usage); | |
| $this->assertEquals($expectedAmount, $amount->getAmount()); | |
| } |
🧰 Tools
🪛 GitHub Actions: phpunit-tests
[error] 235-235: PHPUnit test failure: Failed asserting that 0.006043 is identical to 6043 in testProgressivePriceSmallUsage.
| ); | ||
| $this->usage = Quantity::bps(6043); | ||
| $usage = $price->calculateUsage($this->usage); | ||
| $this->assertSame($this->usage->getQuantity(), $usage->getQuantity()); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix the assertion error in the pipeline test failure
The pipeline is reporting a failure with: "Failed asserting that 0.006043 is identical to 6043". This suggests that quantity values are being converted between different units during processing.
Let's investigate how the different units are being handled in the codebase:
🏁 Script executed:
#!/bin/bash
# Search for Quantity class implementation to understand unit handling
rg -A 10 -B 5 "class Quantity"
# Search for where unit conversions might happen
rg -A 5 -B 5 "convert|getUnit|getQuantity"
# Search for how bps is defined or used
rg -A 3 "bps|Quantity::bps"Length of output: 56761
Update ProgressivePriceTest to assert converted usage correctly
In tests/unit/price/ProgressivePriceTest.php at line 235, the assertion is comparing the original bps quantity (6043) against the converted mbps quantity (0.006043), which will always fail. You should assert the converted value (in the price’s prepaid unit) with a small delta for floating-point precision:
• File: tests/unit/price/ProgressivePriceTest.php
Line: 235
Replace:
$this->assertSame(
$this->usage->getQuantity(),
$usage->getQuantity()
);With:
$this->assertEqualsWithDelta(
// expected: original usage converted into the price’s prepaid unit
$this->usage->convert($price->getPrepaid()->getUnit())->getQuantity(),
$usage->getQuantity(),
1e-7,
'Usage should be converted to the price’s prepaid unit before assertion'
);This ensures you’re comparing the correctly converted floating-point value rather than the raw integer and accounts for any minor rounding differences.
🧰 Tools
🪛 GitHub Actions: phpunit-tests
[error] 235-235: PHPUnit test failure: Failed asserting that 0.006043 is identical to 6043 in testProgressivePriceSmallUsage.
2 participants
Summary by CodeRabbit