Problem

  • When calling file_unmanaged_*() functions in a test, exceptions are thrown, because custom stream wrappers are not properly registered.

Cause

  1. All registered custom stream wrappers of the parent site (test runner) are still registered when executing a (simple)test → they leak into the test environment.
  2. Tests do not register their own stream wrappers specific to the executed test.

Details

  • file_get_stream_wrappers() only works for WebTestBase, after static variables have been reset.
  • The private and temporary stream wrappers can only work if System module and its configuration has been installed.
  • Raw unit tests should not have access to any custom stream wrappers, but currently they do, because the stream wrappers of the test runner environment are leaking into unit tests, too.
  • Unlike all other global state in Drupal and tests, stream wrappers are a native global state construct of PHP core.

    → The state of registered stream wrappers has to properly managed and maintained manually.

Proposed solution

  1. In TestBase::prepareEnvironment(), all custom stream wrappers of the parent site are unregistered/removed.
  2. UnitTestBase does not get any stream wrappers.
  3. DrupalUnitTestBase gets dedicated methods for managing stream wrappers and it ensures to clean up after itself in tearDown().
  4. WebTestBase is not changed. file_get_stream_wrappers() returns the new set of custom stream wrappers after the test site has been installed.

.

Notes

  1. As stated in #2028109-108: Convert hook_stream_wrappers() to tagged services. (and following), Drupal's Stream Wrapper API needs a major overhaul for D8 to properly maintain the state of registered stream wrappers for a particular DrupalKernel boot, build, and rebuild.

    The concept required for DrupalKernel essentially has to follow the concept that is introduced for DrupalUnitTestBase here.

    → The admittedly ugly workarounds for file_get_stream_wrappers() should not hold up this patch, because this entire functionality has to be moved into DrupalKernel anyway.

  2. This issue blocks #2176141: Add a return value to file_save_htaccess()

    → which in turn blocks #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead

Files: 
CommentFileSizeAuthor
#39 interdiff.txt12.03 KBsun
#39 test.streams.39.patch39.78 KBsun
PASSED: [[SimpleTest]]: [MySQL] 63,418 pass(es).
[ View ]
#36 test.streams.36.patch36.6 KBsun
PASSED: [[SimpleTest]]: [MySQL] 63,235 pass(es).
[ View ]
#30 interdiff.txt1.27 KBsun
#30 test.streams.30.patch36.57 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch test.streams.30.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#28 interdiff.txt5.03 KBsun
#28 test.streams.28.patch36.34 KBsun
PASSED: [[SimpleTest]]: [MySQL] 60,021 pass(es).
[ View ]
#26 interdiff.txt5.58 KBsun
#26 test.streams.26.patch36.33 KBsun
PASSED: [[SimpleTest]]: [MySQL] 59,988 pass(es).
[ View ]
#24 interdiff.txt1.61 KBsun
#24 test.streams.23.patch35.32 KBsun
FAILED: [[SimpleTest]]: [MySQL] 60,008 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#22 test.streams.21.patch36.65 KBsun
FAILED: [[SimpleTest]]: [MySQL] 59,771 pass(es), 286 fail(s), and 300 exception(s).
[ View ]
#19 drupal8.test-stream-wrappers.19.patch11.92 KBsun
FAILED: [[SimpleTest]]: [MySQL] 43,178 pass(es), 8,191 fail(s), and 4,200 exception(s).
[ View ]
#17 drupal8.test-stream-wrappers.17.patch11.01 KBsun
FAILED: [[SimpleTest]]: [MySQL] 43,146 pass(es), 8,193 fail(s), and 4,247 exception(s).
[ View ]
#15 drupal8.test-bootstrap.15.patch4.75 KBsun
FAILED: [[SimpleTest]]: [MySQL] 22,420 pass(es), 268 fail(s), and 1,041 exception(s).
[ View ]
#12 drupal8.test-bootstrap.12.patch4.75 KBsun
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#12 drupal8.test-bootstrap.12.test-only.patch915 bytessun
FAILED: [[SimpleTest]]: [MySQL] 37,018 pass(es), 27 fail(s), and 99 exception(s).
[ View ]
#8 drupal8.test-bootstrap.8.patch7.11 KBsun
FAILED: [[SimpleTest]]: [MySQL] 34,174 pass(es), 143 fail(s), and 48 exception(es).
[ View ]
#6 drupal8.file-remote-stream-wrappers.6.patch5.87 KBsun
FAILED: [[SimpleTest]]: [MySQL] 34,201 pass(es), 45 fail(s), and 44 exception(es).
[ View ]
#3 drupal8.file-remote-stream-wrappers.3.patch2.24 KBsun
FAILED: [[SimpleTest]]: [MySQL] 33,846 pass(es), 220 fail(s), and 101 exception(es).
[ View ]
#2 drupal8.file-remote-stream-wrappers.2.patch512 bytessun
FAILED: [[SimpleTest]]: [MySQL] 34,219 pass(es), 27 fail(s), and 99 exception(es).
[ View ]
#1 drupal8.file-remote-stream-wrappers.1.patch2.98 KBsun
FAILED: [[SimpleTest]]: [MySQL] 34,228 pass(es), 27 fail(s), and 99 exception(es).
[ View ]
drupal8.file-unmanaged-stream-wrappers.0.patch4.76 KBsun
PASSED: [[SimpleTest]]: [MySQL] 34,100 pass(es).
[ View ]

Comments

StatusFileSize
new2.98 KB
FAILED: [[SimpleTest]]: [MySQL] 34,228 pass(es), 27 fail(s), and 99 exception(es).
[ View ]

Attached patch should expose the test failures.

StatusFileSize
new512 bytes
FAILED: [[SimpleTest]]: [MySQL] 34,219 pass(es), 27 fail(s), and 99 exception(es).
[ View ]

@beejeebus made me aware of the fact that all of these inherit from FileTestCase ;)

StatusFileSize
new2.24 KB
FAILED: [[SimpleTest]]: [MySQL] 33,846 pass(es), 220 fail(s), and 101 exception(es).
[ View ]

1) Stream wrappers should be registered in a full bootstrap, but setUp() eliminates pretty much everything that has been set up during bootstrap.

2) Even if some stuff would be retained, that wouldn't solve this problem. That is, because those File API tests attempt to directly use native PHP file functions on file URIs using a custom stream wrapper. The stream wrapper is registered through file_test.module only. And, circling back into 1), setUp() does not perform a full bootstrap after resetting everything and before executing the test.

Not sure why attached patch does not work. Technically, it should run a total bootstrap again, but the second debug() yields a NULL for me.

Issue tags:+DrupalWTF

sooo. the reason these tests pass with the bloat standard profile is:

1. we blow away all statics in setUp().

2. we have code in the standard profile that calls in to file_get_stream_wrappers():

standard_install --> theme_disable --> menu_rebuild --> menu_router_build --> call_user_func --> image_menu --> file_stream_wrapper_get_instance_by_scheme --> file_stream_wrapper_get_class --> file_get_stream_wrappers

3. file_get_stream_wrappers, which has no static cache anymore, *registers the 'dummy' and 'dummy-remote' wrappers with php*. obvious, yeah?

the upshot is: enabling a module does not register any stream wrappers it implements, until file_get_stream_wrappers' static cache is cleared AND it is called again.

hello drupalwtf.

Priority:Normal» Major
Status:Needs work» Needs review
StatusFileSize
new5.87 KB
FAILED: [[SimpleTest]]: [MySQL] 34,201 pass(es), 45 fail(s), and 44 exception(es).
[ View ]

So let's perform a proper, clean, and reliable bootstrap into the testing environment.

Apparently, the entire current testing environment is primed with data from the parent site.

That's mostly because we're resetting $conf and statics way too early; the subsequent functions calls are priming the statics again -- before we install the system.

A proper bootstrap also eliminates the resetAll() within the setUp() - as there's nothing anymore that could be reset!

Title:Drupal stream wrappers are not necessarily registered when calling file_unmanaged_*() functionsTests are executed in incomplete environment (was: Stream wrappers not registered when calling file_unmanaged_*())
Component:file system» simpletest.module
Status:Needs work» Needs review
Issue tags:+Testing system
StatusFileSize
new7.11 KB
FAILED: [[SimpleTest]]: [MySQL] 34,174 pass(es), 143 fail(s), and 48 exception(es).
[ View ]

Backup of my latest code; contains some @todos.

Summary:

  1. Some of the failing tests are the same as in #1373142: Use the Testing profile. Speed up testbot by 50%. This is important. It means that some of the failing tests actually do not pass. They only pass due to some weird intermixing of environment data from the parent site executing the test; e.g., as outlined in #4.
  2. We can resolve this by fixing setUp() to perform a clean and full bootstrap into the testing environment. A full bootstrap sets up various data; among that happens to be the registration of stream wrappers defined by modules. Essentially, the goal is to perform a bootstrap as in Drupal's installer, just without the installer.
  3. We are only touching the surface of #1215104: Use the non-interactive installer in WebTestBase::setUp() here. But it makes sense to continue with this patch, because the errors and exceptions we're bumping into seem to be the same I encountered for the patch over there. In other words, this patch will help us to unblock and do the other work.

Status:Needs work» Needs review

+++ b/core/modules/simpletest/drupal_web_test_case.php
@@ -1363,6 +1363,15 @@ class DrupalWebTestCase extends DrupalTestCase {
+    // @todo Theme is not reset after test bootstrap yet. Why?

See #1213536: Non-resettable theme_get_registry() cache causes problems for non-interactive installations

Issue tags:+needs backport to D7

Tagging.

StatusFileSize
new915 bytes
FAILED: [[SimpleTest]]: [MySQL] 37,018 pass(es), 27 fail(s), and 99 exception(s).
[ View ]
new4.75 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Holy cow. It's possible that #12 revealed some tests that false-positively pass in HEAD. (check the review log)

Priority:Major» Normal
Status:Needs work» Needs review
StatusFileSize
new4.75 KB
FAILED: [[SimpleTest]]: [MySQL] 22,420 pass(es), 268 fail(s), and 1,041 exception(s).
[ View ]

Re-rolled against HEAD.

It's possible that #1215104: Use the non-interactive installer in WebTestBase::setUp() might resolve parts of this patch though.

Issue summary:View changes

Updated issue summary.

Title:Tests are executed in incomplete environment (was: Stream wrappers not registered when calling file_unmanaged_*())Stream wrappers of parent site are leaking into all tests
Assigned:Unassigned» sun
Priority:Normal» Major
Issue summary:View changes
Status:Needs work» Needs review
Parent issue:» #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead
Related issues:+#2176141: Add a return value to file_save_htaccess()
StatusFileSize
new11.01 KB
FAILED: [[SimpleTest]]: [MySQL] 43,146 pass(es), 8,193 fail(s), and 4,247 exception(s).
[ View ]

Status:Needs work» Needs review
StatusFileSize
new11.92 KB
FAILED: [[SimpleTest]]: [MySQL] 43,178 pass(es), 8,191 fail(s), and 4,200 exception(s).
[ View ]

Hopefully this one resolves most of the test failures. Cherry-tested a few locally and they passed.

Status:Needs work» Needs review
StatusFileSize
new36.65 KB
FAILED: [[SimpleTest]]: [MySQL] 59,771 pass(es), 286 fail(s), and 300 exception(s).
[ View ]

Fixed a whole bunch of tests.

Also fixed File Managed API tests (File module) to no longer use the File API test base class (System module).

Status:Needs work» Needs review
StatusFileSize
new35.32 KB
FAILED: [[SimpleTest]]: [MySQL] 60,008 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new1.61 KB

Reverted TestBase::$public_files_directory path change → that's only possible with #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead

StatusFileSize
new36.33 KB
PASSED: [[SimpleTest]]: [MySQL] 59,988 pass(es).
[ View ]
new5.58 KB
  1. Clarified and polished new documentation about stream wrappers in tests.
  2. Reverted unnecessary relocation of test files directory deletion (does not use a stream wrapper).

Is there any way to get a test-only patch to fail here a la #12? That would be pretty cool IMO to understand and prove what is currently broken.

Other than that I only have nitpicks.

  1. +++ b/core/modules/file/lib/Drupal/file/Tests/FileManagedTestBase.php
    @@ -90,6 +90,51 @@ function assertFileHookCalled($hook, $expected_count = 1, $message = NULL) {
    +   * timestamp.
    ...
    +   * Check that two files have the same values for all fields other than the

    Check -> Checks

    Also should be on one line. I would suggest leaving the timestamp info out of the one-line description and sticking that into a separate line.

  2. +++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
    @@ -64,6 +64,15 @@
    +  private $stream_wrappers = array();

    Since this seems to be introduced here, it should be $streamWrappers

  3. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
    @@ -963,6 +963,19 @@ protected function prepareEnvironment() {
    +    // - WebTestBase re-initializes Drupal stream wrappers are installation.

    "after" installation?

Issue summary:View changes
StatusFileSize
new36.34 KB
PASSED: [[SimpleTest]]: [MySQL] 60,021 pass(es).
[ View ]
new5.03 KB

Fixed coding style and comment issues.

Updating issue summary to properly reflect the original bug report and proposed solution.

  1. +++ b/core/includes/file.inc
    @@ -191,12 +191,16 @@ function file_get_stream_wrappers($filter = STREAM_WRAPPERS_ALL) {
    +      drupal_alter('stream_wrappers', $wrappers);

    Nitpick: We could just use the alter method on the module handler instead.

  2. +++ b/core/modules/file/lib/Drupal/file/Tests/DownloadTest.php
    @@ -17,7 +17,7 @@ public static function getInfo() {
    +++ b/core/modules/file/lib/Drupal/file/Tests/CopyTest.php
    @@ -15,7 +15,7 @@ public static function getInfo() {
           'description' => 'Tests the file copy function.',
    -      'group' => 'File API',
    +      'group' => 'File Managed API',
         );
    +++ b/core/modules/file/lib/Drupal/file/Tests/DeleteTest.php
    @@ -15,7 +15,7 @@ public static function getInfo() {
       }
    +++ b/core/modules/file/lib/Drupal/file/Tests/DownloadTest.php
    @@ -17,7 +17,7 @@ public static function getInfo() {
           'name' => 'File download',
           'description' => 'Tests for file download/transfer functions.',
    -      'group' => 'File API',
    +      'group' => 'File Managed API',
         );
    +++ b/core/modules/file/lib/Drupal/file/Tests/DeleteTest.php
    @@ -15,7 +15,7 @@ public static function getInfo() {
    -      'group' => 'File API',
    +      'group' => 'File Managed API',
         );

    These changes are a bit out of scope but yeah do whatever you think is good.

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/File/UrlRewritingTest.php
    @@ -95,6 +97,11 @@ function testRelativeFileURL() {
    +    // Create a mock Request for file_url_transform_relative().
    +    $request = Request::create($GLOBALS['base_url']);
    +    $this->container->set('request', $request);

    This is a bit odd, have you tried using preparerRequestForGenerator instead?

  4. +++ b/core/modules/system/lib/Drupal/system/Tests/File/RemoteFileDirectoryTest.php
    @@ -19,6 +19,9 @@ class RemoteFileDirectoryTest extends DirectoryTest {
    +  protected $classname = 'Drupal\file_test\DummyRemoteStreamWrapper';
    ...
    +  protected $scheme = 'dummy-remote';

    Can we document these two variables on the baseclass?

StatusFileSize
new36.57 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch test.streams.30.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.27 KB
  1. Use ModuleHandler::alter() in file_get_stream_wrappers() while being there.
  2. Added phpDoc for preexisting FileTestBase::$scheme and $classname properties while being there.

re: group changes:

There was a really weird intermixing of classes going on — File module's FileManagedTestBase extended System module's FileTestBase class, whereas both serve completely different purposes. Not even the System\FileTestBase class methods were used by System's File API tests.

Additionally, only the tests based on FileManagedTestBase are web tests. All of the File API tests are low-level tests and hence converted to DrupalUnitTestBase here → massive speedup.

That said, FileTestBase is primarily converted to DUTB to ensure that the stream wrapper management facility works correctly for those tests.

re: mock Request in UrlRewritingTest:

The preparerRequestForGenerator() method only exists on WebTestBase (while this is DUTB) and only makes sense there. The mock Request is merely needed, because file_url_transform_relative() needs one, whereas DUTB tests don't have one (and probably shouldn't have one).

Status:Needs review» Reviewed & tested by the community

Looked through this again and looks fine to me.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 30: test.streams.30.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new36.6 KB
PASSED: [[SimpleTest]]: [MySQL] 63,235 pass(es).
[ View ]

Merged 8.x. Simple diff context conflict, so no interdiff, and back to RTBC.

36: test.streams.36.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work
  1. +++ b/core/includes/file.inc
    @@ -202,12 +202,16 @@ function file_get_stream_wrappers($filter = STREAM_WRAPPERS_ALL) {
    +    $container = \Drupal::getContainer();
    +    if (is_object($container) && $container->has('module_handler')) {

    Considering that \Drupal::getContainer() is currently deprecated perhaps we should add Drupal::has() that checks both the container's and the service's existence?

  2. +++ b/core/modules/file/lib/Drupal/file/Tests/FileManagedTestBase.php
    @@ -7,14 +7,14 @@
      * hooks.
    +++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
    @@ -132,10 +142,28 @@ protected function setUp() {
    +    // The private and temporary stream wrappers depend on the system.file
    +    // configuration. If System module was enabled, the test will most likely
    +    // install System module's configuration, so we can register them here.

    That is an interesting assumption that I think is often not true.

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/File/RemoteFileUnmanagedSaveDataTest.php
    @@ -19,6 +19,9 @@ class RemoteFileUnmanagedSaveDataTest extends UnmanagedSaveDataTest {
    +  protected $scheme = 'dummy-remote';
    +  protected $classname = 'Drupal\file_test\DummyRemoteStreamWrapper';
    +++ b/core/modules/system/lib/Drupal/system/Tests/File/RemoteFileUnmanagedMoveTest.php
    @@ -19,6 +19,9 @@ class RemoteFileUnmanagedMoveTest extends UnmanagedMoveTest {
    +  protected $scheme = 'dummy-remote';
    +  protected $classname = 'Drupal\file_test\DummyRemoteStreamWrapper';
    +++ b/core/modules/system/lib/Drupal/system/Tests/File/RemoteFileUnmanagedDeleteTest.php
    @@ -19,6 +19,9 @@ class RemoteFileUnmanagedDeleteTest extends UnmanagedDeleteTest {
    +  protected $scheme = 'dummy-remote';
    +  protected $classname = 'Drupal\file_test\DummyRemoteStreamWrapper';
    +++ b/core/modules/system/lib/Drupal/system/Tests/File/RemoteFileUnmanagedDeleteRecursiveTest.php
    @@ -19,6 +19,9 @@ class RemoteFileUnmanagedDeleteRecursiveTest extends UnmanagedDeleteRecursiveTes
    +  protected $scheme = 'dummy-remote';
    +  protected $classname = 'Drupal\file_test\DummyRemoteStreamWrapper';
    +++ b/core/modules/system/lib/Drupal/system/Tests/File/RemoteFileUnmanagedCopyTest.php
    @@ -19,6 +19,9 @@ class RemoteFileUnmanagedCopyTest extends UnmanagedCopyTest {
    +  protected $scheme = 'dummy-remote';
    +  protected $classname = 'Drupal\file_test\DummyRemoteStreamWrapper';
    +++ b/core/modules/system/lib/Drupal/system/Tests/File/RemoteFileScanDirectoryTest.php
    @@ -19,6 +19,9 @@ class RemoteFileScanDirectoryTest extends ScanDirectoryTest {
    +  protected $scheme = 'dummy-remote';
    +  protected $classname = 'Drupal\file_test\DummyRemoteStreamWrapper';
    +++ b/core/modules/system/lib/Drupal/system/Tests/File/RemoteFileDirectoryTest.php
    @@ -19,6 +19,9 @@ class RemoteFileDirectoryTest extends DirectoryTest {
    +  protected $scheme = 'dummy-remote';
    +  protected $classname = 'Drupal\file_test\DummyRemoteStreamWrapper';

    Missing doc blocks

Status:Needs work» Needs review
StatusFileSize
new39.78 KB
PASSED: [[SimpleTest]]: [MySQL] 63,418 pass(es).
[ View ]
new12.03 KB
  1. Not going to address that here, since #2028109: Convert hook_stream_wrappers() to tagged services. will remove that entire function anyway later on.
  2. Removed assumption about private stream wrapper + System module config from DrupalUnitTestBase.
  3. Added phpDoc for preexisting $scheme and $classname test properties.

Given how many other major/critical issues are currently blocked on this fix, it would be great to move forward here as quickly as possible.

I agree about #38.1. That code won't be living long.

Everything else has been addressed. RTBC assuming this comes back green.

Status:Needs review» Needs work

The last submitted patch, 39: test.streams.39.patch, failed testing.

39: test.streams.39.patch queued for re-testing.

Status:Needs work» Reviewed & tested by the community

Back to RTBC, I guess.

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Committed 856d6e1 and pushed to 8.x. Thanks!

Assigned:sun» Unassigned

Hm. Given that there are a few necessary adjustments to individual tests, I'm not sure whether it is safe to backport this change.

We can certainly try, of course.

A backport is only acceptable if it is possible with zero changes to individual test classes.