Make WordPress Core

Opened 7 years ago

Last modified 7 years ago

#43663 new defect (bug)

Unit Test test_theme_file_uri_returns_valid_uri fails on directories with spaces

Reported by: mattkeys's profile mattkeys Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.9.5
Component: Build/Test Tools Keywords: dev-feedback has-patch
Focuses: Cc:

Description

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.

Attachments (3)

43663.diff (0 bytes) - added by mattkeys 7 years ago.
43663.2.diff (795 bytes) - added by mattkeys 7 years ago.
43636.3.diff (703 bytes) - added by birgire 7 years ago.

Download all attachments as: .zip

Change History (7)

@mattkeys
7 years ago

@mattkeys
7 years ago

#1 @mattkeys
7 years ago

  • Keywords dev-feedback has-patch needs-testing added

@birgire
7 years ago

#2 @birgire
7 years ago

@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.

#3 @mattkeys
7 years ago

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)

#4 @birgire
7 years ago

  • Keywords needs-testing removed

@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.

Note: See TracTickets for help on using tickets.