Problem/Motivation

It looks like #2928249: Introduce a PHPUnit 6+ compatibility layer for Drupal\Tests\Listeners classes broke testing on Windows. 

php ../vendor/phpunit/phpunit/phpunit --group Imagemagick --verbose
PHP Fatal error:  Uncaught preg_match(): Compilation failed: unknown property name after \P or \p at offset 5
C:\projects\drupal8\vendor\symfony\phpunit-bridge\DeprecationErrorHandler.php:105
C:\projects\drupal8\core\tests\TestSuites\TestSuiteBase.php:47
C:\projects\drupal8\core\tests\TestSuites\TestSuiteBase.php:48
C:\projects\drupal8\core\tests\TestSuites\UnitTestSuite.php:22
  thrown in C:\projects\drupal8\core\tests\TestSuites\TestSuiteBase.php on line 47

That's only the beginning of the the problem because the rest of the regex on line 47 has the directory separator hard coded as well and it will fail to match without complex logic..

return !preg_match("@^$root/core/tests/Drupal/Tests/Listeners(/|$)@", dirname($test));

Probably in Windows we should have backslashes.

Proposed resolution

Use namespace as data for filtering, because it is OS independent data.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#18 2932715-18.patch1.08 KBalexpott
#18 6-18-interdiff.txt969 bytesalexpott
#6 2932715-6.patch1.08 KBalexpott
#2 2932715-2.patch924 bytesmondrake

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Needs review
StatusFileSize
new 2932715-2.patch924 bytes

Maybe this is sufficient.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Needs work
Issue tags: +Needs manual testing

Is there a way we can get a testbot on windows to make sure we don't regress here, or is this really sufficient? In any case, this needs manual verification from windows users that this works.

Back to needs work for manual tests (I think this is the right status change).

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

Works for me in an Appveyor build of the Imagemagick module.

Without the patch: https://ci.appveyor.com/project/mondrake/imagemagick/build/1.0.201

With the patch:
https://ci.appveyor.com/project/mondrake/imagemagick/build/1.0.204

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

#3:

Is there a way we can get a testbot on windows to make sure we don't regress here, or is this really sufficient?

We already have a series of tests that fail on Windows due to difference with '/' and '\' in path-assertions. Usually we ignore these fails, because without testbot on windows (or CS rules) it will be difficult to control them.

But in this case any tests fail, regardless of their code. Therefore, +1 for a more loyal filter pattern.

Manual testing done:

Command:
php ../vendor/phpunit/phpunit/phpunit --group action --verbose

Without patch:

Fatal error: Uncaught preg_match(): Compilation failed: missing opening brace after \o at offset 4

D:\os\domains\d8x.loc\subfolder\vendor\symfony\phpunit-bridge\DeprecationErrorHandler.php:105
D:\os\domains\d8x.loc\subfolder\core\tests\TestSuites\TestSuiteBase.php:47
D:\os\domains\d8x.loc\subfolder\core\tests\TestSuites\TestSuiteBase.php:48
D:\os\domains\d8x.loc\subfolder\core\tests\TestSuites\UnitTestSuite.php:22

  thrown in D:\os\domains\d8x.loc\subfolder\core\tests\TestSuites\TestSuiteBase.php on line 47

With #2 patch:
OK (7 tests, 169 assertions)

Thank you, @mondrake!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2932715-6.patch1.08 KB 
+++ b/core/tests/TestSuites/TestSuiteBase.php
@@ -42,9 +42,9 @@ protected function addTestsBySuiteNamespace($root, $suite_namespace) {
-        return !preg_match("@^$root/core/tests/Drupal/Tests/Listeners(/|$)@", dirname($test));
+        return !preg_match('/.+core.tests.Drupal.Tests.Listeners(?:.|$)/', dirname($test));

This is quite a bit of loosening of the regular expression. Not sure that that is a great idea. We can do better. I don't have windows but the patch attached should be OS independent because we don't use filepath matching anymore. Also 4 backslashes - https://www.developwebsites.net/match-backslash-preg_match-php/ :)

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Reviewed & tested by the community

#6: Works on Windows perfect too. Nice idea!

mondrake’s picture

mondrake
Location 🇮🇹
commented

Much better indeed. RTBC +1

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 2932715-6.patch, failed testing. View results

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Reviewed & tested by the community

bot fluke

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

catch
he/him
Primary language English
commented
Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!

  • catch committed 4352db0 on 8.6.x 
    Issue #2932715 by mondrake, alexpott, vaplas: PHPUnit testing fails on...

  • catch committed 494bbac on 8.5.x 
    Issue #2932715 by mondrake, alexpott, vaplas: PHPUnit testing fails on...
tacituseu’s picture

tacituseu commented
Status: Fixed » Needs work

Fails on PHP 5.5
https://www.drupal.org/pift-ci-job/874218
https://www.drupal.org/pift-ci-job/874226

1) Drupal\Tests\Core\Test\TestSuiteBaseTest::testAddTestsBySuiteNamespaceCore with data set "unit-tests" (array(array(array(), array(), array(array(array('

  • alexpott committed 10272b9 on 8.6.x 
    Revert "Issue #2932715 by mondrake, alexpott, vaplas: PHPUnit testing...

  • alexpott committed 4376366 on 8.5.x 
    Revert "Issue #2932715 by mondrake, alexpott, vaplas: PHPUnit testing...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 6-18-interdiff.txt969 bytes
new 2932715-18.patch1.08 KB

Yeah array_filter() is a better approach but unfortunately ARRAY_FILTER_USE_KEY is not part of PHP5.5. Soon we'll be able to use it but for now we need to flip the array a couple fo times.

neclimdul’s picture

neclimdul
He/Him
commented

Alternative approach developed for different reasons.
#2934670: Escape site root in TestSuite resolution

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@neclimdul I must be missing something but I can't see how #2934670: Escape site root in TestSuite resolution is going to solve the windows problem whereas I can see how the solution here solves that problem.

neclimdul’s picture

neclimdul
He/Him
commented

Escaping the path should fix the regex compilation fixing the original code. The problem 2934670 solves is essentially the same as this only the character triggering it was '@' and not on windows. I'm not tied to it as a solution I just wanted to bring it into the discussion since this was reverted.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@neclimdul from the other patch:
return !preg_match("@^$quoted_root/core/tests/Drupal/Tests/Listeners(/|$)@", dirname($test));
That is still not going to work for the Window's directory separator.

neclimdul’s picture

neclimdul
He/Him
commented

It should preg_quote will escape the backslashes. https://3v4l.org/Z0ggj

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@neclimdul I must be missing something. I can't see how the preg_quote is going to help Window's backslashes match the forward slashes in the preg_match's pattern.

Anonymous’s picture

Anonymous (not verified) commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

#18 excellent correction for php 5.5. Back to RTBC.

IS updated for better clarification this issue. Also a little more here:

  • @neclimdul, you are absolutely right about the benefits of preg_quote, when in regular expression there are unknown components, like $root. And #19 related issue has nice demonstration tests!
  • But in this issue we are not talking about special symbols, but about different symbols: '/' (Unix), '\' (Windows). And we want to check them with one regular expression.
  • We can use the common symbol "." for this (#2), but it gives a non-zero probability of incorrect processing (#6).
  • We can use something like \\|/ - but this is more complicated and we can not say that there is no third OS where another directory separator is used :)
  • @alexpott proposed a diamond solution - not to depend on the file path at all. Instead, use a namespace. As an excellent addition, it was possible to get rid of unknown characters from $root.
  • So we have a static regular expression, so all is fine. (All except php 5.5, of course). But #18 defeated this version. Woot!
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!

  • catch committed b8d89f6 on 8.6.x 
    Issue #2932715 by alexpott, mondrake, vaplas: PHPUnit testing fails on...

  • catch committed e8a2d7c on 8.5.x 
    Issue #2932715 by alexpott, mondrake, vaplas: PHPUnit testing fails on...
neclimdul’s picture

neclimdul
He/Him
commented

I'm really lost on what you guys are saying because the IS is specifically about special characters in the path.

PHP Fatal error: Uncaught preg_match(): Compilation failed: unknown property name after \P or \p at offset 5

.
I must be missing something.

neclimdul’s picture

neclimdul
He/Him
commented
Issue summary: View changes

Alex got me straight, I was so focused on the root and not catching that everyone was talking about the other half of the regex.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.