WordPress.org

Make WordPress Core

Opened 19 months ago

Last modified 19 months ago

#43663 new defect (bug)

Unit Test test_theme_file_uri_returns_valid_uri fails on directories with spaces

Reported by: 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:
PR Number:

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 19 months ago.
43663.2.diff (795 bytes) - added by mattkeys 19 months ago.
43636.3.diff (703 bytes) - added by birgire 19 months ago.

Download all attachments as: .zip

Change History (7)

@mattkeys
19 months ago

@mattkeys
19 months ago

#1 @mattkeys
19 months ago

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

@birgire
19 months ago

#2 @birgire
19 months 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
19 months 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
19 months 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.