Extend tests of bcmath extension #11563

jorgsowa · 2023-06-29T22:37:29Z

I wanted to extend the BC library, but before that I thought about strengthening the test suite for the existing functionalities.

During those test I found some things which can be improved later.

I covered all of the possible types of numbers (accordingly to the functions):

different formats of zero (0, 0.0)
small integers and decimals
large integers and large decimals with large fractional part
negative numbers for all of the above ones

iluuu1994 · 2023-06-30T07:48:37Z

Thank you @jorgsowa! Maybe @Girgias would be interested to review this?

Girgias

Considering the 3 nested foreach loops are repeated in every test, I would rather have an included file that defines a function that takes the required arguments + function to execute + symbol to display.

Lot of missing EOLs before EOF.

Those tests are generate a lot of output, that I only skimmed through the actual results.

Girgias · 2023-06-30T13:53:30Z

ext/bcmath/tests/bcadd_zero.phpt

+$firstSummands = ["0", "0.00", "-0", "-0.00"];
+$secondSummands = array_merge($firstSummands, ["15", "-15", "1", "-9", "14.14", "-16.60", "0.15", "-0.01", "15151324141414.412312232141241", "-132132245132134.1515123765412", "141241241241241248267654747412", "-149143276547656984948124912", "0.1322135476547459213732911312", "-0.123912932193769965476541321"]);
+$scales = [0,10];


Please split that second array into multiple lines or have it as a constant at the beginning of the file.

As this line is just huge.

Girgias · 2023-06-30T13:54:34Z

ext/bcmath/tests/bcadd_zero.phpt

+	foreach($firstSummands as $firstSummand) {
+    	echo "Number \"$firstSummand\" (scale $scale)\n";
+		foreach($secondSummands as $secondSummand) {
+			echo str_pad($firstSummand, 6, ' ', STR_PAD_LEFT), " + ", str_pad($secondSummand, 30), ' = ', bcadd($firstSummand, $secondSummand, $scale),"\n";


Please use constants for the padded length.

Girgias · 2023-06-30T13:55:33Z

ext/bcmath/tests/bccomp.phpt

 ?>
 --EXPECT--
 -1
 -1
 0
 1
+-1
+1
+0


EOL before EOF

Girgias · 2023-06-30T13:55:51Z

ext/bcmath/tests/bccomp_variation001.phpt

 ?>
 --EXPECT--
 0
 1
 -1
+1


EOL before EOF

Girgias · 2023-06-30T13:56:04Z

ext/bcmath/tests/bccomp_variation002.phpt

@@ -20,3 +21,4 @@ echo bccomp("-2.29", "-2.3", "1");
 -1
 1
 1
+-1


EOL before EOF

Girgias · 2023-06-30T14:15:45Z

ext/bcmath/tests/bcmul.phpt

+$firstFactors = ["15", "-15", "1", "-9", "14.14", "-16.60", "0.15", "-0.01"];
+$secondFactors = array_merge($firstFactors, ["0", "0.00", "-0", "-0.00", "15151324141414.412312232141241", "-132132245132134.1515123765412", "141241241241241248267654747412", "-149143276547656984948124912", "0.1322135476547459213732911312", "-0.123912932193769965476541321"]);
+$scales = [0,10];
+
+foreach($scales as $scale) {
+	foreach($firstFactors as $firstFactor) {
+    	echo "Number \"$firstFactor\" (scale $scale)\n";
+		foreach($secondFactors as $secondFactor) {
+			echo str_pad($firstFactor, 6, ' ', STR_PAD_LEFT), " × " , str_pad($secondFactor, 30), ' = ', bcmul($firstFactor, $secondFactor, $scale),"\n";
+		}
+		echo "\n";
+	}
+}


Girgias · 2023-06-30T14:15:56Z

ext/bcmath/tests/bcmul.phpt

+ -0.01 × 141241241241241248267654747412 = -1412412412412412482676547474.1200000000
+ -0.01 × -149143276547656984948124912   = 1491432765476569849481249.1200000000
+ -0.01 × 0.1322135476547459213732911312 = -0.0013221354
+ -0.01 × -0.123912932193769965476541321 = 0.0012391293


Girgias · 2023-06-30T14:16:40Z

ext/bcmath/tests/bcadd_large_numbers.phpt

+$firstSummands = ["15151324141414.412312232141241", "-132132245132134.1515123765412", "141241241241241248267654747412", "-149143276547656984948124912", "0.1322135476547459213732911312", "-0.123912932193769965476541321"];
+$secondSummands = array_merge($firstSummands, ["0", "0.00", "-0", "-0.00", "15", "-15", "1", "-9", "14.14", "-16.60", "0.15", "-0.01"]);
+$scales = [0,10];
+
+foreach($scales as $scale) {
+	foreach($firstSummands as $firstSummand) {
+    	echo "Number \"$firstSummand\" (scale $scale)\n";
+		foreach($secondSummands as $secondSummand) {
+			echo $firstSummand, " + ", str_pad($secondSummand, 30), ' = ', bcadd($firstSummand, $secondSummand, $scale),"\n";
+		}
+		echo "\n";
+	}
+}
+


Girgias · 2023-06-30T14:17:14Z

ext/bcmath/tests/bcadd.phpt

+$firstSummands = ["15", "-15", "1", "-9", "14.14", "-16.60", "0.15", "-0.01"];
+$secondSummands = array_merge($firstSummands, ["0", "0.00", "-0", "-0.00", "15151324141414.412312232141241", "-132132245132134.1515123765412", "141241241241241248267654747412", "-149143276547656984948124912", "0.1322135476547459213732911312", "-0.123912932193769965476541321"]);
+$scales = [0,10];
+
+foreach($scales as $scale) {
+	foreach($firstSummands as $firstSummand) {
+    	echo "Number \"$firstSummand\" (scale $scale)\n";
+		foreach($secondSummands as $secondSummand) {
+			echo str_pad($firstSummand, 6, ' ', STR_PAD_LEFT), " + ", str_pad($secondSummand, 30), ' = ', bcadd($firstSummand, $secondSummand, $scale),"\n";
+		}
+		echo "\n";
+	}
+}


Girgias · 2023-06-30T14:17:24Z

ext/bcmath/tests/bcadd.phpt

+ -0.01 + 141241241241241248267654747412 = 141241241241241248267654747411.9900000000
+ -0.01 + -149143276547656984948124912   = -149143276547656984948124912.0100000000
+ -0.01 + 0.1322135476547459213732911312 = 0.1222135476
+ -0.01 + -0.123912932193769965476541321 = -0.1339129321


jorgsowa · 2023-07-01T22:40:28Z

@Girgias, thank you for the comprehensive review. I applied the changes and I will focus more on a proper formatting next time. I hope that extended tests will help with refactoring of this library within your existing PR: #10774

Girgias · 2023-07-03T04:12:47Z

ext/bcmath/tests/run_bcmath_tests_function.inc

+    }
+}
+
+?>


To prevent output happening

Suggested change

?>

Girgias · 2023-07-03T04:16:04Z

ext/bcmath/tests/bcpow.phpt

-echo bcpow("2", "64"),"\n";
-echo bcpow("-2.555", "5", 2),"\n";
+
+$exponents = ["15", "-15", "1", "-9", "0", "-0", "252", "-112"];


Seems the large exponents where moved to another test but not dropped here?

Girgias · 2023-07-03T04:17:29Z

ext/bcmath/tests/bcmul_large_numbers.phpt

+    "0",
+    "0.00",
+    "-0",
+    "-0.00",


As multiplying with 0 is already done in another test those should be removed IMHO

Girgias · 2023-07-03T04:18:03Z

ext/bcmath/tests/bcmul.phpt

+    "0",
+    "0.00",
+    "-0",
+    "-0.00",


Girgias · 2023-07-03T04:18:58Z

ext/bcmath/tests/bcdiv_zero.phpt

+-0.00 / 141241241241241248267654747412 = 0.0000000000
+-0.00 / -149143276547656984948124912   = 0.0000000000
+-0.00 / 0.1322135476547459213732911312 = 0.0000000000
+-0.00 / -0.123912932193769965476541321 = 0.0000000000


EOL before EOF

Girgias · 2023-07-03T04:20:08Z

ext/bcmath/tests/bccomp_variation002.phpt

+echo bccomp("-2.29", "-2.3", "1")."\n";
+echo bccomp("-2.29", "0", "1");


Suggested change

echo bccomp("-2.29", "-2.3", "1")."\n";

echo bccomp("-2.29", "0", "1");

echo bccomp("-2.29", "-2.3", "1")."\n";

echo bccomp("-2.29", "0", "1")."\n";

Just to be consistent.

Girgias · 2023-07-03T04:20:29Z

ext/bcmath/tests/bccomp_variation001.phpt

+echo bccomp("2.29", "2.3", "2")."\n";
+echo bccomp("2.29", "0", "2");


Suggested change

echo bccomp("2.29", "2.3", "2")."\n";

echo bccomp("2.29", "0", "2");

echo bccomp("2.29", "2.3", "2")."\n";

echo bccomp("2.29", "0", "2")."\n";

Just to be consistent

Girgias · 2023-07-03T04:20:48Z

ext/bcmath/tests/bccomp.phpt

+echo bccomp("1", "0"), "\n";
+echo bccomp("0.000", "0", 3);


Suggested change

echo bccomp("1", "0"), "\n";

echo bccomp("0.000", "0", 3);

echo bccomp("1", "0"), "\n";

echo bccomp("0.000", "0", 3), "\n";

Consistency

Girgias · 2023-07-03T04:21:24Z

ext/bcmath/tests/bcadd_large_numbers.phpt

+    "0",
+    "0.00",
+    "-0",
+    "-0.00",


We're already testing 0 in another test

We test it, but as a first arguments of the function. In this test we pass zeroes only as second argument of the function. The same with other tests. Do you think it's too much coverage and should be removed?

Ah right, should be fine then.

ext/bcmath/tests/bcadd.phpt

Girgias

Did you forget to push the changes while asking for a rereview?

jorgsowa · 2023-07-04T21:41:57Z

@Girgias, thank you for the review. I pushed another small set of improvements. Found some duplicated values in tests. I'm not sure about removing zeroes from other tests, but if you think it's not needed then I can remove it.

Girgias · 2023-07-05T16:43:38Z

Thank you!

jorgsowa added 6 commits June 27, 2023 23:20

Extended tests of bcmath function

1e7e2cc

Extended bcmath tests

3170607

Extend tests

2d873e6

Merge remote-tracking branch 'origin/master' into extend_bcmath_tests

be22a59

Removed simple tests which are covered already

a5a312d

Improved the code structure and the naming of the tests

fcccfb6

github-actions bot added the Extension: bcmath label Jun 29, 2023

Girgias requested changes Jun 30, 2023

View reviewed changes

jorgsowa added 5 commits July 1, 2023 21:07

Added EOLs on the EOF

8dfaf52

reformatted all the PHP code in tests

5e58636

Changed symbol of division from ÷ to /

898a561

Extract large exponents from bcpow.phpt into separate test file

2dc0cdf

Extracted function used in tests to separate file

72e2499

jorgsowa added 3 commits July 2, 2023 23:26

Updated the reqquire statement

f446ce4

Merge branch 'php:master' into extend_bcmath_tests

4f28e40

Fixed include path of helper function

79e5c80

Girgias reviewed Jul 3, 2023

View reviewed changes

jorgsowa requested a review from Girgias July 3, 2023 06:50

Girgias reviewed Jul 3, 2023

View reviewed changes

jorgsowa added 3 commits July 4, 2023 23:22

Improved formatting of the tests

6bf9713

Improved formatting of the tests

3622e9c

Removed repeated test numbers

2da884a

jorgsowa requested a review from Girgias July 4, 2023 21:42

Girgias merged commit ee22612 into php:master Jul 5, 2023

@@ @@ -20,3 +21,4 @@ echo bccomp("-2.29", "-2.3", "1"); @@
               -1
 
 
+              -1

+                  }
+              }
+              ?>

		echo bccomp("-2.29", "-2.3", "1")."\n";
		echo bccomp("-2.29", "0", "1");

		echo bccomp("2.29", "2.3", "2")."\n";
		echo bccomp("2.29", "0", "2");

Extend tests of bcmath extension #11563

Extend tests of bcmath extension #11563

Uh oh!

Conversation

jorgsowa commented Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iluuu1994 commented Jun 30, 2023

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgsowa commented Jul 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgsowa Jul 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

jorgsowa commented Jul 4, 2023

Uh oh!

Girgias commented Jul 5, 2023

Uh oh!

Uh oh!

jorgsowa commented Jun 29, 2023 •

edited

Loading

jorgsowa Jul 4, 2023 •

edited

Loading