Fix for Unit Test on Windows

The test results on Windows 10, PHP version 7.0.2

PS C:\xampp\htdocs\d8\core> php ..\vendor\phpunit\phpunit\phpunit .\tests\Drupal\Tests\Component\FileCache\FileCacheFactoryTest.php
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Component\FileCache\FileCacheFactoryTest
...........

Time: 8.69 seconds, Memory: 6.00MB

OK (11 tests, 13 assertions)

PS C:\xampp\htdocs\d8\core> php ..\vendor\phpunit\phpunit\phpunit .\tests\Drupal\Tests\Component\ClassFinder\ClassFinderTest.php
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Component\ClassFinder\ClassFinderTest
.

Time: 8.67 seconds, Memory: 6.00MB

OK (1 test, 4 assertions)

PS C:\xampp\htdocs\d8\core> php ..\vendor\phpunit\phpunit\phpunit .\tests\Drupal\Tests\Component\Discovery\YamlDirectoryDiscoveryTest.php
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Component\Discovery\YamlDirectoryDiscoveryTest
....

Time: 8.77 seconds, Memory: 6.00MB

OK (4 tests, 16 assertions)

Works on Windows.

Just resave the issue, because the test failed on missing CI Job

Since this is a change that impacts the test runner, let's make sure we have a passing test run. I am re-queuing the test to make sure the missing job was unrelated.

1. +++ b/core/tests/Drupal/Tests/Component/Discovery/YamlDirectoryDiscoveryTest.php
   @@ -80,18 +80,18 @@ public function testDiscovery() {
   -    $this->assertSame(['id' => 'item1', 'name' => 'test1 item 1', YamlDirectoryDiscovery::FILE_KEY => 'vfs://modules/test_1/subdir1/item_1.test.yml'], $data['test_1']['item1']);
   -    $this->assertSame(['id' => 'item2', 'name' => 'test1 item 2', YamlDirectoryDiscovery::FILE_KEY => 'vfs://modules/test_1/subdir2/item_2.test.yml'], $data['test_1']['item2']);
   +    $this->assertSame(['id' => 'item1', 'name' => 'test1 item 1', YamlDirectoryDiscovery::FILE_KEY => 'vfs://modules/test_1/subdir1' . DIRECTORY_SEPARATOR . 'item_1.test.yml'], $data['test_1']['item1']);
   +    $this->assertSame(['id' => 'item2', 'name' => 'test1 item 2', YamlDirectoryDiscovery::FILE_KEY => 'vfs://modules/test_1/subdir2' . DIRECTORY_SEPARATOR . 'item_2.test.yml'], $data['test_1']['item2']);
   ...
   -    $this->assertSame(['id' => 'item3', 'name' => 'test2 item 3', YamlDirectoryDiscovery::FILE_KEY => 'vfs://modules/test_2/subdir1/item_3.test.yml'], $data['test_2']['item3']);
   +    $this->assertSame(['id' => 'item3', 'name' => 'test2 item 3', YamlDirectoryDiscovery::FILE_KEY => 'vfs://modules/test_2/subdir1' . DIRECTORY_SEPARATOR . 'item_3.test.yml'], $data['test_2']['item3']);
   ...
   -    $this->assertSame(['id' => 'item4', 'name' => 'test4 item 4', YamlDirectoryDiscovery::FILE_KEY => 'vfs://modules/test_4/subdir1/item_4.test.yml'], $data['test_4']['item4']);
   -    $this->assertSame(['id' => 'item5', 'name' => 'test4 item 5', YamlDirectoryDiscovery::FILE_KEY => 'vfs://modules/test_4/subdir1/item_5.test.yml'], $data['test_4']['item5']);
   -    $this->assertSame(['id' => 'item6', 'name' => 'test4 item 6', YamlDirectoryDiscovery::FILE_KEY => 'vfs://modules/test_4/subdir1/item_6.test.yml'], $data['test_4']['item6']);
   +    $this->assertSame(['id' => 'item4', 'name' => 'test4 item 4', YamlDirectoryDiscovery::FILE_KEY => 'vfs://modules/test_4/subdir1' . DIRECTORY_SEPARATOR . 'item_4.test.yml'], $data['test_4']['item4']);
   +    $this->assertSame(['id' => 'item5', 'name' => 'test4 item 5', YamlDirectoryDiscovery::FILE_KEY => 'vfs://modules/test_4/subdir1' . DIRECTORY_SEPARATOR . 'item_5.test.yml'], $data['test_4']['item5']);
   +    $this->assertSame(['id' => 'item6', 'name' => 'test4 item 6', YamlDirectoryDiscovery::FILE_KEY => 'vfs://modules/test_4/subdir1' . DIRECTORY_SEPARATOR . 'item_6.test.yml'], $data['test_4']['item6']);
   
   @@ -113,7 +113,7 @@ public function testDiscoveryAlternateId() {
   -    $this->assertSame(['alt_id' => 'item1', 'id' => 'ignored', YamlDirectoryDiscovery::FILE_KEY => 'vfs://modules/test_1/item_1.test.yml'], $data['test_1']['item1']);
   +    $this->assertSame(['alt_id' => 'item1', 'id' => 'ignored', YamlDirectoryDiscovery::FILE_KEY => 'vfs://modules/test_1' . DIRECTORY_SEPARATOR . 'item_1.test.yml'], $data['test_1']['item1']);
   

   Why is it that only some slashes need to be converted/fixed in these?

2. +++ b/core/tests/Drupal/Tests/Component/Discovery/YamlDirectoryDiscoveryTest.php
   @@ -124,7 +124,7 @@ public function testDiscoveryAlternateId() {
   -    $this->setExpectedException(DiscoveryException::class, 'The vfs://modules/test_1/item_1.test.yml contains no data in the identifier key \'id\'');
   +    $this->setExpectedException(DiscoveryException::class, 'The vfs://modules/test_1' . DIRECTORY_SEPARATOR . 'item_1.test.yml contains no data in the identifier key \'id\'');
   
   @@ -144,7 +144,7 @@ public function testDiscoveryNoIdException() {
   -    $this->setExpectedException(DiscoveryException::class, 'The vfs://modules/test_1/item_1.test.yml contains invalid YAML');
   +    $this->setExpectedException(DiscoveryException::class, 'The vfs://modules/test_1' . DIRECTORY_SEPARATOR . 'item_1.test.yml contains invalid YAML');
   

   Do we need to make the change in the test messages?

At the very least don't touch the VFS entries. If those are failing we need a more detailed report because those do not touch the file system and are handled entirely in code through streams not by the operating system.

I'd like more information on what the errors are specifically being reported are so I can try to recreate them since Windows should handle the forward slashes just fine.

Only reroll #4 patch for 8.7.x

Re-rolled #19 for 9.4.

Reroll the path #28 with 10.1.x.

Running the tests mentioned in the IS on Windows 11 _with_ the patch on 10.1.x:

- Both FileCacheTes and ClassFinderTest now pass.
- YamlDirectoryDiscoveryTest now fails with:

$ ../vendor/bin/phpunit tests/Drupal/Tests/Component/Discovery/YamlDirectoryDiscoveryTest.php
PHPUnit 9.5.26 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Component\Discovery\YamlDirectoryDiscoveryTest
..FF                                                                4 / 4 (100%)

Time: 00:00.385, Memory: 10.00 MB

There were 2 failures:

1) Drupal\Tests\Component\Discovery\YamlDirectoryDiscoveryTest::testDiscoveryNoIdException
Failed asserting that exception message 'The vfs://modules/test_1\item_1.test.yml contains no data in the identifier key 'id'' contains 'The vfs://modules/test_1/item_1.test.yml contains no data in the identifier key 'id''.

D:\htdocs\drupal\vendor\phpunit\phpunit\src\Framework\Constraint\Constraint.php:122
D:\htdocs\drupal\vendor\phpunit\phpunit\src\Framework\Constraint\Constraint.php:55
D:\htdocs\drupal\vendor\phpunit\phpunit\src\Framework\TestResult.php:728
D:\htdocs\drupal\vendor\phpunit\phpunit\src\Framework\TestSuite.php:673
D:\htdocs\drupal\vendor\phpunit\phpunit\src\TextUI\TestRunner.php:661
D:\htdocs\drupal\vendor\phpunit\phpunit\src\TextUI\Command.php:144
D:\htdocs\drupal\vendor\phpunit\phpunit\src\TextUI\Command.php:97

2) Drupal\Tests\Component\Discovery\YamlDirectoryDiscoveryTest::testDiscoveryInvalidYamlException
Failed asserting that exception message 'The vfs://modules/test_1\item_1.test.yml contains invalid YAML' contains 'The vfs://modules/test_1/item_1.test.yml contains invalid YAML'.

D:\htdocs\drupal\vendor\phpunit\phpunit\src\Framework\Constraint\Constraint.php:122
D:\htdocs\drupal\vendor\phpunit\phpunit\src\Framework\Constraint\Constraint.php:55
D:\htdocs\drupal\vendor\phpunit\phpunit\src\Framework\TestResult.php:728
D:\htdocs\drupal\vendor\phpunit\phpunit\src\Framework\TestSuite.php:673
D:\htdocs\drupal\vendor\phpunit\phpunit\src\TextUI\TestRunner.php:661
D:\htdocs\drupal\vendor\phpunit\phpunit\src\TextUI\Command.php:144

Basically I think the / between The vfs://modules/test_1 and item_1.test.yml ... needs to be replaced with '. DIRECTORY_SEPARATOR . ' in both $this->expectExceptionMessages in YamlDirectoryDiscoveryTest::testDiscoveryNoIdException and ::testDiscoveryInvalidYamlException.

After that all tests should pass on Windows and TestBot.

Adding to my queue to address issues #34

Like this?

@smustgrave: I was thinking more along the lines of the attached interdiff against your last patch.

Sorry for the delay.

Updated based on #37

Thanks @smustgrave.

After applying patch #38, all three tests mentioned in the IS now pass on my local Windows 11 install:

sheet@LAPTOP-EMEOAUGS MINGW64 /d/htdocs/drupal (3123216-10.1-replace-non-test-usages)
$ patch -p1 < 2911377
bash: 2911377: No such file or directory

sheet@LAPTOP-EMEOAUGS MINGW64 /d/htdocs/drupal (10.1.x)
$ patch -p1 < 2911377-38.patch
patching file core/tests/Drupal/Tests/Component/ClassFinder/ClassFinderTest.php
patching file core/tests/Drupal/Tests/Component/Discovery/YamlDirectoryDiscoveryTest.php
patching file core/tests/Drupal/Tests/Component/FileCache/FileCacheTest.php

sheet@LAPTOP-EMEOAUGS MINGW64 /d/htdocs/drupal (10.1.x)
$ vendor/bin/phpunit -c core --stop-on-failure core/tests/Drupal/Tests/Component/ClassFinder/ClassFinderTest.php
PHPUnit 9.5.26 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Component\ClassFinder\ClassFinderTest
.                                                                   1 / 1 (100%)

Time: 00:00.050, Memory: 10.00 MB

OK (1 test, 4 assertions)

sheet@LAPTOP-EMEOAUGS MINGW64 /d/htdocs/drupal (10.1.x)
$ vendor/bin/phpunit -c core --stop-on-failure core/tests/Drupal/Tests/Component/Discovery/YamlDirectoryDiscoveryTest.php
PHPUnit 9.5.26 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Component\Discovery\YamlDirectoryDiscoveryTest
....                                                                4 / 4 (100%)

Time: 00:00.100, Memory: 10.00 MB

OK (4 tests, 16 assertions)

sheet@LAPTOP-EMEOAUGS MINGW64 /d/htdocs/drupal (10.1.x)
$ vendor/bin/phpunit -c core --stop-on-failure core/tests/Drupal/Tests/Component/FileCache/FileCacheTest.php
PHPUnit 9.5.26 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Component\FileCache\FileCacheTest
....                                                                4 / 4 (100%)

Time: 00:00.057, Memory: 10.00 MB

Seeing that those tests also pass on (non-Windows) TestBot, I'm going to mark this RTBC.

Why is it only these tests that fail?

I think we can make these changes easier to read (maintain). And they also need a comment explaining why the path is being altered.

1. +++ b/core/tests/Drupal/Tests/Component/ClassFinder/ClassFinderTest.php
   @@ -20,7 +20,7 @@ public function testFindFile() {
   +    $this->assertStringEndsWith('core' . DIRECTORY_SEPARATOR . 'tests' . DIRECTORY_SEPARATOR . 'Drupal' . DIRECTORY_SEPARATOR . 'Tests' . DIRECTORY_SEPARATOR . 'Component' . DIRECTORY_SEPARATOR . 'ClassFinder' . DIRECTORY_SEPARATOR . 'ClassFinderTest.php', $finder->findFile(ClassFinderTest::class));
   
   @@ -30,7 +30,7 @@ public function testFindFile() {
   +    $this->assertStringEndsWith('core' . DIRECTORY_SEPARATOR . 'tests' . DIRECTORY_SEPARATOR . 'Drupal' . DIRECTORY_SEPARATOR . 'Tests' . DIRECTORY_SEPARATOR . 'Component' . DIRECTORY_SEPARATOR . 'ClassFinder' . DIRECTORY_SEPARATOR . 'ClassFinderTest.php', $finder->findFile(ClassFinderTest::class));
   

   I'm thinking a simple str_replace, with a comment, is easier to read.

       // comment to be added
       $path = str_replace('/', DIRECTORY_SEPARATOR, 'core/tests/Drupal/Tests/Component/ClassFinder/ClassFinderTest.php');
       $this->assertStringEndsWith($path, $finder->findFile(ClassFinderTest::class));

2. +++ b/core/tests/Drupal/Tests/Component/Discovery/YamlDirectoryDiscoveryTest.php
   @@ -80,18 +80,18 @@ public function testDiscovery() {
   +    $this->assertSame(['id' => 'item1', 'name' => 'test1 item 1', YamlDirectoryDiscovery::FILE_KEY => 'vfs://modules/test_1/subdir1' . DIRECTORY_SEPARATOR . 'item_1.test.yml'], $data['test_1']['item1']);
   ...
   +    $this->assertSame(['id' => 'item3', 'name' => 'test2 item 3', YamlDirectoryDiscovery::FILE_KEY => 'vfs://modules/test_2/subdir1' . DIRECTORY_SEPARATOR . 'item_3.test.yml'], $data['test_2']['item3']);
   ...
   +    $this->assertSame(['id' => 'item4', 'name' => 'test4 item 4', YamlDirectoryDiscovery::FILE_KEY => 'vfs://modules/test_4/subdir1' . DIRECTORY_SEPARATOR . 'item_4.test.yml'], $data['test_4']['item4']);
   

   Why is the separator only changed in the middle of the path? This question applies to other lines in the patch as well. And like above let's make it easier to read.

@quietone updated #40.1

but #40.2 not sure how str_replace helps here as DIRECTORY_SEPARATOR is only used once per line

As to the reason why, I'm not going to pretend I understand windows but looking at the failure from #33

vfs://modules/test_1\item_1.test.yml

Seems the last separator is reversed in windows?

+++ b/core/tests/Drupal/Tests/Component/ClassFinder/ClassFinderTest.php
@@ -20,7 +20,8 @@
+    $path = str_replace('/', DIRECTORY_SEPARATOR, 'core/tests/Drupal/Tests/Component/ClassFinder/ClassFinderTest.php');

Ah, this would be even better as $expected_path. Sorry, I should have thought of that.

As for a comment, how about "The file path is dependent on the operating system, so we adjust the directory separator." ? This comment, or similar, could before the first assertSame in YamDirectoryDiscorveryTest.

str_replace probably isn't a great option. it replaces things that might not be directory separators.

The reason the tests fail is because despite php handling file magic when talking to the filesystem, if you ask the file system what the path is it will use Windows broken separators. It might be more readable to do an implode in some cases.

To understand consider the following contrived example. I don't have a windows system to validate but it should be conceptually correct.

var_export(file_exists('/foo/bar.txt')); //true
var_export(real_path('/foo/bar.txt')); // C:\foo\bar.txt

The patch doesn't capture the problem very well but you should be able to find some similar calculation around most of them. Occasionally libraries might has something like $base . DIRECTORY_SEPERATOR . $part which might be what VFS is doing. Generally this shouldn't be necessary (why we're only looking at tests) but it could happen.

Practically this sort of munging should be transparent during normal runtime. But for tests asserting specific string values for a path they're different and fails.

edit: Woops, I missed the question was specific to vfs... leaving explanation in case it helps someone but sorry.

Addressed comments in #42

Just ran all three changed test classes successfully on a Windows 11 machine using XAMPP and using MINGW64 as bash shell.

Still not plused about the str_replace use. I guess I'll need to write a different patch though to get it addressed. Does anyone have steps to recreate these failures?

Sorry, that was a genuine request. I don't know how to recreate this to test this and try some alternate approaches.

Moving this back to needs review. Pinged @Spokje in slack to see if he checked the failure case (which I understand to be just running these unit tests on a windows machine, but have no way to validate).

The obvious alternative to str_replace() would be a big concat of the string which was in earlier patches - which is guaranteed not to replace anything it shouldn't since it'd all be hardcoded. But also given the string we're runnign str_replace() in is a hard-coded string in a test anyway, I don't see how the end result could possibly be different.

Pinged @Spokje in slack to see if he checked the failure case (which I understand to be just running these unit tests on a windows machine, but have no way to validate).

Yep, the test failures in the IS is me running the tests on the 10.1.x branch without any patches: https://www.drupal.org/node/2911377/revisions/view/12852063/12894061

Back to RTBC

Seemed random

Random Failure, back to RTBC.

Committed to 10.1.x and backported to 10.0.x and 9.5.x to keep things consistent

Posthumously updating the priority to minor.