Problem/Motivation

When running functional tests with a different user from the one that is running your webserver, an exception can be thrown and cause confusion.

1) Drupal\Tests\help\Functional\HelpTest::testHelp
Exception: Warning: mkdir(): Permission denied
Drupal\Component\PhpStorage\FileStorage->createDirectory() (Line: 165)

The error is misleading because even if you're running the command as root or have 777 permissions on *all* your docroot (for testing purposes only), this will not make a difference as you'd still not be running the command with the user PHPUnit expects.

Proposed resolution

If we cannot improve the exception, at least let's update the documentation in code so that people know what to expect and make changes to their command accordingly.

It'll be a good idea to also update https://www.drupal.org/node/2116263 after that issue has been committed.

Remaining tasks

Review patch. Agree on the approach.

User interface changes

None.

API changes

None.

Data model changes

None.

Comments

anavarre created an issue. See original summary.

anavarre’s picture

StatusFileSize
new1.45 KB

Comments in README files should also probably be wrapped at 80 cols.

anavarre’s picture

Issue summary: View changes
anavarre’s picture

Issue tags: +PHPUnit, +Documentation
cilefen’s picture

This is the same situation as with simpletest in Drupal 7. It isn't actually true that it must be the same user. A user in the same group as the web server user will do if file permissions are configured accordingly.

anavarre’s picture

StatusFileSize
new1.46 KB
new754 bytes

Works better now?

anavarre’s picture

StatusFileSize
new1.46 KB
new535 bytes

Typo made it through.

anavarre’s picture

StatusFileSize
new1.73 KB
new718 bytes

Came back to this because I found a tricky one with printer which needs to be printerClass instead. Code was correct in http://cgit.drupalcode.org/drupal/tree/core/phpunit.xml.dist?id=9d5b5a53... but there was a regression in #2706113: Disable Drupal\Tests\Listeners\HtmlOutputPrinter by default because PHPStorm is broken it seems.

anavarre’s picture

Issue summary: View changes
jibran’s picture

Status: Needs review » Needs work
+++ b/core/tests/README.md
@@ -13,3 +13,17 @@
   ./vendor/bin/phpunit -c core --testsuite functional
   ./vendor/bin/phpunit -c core --testsuite functional-javascript
...
+sudo -u www-data ./vendor/bin/phpunit -c core --testsuite functional
+sudo -u www-data ./vendor/bin/phpunit -c core --testsuite functional-javascript

There should be an equal sign '=' between --testsuite and the argument.

n.kishorekumar’s picture

Assigned: Unassigned » n.kishorekumar
anavarre’s picture

StatusFileSize
new1.95 KB
new935 bytes
new1.95 KB
new935 bytes

Help command said --testsuite <pattern> Filter which testsuite to run. so it didn't look obvious.

EDIT: @N.kishorekumar and I have cross-posted, thus why the mess with the file upload. Sorry about that.

anavarre’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/phpunit.xml.dist
@@ -6,8 +6,11 @@
+-->
+<!-- PHPUnit expects functional tests to be run with either a privileged user
+ or your Unix user. See core/tests/README.md for details.
 -->

I really like this! Of course we can improve this documentation a lot more, its a process, rather than a steady state, but this itself improves things. I have a bit of a feeling that the grammar is weird, but its really just a feeling.

claudiu.cristea’s picture

Status: Reviewed & tested by the community » Needs work

Who's running tests? In the most of the cases the developer who also wrote that test. And writing a test always brings you to BrowserTestBase class. I would provide full instructions in the docblock of that class. BTW, it's the first time I notice that we have a core/tests/README.md file :)

Also I would add that printerClass attribute/value should go inside <phpunit ...> tag. I was confused initially by the phpunit.xml.dist documentation, it was not very clear where that should go.

anavarre’s picture

The question should probably be: to what extent what already exists in the documentation page should be added in code as well? Perhaps we should simply keep things fairly straight forward and link to https://www.drupal.org/node/2116263 for more details?

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community

Ok, then.

klausi’s picture

meh, don't do the "su www-data" dance if you are developing locally. Run Apache/Nginx under your own user account instead, as described in https://klau.si/dev

The last submitted patch, 12: 2760905-11.patch, failed testing.

anavarre’s picture

Assigned: n.kishorekumar » Unassigned
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new3.15 KB
new2.43 KB

This new patch addresses the concern raised in #18 and a mix of #15 and #16.

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

This is a really nice improvment and will help people. But...

+++ b/core/phpunit.xml.dist
@@ -1,19 +1,23 @@
+<!-- PHPUnit expects functional tests to be run with either a privileged user
+ or your Unix user. See core/tests/README.md and

+++ b/core/tests/README.md
@@ -10,6 +10,29 @@
+web server user. You can either configure Apache (or nginx) to run as your own
+Unix user or run tests as a privileged user instead.
...
+To develop locally, the recommended approach is to run tests as your own Unix
+user. To achieve that, configure `/etc/apache2/envvars` and change the default

I think saying "Unix user" is a bit weird. What is someone who uses OS X or windows to think?

claudiu.cristea’s picture

Status: Needs work » Needs review
StatusFileSize
new3.17 KB
new1.36 KB
claudiu.cristea’s picture

Status: Needs work » Needs review
StatusFileSize
new3.17 KB
new1.36 KB
claudiu.cristea’s picture

Sorry for posting twice. The site was stuck and I pressed again the button.

Status: Needs review » Needs work

The last submitted patch, 23: 2760905-23.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
anavarre’s picture

Status: Needs review » Reviewed & tested by the community

Fair enough.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/phpunit.xml.dist
    @@ -1,19 +1,23 @@
     <!-- TODO set checkForUnintentionallyCoveredCode="true" once https://www.drupal.org/node/2626832 is resolved. -->
    

    Why is this TODO staying here when you move the other... I think we should move this one too.

  2. +++ b/core/tests/README.md
    @@ -10,6 +10,29 @@
    +  ./vendor/bin/phpunit -c core --testsuite=functional
    +  ./vendor/bin/phpunit -c core --testsuite=functional-javascript
    ...
    +sudo -u www-data ./vendor/bin/phpunit -c core --testsuite=functional
    +sudo -u www-data ./vendor/bin/phpunit -c core --testsuite=functional-javascript
    

    This is not right - at least according to the help when I do phpunit --help

    Test Selection Options:
    
      --filter <pattern>        Filter which tests to run.
      --testsuite <pattern>     Filter which testsuite to run.
    
  3. Testing for me shows that #10 is not right.

anavarre’s picture

1.

Why is this TODO staying here when you move the other... I think we should move this one too.

It's been moved following the comment in #15 because it can be confusing for people to know where to put printerClass - I tried to make sense of https://youtrack.jetbrains.com/issue/WI-24808 but couldn't really tell if it's fixed or not. If it is, then we can remove the ToDo altogether and add printerClass by default, I think. If we still need it, then IDK if it's important to keep the ToDos grouped or not? It was only moved to prevent any confusion with printerClass, nothing else.

2. I was also under the impression --filter <pattern> didn't need the equal sign but it works equally well so I don't have any strong opinion.

anavarre’s picture

Any additional feedback?

dawehner’s picture

2. I was also under the impression --filter
didn't need the equal sign but it works equally well so I don't have any strong opinion.

Well, let's go with what the official documentation says ...

anavarre’s picture

Status: Needs work » Needs review
StatusFileSize
new2.94 KB
new935 bytes

Alright.

amitaibu’s picture

+++ b/core/tests/README.md
@@ -13,3 +13,26 @@
+user. To achieve that, configure `/etc/apache2/envvars` and change the default
+Apache user to run as your system user. Example:

I'm running into this problem, and that file doesn't seem to exist on my Mac (v10.10)

amitaibu’s picture

On Mac the config should be under /etc/apache2/httpd.conf. I think it's probably better to document the location in the docs, instead of the patch - as I suspect it will differ from installation to installation.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

amitaibu’s picture

To continue from the above comment, and more specifically, for mac I found installation under /etc/apache2/httpd.conf in this section:

<IfModule unixd_module>
#
# If you wish httpd to run as a different user or group, you must run
# httpd as root initially and it will switch.
#
# User/Group: The name (or #number) of the user/group to run httpd as.
# It is usually good practice to create a dedicated user and group for
# running httpd, as with most system services.
#
User _www
Group _www

</IfModule>
anavarre’s picture

StatusFileSize
new3.02 KB
new646 bytes

I agree it's better to have this in the d.o. documentation. However, improving the README file can't really hurt unless the file name changes indeed. How about this patch for now?

dawehner’s picture

+++ b/core/tests/README.md
@@ -13,3 +13,29 @@
+To develop locally, the recommended approach is to run tests as your own system
+user. To achieve that, change the default Apache user to run as your system
+user. Typically, you'd need to modify `/etc/apache2/envvars` on Linux or

Mh, this approach is potentially less secure, as your database server then might be able to access any data of your user. We should at least have some sort of notion that this is less secure than the other option.

anavarre’s picture

StatusFileSize
new3.05 KB
new858 bytes

I thought mentioning it's for developing locally was enough. Improved verbiage in this patch.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you! Personally I think we can always iterate on here

amitaibu’s picture

+++ b/core/tests/README.md
@@ -13,3 +13,29 @@
+`/etc/apache2/envvars` on Linux or `/etc/apache2/httpd.conf` on the Mac.

the Mac => Mac

Also, maybe we can give a Mac example:

User <your-user>
Group <your-group>
anavarre’s picture

StatusFileSize
new3.12 KB
new819 bytes

There you go.

amitaibu’s picture

Great, thanks. Seems ready.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This is helpful and a good start and I agree with @dawehner this can be iterated on further.

Committed and pushed cf37e90 to 8.3.x and c40dd5d to 8.2.x and dbcfdad to 8.1.x. Thanks!

  • alexpott committed cf37e90 on 8.3.x
    Issue #2760905 by anavarre, claudiu.cristea, amitaibu, dawehner: The...

  • alexpott committed c40dd5d on 8.2.x
    Issue #2760905 by anavarre, claudiu.cristea, amitaibu, dawehner: The...

  • alexpott committed dbcfdad on 8.1.x
    Issue #2760905 by anavarre, claudiu.cristea, amitaibu, dawehner: The...

Status: Fixed » Closed (fixed)

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