[4.0] Fix exit code when phpcbf is processing STDIN #1175
Conversation
|
@jrfnl how would you like to cover this change with tests? |
I imagine it may be difficult to cover this change via the PHPUnit setup as it would require mocking |
|
@fredden Just checking - will you have time over the next few days to add a few bashunit tests for STDIN handling ? Or would you like me to add something ? |
|
I'm happy to update this, but I'm away at the moment. Earliest that I can pick this up again would be Wednesday/Thursday this week. I do have a bash script that I used for local development which will be trivial to convert into the format needed for bashunit. If you want to land this before Friday, feel free to add some tests. |
This also includes tests for when files are supplied via STDIN.
fredden
force-pushed
the
issue-1082/exit-code-when-fixing-stdin
branch
from
4fed094 to
7bfebcc
Compare
|
|
||
| namespace PHP_CodeSniffer\Tests\EndToEnd\Fixtures; | ||
|
|
||
| # The brace on the following line should be on a line by itself. This can be fixed with phpcbf. |
There was a problem hiding this comment.
Quick question: any particular reason to use #-style inline comments instead of //-style inline comments in the test fixtures ? Or is this accidental because of bash using #-style comments ?
There was a problem hiding this comment.
This is probably because Terraform prefers # over // and I'm writing more Terraform than PHP these days. I imagine that because I was also writing shell script, that the hash seemed more natural.
This should be trivial to change. If we end up modifying this pull request, I will change these lines.
| function test_phpcbf_exit_code_clean() { | ||
| bin/phpcbf --suffix=.fixed --standard=tests/EndToEnd/Fixtures/endtoend.xml.dist tests/EndToEnd/Fixtures/ClassOneWithoutStyleError.inc | ||
| assert_exit_code 0 | ||
|
|
||
| # Same result via STDIN | ||
| bin/phpcbf --suffix=.fixed --standard=tests/EndToEnd/Fixtures/endtoend.xml.dist < tests/EndToEnd/Fixtures/ClassOneWithoutStyleError.inc | ||
| assert_exit_code 0 | ||
| } |
There was a problem hiding this comment.
I've just been having a play with this and I believe it would be prudent to split each of these test methods into two separate test methods:
- The first (normal) command + check the exit code.
- The command with STDIN + check the exit code.
Reason being, is that if either of the assertions would fail, the resulting error message would be the same, so we have no way of knowing which of the two assertions failed.
To demonstrate, this would be the output from the new tests if the fix was removed:
... which doesn't give any clue as to whether the exit code setting is broken completely or only for STDIN.
I imagine, we could even consider moving these tests into separate test scripts too (phpcs_exitcodes_test.sh and phpcbf_exitcodes_test.sh).
There was a problem hiding this comment.
If you're open to having additional files in this directory, then I'd like to put all these exit code tests in a single file of their own.
I'll get that sorted in the next few days.
There was a problem hiding this comment.
I see no reason to limit the tests in that directory to just the current files, so, yes please
I think the original tests were split into two files because phpcbf needs a tear_down() method, while phpcs does not, but running that tear_down() after a phpcs run shouldn't do any harm, so one file for these tests could work.
Description
When running
phpcbfwith a PHP file as input (eg,phpcbf < test.php), the exit code was not reported correctly. This pull request fixes that bug. For more details, see #1082.Suggested changelog entry
Fixed exit code when phpcbf is processing STDIN
Related issues/external references
Fixes #1082
Types of changes
PR checklist