Add test coverage for ".fixed" file #1690
Conversation
|
To be honest, the intent is to switch to a base test class from the PHPCSUtils TestUtils once it is available and move away from our own test case.... |
|
@jrfnl is that change ready to be proposed / merged in? As far as I can tell, this change is ready for review / to be merged. Or are you saying that you do not want to make any changes to the test suite here until work elsewhere is completed? That sounds like all work here is on hold until the other task/project is complete. |
What I'm saying is that I would prefer to spend my (limited) time wisely and spend it on long term improvements instead of on short-term wins. Having said that, I have no objections to accepting this change, but this PR does need work before it could be considered for merge. |
| * @param string $pathToTestFile Absolute path to the file to sniff. | ||
| * Allows for passing __FILE__ from the unit test | ||
| * file. In that case, the test case file is presumed | ||
| * to have the same name, but with an `inc` extension. |
There was a problem hiding this comment.
The functionality does not match the documentation.
| $this->assertTrue(file_exists($pathToTestFile), 'Test file exists'); | ||
| $this->assertTrue(file_exists($pathToFixedFile), 'Fixed file exists'); |
There was a problem hiding this comment.
- Any particular reason why you are not using
assertFileExists()here ? - The
$messageparameter should reflect the failure message. The text as-is, is the opposite of a failure message. (here and everywhere else)
| $this->assertGreaterThan(0, $phpcsFile->getFixableCount(), 'There are fixable errors or warnings'); | ||
|
|
||
| // Attempt to fix the errors. | ||
| $phpcsFile->fixer->fixFile(); |
There was a problem hiding this comment.
The test run order is not fixed. If the fixer test would be run (for whatever reason) before the "sniffing tests", the sniff tests would likely fail as the $phpcsFile object will have been modified by the fixer run.
|
|
||
| $this->assertSame(0, $fixable, 'Sniff can actually fix all "fixable" errors'); | ||
|
|
||
| if ($phpcsFile->fixer->getContents() !== file_get_contents($pathToFixedFile)) { |
There was a problem hiding this comment.
Any particular reason not to use assertStringEqualsFile() ?
| * | ||
| * @return void | ||
| */ | ||
| protected function assertFixerOutputMatches($pathToTestFile, $targetPhpVersion = 'none') |
There was a problem hiding this comment.
This setup does not allow for testing that fixes may be different depending on the $targetPhpVersion, nor allow for a $targetPhpVersion in which no fixes would be made.
| */ | ||
| public function testFixer() | ||
| { | ||
| $this->assertFixerOutputMatches(str_replace('.php', '.inc', __FILE__)); |
There was a problem hiding this comment.
This works more by accident than by design without passing the testVersion... Also see #142.
|
As discussed in a call, this PR as-is will not be merged. The change to the Wider context: those issues flagged in the review in this PR have been resolved already in the WIP TestUtils TestCase, so probably best to focus on finishing that sooner rather than later instead of spending a lot of time on this PR, which would delay the TestUtils PR. |
While working on #1689, I noticed that there is one existing ".fixed" file in this repository, however there was no code that did anything with the file. This pull request adds the ability to verify fixer output with an expected file.
It looks like the ".fixed" file has already diverged from its corresponding source file. I have fixed that in the same commit as I'm adding the test as the divergence was trivial. Please let me know if you would prefer this in a separate commit.