Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem
The following Unit Test does not pass in Windows environment due to hardcoded use of directory separator.
- www/core/tests/Drupal/Tests/Component/FileCache/FileCacheTest.php
- www/core/tests/Drupal/Tests/Component/ClassFinder/ClassFinderTest.php
- www/core/tests/Drupal/Tests/Component/Discovery/YamlDirectoryDiscoveryTest.php
Running the tests mentioned in the IS on Windows 11 _without_ the patch on 10.1.x
:
$ ../vendor/bin/phpunit tests/Drupal/Tests/Component/FileCache/FileCacheTest.php
PHPUnit 9.5.26 by Sebastian Bergmann and contributors.
Testing Drupal\Tests\Component\FileCache\FileCacheTest
F.F. 4 / 4 (100%)
Time: 00:30.572, Memory: 10.00 MB
There were 2 failures:
1) Drupal\Tests\Component\FileCache\FileCacheTest::testGet
Failed asserting that null matches expected 42.
D:\htdocs\drupal\vendor\phpunit\phpunit\src\Framework\Constraint\Equality\IsEqual.php:96
D:\htdocs\drupal\core\tests\Drupal\Tests\Component\FileCache\FileCacheTest.php:60
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\FileCache\FileCacheTest::testSet
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
Array (
'prefix:test:D:\htdocs\drupal\core\tests\Drupal\Tests\Component\FileCache\Fixtures\llama-23.txt' => Array (
'mtime' => 1661603362
- 'filepath' => 'D:\htdocs\drupal\core\tests\Drupal\Tests\Component\FileCache\Fixtures\llama-23.txt'
+ 'filepath' => 'D:\htdocs\drupal\core\tests\Drupal\Tests\Component\FileCache/Fixtures/llama-23.txt'
'data' => 23
)
)
D:\htdocs\drupal\vendor\phpunit\phpunit\src\Framework\Constraint\Equality\IsEqual.php:96
D:\htdocs\drupal\core\tests\Drupal\Tests\Component\FileCache\FileCacheTest.php:116
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
FAILURES!
Tests: 4, Assertions: 8, Failures: 2.
$ ../vendor/bin/phpunit tests/Drupal/Tests/Component/ClassFinder/ClassFinderTest.php
PHPUnit 9.5.26 by Sebastian Bergmann and contributors.
Testing Drupal\Tests\Component\ClassFinder\ClassFinderTest
F 1 / 1 (100%)
Time: 00:00.253, Memory: 10.00 MB
There was 1 failure:
1) Drupal\Tests\Component\ClassFinder\ClassFinderTest::testFindFile
Failed asserting that 'D:\htdocs\drupal\core\tests\Drupal\Tests\Component\ClassFinder\ClassFinderTest.php' ends with "core/tests/Drupal/Tests/Component/ClassFinder/ClassFinderTest.php".
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\core\tests\Drupal\Tests\Component\ClassFinder\ClassFinderTest.php:23
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
FAILURES!
Tests: 1, Assertions: 1, Failures: 1.
$ ../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
FFFF 4 / 4 (100%)
Time: 00:00.416, Memory: 10.00 MB
There were 4 failures:
1) Drupal\Tests\Component\Discovery\YamlDirectoryDiscoveryTest::testDiscovery
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
Array &0 (
'id' => 'item1'
'name' => 'test1 item 1'
- '_discovered_file_path' => 'vfs://modules/test_1/subdir1/item_1.test.yml'
+ '_discovered_file_path' => 'vfs://modules/test_1/subdir1\item_1.test.yml'
)
D:\htdocs\drupal\vendor\phpunit\phpunit\src\Framework\Constraint\Constraint.php:122
D:\htdocs\drupal\vendor\phpunit\phpunit\src\Framework\Constraint\IsIdentical.php:79
D:\htdocs\drupal\core\tests\Drupal\Tests\Component\Discovery\YamlDirectoryDiscoveryTest.php:83
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::testDiscoveryAlternateId
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
Array &0 (
'alt_id' => 'item1'
'id' => 'ignored'
- '_discovered_file_path' => 'vfs://modules/test_1/item_1.test.yml'
+ '_discovered_file_path' => 'vfs://modules/test_1\item_1.test.yml'
)
D:\htdocs\drupal\vendor\phpunit\phpunit\src\Framework\Constraint\Constraint.php:122
D:\htdocs\drupal\vendor\phpunit\phpunit\src\Framework\Constraint\IsIdentical.php:79
D:\htdocs\drupal\core\tests\Drupal\Tests\Component\Discovery\YamlDirectoryDiscoveryTest.php:116
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
3) 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
4) 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
D:\htdocs\drupal\vendor\phpunit\phpunit\src\TextUI\Command.php:97
FAILURES!
Tests: 4, Assertions: 6, Failures: 4.
Proposed resolution
Use of DIRECTORY_SEPARATOR constant value instead of the hardcoding the directory separator in paths.
Attached in the patch file.
Comment | File | Size | Author |
---|---|---|---|
#44 | 2911377-44.patch | 8.8 KB | smustgrave |
| |||
#44 | interdiff-41-44.txt | 2.32 KB | smustgrave |
Comments
Comment #2
ProFire CreditAttribution: ProFire commentedComment #4
ProFire CreditAttribution: ProFire commentedComment #5
ProFire CreditAttribution: ProFire commentedComment #6
michaellenahan CreditAttribution: michaellenahan at erdfisch commentedComment #7
michaellenahan CreditAttribution: michaellenahan at erdfisch commentedComment #8
a.dmitriiev CreditAttribution: a.dmitriiev as a volunteer commentedThe 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)
Comment #9
a.dmitriiev CreditAttribution: a.dmitriiev as a volunteer commentedWorks on Windows.
Comment #11
a.dmitriiev CreditAttribution: a.dmitriiev as a volunteer commentedJust resave the issue, because the test failed on missing CI Job
Comment #12
xjmSince 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.
Comment #13
Gábor HojtsyWhy is it that only some slashes need to be converted/fixed in these?
Do we need to make the change in the test messages?
Comment #14
ProFire CreditAttribution: ProFire at Singapore Press Holdings commented1) Only some slashes are converted because from Line 60 to Line 78, the slashes are defined statically as forward slash. Thus the output of
YamlDirectoryDiscovery($directories, 'test')
at Line 80 will output exactly as statically defined.Only the slashes that are not statically defined is using the
DIRECTORY_SEPARATOR
constant.2) Yes. Similar to (1), on Line 135 and Line 155, the slashes are statically defined. The exception messages will look like these on Windows respectively:
The vfs://modules/test_1\item_1.test.yml contains no data in the identifier key 'id'
The vfs://modules/test_1\item_1.test.yml contains invalid YAML
Only the slashes that are not statically defined is using the
DIRECTORY_SEPARATOR
constant.Comment #16
neclimdulAt 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.
Comment #18
MerryHamster CreditAttribution: MerryHamster at Skilld commentedComment #19
MerryHamster CreditAttribution: MerryHamster at Skilld commentedOnly reroll #4 patch for 8.7.x
Comment #20
MerryHamster CreditAttribution: MerryHamster at Skilld commentedComment #27
larowlanComment #28
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedRe-rolled #19 for 9.4.
Comment #31
Medha KumariReroll the path #28 with 10.1.x.
Comment #32
SpokjeComment #33
SpokjeRunning the tests mentioned in the IS on Windows 11 _with_ the patch on 10.1.x:
- Both
FileCacheTes
andClassFinderTest
now pass.-
YamlDirectoryDiscoveryTest
now fails with:Comment #34
SpokjeBasically I think the
/
betweenThe vfs://modules/test_1
anditem_1.test.yml ...
needs to be replaced with'. DIRECTORY_SEPARATOR . '
in both$this->expectExceptionMessage
s inYamlDirectoryDiscoveryTest::testDiscoveryNoIdException
and::testDiscoveryInvalidYamlException
.After that all tests should pass on Windows and TestBot.
Comment #35
smustgrave CreditAttribution: smustgrave at Mobomo commentedAdding to my queue to address issues #34
Comment #36
smustgrave CreditAttribution: smustgrave at Mobomo commentedLike this?
Comment #37
Spokje@smustgrave: I was thinking more along the lines of the attached interdiff against your last patch.
Comment #38
smustgrave CreditAttribution: smustgrave at Mobomo commentedSorry for the delay.
Updated based on #37
Comment #39
SpokjeThanks @smustgrave.
After applying patch #38, all three tests mentioned in the IS now pass on my local Windows 11 install:
Seeing that those tests also pass on (non-Windows) TestBot, I'm going to mark this RTBC.
Comment #40
quietone CreditAttribution: quietone at PreviousNext commentedWhy 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.
I'm thinking a simple str_replace, with a comment, is easier to read.
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.
Comment #41
smustgrave CreditAttribution: smustgrave at Mobomo commented@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?
Comment #42
quietone CreditAttribution: quietone at PreviousNext commentedAh, 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.
Comment #43
neclimdulstr_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.
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.
Comment #44
smustgrave CreditAttribution: smustgrave at Mobomo commentedAddressed comments in #42
Comment #45
SpokjeJust ran all three changed test classes successfully on a Windows 11 machine using XAMPP and using MINGW64 as bash shell.
Comment #46
neclimdulStill 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?
Comment #47
neclimdulSorry, that was a genuine request. I don't know how to recreate this to test this and try some alternate approaches.
Comment #48
catchMoving 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.
Comment #49
SpokjeYep, 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
Comment #51
smustgrave CreditAttribution: smustgrave at Mobomo commentedSeemed random
Comment #53
SpokjeRandom Failure, back to RTBC.
Comment #54
larowlanComment #55
larowlanCommitted to 10.1.x and backported to 10.0.x and 9.5.x to keep things consistent
Posthumously updating the priority to minor.