-
Notifications
You must be signed in to change notification settings - Fork 144
[TASK] Add more tests for OutputFormat
#893
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
Conversation
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 like the way these tests are narrowing-down expected behaviour.
A couple of minor issues, and some comments beyond the scope of this PR.
tests/Unit/OutputFormatTest.php
Outdated
/** | ||
* @test | ||
*/ | ||
public function createPrettyReturnsNewOutputFormatInstance(): void |
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 think "New" is redundant in the test method name, but it's consistent with createCompactReturnsNewOutputFormatInstance
, which I signed off 🤷
/** | ||
* @test | ||
*/ | ||
public function createPrettyCalledTwoTimesReturnsDifferentInstances(): void |
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'm thinking (and agreeing) "TwoTimes" is better than "Twice" for non-native-English speaking coders.
{ | ||
$newInstance = OutputFormat::createPretty(); | ||
|
||
self::assertSame([',' => ' '], $newInstance->getSpaceAfterListArgumentSeparators()); |
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.
Should this just test that the key ,
has the value {space} - instead of the whole array?
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.
Or maybe the test method should be renamed to indicate it's checking the whole array by adding On;y
at the end of its name.
Co-authored-by: JakeQZ <[email protected]>
Updated and repushed. |
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.
WFM now
No description provided.