Skip to content

HP-2446: format number as string #98

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
SilverFire merged 2 commits into from
May 7, 2025
Merged

HP-2446: format number as string #98

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a4a4ebe
HP-2446: format number as string
bladeroot Apr 22, 2025
b8a622b
Fix test
bladeroot May 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/price/ProgressivePrice.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,8 @@ public function calculateSum(QuantityInterface $quantity): ?Money
$price = $threshold->price();

$chargedAmount = $price->money()
->multiply((string)$billedUsage->getQuantity())
->divide((string)($price->multiplier()));
->multiply((string)(sprintf('%.14F', $billedUsage->getQuantity())))
->divide((string)(sprintf('%.14F', $price->multiplier())));
Comment on lines +108 to +109
Copy link
Copy Markdown
Member

@SilverFire SilverFire Apr 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, cover this case with a unit test in https://github.com/hiqdev/php-billing/blob/master/tests/unit/price/ProgressivePriceTest.php


$this->calculationTraces[] = new ProgressivePriceCalculationTrace(
$threshold, $billedUsage, $chargedAmount
Expand Down
37 changes: 37 additions & 0 deletions tests/unit/price/ProgressivePriceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,4 +215,41 @@ private function progressivePriceProvider(): Generator
],
];
}

/**
* @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());
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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.


$amount = $price->calculateSum($this->usage);
$this->assertEquals($expectedAmount, $amount->getAmount());
}
Comment on lines +219 to +239
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
/**
* @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.


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',
],
];
}
}
Loading