Problem

PHP Warning:  fopen(/phpunit-1103.xml): failed to open stream: Permission denied
  in /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/phpunit/phpunit/PHPUnit/Util/Printer.php on line 105

PHP Warning:  Invalid argument supplied for foreach()
  in /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh on line 658

Cause

simpletest.module:

function simpletest_phpunit_xml_filepath($test_id) {
  return drupal_realpath('public://simpletest') . '/phpunit-' . $test_id . '.xml';
}

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
File a followup issue Novice
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Status: Active » Needs review
FileSize
487 bytes
sun’s picture

This fixes the issue for me locally.

How to reproduce/test:

$ php core\scripts\run-tests.sh --php php --url http://drupal8.test/ --sqlite test/test.sqlite --dburl sqlite://test/db.sqlite --class Drupal\Tests\Core\Extension\ThemeHandlerTest
sun’s picture

One more tweak - adding $recursive = TRUE to mkdir('public://simpletest')

sun’s picture

The last submitted patch, 1: drupal8.run-tests-stream.1.patch, failed testing.

The last submitted patch, 2: drupal8.run-tests-stream.2.patch, failed testing.

John_B’s picture

Status: Needs review » Reviewed & tested by the community

I reproduced the warning on a clean git copy of 8.x-alpha11 (without an installed site), tested the patch, which works, and read the code.

In the command to reproduce in #2, db name and path should be same for --sqlite and --dburl (?). Also I cannot get run-tests.sh to work without quotation marks round the class name - maybe there should be a new issue to document this?

Marking as RTBC. I am pretty new to this. Still it seems uncontroversial and the code is straightforward.

catch’s picture

Status: Reviewed & tested by the community » Needs review

Is there an issue for

+ // @todo Remove dependency on public://simpletest from Simpletest.

?
From the issue title that's what I expected the patch to do.

ParisLiakos’s picture

i guess we can commit this as a stopgag and continue then here to actually remove the dependency?

sun’s picture

Alright, so here's the fundamental problem space + root cause:

The test runner needs a filesystem path to write to. Period.

Whether that is for PHPUnit XML result files, or for a --sqlite database, or for the --xml junit output, or anything else, doesn't matter. We need a writable filesystem path.

In HEAD, we're working around that problem by installing an entire Drupal site for the test runner. In turn, public://simpletest/ is available to the test runner, which means that it has a dedicated place to write to.

If there is no Drupal site for the test runner, then there is no "public files directory" (at all).

Along the same lines, simpletest_requirements() creates the (writable) /sites/simpletest base directory that holds the test site directories for each executed Simpletest test.

If Simpletest module is not installed, then that directory does not exist. The only reason the testbot did not complain is because the testbot not only operates a root user, but additionally performs a blatant chmod -rf 0777 ./* prior to executing run-tests.sh, which in turn means that the test runner has write access everywhere.

Whenever I'm using the new "headless" --sqlite + --dburl parameters on my local box, I'm specifying a writable path for all the things that need to be written by the test runner; e.g.:

$ rm -rf build
$ mkdir build
$ run-tests.sh \
  --sqlite build/test.sqlite \
  --dburl sqlite://localhost/build/db.sqlite \
  --xml build

However, (1) that doesn't address /sites/simpletest which happens to exist on my disk already, and (2) I'm running on Windows, so I generally face no file permission issues (much rather the opposite; a malformed test run would be able to delete tons of other filesystem data).

The existing patch here essentially retains public://simpletest/ as-is; i.e., for "headless" test runs the default directories of a Drupal site are assumed, even though there is no Drupal site. If you'd start from a completely clean slate, that would translate into:

# sites/default/files == build
# ./simpletest subdirectory is needed for verbose browser output.
# The permissions assume that the webserver system user is not in the same group as current user.
$ mkdir -p sites/default/files/simpletest
$ mkdir sites/simpletest
$ chmod -R 0777 sites/default/files
$ chmod 0777 sites/simpletest
$ run-tests.sh \
  --sqlite sites/default/files/test.sqlite \
  --dburl sqlite://localhost/sites/default/files/db.sqlite \
  --xml sites/default/files

Whichever way we go, we'll need at least these two writable filesystem locations. Since code relies on public://simpletest anyway already, we could just simply make the public files directory of the (assumed) default site that required writable directory.

The sites/simpletest directory is required in any case, because Drupal needs to detect that before Drupal is bootstrapped in any way.


FWIW, this problem space is not limited to the test runner + Simpletest. PHPUnit tests should have a dedicated filesystem location to be able to write to, too. Not only for test results, but also for dynamically generated test data - which is very common for PHPUnit tests normally; our PHPUnit tests in core just weren't able to do that yet, because they have no filesystem directory to write to.

catch’s picture

Just to be clear on #8, I'm fine committing this, just prefer follow-up issues to @todos. A @todo without an issue is either an empty apology or wishful thinking.

xjm’s picture

Issue summary: View changes
Issue tags: +Needs followup, +Novice, +TCDrupal 2014

So to be clear, the only thing blocking this issue is to file the followup issue based on the @todo and #10?

Sounds like a straightforward novice task.

RainbowArray’s picture

Issue summary: View changes
Issue tags: -Needs followup, -Novice

Filed a follow-up issue here. Feel free to rewrite the issue summary if my understanding of the problem is incorrect. #2318797: Remove dependency on public://simplestest from Simpletest

RainbowArray’s picture

Issue tags: +Needs reroll

Patch in #3 no longer applies. Needs reroll.

RainbowArray’s picture

This patch modifies simpletest_script_bootstrap, which seems to no longer exist. Haven't been able to find an equivalent where the code additions in this patch might apply.

xjm’s picture

@mdrummond, maybe some potential leads:

[mandelbrot:drupal | Sun 16:43:21] $ git log -S 'simpletest_script_bootstrap' --after=2014-04-24
commit 5e58da00e6472598de73b9156daa935aa2743910
Author: Nathaniel Catchpole <catch@35733.no-reply.drupal.org>
Date:   Thu Jun 26 11:47:01 2014 +0100

    Issue #2016629 by larowlan, neclimdul, sun, alexpott, jibran, ParisLiakos, donquixote, effulgentsia, msonnabaum: Refactor bootstrap to better utilize the kernel.

commit 6d2ce628bc9362588e209a6ddac37295b40b3ce9
Author: Alex Pott <alex.a.pott@googlemail.com>
Date:   Thu Jun 5 12:53:24 2014 -0500

    Revert "Issue #2016629 by larowlan, neclimdul, sun, alexpott, jibran, ParisLiakos, donquixote, effulgentsia, msonnabaum: Refactor bootstrap to better utilize the kernel."
    
    This reverts commit cda051c425ec10e864d3207cc0e60640bfa16e10.

commit cda051c425ec10e864d3207cc0e60640bfa16e10
Author: Nathaniel Catchpole <catch@35733.no-reply.drupal.org>
Date:   Thu Jun 5 11:30:04 2014 +0100

    Issue #2016629 by larowlan, neclimdul, sun, alexpott, jibran, ParisLiakos, donquixote, effulgentsia, msonnabaum: Refactor bootstrap to better utilize the kernel.
RainbowArray’s picture

simpletest_script_bootstrap was removed in #2016629: Refactor bootstrap to better utilize the kernel. Will look for answers there.

sun’s picture

Re-rolled against HEAD, additionally taking #10 into account.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

That will work for now.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: drupal8.run-tests-stream.18.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 18: drupal8.run-tests-stream.18.patch, failed testing.

Status: Needs work » Needs review
sun’s picture

Status: Needs review » Reviewed & tested by the community

  • alexpott committed 234382b on 8.0.x
    Issue #2248845 by sun: Fixed [run-tests.sh]...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 234382b and pushed to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

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