PHPUnit 6 support + refactoring #23
Conversation
Simplified 'Printer' class designator Added PHPUnit 6 support
changed linefeed detection
There was a problem hiding this comment.
This looks great @VeryStrongFingers!
I've added a couple of inline comments with a few questions and one required thing to fix (newline escaping)
src/Functions/helpers.php
Outdated
| // Some messages might contain paths. Let's convert thost to relative paths too | ||
| $message = relativePath($message); | ||
| $lineFeedPosition = strpos($message, '%0A'); | ||
| if (is_int($lineFeedPosition) === true) { |
There was a problem hiding this comment.
This is an incorrect change imo. The old behaviour stripped linefeeds from the end of the error, whilst the new behaviour truncates any error messages that contain a line feed. Could we revert to the old behaviour?
There was a problem hiding this comment.
that's my bad - I wasn't 100% sure if it was intentional or not, I'll revert it and leave a comment to avoid future developer confusion
There was a problem hiding this comment.
@mheap I'm not sure why, but reverting it is not playing ball with PHPUnit 6
see https://github.com/VeryStrongFingers/phpunit-github-actions-printer/runs/1377052224?check_suite_focus=true#step:8:20
any ideas?
There was a problem hiding this comment.
This is very strange. It looks as though PHPUnit 6 does not add the additional information.
We could have a .phpt file just for PHPUnit 6, or update the printer to add this information (we have it available in file and line)
| class Printer8 extends ResultPrinter | ||
| { | ||
| use Trait8; |
There was a problem hiding this comment.
What was the thinking behind removing the Trait here? Printer8 and Printer9 look like duplications to me now
There was a problem hiding this comment.
Printer9 and Printer8 extend different classes, is the only difference - otherwise Printer9 could just extend Printer8.
I left them both as individual concrete classes otherwise just in case adjustments need to be made specific to that version in the future.
(please note I am personally biased against the use of traits for various reasons).
I could make an abstract base class, not extending PHPUnit to avoid this duplication, if preferred?
There was a problem hiding this comment.
Using a trait allows us to keep the functionality DRY whilst still extending from different classes.
What are the benefits/risks of using a base class rather than a trait in this case?
There was a problem hiding this comment.
Because the method signatures vary over the PHPUnit versions, and traits are not actually concrete testable structures, sort of invisible magic.
Trait8 & Printer7 are already repeating logic because of this.
It would be a valid use case of traits if the method signatures were the same, but you'll end up with less codeby decorating the common methods with helper functions or something alike
|
@mheap I had a change of heart, and decided not to implement PHPUnit v6 - as it is EOL, and don't really want to encourage using unsupported tooling. |
|
Thanks for this @VeryStrongFingers! Although PHPUnit 6 is EOL there are still people out there using it, and given that it's not too much more code to maintain I think it's worth adding. I appreciate the refactoring, and I even learned about |
Was originally just adding PHPUnit 6 support (I know, gross and old :( )Somehow ended up refactoring the various "Printers", essentially to centralise repeated code (ie. Don't Repeat Yourself).
Overall additions:
Added PHPUnit v6 supportshivammathur/setup-php@v2Github action (v1 is deprecated)Fixed the weird linefeed bug? master...VeryStrongFingers:master#diff-89d7f18c9669dcd34355be39484d32089aabe106edec6391460288551e6112b2P.S. I tested the Github actions workflow @ https://github.com/VeryStrongFingers/phpunit-github-actions-printer/actions/runs/354432750
P.P.S. You might find value running those Github actions workflow for
pull_requesttoo