Skip to content

Conversation

@peter279k
Copy link
Contributor

Changed log

  • add other test.
  • add the suggest key in composer.json to suggest the extensions.

@BenMorel
Copy link
Member

suggest has already been proposed (and declined) in #5, see the reasons there!

@@ -0,0 +1,102 @@
<?php

namespace Brick\Math\Tests;
Copy link
Member

Choose a reason for hiding this comment

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

Should be Brick\Math\Tests\Internal\Calculator and file moved to the appropriate directory

$this->assertBigIntegerEquals($value, unserialize(serialize($number)));
}

public function testJsonSerialize()
Copy link
Member

Choose a reason for hiding this comment

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

Please move this test below testDirectCallToUnserialize!

@BenMorel
Copy link
Member

Note to self: these lines 1, 2, 3 can never be executed currently, due to higher-level optimizations such as 1, 2, 3. NativeCalculatorTest is needed if we want to cover them.

@peter279k
Copy link
Contributor Author

peter279k commented Mar 19, 2018

Hi @BenMorel, thank you for your reply.
I think we have the NativeCalclator exist because I want to cover them.
And we should check that uncovered code whether it's worked successfully.

@@ -0,0 +1,103 @@
<?php
Copy link
Member

Choose a reason for hiding this comment

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

File name should be tests/Internal/Calculator/NativeCalculatorTest.php

@BenMorel
Copy link
Member

I know, hence the "note to self" for posterity. Please just move the NativeCalculatorTest as requested and it will be good to merge!

@peter279k
Copy link
Contributor Author

I'm sorry for that delay because I'm busy in the past of two days.

*
* @param string $a The input numerator.
* @param string $b The input denominator.
* @param string $expectedValue The expected denominator.
Copy link
Member

Choose a reason for hiding this comment

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

It's not numerator & denominator here. You can just remove the descriptions.

*
* @param string $a The input numerator.
* @param string $b The input denominator.
* @param string $expectedValue The expected denominator.
Copy link
Member

Choose a reason for hiding this comment

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

Same.

*
* @param string $a The input numerator.
* @param string $b The input denominator.
* @param string $expectedValue The expected denominator.
Copy link
Member

Choose a reason for hiding this comment

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

Same.

@BenMorel BenMorel merged commit b163583 into brick:master Mar 21, 2018
@BenMorel
Copy link
Member

All good, thanks!

@peter279k peter279k deleted the test_enhancement branch August 25, 2020 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants