Rename Path to BasePath #393
Conversation
danon
force-pushed
the
path
branch
6 times, most recently
from
9909b3c
to
98bf91f
Compare
tests/Unit/Support/PathTest.php
Outdated
| // when | ||
| $childPath = $path->append($appendant); | ||
| // then | ||
| $this->assertSameWindows('uno\dos\tres', $childPath->toString()); |
There was a problem hiding this comment.
Please consider making DIRECTORY_SEPARATOR injectable for the Path class. That way unit tests are going to be system agnostic.
There was a problem hiding this comment.
Aren't they now? These tests pass both on windows and unix currently.
There was a problem hiding this comment.
However, to be able to test windows behavior you need to run tests on windows VM. You can't be sure this code works correctly with windows when running tests on a unix-based system.
There was a problem hiding this comment.
@budziam Is there a feature request to build Windows paths on Unix and vice-versa? Because if there isn't, there's no way to test it. And to test it, it would mean to require that functionallity. However it seems off, doesn't it?
Perhaps to test it both on windows and unix, we can employ the same strategy we use for testing both php8 and php7, which are github actions. It wouldn't be too hard to create runner-matrix with values runs-on: ubuntu-latest and runs-on: windows-latest (also macos-lates is also available). We can also design the matrix in such a way that tests php5.6-php8.1 run on ubuntu, and only one version of php runs on windows, to save resources and time.
Please consider making
DIRECTORY_SEPARATORinjectable for thePathclass. That way unit tests are going to be system agnostic.
Tests would become more flexible, yes, but the implementation would become less cohesive.
If you parametrize the separator, a smart way to do would be for example to throw exception if someone passes "|" or "" as a separator, but that's unnecessary logic that will never be used in production, since 100% usages will be "build windows path on windows" or "build unix path on unix".
There was a problem hiding this comment.
CPU costs money and shop sms was never intended to be run on windows instances. Running tests against windows VMs is not an acceptable approach.
That leaves us with 2 options:
- pass DIRECTORY_SEPARATOR to the Path class
- mock
DIRECTORY_SEPARATORon the global level (I don't know if that's even possible)
There was a problem hiding this comment.
Github Actions cost money?
shop sms was never intended to be run on windows instances
We either support windows or we don't.
If we do, running CI/CD on windows is a good idea. If we don't, then this dilema doesn't make sense and making Path work on Windows doesn't make sense anyway.
But I understand - you work on Unix machine, and want to make sure it works on Windows, without running windows yourself. That makes sense.
|
@budziam Idea of testing code for a given system without that system is not generally a doable thing. For example, some operations behave differently on 32 and 64 bit systems, and you can't test what application would bahave on it, without actually running it, you can't mock the max integer value for example, there's just no way. This is generally true for all kinds of system dependendencies. I think But if you're certain you want to parametrize the separator, then we'll do it that way. |
danon
force-pushed
the
path
branch
3 times, most recently
from
abc4177
to
01c91cc
Compare
…itectures can be tested on either of them
No description provided.