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

Add an OS agnostic path assertion #5

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mallardduck
Copy link

Use Case

Test the expectations of methods that return paths constructed based on PHP constants that will vary by OS platforms. For instance, any PHP constant related to paths will vary from Unix(mac/linux/etc) and Windows due to use of differing directory seperators; even if they are the same equivalent folder.

Packages that seek to have their test suites pass on both Unix and Windows platforms with the same tests will quickly run into test that may pass on linux but fail on windows.

Example

Per usual, you would provide the expected value first and the actual value second.

\Astrotomic\PhpunitAssertions\PathAssertions::assertOsAgnosticPath(__DIR__ . '/resources/css/main.css', __DIR__ . '\resources\css\main.css');

This is a quite literal example where I'm providing literal windows paths for the actual. However consider an application with functions that return paths to resources managed by the app. (laravels resource_path and similar) My Kickflip project would also be a similar project that could use this like:

Current:

        $pageData = ... set to 404 page...;
        self::assertEquals(
            self::agnosticPath('source/404.blade.php'),
            $pageData->source->getRelativePath(),
        );
        $pageData = ... set to 404 page...;
        PathAssertions::assertOsAgnosticPath(
            'source/404.blade.php',
            $pageData->source->getRelativePath(),
        );

In this case the getRelativePath method will derive the relative path based on the full file path. It will just return the relative part based on the "source root folder". So the expected path is predictable however will vary based on OS directory separator - hence the perfect use-case for an assertion like this!

LMK if you have any questions!

@Gummibeer
Copy link
Member

Haven't checked the code and why Style ci complains as I'm on vacation. But how about assertEquals() 🤔
In normal phpunit the differentiation between same and equals is always that equals allows some kind of casting. In context of path assertions casting would be exactly that for me - making things comparable.

The OS agnostic naming is super specific but makes the method relatively long and not so easy to read.

@Gummibeer
Copy link
Member

In train right now so a few minutes for review. 🙈

  1. question: is a backslash (Windows style) allowed at all in Unix systems? 🤔
    /Users/Tom/Downloads/My\Windows.file not talking about escaping - am not able to try right now and also don't know any ISOs or RFCs so I'm asking.
  2. I see why you only normalize the expected as you adjust it to the OS standard and expect the actual to be OS standard. But is this really a safe assumption!? 🤔 Just asking as I can think about some cases but in all these scenarios I get to the point that this assertion wouldn't be right for that.
  3. could we make all existing assertions in the path class OS agnostic? 🤔 This would make stuff like asserting only for directory of any path easier. For the future I plan two more assertions for "path starts with" and "path ends with". So that you could for example skip the home directory naming and only assert a relative structure. Or don't care about a sub structure and expect something to be in /tmp for example. These don't have to be added with that PR. 👍

@mallardduck
Copy link
Author

  1. question: is a backslash (Windows style) allowed at all in Unix systems? 🤔
    /Users/Tom/Downloads/My\Windows.file not talking about escaping - am not able to try right now and also don't know any ISOs or RFCs so I'm asking.

CRAP. This is a big oversight on my part - as this is totally possible. Just not something I needed to test for.
You would indeed have to escape it properly, but you could make a file with that name by doing:

touch My\\Windows.file
  1. I see why you only normalize the expected as you adjust it to the OS standard and expect the actual to be OS standard. But is this really a safe assumption!? 🤔 Just asking as I can think about some cases but in all these scenarios I get to the point that this assertion wouldn't be right for that.

The general idea behind not normalizing the actual value is that this would be the value produced by the code being tested. This way the onus is on the application code (and there by its Dev) to properly format the paths for the OS it is running on.

For instance, imagine a library only created and tested on Linux that uses paths a lot. If the code producing a path with Linux style paths caused a bug when actually run on Windows, then the app should change to fit that. However if we normalized the actual value along with the expected value then a test for this would provide a false-positive.

  1. could we make all existing assertions in the path class OS agnostic? 🤔 This would make stuff like asserting only for directory of any path easier. For the future I plan two more assertions for "path starts with" and "path ends with". So that you could for example skip the home directory naming and only assert a relative structure. Or don't care about a sub structure and expect something to be in /tmp for example. These don't have to be added with that PR. 👍

This is a really good train of thought IMO. I do have concerns about if it may confuse folks having these "os aware" assertions mixed with "vanilla" assertions. So that made me wonder if it could be worth making a new trait just for these more "magic" assertions - since they do more than a basic one.

However even that seems overkill - I'll keep playing around with the idea and maybe even change to a Docs Driven Development mindset to find what would look best from a Docs/end-user point of view.

@Gummibeer
Copy link
Member

Gummibeer commented Jun 7, 2022

Yeah, I had the same idea with a dedicated class. This one or utilizing the equals and same naming for OS agnostic and exact matching.
To me adjusting all path assertions to be OS agnostic makes total sense as it's an opinionated package of additional assertions and in 99% of the cases no one will care. As most packages are only made and tested with Unix. Check how many packages run tests with Windows. Some even reject windows bugs as "your fault to don't use Unix". I even think that not properly handling \ in a Unix filename isn't a problem as, so far I remember, your way of doing it is the common one - flysystem for example. And as things would get damn nasty when this file is copied to windows I would even say that even if theoretically possible no one will ever do it. 😅

Okay, one case for converting the actual value but I wouldn't add actual normalization because of it but make the normalizing method public.
A library to convert paths to a specific OS or the current one. So running on Unix but a method Path::toWindows() - yes it could use assertSame but for a random dataset test I would combine this packages Path::assertEquals and a "string not contains /" and "string contains \" assertions.

I'm also pretty sure that this OS agnostic testing is only possible for relative paths. As Unix always starts with / and windows with C: or any other character. 🤔 So besides different namings /Users/Tom/Downloads/package.zip wouldn't equal C:\\Users\Tom\Downloads\packages.zip even if super theoretically they are the OS pendants for each other. 🙈

Memo to myself: assertions for "is relative" and "is absolute" paths.

@mallardduck
Copy link
Author

I've been one of those rare devs who tests their packages on windows too (well a good deal of packages here and there). So I was just reviewing some of my varying packages that are tested on windows and looking over changes I made to fix tests on windows before creating helper methods.

Almost all of them do have one thing in common that's particularly relevant to your point about relative vs absolute paths. While they actual assertions they use are comparing absolute paths, they are all creating the expected value by:

  1. Having a known base dir for tests that can be grabbed as a native OS path via PHP means (DIR, FILE, realpath, etc),
  2. Creating a relative path for the file in question using Unix style paths,
  3. Normalizing that relative path, and
  4. finally concatenating the relative path onto the base dir before comparing.

So based on that workflow alone, a more optimal assertion signature might be:

function assertEqauls(string $baseDir, string $expected, $actual): void
{
        $osNormalizedExpected = PathAssertions::agnosticPath($expected);
        PHPUnit::assertIsString($actual);
        PHPUnit::assertSame($baseDir . $osNormalizedExpected, $actual)
}

Ultimately while this enables full path assertions, you hit the nail on the head that only the relative path can be normalized to the OS platform it's running on. One neat aspect of this tactic would be that we could even add "training wheels" onto this assertion; for instance it could safety check the $baseDir input to verify the base is a valid path on the active OS.

This could allow me to change a test from:

        self::assertEquals($expectingBaseDir . DIRECTORY_SEPARATOR . "json.txt", $jsonFile->getRealPath());

To:

        OsAwarePathAssertions::assertEquals($expectingBaseDir, "/json.txt", $jsonFile->getRealPath());

@Gummibeer
Copy link
Member

Okay, so after thinking about all these I have this API in mind:

PathAssertions::assertEquals(string $base, string $expected, mixed $actual); // $base.$expected == $base.$actual
PathAssertions::assertEndsWith(string $expected, mixed $actual); // str_ends_with($actual, $expected)

For sure with normalization and for me also important for great DX - automatically trimming the DIRECTORY_SEPERATOR before concatenating. ltrim($base, DIRECTORY_SEPERATOR) and rtrim($expected, DIRECTORY_SEPERATOR) as I hate to care about which of both starts/ends with the slash. 🤯

But one thing/idea: everything around paths isn't that well/natively supported in PHP, even worse when it gets to OS agnostic. So how about an enhanced node.js path port in PHP and with that it will be only a assertTrue(). 🤔
The idea behind is: why should we put all that logic and brain in a single assertion when we could build a dedicated path package and utilize it everywhere. I also often have a need for a path_join() or Path::join() for example. And a real new Path($filepath) simple value object with all the cool things would be also great.
What do you think? I just believe that it's wasted to put the logic here when everyone again needs these helpers. Haven'T done full research but after a quick check I haven't found any package.

@mallardduck
Copy link
Author

One thing to clarify my example - the $jsonFile->getRealPath() call would return an absolute/full path to the file.
So ideally I was imagining that the base only applied to the expected value. Otherwise - if you're appending both expected and actual values - that's just relative path comparison with extra steps.

@mallardduck
Copy link
Author

So how about an enhanced node.js path port in PHP

🤝 I'm 100% into this idea.

Once a stable version of that package exists then -in theory- providing test assertion helpers like I had in mind here should be easier too. Heck at that point it'd be super easy to just pull that library in and make all the path assertions work without care for OS like you suggested earlier.

@mallardduck
Copy link
Author

@Gummibeer - How would you want to start on the port project? Would you like to setup a repo for us to work in? Or should I maybe give a first pass at getting something built and we can move it under the Astrotomic name later?

@Gummibeer
Copy link
Member

https://github.com/Astrotomic/php-path/invitations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants