Problem/Motivation

    Download (Drupal\Tests\file\Functional\Download)
     ✔ Public file transfer
     ✘ Private file transfer without page cache
       ┐
       ├ Correctly denied access to a file when file_test sets the header to -1.
       ├ Failed asserting that 200 is identical to 403.                         
       │
       │ /builds/issue/drupal-3069442/core/modules/file/tests/src/Functional/DownloadTest.php:138
       │ /builds/issue/drupal-3069442/core/modules/file/tests/src/Functional/DownloadTest.php:76
       ┴
     ✔ File create url
    
    FAILURES!
    Tests: 3, Assertions: 46, Failures: 1.

also

    Download (Drupal\Tests\file\Functional\Download)
     ✔ Public file transfer
     ✘ Private file transfer without page cache
       ┐
       ├ Failed asserting that null matches expected 'private://wqelrzjx.txt'.
       │
       │ /builds/issue/drupal-3496529/core/modules/file/tests/src/Functional/DownloadTest.php:108
       │ /builds/issue/drupal-3496529/core/modules/file/tests/src/Functional/DownloadTest.php:77
       ┴
     ✔ File create url
    
    1 test triggered 1 PHP warning:
    
    1) /builds/issue/drupal-3496529/core/modules/file/tests/src/Functional/DownloadTest.php:108
    Trying to access array offset on null
    
    Triggered by:
    
    * Drupal\Tests\file\Functional\DownloadTest::testPrivateFileTransferWithoutPageCache (3 times)
      /builds/issue/drupal-3496529/core/modules/file/tests/src/Functional/DownloadTest.php:76
    
    FAILURES!
    Tests: 3, Assertions: 37, Failures: 1, Warnings: 1.

Steps to reproduce

Example: https://git.drupalcode.org/issue/drupal-3069442/-/jobs/3468073#L526

Proposed resolution

Replace calls to State with calls to KeyValue.

The root cause of this issue is discussed in #3496257: Race conditions in CacheCollector/State (again)

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3496529

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

spokje created an issue. See original summary.

spokje opened merge request !10736

spokje’s picture

spokje
Primary language Dutch
commented
Title: [random test failure] EditorSecurityTest::testEditorXssFilterOverride » [random test failure] DownloadTest::DownloadTest

spokje opened merge request !10739

spokje changed the visibility of the branch 3496529-multi-run-as-is to hidden.

spokje changed the visibility of the branch 3496529-multi-run-as-is to active.

spokje closed merge request !10739

spokje changed the visibility of the branch 11.x to hidden.

spokje opened merge request !10741

spokje opened merge request !10742

spokje’s picture

spokje
Primary language Dutch
commented
Issue summary: View changes
spokje’s picture

spokje
Primary language Dutch
commented
Issue summary: View changes
spokje’s picture

spokje
Primary language Dutch
commented
Assigned: Unassigned » spokje
Status: Active » Needs work

spokje closed merge request !10742

spokje opened merge request !10746

spokje’s picture

spokje
Primary language Dutch
commented
Title: [random test failure] DownloadTest::DownloadTest » [random test failure] DownloadTest::doPrivateFileTransferTest
Assigned: spokje » Unassigned
Status: Needs work » Needs review

MR !10736 (which is \Drupal\Tests\file\Functional\DownloadTest::doPrivateFileTransferTest and some pipeline changes to make it, and only it, run 7500 times) gives us 76 fails out of the 7500 runs.

MR !10741 shows that changing \Drupal::state()->set()/get() to Drupal::keyValue()->set()/get(); passes a 7500 times run without errors.

Finally, MR!10746 is the one we want committed after review.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Same deal as others. #16 shows the net gain so lets do it!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

https://git.drupalcode.org/project/drupal/-/merge_requests/10746/diffs is removing test coverage which looks unintentional?

acbramley’s picture

acbramley commented

Encountered this twice in a row this morning, will try to tidy up the MRs here and get it back to NR.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review

Rebased and changed the get calls in the test to use FileTestHelper::getCalls

godotislate’s picture

godotislate
he/him
commented
Status: Needs review » Reviewed & tested by the community

Searched and found no other usages of file_test.results or file_test.return in the code base. LGTM.

  • catch committed 0c1fb826 on 11.x 
    Issue #3496529 by spokje, acbramley: [random test failure] DownloadTest...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, thanks!

catch closed merge request !10746

Status: Fixed » Closed (fixed)

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