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 pageAn 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()
fromDrupal\simpletest\TestBase
andDrupal\simpletest\BrowserTestBase
.
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#79 | 2745123-2-79.patch | 682 bytes | alexpott |
#78 | 2745123_78.patch | 5.76 KB | jofitz |
#76 | simpletest-simpletest-module-crashes-2745123-76-D8.patch | 6.65 KB | nikolas.costa |
#74 | 2745123_74.patch | 1.12 KB | drugan |
#71 | interdiff-67-71.txt | 5.16 KB | drugan |
Comments
Comment #2
benjy CreditAttribution: benjy at PreviousNext commentedThis 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.
Comment #3
drunken monkeyI 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.
Comment #4
fietserwinI 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:
Comment #5
dawehnerThe current patch is certainly just a workaround. Let's be honest about the status :)
Comment #7
yogeshmpawarI have rerolled the patch for 8.3.x-dev.
Comment #8
Mile23Looking 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:...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 duringaddResultForm()
, because we'd always be checking during a test.Comment #9
Mile23Comment #10
Mile23I'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:
Basically
_simpletest_batch_finished()
callssimpletest_log_read()
, which needs to autoload the trait forTestBase
.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:
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.
Comment #11
Mile23Comment #12
blazey CreditAttribution: blazey as a volunteer commentedPatch 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.Comment #13
dawehnerWe got a couple of people which say that this fixes the problem for them.
Automatic tests would still be nice.
Comment #14
Mile23Indeed they would.
Here's your brain-teaser for the morning: How can you mock the behavior of
simpletest_last_test_get()
,simpletest_clean_database()
, andsimpletest_clean_results_table()
?Comment #15
dawehnerI agree, that would be tricky. Are we sure we cannot provide some form of functional test?
Comment #16
Mile23The 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.
Comment #17
cilefen CreditAttribution: cilefen commentedComment #18
Mile23Here'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()
fromTestBase
toTestDatabase
(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 movingTestBase::filePreDeleteCallback()
tosimpletest_temp_directory_pre_delete_callback()
(with BC) insimpletest.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.
Comment #19
dawehnerI agree conceptually this
insertAssert
method can totally live on the TestDatabase class. It is cool if this fixes this particular issue here :)Does that mean we should deprecate insertAssert?
Is it just me who feels a little bit uncomfortable with adding some procedural code here?
Comment #20
Mile23I'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.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.Comment #21
Mile23This problem breaks uninstalls, too; #2799333: Fatal error when uninstall Testing module due to traits of TestBase
Comment #22
Mile23OK, so this patch does the following:
Moves
TestBase::insertAssert()
toTestDatabase::insertAssert()
, with BC. Updated issue to reflect that we need a follow-up about deprecation in 8.3.x.Calls
TestDiscovery::registerTestNamespaces()
insimpletest_clean_environment()
, with @todos about #2800267: Turn simpletest_clean*() functions into a helper class This is essentially the change from the patch in #12.Comment #23
slasher13works for me
Comment #25
alexpottWe should also add a test of pressing the clean environment button.
Comment #26
slasher13re-roll
Comment #27
dawehner@alexpott
Do you argue for a test for this particular bug, or a general test for the button itself?
Comment #28
Mile23We 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
Comment #29
drugan CreditAttribution: drugan as a volunteer commentedThe 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().
Comment #30
Jaypan CreditAttribution: Jaypan at Jaypan commentedI had this error when trying to uninstall simpletest. The patch in #29 solved the issue for me.
Comment #31
Mile23Ran 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()
.Use
\Drupal::root();
instead ofDRUPAL_ROOT
.That way we can have a unit test of
simpletest_clean_temporary_directories()
behavior using vfsstream without changing constants.Comment #32
drugan CreditAttribution: drugan as a volunteer commented#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.
Comment #33
drugan CreditAttribution: drugan as a volunteer commentedComment #35
drugan CreditAttribution: drugan as a volunteer commentedThe 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.
Comment #36
penyaskitoAnother way of reproduce this issue easier:
If you need to execute this, as a workaround you can use in the meanwhile
Comment #37
penyaskitoTranslatable strings should only change for 8.3.x at this point. Are these changes really needed?
Comment #38
drugan CreditAttribution: drugan as a volunteer commentedMaking 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:
Comment #40
drugan CreditAttribution: drugan as a volunteer commentedNot 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.
Comment #42
drugan CreditAttribution: drugan as a volunteer commentedAppears 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.
Comment #43
drugan CreditAttribution: drugan as a volunteer commentedThe #2836381: Seven's entity-add-list template omits link attributes is reverted therefore the #40 is green as expected.
Comment #44
hgoto CreditAttribution: hgoto at Studio Umi commentedI 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.)Comment #46
othermachines CreditAttribution: othermachines commented#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:
On refresh page:
Thought I'd try a cache clear and test clean to get things working again:
Applied patch in #40 and got a much improved result:
Thanks!
Comment #47
benjifisherI 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) insidesimpletest_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 tofile_unmanaged_delete_recursive()
can be replaced with calls to the improvedsimpletest_clean_temporary_directories()
.I do see some problems here, so I am moving the issue back to "Needs Work".
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?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.
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"
.It might be clearer to put the comment before the
$result = ...
line instead of inside the anonymous function.Comment #48
benjifisherComment #49
drugan CreditAttribution: drugan as a volunteer commentedWell, the #40 patch was refactored accordingly suggestions made by @benjifisher. Thanks for the points.
Comment #50
drugan CreditAttribution: drugan as a volunteer commentedComment #51
benjifisher@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.
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.This is clearer than the original, but I think there is still room for improvement.
$directory
used later, or is it just needed to get$files
?if(...) { $files = [...]; } else { $files = []; }
.preg_match()
rather thanexplode()
: what happens if$directory
is set but does not start with 'sites/simpletest/'?$directory
is set toFALSE
and we testisset($directory[1])
?Comment #52
drugan CreditAttribution: drugan as a volunteer commentedAnother attempt to improve logic and clarity on the patch.
Comment #54
mpdonadioI 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.
Comment #55
mpdonadioOk, 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:
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.
Comment #56
drugan CreditAttribution: drugan as a volunteer commentedUPD: PLease, do not use this patch. It excludes system messages everywhere. Instead use the patch on #57 comment.
Comment #57
drugan CreditAttribution: drugan as a volunteer commentedThe 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.
Comment #58
alexpottThis looks weird. Why not just do
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.
Comment #59
drugan CreditAttribution: drugan as a volunteer commented@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.
Comment #60
mpdonadioI helps to post an interdiff to make patch review changes easier. Here are the last two. Few small comments.
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?
So, could this be `return $count > 0;` or folded into the logic above it?
Comment #61
alexpottThe 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.
Comment #62
drugan CreditAttribution: drugan as a volunteer commented@mpdonadio
Thanks for the interdiffs done.
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.
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.
Comment #63
mpdonadio#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.
Comment #64
drugan CreditAttribution: drugan as a volunteer commented@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
BTB finds not only simpletest.module functions but the Drupal system functions too. For example, here:
As I understand BTB cannot be used beyond the Drupal and simpletest module at all.
Comment #65
alexpottBTB 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.
Comment #66
alexpottThe 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?
Comment #67
drugan CreditAttribution: drugan as a volunteer commentedThe 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.
Comment #69
drugan CreditAttribution: drugan as a volunteer commentedWell, 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.
Comment #70
Mile23Don'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 ofsimpletest_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 tosimpletest_clean_temporary_directories()
, and not the other dependencies ofsimpletest_clean_environment()
(and thus the button).Other than that...
Needs proper camelCase for the method name.
Are we sure that $test_site_directory will never be empty at test time?
Comment #71
drugan CreditAttribution: drugan as a volunteer commented@Mile23
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.
I am not agree with the word "completely". Basically, here:
...we are doing the same as here:
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.
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?
Comment #72
alexpottOne 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.
The reason why testing this has proved tricky is
Personally, I think we should just do the simple thing here and do something like:
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.
Comment #73
alexpott@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.
Comment #74
drugan CreditAttribution: drugan as a volunteer commented@alexpott
Done.
Comment #75
Mile23@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.
Comment #76
nikolas.costa CreditAttribution: nikolas.costa at bmeme commentedThis is the diff to all patch.
Comment #78
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled (ignoring @nikloas.costa's changes in #76 to settings.local.php and development.services.yml which I assume were included in error).
Comment #79
alexpottHere's a proper re-roll of #74.
Comment #80
jofitz CreditAttribution: jofitz at ComputerMinds commentedGah, I re-rolled the wrong one, sorry.
Comment #81
alexpott@Jo Fitzgerald no worries! Thanks for all the contributions - I've posted many messed up rerolls too.
Comment #83
alexpottCommitted 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.
Comment #86
benjifisherThanks 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.