Feature/blt phpcbf #1019
Feature/blt phpcbf #1019
Conversation
|
@grasmash this is ready to be reviewed. What action should I take to indicate that? |
|
@dooleymatt Please add a test for this feature. Ideally, you should have a tests that asserts both positive and negative cases. |
|
Thanks @grasmash. I took a look at the existing tests to see how to write one for this, but I'm not really sure what I should I be testing. It seems like most of the existing tests check for files that have been added, removed, or changed, but I'm not sure what similar tests could be performed for this new task. I am hoping either you or @TravisCarden could offer some direction. Thanks! |
|
I'd be happy to chat about it, @dooleymatt. |
9281b6f to
18916bc
Compare
|
@grasmash Test has been added. |
tests/phpunit/BltProject/FixTest.php
Outdated
| throw new \Exception("Unable to create temporary file."); | ||
| } | ||
| $file_contents = "\"{$this->projectDirectory}/docroot/foo.php\",1,1,error,\"Line indented incorrectly; expected 2 spaces, found 4\",Drupal.WhiteSpace.ScopeIndent.IncorrectExact,5,1"; | ||
| $command = "echo '$file_contents' > {$tmp_file}"; |
There was a problem hiding this comment.
@dooleymatt file_put_contents() would be simpler, since we're writing PHP. (I realize I put you on the wrong scent by using the word "echo".)
tests/phpunit/BltProject/FixTest.php
Outdated
| } | ||
|
|
||
| /** | ||
| * Helper method that xecutes the phpcbf-file-list.sh script. |
tests/phpunit/BltProject/FixTest.php
Outdated
| * Tests that a file list is generated. | ||
| */ | ||
| public function testFixPhpcbfWithArguments() { | ||
| $report = $this->createTestReport(); |
There was a problem hiding this comment.
@dooleymatt I'm confused: Don't you want to be creating a test file rather than a test report? A PHP file with standards violations in it so you can assert that PHPCBF correctly finds it?
BLT-977: Updates fix phing targets. BLT-977: Cleans up comments and fix:phpcbf:files target. BLT-977: Adds files.php.custom.themes to phpcbf filesets.
18916bc to
583ce73
Compare
|
@TravisCarden Thanks for reviewing this. I initially didn't include a test like you described because I felt like it might have been outside the scope of this to test whether or not phpcbf actually does what it's supposed to do. However, I've updated the test to include that now, and have also updated the way that the phpcs csv file report is generated. |
|
@dooleymatt This looks great. My only request is that we put this in the |
|
@grasmash I based that change on Travis's request here: #977 (comment) |
|
Eh, alright. The argument makes sense. |
Fixes # 977.
Changes proposed:
fix:phpcbftarget to automatically fix violations found by phpcs.