-
Notifications
You must be signed in to change notification settings - Fork 345
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.
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
Simplify tests #90
Simplify tests #90
Conversation
6ce8d89
to
13cc5b0
Compare
public function testPhoneNumber() | ||
{ | ||
for ($i = 0; $i < 100; $i++) { | ||
for ($i = 0; $i < 100; ++$i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could remove all of these for loops that test different randomized versions when we're seeding the number generator I guess?
* This class tests a large portion of all locale specific providers. It does not test the entire stack, because each | ||
* locale specific provider (can) has specific implementations. The goal of this test is to test the common denominator | ||
* and to try to catch possible invalid multi-byte sequences. | ||
* Class ProviderOverrideTest. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the explanation?
Just repeating the class name doesn't add much value. This comment might as well be removed completely.
self::assertEquals('0', $digits[0]); | ||
} | ||
|
||
$areaCode = $digits[0] . $digits[1] . $digits[2] . $digits[3]; | ||
self::assertContains($areaCode, array('0800', '0860', '0861', '0862')); | ||
$areaCode = $digits[0].$digits[1].$digits[2].$digits[3]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I don't like this, is this really in our style guide?
@@ -8,7 +8,7 @@ | |||
final class CompanyTest extends TestCase | |||
{ | |||
/** | |||
* national Id format validator | |||
* national Id format validator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capitalize this please.
Please revert all code style changes, and send them as a separate PR. The current code style decision was to use PSR-12, enforced by StyleCI, as per the contribution guidelines. Don't run external code style fixing tools. |
|
||
self::assertTrue(checkdate($month, $day, $year), "Birth number $birthNumber: date $year/$month/$day is invalid."); | ||
|
||
// check CRC if presented | ||
if (strlen($birthNumber) == 10) { | ||
if (10 == strlen($birthNumber)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change conditions to Yoda notation? That's not how it's done in most of the src code (fortunately imo).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see one advantage here, you don't have to scan to the end of the (maybe very long) statement, to see the number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meta discussion/idea: can either style be enforced with the style fixer?
Then there's no discussion anymore :)
Please re-send this without any code style changes. If you want to change the code style, please open a separate PR and do it only using StyleCI. |
This can be done using StyleCI. |
This PR removes a lot of duplicated code and refactors all the tests by:
Generator
instance for each test insetUp
method1
to make results predictableprotected function getProviders(): iterable
allowing developers to easily specify providers needed in the test$this->assert*
and$this->fail()
to static calls@Symfony
rule