Problem/Motivation

We see: Fatal error: Trait 'Drupal\Tests\SessionTestTrait' not found See comment #10 for a stack trace.

Steps to reproduce (only with Drupal Core)

  • run on the Testing UI a test that ends up in a fatal error
  • end up on a page like e.g.
    An error has occurred.
    Please continue to the error page
    An AJAX HTTP error occurred.
    HTTP Result Code: 200
    Debugging information follows.
    Path: /[...]/batch?id=67&op=do_nojs&op=do
    StatusText: OK
    ResponseText: 
    Fatal error:  Call to undefined method Drupal\file_mdm\FileMetadataManager::useUri() in /[...]/modules/file_mdm_exif/src/Tests/FileMetadataExifTest.php on line 107
  • click on the link to the error page
  • bum: Fatal error: Trait 'Drupal\Tests\SessionTestTrait' not found in /[...]/core/modules/simpletest/src/TestBase.php on line 28

Steps to reproduce (using Drush)
drush php-eval 'simpletest_clean_environment()'

It turns out that some refactoring related to WTB->BTB conversions moved the traits required by TestBase into the core/tests/ directory, which is only autoloadable during test run-time. This means that during normal run-time those traits are not discoverable.

This also led to another issue with simpletest_clean_environment(), which similarly needs to load TestBase. During uninstall time for simpletest, PHP is unable to completely load TestBase: #2799333: Fatal error when uninstall Testing module due to traits of TestBase

Proposed resolution

For _simpletest_batch_finished():

Minimally refactor TestBase::insertAssert() to TestDatabase::insertAssert(). This is a better place for that method anyway, and TestDatabase does not require special class loading.

For simpletest_clean_environment():

Ensure that simpletest_clean_environment() loads the \Drupal\Testing namespace by calling registerTestNamespaces() on the test_discovery service.

Remaining tasks

File an issue to deprecate TestBase::insertAssert() and TestBase::deleteAssert() in favor of duplicate methods on TestDatabase.

Refactor the simpletest_cleanup*() methods into a more maintainable helper class/service: #2800267: Turn simpletest_clean*() functions into a helper class

User interface changes

API changes

  • Add an optional parameter to simpletest_clean_temporary_directories() (no change to default behavior).
  • Remove the identical, public methods filePreDeleteCallback() from Drupal\simpletest\TestBase and Drupal\simpletest\BrowserTestBase.

Data model changes

CommentFileSizeAuthor
#79 2745123-2-79.patch682 bytesalexpott
#78 2745123_78.patch5.76 KBjofitz
#76 simpletest-simpletest-module-crashes-2745123-76-D8.patch6.65 KBnikolas.costa
#74 2745123_74.patch1.12 KBdrugan
#71 interdiff-67-71.txt5.16 KBdrugan
#71 2745123_71.patch9.37 KBdrugan
#67 interdiff-59-67.txt11.44 KBdrugan
#67 2745123_67.patch8.5 KBdrugan
#60 interdiff-57-59.txt2.79 KBmpdonadio
#60 interdiff-52-57.txt2.71 KBmpdonadio
#59 2745123_59.patch8.23 KBdrugan
#57 2745123_57.patch8.01 KBdrugan
#56 2745123_56.patch7.99 KBdrugan
#55 2745123-experiment.patch344.08 KBmpdonadio
#54 interdiff-49-52.txt3.2 KBmpdonadio
#52 2745123_52.patch7.5 KBdrugan
#51 interdiff-2745123-40-49.txt2.17 KBbenjifisher
#49 2745123_49.patch6.73 KBdrugan
#40 2745123_40.patch6.31 KBdrugan
#38 2745123_38.patch6.83 KBdrugan
#35 2745123_35.patch7.28 KBdrugan
#32 2745123_32.patch7.65 KBdrugan
#29 2745123_24.patch2.27 KBdrugan
#26 2745123_26.patch6.44 KBslasher13
#22 interdiff.txt2.53 KBMile23
#22 2745123_22.patch6.39 KBMile23
#18 2745123_18.patch6.55 KBMile23
#12 2745123_12.patch1.23 KBblazey
#10 fatal_error_test-do-not-test.patch557 bytesMile23
#10 2745123_10.patch735 bytesMile23
#7 test_base_traits_not_found-2745123-7.patch6.12 KByogeshmpawar
#3 2745123-3--test_base_traits_not_found.patch6.04 KBdrunken monkey
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

benjy’s picture

This also occurs when trying to uninstall simpletest, I can't reliably reproduce but i'm going to guess it has something to do with the fact the simpletest module registers the autoloading namespaces for those classes.

drunken monkey’s picture

Status: Active » Needs review
FileSize
6.04 KB

I can confirm this problem, it has been happening for some time.
The same happens (for me) when clicking on the "Clean environment" button.

Without any closer knowledge of the module or the potential source of the bug, the attached patch would in any case be a working fix for now, to avoid the fatal errors and make things work as normal: just don't use the traits in that class, but copy their contents into it.

By the way: Does this qualify as "Major"? It's a fatal error that happens (as far as I can tell) consistently for everyone – but it's only in the testing module, so no production relevancy.

fietserwin’s picture

I have closed [2777953] as duplicate, I have copied relevant info from the comments (from @dawehner, @xjm and me) over there to this comment.

- Changing to major as per the other issue.
- Probably caused by #2702281: Move Drupal\simpletest\RandomGeneratorTrait, Drupal\simpletest\WebAssert and Drupal\simpletest\SessionTestTrait into Drupal\Tests namespace.
- Autoloader cannot find the trait as there's no reference to the Drupal/Tests namespace.
- Also occurring on 8.1.7 (the above mentioned issue was also committed to 8.1.x).
- Stack trace:

( ! ) Fatal error: Trait 'Drupal\Tests\SessionTestTrait' not found in /Users/dawehner/www/d8/core/modules/simpletest/src/TestBase.php on line 25
Call Stack
#	Time	Memory	Function	Location
1	0.0012	244816	{main}( )	.../index.php:0
2	0.0034	543360	Drupal\Core\DrupalKernel->handle( )	.../index.php:19
3	0.0146	1635088	Stack\StackedHttpKernel->handle( )	.../DrupalKernel.php:649
4	0.0146	1635200	Drupal\Core\StackMiddleware\NegotiationMiddleware->handle( )	.../StackedHttpKernel.php:23
5	0.0146	1635592	Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle( )	.../NegotiationMiddleware.php:50
6	0.0146	1636024	Drupal\page_cache\StackMiddleware\PageCache->handle( )	.../ReverseProxyMiddleware.php:47
7	0.0146	1636528	Drupal\page_cache\StackMiddleware\PageCache->pass( )	.../PageCache.php:78
8	0.0146	1636608	Drupal\Core\StackMiddleware\KernelPreHandle->handle( )	.../PageCache.php:99
9	0.0215	1984272	Drupal\Core\StackMiddleware\Session->handle( )	.../KernelPreHandle.php:47
10	0.0258	2142112	Symfony\Component\HttpKernel\HttpKernel->handle( )	.../Session.php:57
11	0.0259	2142968	Symfony\Component\HttpKernel\HttpKernel->handleRaw( )	.../HttpKernel.php:62
12	0.0723	4010960	call_user_func_array:{/Users/dawehner/www/d8/vendor/symfony/http-kernel/HttpKernel.php:139} ( )	.../HttpKernel.php:139
13	0.0723	4011160	Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}( )	.../HttpKernel.php:139
14	0.0723	4011408	Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext( )	.../EarlyRenderingControllerWrapperSubscriber.php:97
15	0.0724	4023352	Drupal\Core\Render\Renderer->executeInRenderContext( )	.../EarlyRenderingControllerWrapperSubscriber.php:124
16	0.0725	4023800	Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}( )	.../Renderer.php:574
17	0.0725	4023848	call_user_func_array:{/Users/dawehner/www/d8/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php:123} ( )	.../EarlyRenderingControllerWrapperSubscriber.php:123
18	0.0725	4024544	Drupal\Core\Controller\FormController->getContentResult( )	.../EarlyRenderingControllerWrapperSubscriber.php:123
19	0.0736	4217688	Drupal\Core\Form\FormBuilder->buildForm( )	.../FormController.php:74
20	0.1534	22444952	Drupal\Core\Form\FormBuilder->processForm( )	.../FormBuilder.php:314
21	5.0272	78519952	Drupal\Core\Form\FormSubmitter->doSubmitForm( )	.../FormBuilder.php:583
22	5.0272	78520064	Drupal\Core\Form\FormSubmitter->executeSubmitHandlers( )	.../FormSubmitter.php:51
23	5.0272	78520872	call_user_func_array:{/Users/dawehner/www/d8/core/lib/Drupal/Core/Form/FormSubmitter.php:111} ( )	.../FormSubmitter.php:111
24	5.0272	78524296	simpletest_clean_environment( )	.../FormSubmitter.php:111
25	5.7590	79062368	simpletest_clean_temporary_directories( )	.../simpletest.module:634
26	5.7611	79080048	file_unmanaged_delete_recursive( )	.../simpletest.module:682
27	5.7611	79080096	call_user_func:{/Users/dawehner/www/d8/core/includes/file.inc:896} ( )	.../file.inc:896
28	5.7611	79080488	spl_autoload_call ( )	.../file.inc:896
29	5.7611	79080536	Composer\Autoload\ClassLoader->loadClass( )	.../file.inc:896
30	5.7612	79080664	Composer\Autoload\includeFile( )	.../ClassLoader.php:301
31	5.7624	79384984	include( '/Users/dawehner/www/d8/core/modules/simpletest/src/TestBase.php' )	.../ClassLoader.php:412
dawehner’s picture

Status: Needs review » Needs work

The current patch is certainly just a workaround. Let's be honest about the status :)

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.

yogeshmpawar’s picture

Status: Needs work » Needs review
FileSize
6.12 KB

I have rerolled the patch for 8.3.x-dev.

Mile23’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs tests

Looking at the stack trace in #4, it's apparent that the form controller is never called. This means the autoloading performed in SimpletestResultsForm::addResultForm() never happens. addResultForm() says:

\Drupal::service('test_discovery')->registerTestNamespaces();

...which is where \Drupal\Tests is set up for autoload.

Basically, this issue occurs because 1) The form is cached, and 2) Even though simpletest module is enabled, \Drupal\Tests is not autoloaded.

So #4 is correct that #2702281: Move Drupal\simpletest\RandomGeneratorTrait, Drupal\simpletest\WebAssert and Drupal\simpletest\SessionTestTrait into Drupal\Tests namespace caused it. That issue didn't change SimpletestResultsForm::addResultForm() because run-tests.sh never caches, and because the test namespaces have always been discovered, and because not a lot of people use Simpletest UI. :-)

There's a semi-brittle solution of making sure the form is never cached. Please someone think up a better one. :-)

This issue needs a test because it's a regression. But I'm not sure how we'll test that a) there's a fatal, since simpletest UI is good at handling fatals, and/or b) that \Drupal\Tests is autoloaded during addResultForm(), because we'd always be checking during a test.

Mile23’s picture

Title: Fatal error: Trait 'Drupal\Tests\SessionTestTrait' not found » Ensure that \Drupal\Tests namespace is loaded for the Simpletest UI report form
Issue summary: View changes
Mile23’s picture

Title: Ensure that \Drupal\Tests namespace is loaded for the Simpletest UI report form » _simpletest_batch_finished() needs to set up autoloading for \Drupal\Tests
Version: 8.3.x-dev » 8.2.x-dev
Status: Needs work » Needs review
Issue tags: +Needs manual testing
FileSize
735 bytes
557 bytes

I'm setting this to 8.2.x because it's a non-disruptive solution to a major testing bug, which seems good enough for beta.

OOOkay I was a bit off base, but let's pretend that was because the stack trace in #4 was old or something.

Here's today's stack trace:

[02-Sep-2016 20:43:45 Europe/Berlin] PHP Fatal error:  Trait 'Drupal\Tests\SessionTestTrait' not found in core/modules/simpletest/src/TestBase.php on line 25
[02-Sep-2016 20:43:45 Europe/Berlin] PHP Stack trace:
[02-Sep-2016 20:43:45 Europe/Berlin] PHP   1. {main}() index.php:0
[02-Sep-2016 20:43:45 Europe/Berlin] PHP   2. Drupal\Core\DrupalKernel->handle() index.php:19
[02-Sep-2016 20:43:45 Europe/Berlin] PHP   3. Stack\StackedHttpKernel->handle() core/lib/Drupal/Core/DrupalKernel.php:649
[02-Sep-2016 20:43:45 Europe/Berlin] PHP   4. Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() vendor/stack/builder/src/Stack/StackedHttpKernel.php:23
[02-Sep-2016 20:43:45 Europe/Berlin] PHP   5. Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php:50
[02-Sep-2016 20:43:45 Europe/Berlin] PHP   6. Drupal\page_cache\StackMiddleware\PageCache->handle() core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php:47
[02-Sep-2016 20:43:45 Europe/Berlin] PHP   7. Drupal\page_cache\StackMiddleware\PageCache->pass() core/modules/page_cache/src/StackMiddleware/PageCache.php:78
[02-Sep-2016 20:43:45 Europe/Berlin] PHP   8. Drupal\Core\StackMiddleware\KernelPreHandle->handle() core/modules/page_cache/src/StackMiddleware/PageCache.php:99
[02-Sep-2016 20:43:45 Europe/Berlin] PHP   9. Drupal\Core\StackMiddleware\Session->handle() core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php:47
[02-Sep-2016 20:43:45 Europe/Berlin] PHP  10. Symfony\Component\HttpKernel\HttpKernel->handle() core/lib/Drupal/Core/StackMiddleware/Session.php:57
[02-Sep-2016 20:43:45 Europe/Berlin] PHP  11. Symfony\Component\HttpKernel\HttpKernel->handleRaw() vendor/symfony/http-kernel/HttpKernel.php:62
[02-Sep-2016 20:43:45 Europe/Berlin] PHP  12. call_user_func_array:{vendor/symfony/http-kernel/HttpKernel.php:139}() vendor/symfony/http-kernel/HttpKernel.php:139
[02-Sep-2016 20:43:45 Europe/Berlin] PHP  13. Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() vendor/symfony/http-kernel/HttpKernel.php:139
[02-Sep-2016 20:43:45 Europe/Berlin] PHP  14. Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php:97
[02-Sep-2016 20:43:45 Europe/Berlin] PHP  15. Drupal\Core\Render\Renderer->executeInRenderContext() core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php:124
[02-Sep-2016 20:43:45 Europe/Berlin] PHP  16. Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() core/lib/Drupal/Core/Render/Renderer.php:574
[02-Sep-2016 20:43:45 Europe/Berlin] PHP  17. call_user_func_array:{core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php:123}() core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php:123
[02-Sep-2016 20:43:45 Europe/Berlin] PHP  18. Drupal\system\Controller\BatchController->batchPage() core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php:123
[02-Sep-2016 20:43:45 Europe/Berlin] PHP  19. _batch_page() core/modules/system/src/Controller/BatchController.php:55
[02-Sep-2016 20:43:45 Europe/Berlin] PHP  20. _batch_finished() core/includes/batch.inc:81
[02-Sep-2016 20:43:45 Europe/Berlin] PHP  21. call_user_func_array:{core/includes/batch.inc:414}() core/includes/batch.inc:414
[02-Sep-2016 20:43:45 Europe/Berlin] PHP  22. _simpletest_batch_finished() core/includes/batch.inc:414
[02-Sep-2016 20:43:45 Europe/Berlin] PHP  23. simpletest_log_read() core/modules/simpletest/simpletest.module:474
[02-Sep-2016 20:43:45 Europe/Berlin] PHP  24. spl_autoload_call() core/modules/simpletest/simpletest.module:533
[02-Sep-2016 20:43:45 Europe/Berlin] PHP  25. Composer\Autoload\ClassLoader->loadClass() core/modules/simpletest/simpletest.module:0
[02-Sep-2016 20:43:45 Europe/Berlin] PHP  26. Composer\Autoload\includeFile() vendor/composer/ClassLoader.php:301
[02-Sep-2016 20:43:45 Europe/Berlin] PHP  27. include() vendor/composer/ClassLoader.php:412

Basically _simpletest_batch_finished() calls simpletest_log_read(), which needs to autoload the trait for TestBase.

So the solution is to have _simpletest_batch_finished() call \Drupal::service('test_discovery')->registerTestNamespaces().

Unfortunately this is very hard to test and kind of hard to debug because you have to turn off xdebug, let the test fatal, turn on xdebug, and then click on the 'show error' page in order to get a stack trace.

How to repro:

  • Apply the fatal_error_test.patch patch.
  • Run the test in simpletest UI.
  • Click on 'the error page' link.
  • See PHP Fatal error: Trait 'Drupal\Tests\SessionTestTrait' not found in core/modules/simpletest/src/TestBase.php on line 25 in your error log.

Manually test the patch by applying it and then following the steps above.

Mile23’s picture

Issue summary: View changes
blazey’s picture

Patch from #10 fixes the problem after test execution, but the the error persist when 'CLEAN TEST ENVIRONMENT' form is submitted. Registering the namespace in simpletest_clean_environment() fixes that.

dawehner’s picture

Issue tags: -Needs manual testing

We got a couple of people which say that this fixes the problem for them.

Automatic tests would still be nice.

Mile23’s picture

Automatic tests would still be nice.

Indeed they would.

Here's your brain-teaser for the morning: How can you mock the behavior of simpletest_last_test_get(), simpletest_clean_database(), and simpletest_clean_results_table()?

dawehner’s picture

Here's your brain-teaser for the morning: How can you mock the behavior of simpletest_last_test_get(), simpletest_clean_database(), and simpletest_clean_results_table()?

I agree, that would be tricky. Are we sure we cannot provide some form of functional test?

Mile23’s picture

The functional test would have to use testception to run a fatal test under the UI, and then survive.

Unfortunately this is also problematic because the Simpletest UI doesn't always run the same PHP binary as run-tests.sh. See #2476139: Fix UI test fail in SimpleTestBrowserTest

This makes it difficult to develop a test without creating a specific environment for it, and we can't guarantee the test will pass unless we know the same version of PHP is being used for both run-tests.sh and the web server that eventually launches the UI test run.

cilefen’s picture

Mile23’s picture

Here's what I think is a better solution.

Instead of depending on the classload status of a class that should not be autoloadable at this point in runtime, we should move the desired behavior to another class that *should* be loadable.

So we move insertAssert() from TestBase to TestDatabase (with BC).

There should be a follow-up to move deleteAssert() as well, and any future refactoring should turn these behaviors into a service.

That solves the problem with _simpletest_batch_finished().

The problem with simpletest_clean_environment() is similarly solved by moving TestBase::filePreDeleteCallback() to simpletest_temp_directory_pre_delete_callback() (with BC) in simpletest.module. There is no other good place to put this callback. Any alternate suggestion entertained. :-) All the function does is chown the file so it can be deleted.

In an ideal world, there would be a class which does unmanaged deletes, and we could subclass it and add our chown. But we don't have that.

dawehner’s picture

I agree conceptually this insertAssert method can totally live on the TestDatabase class. It is cool if this fixes this particular issue here :)

  1. +++ b/core/modules/simpletest/src/TestBase.php
    @@ -457,33 +457,7 @@ protected function assert($status, $message = '', $group = 'Other', array $calle
        * @see \Drupal\simpletest\TestBase::deleteAssert()
        */
       public static function insertAssert($test_id, $test_class, $status, $message = '', $group = 'Other', array $caller = array()) {
    

    Does that mean we should deprecate insertAssert?

  2. +++ b/core/modules/simpletest/src/TestBase.php
    @@ -1537,7 +1511,7 @@ public static function generatePermutations($parameters) {
       public static function filePreDeleteCallback($path) {
    -    chmod($path, 0700);
    +    simpletest_temp_directory_pre_delete_callback($path);
       }
    

    Is it just me who feels a little bit uncomfortable with adding some procedural code here?

Mile23’s picture

Does that mean we should deprecate insertAssert?

I'm not sure that's in scope here, and we'll definitely need to follow up with an issue about deleteAssert(), so maybe do it there.

Is it just me who feels a little bit uncomfortable with adding some procedural code here?

Figure out a better place for it and we'll do that. :-)

Actually, we might turn that into another issue, because I just refactored simpletest_clean*() methods to a helper class. #2800267: Turn simpletest_clean*() functions into a helper class

That would be in 8.3.x. For 8.2.x we could fix this by calling registerTestNamespaces() with a @todo.

Mile23’s picture

Mile23’s picture

OK, so this patch does the following:

Moves TestBase::insertAssert() to TestDatabase::insertAssert(), with BC. Updated issue to reflect that we need a follow-up about deprecation in 8.3.x.

Calls TestDiscovery::registerTestNamespaces() in simpletest_clean_environment(), with @todos about #2800267: Turn simpletest_clean*() functions into a helper class This is essentially the change from the patch in #12.

slasher13’s picture

Status: Needs review » Reviewed & tested by the community

works for me

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 22: 2745123_22.patch, failed testing.

alexpott’s picture

We should also add a test of pressing the clean environment button.

slasher13’s picture

Status: Needs work » Needs review
FileSize
6.44 KB

re-roll

dawehner’s picture

@alexpott
Do you argue for a test for this particular bug, or a general test for the button itself?

Mile23’s picture

We should have a test of buttons and stuff to make sure they work in general, but this bug is tricky to test because the test namespaces will always exist if we're running tests. Also, clicking 'clean environment' will remove test results for the test doing the clicking... We have testception in SimpleTestTest, but it's pretty fragile.

We'll be able to do better testing with less effort after #2800267: Turn simpletest_clean*() functions into a helper class

drugan’s picture

The patch resolves the crash issue when clicking at the relative button but not the autoloading issue.

My question is: Do we really need to use the TestBase and thus all the traits used by this class when deleting simpletest directories?

... taking into account all the troubles issuing from trying use the class from a non-object. And just for the sake of chmod-ing directories intended for deletion?

Let us add a dozen lines of code and forget about this headache forever.

The comment about chmod is duplicated from TestBase::filePreDeleteCallback().

Jaypan’s picture

I had this error when trying to uninstall simpletest. The patch in #29 solved the issue for me.

Mile23’s picture

Title: _simpletest_batch_finished() needs to set up autoloading for \Drupal\Tests » Simpletest module crashes on cleanup, uninstall
Status: Needs review » Needs work

Ran into this problem again today...

I like the simple approach of #29.

If we're doing that, we need to remove Drupal\simpletest\TestBase::filePreDeleteCallback().

+++ b/core/modules/simpletest/simpletest.module
@@ -676,14 +676,39 @@ function simpletest_clean_database() {
+  $root = DRUPAL_ROOT . '/sites/simpletest';

Use \Drupal::root(); instead of DRUPAL_ROOT.

That way we can have a unit test of simpletest_clean_temporary_directories() behavior using vfsstream without changing constants.

drugan’s picture

#29 is changed with suggestions made by @Mile23 at #31.

All the filePreDeleteCallback() methods are removed. Added optional $directory parameter to simpletest_clean_temporary_directories() as some test scripts require to remove only directory of a particular test site. Also, messages about removed directories is appended with the path to those directories. Helpful to check if the directories were actually removed.

drugan’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 32: 2745123_32.patch, failed testing.

drugan’s picture

Status: Needs work » Needs review
FileSize
7.28 KB

The chmod-ing is refactored into lambda function, as the replacement of filePreDeleteCallback() method. Then temporary test directory path is passed to file_unmanaged_delete_recursive() with the lambda function.

penyaskito’s picture

Issue summary: View changes

Another way of reproduce this issue easier:

drush php-eval 'simpletest_clean_environment()'

If you need to execute this, as a workaround you can use in the meanwhile

drush php-eval "\Drupal::service('test_discovery')->registerTestNamespaces(); simpletest_clean_environment()"
penyaskito’s picture

+++ b/core/modules/simpletest/simpletest.module
@@ -674,24 +674,35 @@ function simpletest_clean_database() {
-    drupal_set_message(\Drupal::translation()->formatPlural($count, 'Removed 1 temporary directory.', 'Removed @count temporary directories.'));
+    drupal_set_message(\Drupal::translation()->formatPlural($count, 'Removed 1 temporary directory in @root.', 'Removed @count temporary directories in @root.', ['@root' => $root]));
...
-    drupal_set_message(t('No temporary directories to remove.'));
+    drupal_set_message(t('No temporary directories to remove in @root.', ['@root' => $root]));

+++ b/core/scripts/run-tests.sh
@@ -871,12 +871,10 @@ function simpletest_script_cleanup($test_id, $test_class, $exitcode) {
-    $messages[] = "- Removed test site directory.";
...
+    $messages[] = "- Removed test site directory in {$test_directory}.";

Translatable strings should only change for 8.3.x at this point. Are these changes really needed?

drugan’s picture

Making things even better I've removed appended simpletest/directory path in messages (thanks @penyaskito for the hint) and changed the expected argument value when deleting test directory from within a running test. Now you can do this in your test:

simpletest_clean_temporary_directories($this->siteDirectory);

Status: Needs review » Needs work

The last submitted patch, 38: 2745123_38.patch, failed testing.

drugan’s picture

Status: Needs work » Needs review
FileSize
6.31 KB

Not sure about the test fail on the #38 but anyway I've made changes in run-tests.sh as to not touch existing code as much as possible.

Status: Needs review » Needs work

The last submitted patch, 40: 2745123_40.patch, failed testing.

drugan’s picture

Appears that the #38 and #40 are failed because of the latest commit on the core:

#2836381: Seven's entity-add-list template omits link attributes

...and not only on this issue: https://www.drupal.org/node/2836381#comment-11837422

Let us wait and see what they'll say.

drugan’s picture

Status: Needs work » Needs review

The #2836381: Seven's entity-add-list template omits link attributes is reverted therefore the #40 is green as expected.

hgoto’s picture

I met the same error related with Drupal\Tests\SessionTestTrait and the patch #40 worked for me. (I only checked the behavior and didn't check the code.)

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

othermachines’s picture

#40 applies to 8.4.x and fixes the problem for me.

I ran into this today on a fresh installation. Started with a fatal error while running tests via UI:

----
[04-Feb-2017 18:59:00 America/Edmonton] PHP Fatal error:  Maximum execution time of 240 seconds exceeded in C:\wamp\www\drupal_84x\core\modules\simpletest\simpletest.module on line 358
----

On refresh page:

----
[04-Feb-2017 18:59:14 America/Edmonton] Uncaught PHP Exception InvalidArgumentException: "Invalid database prefix: " at C:\wamp\www\drupal_84x\core\lib\Drupal\Core\Test\TestDatabase.php line 81
----

Thought I'd try a cache clear and test clean to get things working again:

----
$ drush cr
$ drush php-eval 'simpletest_clean_environment()'

Fatal error: Trait 'Drupal\Tests\SessionTestTrait' not found in C:\wamp\www\drupal_84x\core\modules\simpletest\src\TestBase.php on line 27
Drush command terminated abnormally due to an unrecoverable error.                                                                                                                  [error]

Error: Trait 'Drupal\Tests\SessionTestTrait' not found in C:\wamp\www\drupal_84x\core\modules\simpletest\src\TestBase.php, line 27
----

Applied patch in #40 and got a much improved result:

----
$ drush php-eval 'simpletest_clean_environment()'
No leftover tables to remove.                                                                                                                                                      [status]

Removed 3 temporary directories.                                                                                                                                                   [status]

Removed 2 test results.
----

Thanks!

benjifisher’s picture

Status: Needs review » Needs work

I ran into this problem, and I am a little surprised that it has been around for so long. Let's see if we can get it to RTBC.

If the old PreDeleteCallback() methods can be moved into an anonymous callback function (a.k.a. lambda) inside simpletest_clean_temporary_directories(), then I think that is a good thing. I hope it is not considered too much refactoring to fix this issue. I also really like that several calls to file_unmanaged_delete_recursive() can be replaced with calls to the improved simpletest_clean_temporary_directories().

I do see some problems here, so I am moving the issue back to "Needs Work".

 /**
  * Finds all leftover temporary directories and removes them.
  */
-function simpletest_clean_temporary_directories() {
+function simpletest_clean_temporary_directories($directory = NULL) {

Adding a parameter means you have to add an @param comment to the doc block. You might also want to edit the short description (first line of the doc block). Please make the documentation helpful, not perfunctory. What does the parameter do, and what happens if it is omitted?

-  if (is_dir(DRUPAL_ROOT . '/sites/simpletest')) {
-    $files = scandir(DRUPAL_ROOT . '/sites/simpletest');
+  $root = \Drupal::root() . '/sites/simpletest/';
+  $dir = $directory ? explode('sites/simpletest/', $directory) : FALSE;
+
+  if (is_dir($root)) {
+    $files = !$dir ? scandir($root) : (isset($dir[1]) && is_dir("{$root}{$dir[1]}") ? [$dir[1]] : []);

Sorry for the harsh words, but this is clear as mud. I do not think that efficiency is important here, so let's make it as clear as possible. I think that one or two if blocks would be a lot easier to follow than the boolean-or-array $dir variable and the nested ternary operator.

The rest of my comments are just suggestions.

+        $path = "{$root}{$file}";

You can call this a question of personal style and leave it as is, but I think it is clearer to leave off the braces when they are optional: "$root$file".

+        $result = file_unmanaged_delete_recursive($path, function ($any_path) {
+          // When the webserver runs with the same system user as the test
+          // runner, we can make read-only files writable again. If not, chmod
+          // will fail while the file deletion still works if file permissions
+          // have been configured correctly. Thus, we ignore any chmod errors.
+          @chmod($any_path, 0700);
+        });

It might be clearer to put the comment before the $result = ... line instead of inside the anonymous function.

benjifisher’s picture

Issue summary: View changes
drugan’s picture

Well, the #40 patch was refactored accordingly suggestions made by @benjifisher. Thanks for the points.

drugan’s picture

Status: Needs work » Needs review
benjifisher’s picture

FileSize
2.17 KB

@drugan, thanks for the quick response!

I do not have time for a full review now, but I have attached an interdiff to help with the review.

+ * @param string $directory
+ *   (optional) The relative path to directory of a particular test site. Must
+ *   to be of the following pattern: sites/simpletest/12345678. If ommited, all
+ *   the test sites' directories found in sites/simpletest will be removed.

That is great. I meant to say that the @param comment should say something about the magic string 'sites/simpletest/' because of the explode() line later.

+    $directory = $directory ? explode('sites/simpletest/', $directory) : FALSE;
+    $files = [];
+
+    if (isset($directory[1]) && is_dir($root . $directory[1])) {
+      $files[] = $directory[1];
+    }

This is clearer than the original, but I think there is still room for improvement.

  1. The parameter is passed in as a string (or NULL) and now it is either an array or boolean FALSE. Is $directory used later, or is it just needed to get $files?
  2. I still think this would be clearer with the structure if(...) { $files = [...]; } else { $files = []; }.
  3. Personally, I would use preg_match() rather than explode(): what happens if $directory is set but does not start with 'sites/simpletest/'?
  4. Won't this code generate at least a PHP notice when $directory is set to FALSE and we test isset($directory[1])?
drugan’s picture

Another attempt to improve logic and clarity on the patch.

Status: Needs review » Needs work

The last submitted patch, 52: 2745123_52.patch, failed testing.

mpdonadio’s picture

FileSize
3.2 KB

I concur with @alexpott that #2846643: DateFormatter should not attempt to load a format from storage if the format is "custom" isn't causing the latest fail. #49 applied to 8.3.x HEAD passes locally and in the retest I triggered. #52 fails locally applied to 8.3.x HEAD and in the retest I triggered.

Here is the interdiff. Looked at it for a bit, but don't see what in particular could be causing these fails.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
344.08 KB

Ok, this is bizarre, and I think a side effect of #2770921: Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits.

The problem is that LocaleTranslatedSchemaDefinitionTest and NodeTypeTranslationTest are changing the default language for the site. In #49, the `drupal_set_message()` at the bottom of simpletest_clean_temporary_directories() were never being called from the tests because of the `return` in the middle:

+++ b/core/modules/simpletest/simpletest.module
@@ -684,15 +684,43 @@ function simpletest_clean_database() {
+        $result = file_unmanaged_delete_recursive($path, function ($any_path) {
+          @chmod($any_path, 0700);
+        });
+        if ($directory) {
+          // We do not need any more if this function was called from within a
+          // running test instead of using "Clean environment" button.
+          return $result;
+        }
         $count++;

With #52, those statements are being hit, and the system language is still 'fr' or 'es', but it looks like the languages are gone by that point (I think from one of the parent tearDown() functions).

Here is an experiment that reverts 2770921, but keeps the changes in #52. Just want to see what happens here.

drugan’s picture

FileSize
7.99 KB

UPD: PLease, do not use this patch. It excludes system messages everywhere. Instead use the patch on #57 comment.

drugan’s picture

FileSize
8.01 KB

The system messages of successful test directory removing are excluded from display when the simpletest_clean_temporary_directories() is called from within a running test instead of using Clean environment button.

alexpott’s picture

+++ b/core/modules/simpletest/simpletest.module
@@ -684,21 +684,68 @@ function simpletest_clean_database() {
-  if ($count > 0) {
+  if (!is_null($directory) && $directory != 'NULL') {
+    // We do not need anything else if this function was called from within a
+    // running test instead of using "Clean environment" button. The system
+    // messages below may cause failure on some tests.
+    // @see https://www.drupal.org/node/2745123#comment-11837053
+    return $result;
+  }
+  elseif ($count > 0) {
     drupal_set_message(\Drupal::translation()->formatPlural($count, 'Removed 1 temporary directory.', 'Removed @count temporary directories.'));
   }
   else {

+++ b/core/modules/simpletest/src/TestBase.php
@@ -1393,21 +1393,6 @@ public static function generatePermutations($parameters) {
-   * Some tests chmod generated files to be read only. During

This looks weird. Why not just do

if ($directory === NULL) {
  if ($count > 0) {
    dsm()...
  }
  else {
    dsm()...
  }
}
return $result;
<code>

That said all of that would not be necessary if we didn't change the test classes to call simpletest_clean_temporary_directories()...
<code>
+++ b/core/modules/simpletest/src/TestBase.php
@@ -1231,8 +1231,8 @@ private function restoreEnvironment() {
-    // Delete test site directory.
-    file_unmanaged_delete_recursive($this->siteDirectory, array($this, 'filePreDeleteCallback'));
+    // Delete the current test site directory.
+    simpletest_clean_temporary_directories($this->siteDirectory);

Why is this in scope for fixing this issue? This got introduced by #31 for reasons that don't make sense to me. That code is not broken.

drugan’s picture

@alexpott

First suggestion with messages is done.

Why TestBase.php is also changed to use simpletest_clean_temporary_directories($this->siteDirectory)....

First of all it is a DRY principle. Our filePreDeleteCallback() is duplicated in two places now which is not good. That was an attempt to resolve issue with Trait/classes autoloading but even that duplication does not work in some cases, as we see. We need some function or method on a high level which works for all cases/environments. A good example to resolve this is to create a specific **cleaning** class:

#2800267: Turn simpletest_clean*() functions into a helper class

But even that does not work sometimes. For example, when uninstalling Testing module. Also, as it were noticed by @Mile23 on the #28 we need a test for simpletest_clean_temporary_directories() itself to check how it works in various environments. It's not an easy task. With this patch applied it can be done as it works for all and returns simple TRUE or FALSE if the removing is failed.

Still, it would be interesting to know @Mile23 opinion on the patch and how it could be used for testing.

mpdonadio’s picture

FileSize
2.71 KB
2.79 KB

I helps to post an interdiff to make patch review changes easier. Here are the last two. Few small comments.

  1. +++ b/core/modules/simpletest/simpletest.module
    @@ -684,26 +684,73 @@ function simpletest_clean_database() {
    +        $result = file_unmanaged_delete_recursive($path, function ($any_path) {
    +          // @see http://php.net/manual/en/function.chmod.php
    +          @chmod($any_path, 0700);
    +        });
             $count++;
    

    Since the $result is inside the loop, does this mean that one loop iteration could have a TRUE, and other would have a FALSE and the FALSE would get returned? Also, I think @see on the chmod isn't needed. The comment above that blip explains why it is done.

    If file_unmanaged_delete_recursive() returns a FALSE for some reason, then shouldn't $count not get incremented since FALSE indicates a problem? Is this really an error condition that should be handled properly?

  2. +++ b/core/modules/simpletest/simpletest.module
    @@ -684,26 +684,73 @@ function simpletest_clean_database() {
    +  return $result;
    

    So, could this be `return $count > 0;` or folded into the logic above it?

alexpott’s picture

The DRY principle is a bit weird here. The UI, WebTestBase and BrowserTestBase are not the same system. The patch couples BrowserTestBase to simpletest code - which we should not be doing. It's just not necessary to make this fix. Making these changes are not necessary to make the simpletest_* functions testable.

drugan’s picture

@mpdonadio

Thanks for the interdiffs done.

Since the $result is inside the loop, does this mean that one loop iteration could have a TRUE, and other would have a FALSE and the FALSE would get returned?

No, the $result variable gets the value of the removing of the parent $path directory, not the result of removing any nested directories/files. if any of nested content will not succeed to be removed then the entire $path == \Drupal::root() . '/sites/simpletest/12345678' will fail to be removed and gets FALSE as the $result. That's because the rmdir() can remove only empty directories.

    foreach ($directories as $file) {
      if ($file[0] != '.') {
        $path = $simpletest_root . $file;
&lt;/code&gt;&#10;        $result = file_unmanaged_delete_recursive($path, function ($any_path) {&#10;&lt;code&gt;
        $count++;

The return $count > 0; does not help here because incrementing of $count happens in any case, disregarding of the $result value.

As for the @see on the chmod, yes it might be seen as a redundant one. If that is the only one notice left then I can remove it.

mpdonadio’s picture

#61, what is also weirder is why that is actually working. @dawehner talked a bit in IRC about it, and he did not think that simpletest.module was enabled in BTB. I don't see it in the testing profile, and I don't see it explicitly being enabled in BTB itself. Yet, if I run the test from the CLI phunit, it finds the function.

drugan’s picture

@alexpott

I can see no any harm in coupling BrowserTestBase and WebTestBase with some robustly working procedure, would be it the simpletest.module file or any of its functions. But if you have strong considerations to leave these both classes untouched then I'll rewrite the patch.

@mpdonadio

I don't see it in the testing profile, and I don't see it explicitly being enabled in BTB itself. Yet, if I run the test from the CLI phunit, it finds the function.

BTB finds not only simpletest.module functions but the Drupal system functions too. For example, here:

file_unmanaged_delete_recursive($test_directory, array('Drupal\simpletest\TestBase', 'filePreDeleteCallback'));

As I understand BTB cannot be used beyond the Drupal and simpletest module at all.

alexpott’s picture

BTB does not depend on anything from simpletest and if it does that would be a bug. run-tests.sh hopefully at some point will be completely replaced by just using the phpunit runner - complicated because phpunit can't run concurrent test atm.

alexpott’s picture

The changes to run-tests.sh, WebTestBase and BrowserTestBase are completely unnecessary to enact the fix. Rather than arguing about this how about writing a test to prove the cleaning test directories from the UI is not broken?

drugan’s picture

The changes in run-tests.sh, WebTestBase and BrowserTestBase are excluded from the patch.

Added UiCleanTemporaryDirectoriesTest.php to prove the cleaning a test directory from the UI by pressing Clean environment button. The main idea behind the test is to change a callback for the button and then to make the exact copy of $this->siteDirectory() and then trying to remove the copied directory from the UI.

At first it was a thought to make multiple clones of the directory and then temporarily to hide it by adding a dot to the directory name and then to call simpletest_clean_temporary_directories() without an argument, therefore removing all visible directories in the sites/simpletest folder. After that the original directory name might be reverted in order to normally terminate the test. Though this scheme is feasible I've refused from it because of concurrency argument allowing to run multiple tests at the same time. Their directories also will be removed with the scheme.

Status: Needs review » Needs work

The last submitted patch, 67: 2745123_67.patch, failed testing.

drugan’s picture

Status: Needs work » Needs review

Well, there is one coding standard error. Now I am waiting for some thoughts on the patch to correct this and may be other lines of the code.

Mile23’s picture

  1. +++ b/core/modules/simpletest/tests/modules/testing_page_test/testing_page_test.module
    @@ -0,0 +1,22 @@
    +function testing_page_test_form_alter(&$form, $form_state, $form_id) {
    +  if ($form_id == 'simpletest_test_form') {
    +    $form['clean']['op']['#submit'] = ['testing_page_test_submit'];
    +  }
    +}
    +
    +/**
    + * Removes test site directory when the 'Clean environment' button is pressed.
    + */
    +function testing_page_test_submit() {
    +  simpletest_clean_temporary_directories('sites/simpletest/test_site_directory');
    +}
    

    Don't get me wrong... This is a good faith effort at trying to satisfy the maintainer. So this is me actually reviewing the requirement that there be a button click. :-)

    So the problem was that there was an autoloading error due to using Drupal\simpletest\TestBase::filePreDeleteCallback(), which isn't reachable outside of test-time.

    The solution here is to fold that method's behavior into simpletest_clean_temporary_directories(). That's fine.

    The way to test that solution would be to make a Kernel test that calls simpletest_clean_temporary_directories() with a mocked file system. This test would then tell us whether we had altered the behavior of simpletest_clean_temporary_directories() when we made this modification. We could run this test with the modification and without it, and compare the results.

    As demonstrated here, the only way to test whether clicking the 'Clean environment' button reliably loads dependencies of simpletest_clean_temporary_directories() without removing all the test results from the system is to completely change the behavior of the form that gives us that button. This means we aren't testing the behavior of the button any more.

    The UI test in this patch is brittle and doesn't really test the behavior of the UI item. We also can't run this test without the changes for a fail, because during test-time TestBase will be available to the autoloader. In the future, the only things that could cause a regression failure would be changes to simpletest_clean_temporary_directories(), and not the other dependencies of simpletest_clean_environment() (and thus the button).

    Other than that...

  2. +++ b/core/modules/simpletest/src/Tests/UiCleanTemporaryDirectoriesTest.php
    @@ -0,0 +1,70 @@
    +  protected function recurse_copy_test_site_directory($source, $destination) {
    

    Needs proper camelCase for the method name.

  3. +++ b/core/modules/simpletest/src/Tests/UiCleanTemporaryDirectoriesTest.php
    @@ -0,0 +1,70 @@
    +    $this->recurse_copy_test_site_directory("$simpletest/$test_site_directory", "$simpletest/test_site_directory");
    

    Are we sure that $test_site_directory will never be empty at test time?

drugan’s picture

@Mile23

Are we sure that $test_site_directory will never be empty at test time?

Yes, we are. You'll get 403 error if the $test_site_directory for some reasons is empty. Even it is not empty but don't have .htkey file inside you'll also get the error and the entire test failure as a result.

Also, I've added an assertEqual() to check if the structure of both original and copied directories are the same. We'll get an exception in $this->recurseCopyTestSiteDirectory() if some file is failed to be copied. Also, an additional exception will be thrown in the testing_page_test_submit() if the copied test site directory is attempted to be removed but returned FALSE as a result of simpletest_clean_temporary_directories().

I think that is enough exceptions just to be sure that all works as expected.

As demonstrated here, the only way to test whether clicking the 'Clean environment' button reliably loads dependencies of simpletest_clean_temporary_directories() without removing all the test results from the system is to completely change the behavior of the form that gives us that button. This means we aren't testing the behavior of the button any more.

I am not agree with the word "completely". Basically, here:

/**
 * Removes test site directory when the 'Clean environment' button is pressed.
 */
function testing_page_test_submit() {
  $cleaned = simpletest_clean_temporary_directories('sites/simpletest/test_site_directory');
// code ...
}

...we are doing the same as here:

/**
 * Removes all temporary database tables and directories.
 */
function simpletest_clean_environment() {
  simpletest_clean_database();
  simpletest_clean_temporary_directories();
// code ....
}

We call the simpletest_clean_temporary_directories() function by pressing the Clean environment button. The only difference is that we skip calling the simpletest_clean_database() function which supposedly may throw an exception and therefore prevent the directories to be removed. But the point is that we don't test the simpletest_clean_environment() here. Instead we test just the simpletest_clean_temporary_directories() function. If that works or not. No more. Remember that for now we have the other way round issue when the database is cleaned but directories are not. If we'll swap the order of calling of both functions then the database cleaning will also fail together with the directories. BTW, that is exactly what may happen in the run-tests.sh file. At the first place it cleans out the test site directory and then database tables.

The way to test that solution would be to make a Kernel test that calls simpletest_clean_temporary_directories() with a mocked file system. This test would then tell us whether we had altered the behavior of simpletest_clean_temporary_directories() when we made this modification. We could run this test with the modification and without it, and compare the results.

Not sure if "modifications" on the function is the right way to build a failure case. Are they take place in a real world working Drupal system? Yes, actually they can but only as a code hack which we don't need to test. The only failure case I can imagine in the real world is when the run-tests.sh was run with the superuser instead of www-data user and the test has crashed and therefore directories are not removed automatically. Then if the www-data user have some weird permissions and you'll try to clean the directories through the UI you can fail to do it but there will be no any fatal errors. Just a warning that some directories cannot be deleted.

Or another failure case when you move Drupal site files together with not cleaned out simpletest directory on the different server where wrong permissions on the www-data user.

In order to write a test for these cases we need to tweak the permissions of the www-data user at the run time. Which is pretty difficult keeping in mind that tests might be run concurrently.

The question is: do we need this failure test at all? Or may be it is enough to have just a successful directories removing test?

alexpott’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

One thing that is super interesting for me is that I can't reproduce the error. Actually on a site via the browser. I'm pretty sure this is because of caching of the autoloader in APC. Via drush the error is easy to reproduce.

  1. Install simpletest
  2. Create a file in sites/simpletest
  3. Use drush to run simpletest_clean_environment()

The reason why testing this has proved tricky is

  // Do not clean the environment in case the Simpletest module is uninstalled
  // in a (recursive) test for itself, since simpletest_clean_environment()
  // would also delete the test site of the parent test process.
  if (!drupal_valid_test_ua()) {
    simpletest_clean_environment();
  }

Personally, I think we should just do the simple thing here and do something like:

diff --git a/core/modules/simpletest/simpletest.module b/core/modules/simpletest/simpletest.module
index c9eed3b..79e2fd6 100644
--- a/core/modules/simpletest/simpletest.module
+++ b/core/modules/simpletest/simpletest.module
@@ -692,7 +692,9 @@ function simpletest_clean_temporary_directories() {
     foreach ($files as $file) {
       if ($file[0] != '.') {
         $path = DRUPAL_ROOT . '/sites/simpletest/' . $file;
-        file_unmanaged_delete_recursive($path, array('Drupal\simpletest\TestBase', 'filePreDeleteCallback'));
+        file_unmanaged_delete_recursive($path, function ($any_path) {
+          @chmod($any_path, 0700);
+        });
         $count++;
       }
     }

Trying to make this method testable is just not worth the additional effort and complexity it brings to the system. In the patch in #71 we're adding a parameter to a function that is only ever passed in during testing. So we are not really testing what happens when people uninstall or click on the button in tests. Therefore I'm removing the "needs tests" tag because it is just not possible to do sensibly and setting back to needs work so we can just get the minimal fix.

alexpott’s picture

@drugan and @Mile23 also thanks for your continued effort on this issue. Testing the test infrastructure is tricky and it is always hard to evaluate the trade-offs between stuff just working, getting things done, making thing simpler or more complex and saving ourselves work in the future. I hope we can agree to just get the simplest thing done.

drugan’s picture

Status: Needs work » Needs review
FileSize
1.12 KB

@alexpott

Done.

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

@alexpott: "Testing the test infrastructure is tricky" Yah, that's why I started out my contributions here by making it testable. :-) But this latter approach is good, easier to understand and evaluate. Making things testable is the realm of other issues on the topic, such as #2800267: Turn simpletest_clean*() functions into a helper class

Assuming the tests pass, I think it's good.

nikolas.costa’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 76: simpletest-simpletest-module-crashes-2745123-76-D8.patch, failed testing.

jofitz’s picture

Status: Needs work » Needs review
FileSize
5.76 KB

Re-rolled (ignoring @nikloas.costa's changes in #76 to settings.local.php and development.services.yml which I assume were included in error).

alexpott’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
682 bytes

Here's a proper re-roll of #74.

jofitz’s picture

Gah, I re-rolled the wrong one, sorry.

alexpott’s picture

@Jo Fitzgerald no worries! Thanks for all the contributions - I've posted many messed up rerolls too.

The last submitted patch, 78: 2745123_78.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed ee17956 to 8.4.x and c9b75b7 to 8.3.x. Thanks!

I've backported this to 8.3.x as it is test system code and does not affect the runtime of a real site.

  • alexpott committed ee17956 on 8.4.x
    Issue #2745123 by drugan, Mile23, mpdonadio, alexpott, slasher13, Jo...

  • alexpott committed c9b75b7 on 8.3.x
    Issue #2745123 by drugan, Mile23, mpdonadio, alexpott, slasher13, Jo...
benjifisher’s picture

Thanks to @drugan, @Mile23 and others for the final push. I am glad that I will not run into this problem again! It was generous of @alexpott to include me in the credits; I did not do all that much.

Status: Fixed » Closed (fixed)

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