Initialize unit tests #2
Conversation
src/Vector3.php
Outdated
| @@ -115,6 +115,10 @@ public function multiply(float $number) : Vector3{ | |||
| } | |||
|
|
|||
| public function divide(float $number) : Vector3{ | |||
| if($number === 0.0){ | |||
| throw new \InvalidArgumentException('Division by zero'); | |||
There was a problem hiding this comment.
This isn't necessary - PHP 7.x will raise a DivisionByZeroError.
src/Vector2.php
Outdated
| @@ -99,6 +99,10 @@ public function multiply(float $number) : Vector2{ | |||
| } | |||
|
|
|||
| public function divide(float $number) : Vector2{ | |||
| if($number === 0.0){ | |||
| throw new \InvalidArgumentException('Division by zero'); | |||
There was a problem hiding this comment.
This isn't necessary - PHP 7.x will raise a DivisionByZeroError.
src/Math.php
Outdated
| @@ -57,6 +57,9 @@ public static function solveQuadratic(float $a, float $b, float $c) : array{ | |||
| (-$b - $sqrtDiscriminant) / (2 * $a) | |||
| ]; | |||
| }elseif($discriminant == 0){ //1 real root | |||
| if($a === 0.0){ | |||
| throw new \InvalidArgumentException('Division by zero'); | |||
There was a problem hiding this comment.
This isn't necessary - PHP 7.x will raise a DivisionByZeroError.
composer.json
Outdated
| }, | ||
| }, | ||
| "require-dev": { | ||
| "phpunit/phpunit": "^6.5" |
There was a problem hiding this comment.
We prefer using a phar for PHPUnit. Also, this can be raised to PHPUnit 7 since this library is strictly PHP 7.2 and upwards only.
tests/AxisAlignedBBTest.php
Outdated
| public function testConstructor(){ | ||
| $axis = new AxisAlignedBB(1.1, 1.2, 1.3, 2.1, 2.2, 2.3); | ||
|
|
||
| $this->assertInstanceOf(AxisAlignedBB::class, $axis); |
There was a problem hiding this comment.
This seems redundant. new ... will always return an instance of that object - we're not working with extensions here.
tests/AxisAlignedBBTest.php
Outdated
| $this->assertSame($expectedValue, $axis->intersectsWith($bb, $epsilon)); | ||
| } | ||
|
|
||
| public function testGetApverageEdgeLength(){ |
tests/Math.php
Outdated
| /** | ||
| * Class for the Math abstract class | ||
| */ | ||
| class Math extends MathAbstract{ |
There was a problem hiding this comment.
I'm not sure I see the point of this?
There was a problem hiding this comment.
Firstly I create the pocketmine\math\tests\Math class and let it extend the pocketmine\math\Math class to let pocketmine\math\tests\MathTest can do unit test for the abstract class pocketmine\math\Math.
There was a problem hiding this comment.
I don't see why this is necessary - it's not instantiated anyway.
There was a problem hiding this comment.
To do the unit test for pocketmine\math\Math, I think it should instantiate.
Otherwise it will not do this unit test.
There was a problem hiding this comment.
The test is not instantiating the Math class anywhere - it's only ever used for static function access. Excuse me if I'm missing the point, but this class is not instantiated anywhere I can see in these tests.
There was a problem hiding this comment.
This Math class is used in the MathTest class.
There was a problem hiding this comment.
... but you still don't instantiate it, you only statically access its methods.
There was a problem hiding this comment.
Thank you for your explanation and I know that.
I remove those non-instantiate abstract classes.
tests/MathTest.php
Outdated
| } | ||
|
|
||
| public function testSolveQuadraticThrowsInvalidArgumentException(){ | ||
| $this->expectException(\InvalidArgumentException::class); |
There was a problem hiding this comment.
this should be changed to DivisionByZeroError (see above).
| public function testConstructor(){ | ||
| $matrix = new Matrix(1, 2); | ||
|
|
||
| $this->assertSame(1, $matrix->getRows()); |
There was a problem hiding this comment.
not sure why assert that the row count is correct but not the column count? seems like a logic hole.
There was a problem hiding this comment.
You're right, I add the getColumn to assert the matrix column :-).
tests/RayTraceResultTest.php
Outdated
| $hitVector = new Vector3(1.2, 1.3, 1.5); | ||
| $rayTraceResult = new RayTraceResult($bb, 1, $hitVector); | ||
|
|
||
| $this->assertInstanceOf(RayTraceResult::class, $rayTraceResult); |
|
It seems that this PR is outdated. Can I recreate this PR to let @dktapps review? |
Changed log