Unit Test test_theme_file_uri_returns_valid_uri fails on directories with spaces

[mattkeys]

Setting up PHPUnit and running the unit tests for the first time produced some failures because I was running from a directory with spaces in the name (WordPress Unit Tests).

4 assertions in total failed, but all 4 of them reference the same function, here is one of the examples:

1) Test_Theme_File::test_theme_file_uri_returns_valid_uri with data set #0 ('parent-only.php', 'theme-file-parent', array('theme-file-parent'))
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'/Users/mattkeys/Desktop/W/WordPress%20Unit%20Tests/tests/phpunit/includes/../data/themedir1/default/parent-only.php'
+'/Users/mattkeys/Desktop/W/WordPress Unit Tests/tests/phpunit/includes/../data/themedir1/default/parent-only.php'

Looking at this test, it makes use of esc_url_raw() which encodes the spaces as %20, then compares them against the original URI which has does not have the spaces encoded, so they are not the same.

If this is something that we want to fix, an easy way would be to str_replace spaces > %20 before running the assertion. Patch attached.

[birgire]

@mattkeys thanks for the report and the patch.

It looks to me that the problem with the test_theme_file_uri_returns_valid_uri() is that the current active theme is the one from: /full/path/to/tests/phpunit/includes/../data/themedir1/default. The themes from the test method's data provider haven't been activated. So we are dealing with a special case when get_theme_root_uri() can't determine the theme's URL and it returns the full directory path instead, namely this part:

https://core.trac.wordpress.org/browser/tags/4.9/src/wp-includes/theme.php#L595

If we activate the themes from the data provider with:

switch_theme( $expected_theme );

before the code in test_theme_file_uri_returns_valid_uri() then we are avoiding the above edge case and the spaces in the path are not involved.

That's the case with 43636.3.diff​.

[mattkeys]

Thanks @birgire, I just tested out your patch and I can confirm that it fixes the issues I was seeing. Here is output from testing the themeFile.php tests:

Matts-MacBook-Pro:WordPress Unit Tests mattkeys$ phpunit tests/phpunit/tests/link/themeFile.php
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 6.5.7 by Sebastian Bergmann and contributors.

........................                                          24 / 24 (100%)

Time: 1.28 seconds, Memory: 28.00MB

OK (24 tests, 48 assertions)

[birgire]

@mattkeys great, thanks for the testing.

ps:

There is ticket #30206 but I don't think it's the same issue though.

Then one can wonder about the edge case, when the full directory path is returned, if that should be handled to support spaces?

But I just stumbled up on this comment by @nacin, an interesting idea to add a second URL argument to register_theme_directory().

That would be one way to avoid the above edge case.