Problem/Motivation

The hold_test module creates files in
\Drupal::root() . '/sites/default/files/simpletest/'
so they are not test environment specific which means that two tests using this module can have random fails when run together.

For example: \Drupal\Tests\quickedit\FunctionalJavascript\QuickEditIntegrationTest and \Drupal\FunctionalJavascriptTests\Ajax\ThrobberTest

Proposed resolution

Use the test sites public files directory.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#12 3175112-12.patch1.46 KBandypost
#3 3175112-3.patch2.78 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

Oh, nice find. MultipleFileUploadTest seems similarly affected, but maybe not a problem here:

core/modules/file/tests/src/Functional/MultipleFileUploadTest.php:45:      $file_path = $this->root . "/sites/default/files/simpletest/$file";
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Active » Needs review
FileSize
3175112-3.patch2.78 KB

RE #2 I think we should tackle that in a follow-up. I think we need to deprecate this directory. It's created in \Drupal\Core\Test\TestRunnerKernel::boot() - I think we need to ensure it is empty and issue a deprecation if not and then we can remove it.

Here's a patch to fix the potential random fail. It places the file in the site directory and not the files directory as this is writable under testing and keeps these files out of harms way. Plus they'll now be tidied up after the tests.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

I created the follow-up for the more general issue - see #3175141: Deprecate sites/default/files/simpletest

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

Changes here look great, also +1 to removing the possibility of a race condition here entirely in the followup.

  • catch committed 5e27a9f on 9.1.x 
    Issue #3175112 by alexpott, longwave: hold_test module creates files in...

  • catch committed fa81641 on 9.0.x 
    Issue #3175112 by alexpott, longwave: hold_test module creates files in...

  • catch committed 1696301 on 8.9.x 
    Issue #3175112 by alexpott, longwave: hold_test module creates files in...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Good chance this fixes #3037436: [random test failure] Make QuickEditIntegrationTest more robust and fail proof, which would be amazing, possibly the most long-running random test failure.

Committed 5e27a9f and pushed to 9.1.x. Backported to 9.0.x and 8.9.x too.

TR’s picture

TR CreditAttribution: TR commented
Version: 9.1.x-dev » 8.9.x-dev
Priority: Major » Critical
Status: Fixed » Needs work

This commit causes core 8.9.x tests to fail (https://www.drupal.org/node/3060/qa) because site.path hasn't been backported to D8 yet. See #3074585: [Symfony 5] Replace app.root and site.path string services with container parameters

Re-opening and moving to critical, as core tests are failing because of this.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Related issues: +#3074585: [Symfony 5] Replace app.root and site.path string services with container parameters
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs work » Needs review
FileSize
3175112-12.patch1.46 KB

It makes pass following tests locally
- core/tests/Drupal/FunctionalJavascriptTests/Ajax/ThrobberTest.php
- core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditIntegrationTest.php

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs review » Reviewed & tested by the community

This fix looks great.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed bd65826 and pushed to 8.9.x. Thanks!

  • alexpott committed bd65826 on 8.9.x 
    Issue #3175112 follow-up by andypost, TR: hold_test module creates files...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Priority: Critical » Major

Status: Fixed » Closed (fixed)

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