[TASK] Add some tests for OutputFormatter (part 2)
#949
Conversation
oliverklee
added this to the 9.0.0: Drop support for PHP < 7.2 and clean up things milestone
oliverklee
force-pushed
the
task/tests/outputformatter-2
branch
from
e8ba8a9 to
b7edab7
Compare
There was a problem hiding this comment.
The tests all look good.
One needs renaming (missing "Returns").
I think in the semicolon-removal tests we should have examples that look more like CSS.
I put some other questions inline.
If we want to continue the whimsical naming of example comments used in tests, I've made some suggestions and pointed out some opportunities :)
tests/Unit/OutputFormatterTest.php
Outdated
| public function commentsWithCommentableWithMoreThanTwoCommentsPutsSpaceAfterBlocksAfterLastRenderedComment(): void | ||
| { | ||
| $this->outputFormat->setRenderComments(true); | ||
| $this->outputFormat->setSpaceBetweenBlocks(' between-space '); |
There was a problem hiding this comment.
No, it indeed isn't. I've removed it now.
oliverklee
force-pushed
the
task/tests/outputformatter-2
branch
2 times, most recently
from
7814e2d to
49c47ba
Compare
There was a problem hiding this comment.
I think a comment to explain the rationale for the setting of the spacing in the tests is required - though only needed in the first test in the list: the reader can infer from that.
I made some other suggestions which are not entirely necessary :)
And added a new issue for the weird semicolon-removal behaviour, which is identified by but beyond the scope of this PR.
tests/Unit/OutputFormatterTest.php
Outdated
| $this->outputFormat->setSpaceBetweenBlocks(' between-space '); | ||
| $this->outputFormat->setSpaceAfterBlocks(' after-space '); |
There was a problem hiding this comment.
I see. The comments() method inserts these spacings (or may do). I think it would be useful to have a comment in the test to explain that - it's far from obvious.
Also autoformat the test class. The new PHPStan warning will go away once we switch the tested class to native type declarations. Part of #757
Co-authored-by: JakeQZ <[email protected]>
Co-authored-by: JakeQZ <[email protected]>
Co-authored-by: JakeQZ <[email protected]>
Co-authored-by: JakeQZ <[email protected]>
Co-authored-by: JakeQZ <[email protected]>
Co-authored-by: JakeQZ <[email protected]>
oliverklee
force-pushed
the
task/tests/outputformatter-2
branch
from
946e463 to
4b95583
Compare
Also autoformat the test class.
The new PHPStan warning will go away once we switch the tested class to native type declarations.
Part of #757