Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename Path to BasePath #393

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions includes/Support/Path.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
<?php
namespace App\Support;

use EmptyIterator;
use Iterator;

/**
* Currently there is no notion of "relative" or "absolute"
* path as of {@see Path} object, because there is no functionality
* of joining paths together. Method {@see Path::toString}
* currently returns paths as absolute paths, but {@see Path}
* itself can be thought of as both absolute and relative.
* The distinction will have to be made when there are methods
* in this class that allow joining paths - those methods
* will have to decide whether to treat other {@see Path} as
* relative or absolute.
*/
class Path
danon marked this conversation as resolved.
Show resolved Hide resolved
{
/** @var string[] */
private array $children;

public function __construct(array $path)
{
$this->children = $path;
}

public static function of(string $path): Path
{
return new Path(\preg_split("#[\\\\/]#", $path));
}

public function append(string $child): Path
{
return new Path([...$this->children, $child]);
}

public function toString(): string
{
return \join(\DIRECTORY_SEPARATOR, \iterator_to_array($this->children()));
}

private function children(): Iterator
{
if (empty($this->children)) {
return new EmptyIterator();
}
return $this->normalizedChildren();
}

private function normalizedChildren(): Iterator
{
[$root, $children] = $this->rootAndChildren();
yield \rTrim($root, "/\\");
foreach ($children as $child) {
if ($child === "") {
continue;
}
yield \trim($child, "/\\");
}
}

private function rootAndChildren(): array
{
$root = \reset($this->children);
return [$root, \array_slice($this->children, 1)];
}
}
23 changes: 23 additions & 0 deletions tests/Psr4/Concerns/PhpunitConcern.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?php
namespace Tests\Psr4\Concerns;

use Exception;

/**
* @method expectException(string $message)
*/
trait PhpunitConcern
{
/**
* This test really is marked as unnecessary. If the condition is not met,
* marking it as unnecessary is preferable to marking it as risky, incomplete
* or skipped.
*
* This test simply doesn't make sense, if the condition is not met.
*/
public function markTestUnnecessary(string $message): void
{
$this->expectException(Exception::class);
throw new Exception($message);
}
}
16 changes: 16 additions & 0 deletions tests/Psr4/PhpUnitPolyfill.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<?php
namespace Tests\Psr4;

use PHPUnit\Framework\Assert;

trait PhpUnitPolyfill
{
public static function assertStringContainsString(
string $needle,
string $haystack,
string $message = ""
): void {
$message = $message ?? "Failed to assert that starting contains substring";
Assert::assertTrue(\str_contains($haystack, $needle), $message);
}
}
3 changes: 2 additions & 1 deletion tests/Unit/Support/BasePathTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,11 @@
use App\Support\BasePath;
use PHPUnit\Framework\TestCase;
use Tests\Psr4\Concerns\SystemConcern;
use Tests\Psr4\PhpUnitPolyfill;

class BasePathTest extends TestCase
{
use SystemConcern;
use SystemConcern, PhpUnitPolyfill;

private BasePath $path;

Expand Down
184 changes: 184 additions & 0 deletions tests/Unit/Support/PathTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
<?php
namespace Support;

use App\Support\Path;
use PHPUnit\Framework\TestCase;
use Tests\Psr4\Concerns\PhpunitConcern;
use Tests\Psr4\Concerns\SystemConcern;

class PathTest extends TestCase
{
use SystemConcern, PhpunitConcern;

/**
* @test
*/
public function shouldGetEmptyPath()
{
// given
$path = new Path([]);
// when
$asString = $path->toString();
// then
$this->assertSame("", $asString);
}

/**
* @test
*/
public function shouldIgnoreEmptyChildren()
{
// given
$path = new Path(["string", "", "", "string"]);
// when
$asString = $path->toString();
// then
$this->assertSameWindows("string\string", $asString);
$this->assertSameUnix("string/string", $asString);
}

/**
* @test
* @dataProvider fileNames
*/
public function shouldGetSingleFile(string $filename)
{
// given
$path = new Path([$filename]);
// when
$asString = $path->toString();
// then
$this->assertSame("file.txt", $asString);
}

public function fileNames(): array
{
return [["file.txt"], ["file.txt/"], ["file.txt\\"]];
}

/**
* @test
* @dataProvider pathPieces
*/
public function shouldGetManyFiles(array $pathPieces)
{
// given
$path = new Path($pathPieces);
// when
$asString = $path->toString();
// then
$this->assertSameWindows('first\second\third\file.txt', $asString);
$this->assertSameUnix("first/second/third/file.txt", $asString);
}

public function pathPieces(): array
{
return [
[["first", "second", "third", "file.txt"]],

[["first", "/second", "/third", "/file.txt"]],
[["first", "\second", '\third', '\file.txt']],

[["first/", "second/", "third/", "file.txt"]],
[["first\\", "second\\", "third\\", "file.txt"]],

[["first/", "/second/", "/third/", "/file.txt"]],
[["first\\", "\\second\\", '\\third\\', '\\file.txt']],
];
}

/**
* @test
* @dataProvider paths
*/
public function shouldRepresentPath(string $stringPath)
{
// given
$path = Path::of($stringPath);
// when
$asString = $path->toString();
// then
$this->assertSameWindows('one\two\three\file.txt', $asString);
$this->assertSameUnix("one/two/three/file.txt", $asString);
}

public function paths(): array
{
return [['one\two\three\file.txt'], ["one/two/three/file.txt"]];
}

/**
* @test
* @dataProvider children
*/
public function shouldAppendPath(Path $path, string $appendant)
{
// when
$childPath = $path->append($appendant);
// then
$this->assertSameWindows('uno\dos\tres', $childPath->toString());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider making DIRECTORY_SEPARATOR injectable for the Path class. That way unit tests are going to be system agnostic.

Copy link
Contributor Author

@danon danon Jun 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't they now? These tests pass both on windows and unix currently.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@danon danon Jun 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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_SEPARATOR injectable for the Path class. 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".

Copy link
Member

@budziam budziam Jun 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_SEPARATOR on the global level (I don't know if that's even possible)

Copy link
Contributor Author

@danon danon Jun 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$this->assertSameUnix("uno/dos/tres", $childPath->toString());
}

public function children(): array
{
return [
[Path::of("uno/dos"), "tres"],
[Path::of("uno/dos"), '\tres'],
[Path::of("uno/dos"), "/tres"],
[Path::of("uno/dos"), "tres/"],
[Path::of("uno/dos"), "tres\\"],

[Path::of("uno/dos/"), "tres"],
[Path::of("uno/dos/"), '\tres'],
[Path::of("uno/dos/"), "/tres"],

[Path::of("uno/dos\\"), "tres"],
[Path::of("uno/dos\\"), '\tres'],
[Path::of("uno/dos\\"), "/tres"],
];
}

/**
* @test
*/
public function shouldAcceptPathWithDriveOnWindows()
{
if ($this->isUnix()) {
$this->markTestUnnecessary("There are no drives on Unix");
}
// given
$path = Path::of("C:\directory");
// when
$child = $path->append("file.txt");
// then
$this->assertSame('C:\directory\file.txt', $child->toString());
}

/**
* @test
*/
public function shouldRemainAbsolutePathOnUnix()
{
if (!$this->isUnix()) {
$this->markTestUnnecessary("There are no leading separators on Windows");
}
// given
$path = Path::of("/usr/bin");
// when
$child = $path->append("local");
// then
$this->assertSame("/usr/bin/local", $child->toString());
}

/**
* @test
*/
public function shouldBeImmutable()
{
// given
$path = Path::of("one/two/three");
// when
$this->assertSame($path->toString(), $path->toString());
}
}