[random test failure] ImageStylesPathAndUrlTest

This use of randomString() looks suspicious.

    $this->drupalGet(\Drupal::service('file_url_generator')->generateAbsoluteString($directory . '/' . $this->randomString()));

EDIT: But does not explain why the test would fail further up.

MR !10701 (which is Drupal\Tests\image\Functional\ImageStylesPathAndUrlTest and some pipeline changes to make it, and only it, run 1750 times) gives us 13 fails out of the 1750 runs.

These 13 fails breaks down to:
2x testImageStyleUrlAndPathPrivateUnclean()
10x testImageStyleUrlAndPathPrivateLanguage()
1x testImageStyleUrlAndPathPrivate()

MR !10702 shows that changing \Drupal::state()->set()/get() to Drupal::keyValue()->set()/get(); as suggested in the Slack thread passes a 1750 times run without errors.
The only reason this MR shows as failed is the fact that the artifacts are too big to upload:

./sites/simpletest/browser_output: found 182292 matching artifact files and directories 
ERROR: Uploading artifacts as "archive" to coordinator... 413 Request Entity Too Large  id=3825475 responseStatus=413 Request Entity Too Large status=413 token=glcbt-64
FATAL: too large     

MR!10703 contains the changes that should be committed after review.

Based on the findings in #9, and I re-ran 10703 a few times and never got these random failures, when before I probably least got 1 each time.

Strongly disagree with bundling more issues together, but feel free to add-n-fix whatever you like.

Problem here IMHO is where the scope-creep ends, also more tests to test 2500x, or at least as-many-times-fit-in-an-hour makes things very slow, distinct tests run lower and fail rate/ success prove drop.

Doubt that it's OK to just create an MR that changes state to keyvalue, without the above test-runs, since the mostly (very) low failure rate will probably mean a single run will pass the tests.

To me that would prove absolutely nothing about the need for the fix, but n=1, YMMV etc, etc :)

Let the powers-that-be decide on this one.
In the mean time I will continue to create single test fix issues, and surely not for the easy core credits, which I really don't need in the first place.

In general I would agree with @dww's comment to expand, but here I think @spokje's reasoning is sound. These types of test failures require really long running multiple tests so it makes sense to fix one at a time.

Further there is a separate issue to address the race condition and anecdotally I've never seen this other tests fail.

I'd say this issue needs an explanation why the problem occurs with state, but not with keyvalue.
From the comments here I understand it is a race condition, and from looking at the code, I assume it is caused by the cache layer in state, because otherwise state is just a wrapper around keyvalue.

as suggested in the Slack thread

So, this should be in the issue summary..

I do find this issue from April 2024, which might be related:
#3438424: [random test failures] Race condition in state when individual keys are set with an empty cache

@donquixote There's a problem with multiple issues: When you're working on a lot of them, you forget that other people didn't. :)

Here's the issue where the root cause is attacked, hope that helps?
#3496257: Race conditions in CacheCollector/State (again)

Generally on the scope:

I would essentially commit any quick fix to ImageStylesPathAndUrlTest to stop it random failing including skipping it, because it is so annoying when it fails all the time.

Fixing it instead of skipping it is much better.

I do think we should switch all test usage from state to key/value because it massively simplifies things, however it's currently useful that we haven't done that because it's the only way I know to reproduce #3496257: Race conditions in CacheCollector/State (again). So would be quite happy to commit individual fixes for the frequently random tests to make the pipelines more reliable, then handle other issues which aren't randomly failing (as often) together in one issue later.

Committed/pushed to 11.x, thanks!

This doesn't apply to any previous branches including 11.1.x. I think it would be worth backporting to 10.5, 11.1, and 10.4 (11.0 and 10.3 if it's an easy cherry pick) so that we get less random failures on branch runs. Probably a backport to one branch would cherry-pick to all the others.

Not sure what either I or this issue has done to deserve waiingt 2:42 minutes for a full spellcheck on all files, but here we are...

Cherry-picked back through to 10.3.x, thanks!

Updated the proposed resolution with the issue where the root cause was discussed.