use consistent directory-separator in baseline #700
Conversation
|
Since |
|
just extracted |
src/AstRunner/AstMap/FileName.php
Outdated
| @@ -19,13 +21,11 @@ public function toString(): string | |||
|
|
|||
| $path = false !== $wd && 0 === strpos($this->path, $wd) ? substr($this->path, strlen($wd)) : $this->path; | |||
|
|
|||
| // make paths/patterns cross-OS compatible | |||
| return str_replace('\\', '/', $path); | |||
| return FileHelper::normalizePath($path); | |||
There was a problem hiding this comment.
From what I see, the path is already normalized here.
There was a problem hiding this comment.
it doesn't really make a difference, but to compensate your comments I got rid of this ugly ternary and reduced the normalization to the very minimal
| public function testPathNormalization(): void | ||
| { | ||
| $fileName = new AstMap\FileName('/path/to/file.php'); | ||
| $this->assertSame('/path/to/file.php', $fileName->getFilepath()); |
There was a problem hiding this comment.
It would be nice if we could mock getcwd method call here.
There was a problem hiding this comment.
why? its not necessary for the current test.
it would only be necessary, in case you want to test the already existing functionality which was not changed by this PR
There was a problem hiding this comment.
You are right, it would help with test coverage. That is what I meant by "nice". Also, it makes the test fragile. (not really on GitHub but locally for sure). If I were to run the test in the path directory, it would fail mysteriously. It is a tiny thing, but it is IMHO good practice to mock out system dependencies, like the current working directory.
There was a problem hiding this comment.
@patrickkusebauch your criticism around mocking getcwd is valid, but I think it's not too pressing. We can cross that bridge when it comes to it.
Thanks @staabm
closes #675