[random test failure] Random failure in PathWorkspacesTest

I was trying to run all tests from @group path, but core/scripts/run-tests.sh doesn't seem to be handle a "simple" thing like --group?

I see support for module, class, directory but not the standard phpunit --group foo?

Ah, crap.

I knew that once, many moons ago.
Thanks @longwave

I think here's you're (rather big) fail-canary-in-a-coal-mine to start with.

Is there any chance that the test generates a node with id() != 1 under some circumstances?

I do not understand any of the workspaces stuff. But from skimming the test it looks like the assertions aren't really consistent. In the first part there is a drupalGet which constructs the path using $node->id() while in the assertions following it looks like the path is hard-coded to preload-paths:/node/1.

  /**
   * Tests path aliases with workspaces.
   */
  public function testPathAliases() {
    // Create a published node in Live, without an alias.
    $node = $this->drupalCreateNode([
      'type' => 'article',
      'status' => TRUE,
    ]);

[...]

    $edit = [
      'path[0][alias]' => '/' . $this->randomMachineName(),
    ];
    $this->drupalGet('node/' . $node->id() . '/edit');
    $this->submitForm($edit, 'Save');

[...]

    // Publish the workspace and check that the alias can be accessed in Live.
    $stage->publish();
    $this->assertAccessiblePaths([$path]);
    $this->assertNotEmpty(\Drupal::cache('data')->get('preload-paths:/node/1'));

n=50, but it seems #3295790: Post-response task running (destructable services) are actually blocking; add test coverage and warn for common misconfiguration is a very likely suspect.

This patch was brought to you by the Coal Mine Canary Liberation Front

That hunk maybe? In that case we'd need a better way to wait for termination to have terminated in the child site.

    // Cacheable normalizations are written after the response is flushed to
    // the client; give the server a chance to complete this work.
    sleep(1);

Yes please!

For me personally sleep(1); is a very undeterministic way of determining something is finished.

Symfony docs about kernel termination state that

you will trigger the kernel.terminate event where you can perform certain actions that you may have delayed in order to return the response as quickly as possible to the client (e.g. sending emails).

and further (probably needs an update now):

at the moment, only the PHP FPM server API is able to send a response to the client while the server's PHP process still performs some tasks. With all other server APIs, listeners to kernel.terminate are still executed, but the response is not sent to the client until they are all completed.

Side note: I guess the symfony docs need an update after upstream upstream PR 46931.

After reading the docs, I feel that the terminate event should be used for stuff which also could be performed in a background task (a queue or cron). Not sure whether the usage of the terminate event in Drupal core and contrib falls into this category.

On the other hand, storing a cache doesn't actually affect the current request. And Drupal needs to cope with concurrent requests / concurrent cache rebuilds anyway. No matter whether concurrent requests come from one user agent or from many.

Checking MR together with patch from #9

Moving around sleeps.

The test fails mostly every time in #3374223: Fix deprecated overloaded function usage in PHP 8.3

Filed a follow-up #3375959: Add a way to delay executions in test runner until terminate event completed in the child site

The sqlite test in #19 failed with General error: 5 database is locked. Is that a result of running tests concurrently?

On the other hand, storing a cache doesn't actually affect the current request. And Drupal needs to cope with concurrent requests / concurrent cache rebuilds anyway. No matter whether concurrent requests come from one user agent or from many.

Yeah I think the way we use it is fine and the problems it's causing are test-specific. #3375959: Add a way to delay executions in test runner until terminate event completed in the child site is a good idea.

Now I hate to be _that_ guy, but hey, I _am_ that guy:

The normal routine to prove a random failure is fixed is to run the failing patch and the patch with the fix at the same time, whilst the latter has to have ~8000 - 10.000 failure free runs to prove it's credibility.

Seeing that the current 50x run takes around 10 minutes and that concurrent tests are, at least in my experience, getting wonky/throwing unpredictable unrelated randoms around the 1 hour mark, I think we need a 300x run-patch, which we then trigger 8000/300 ~ 27 times.

The best way for that is using the "Custom parameters" option when creating a test-run.
After that first time, it's "just" a matter of browser-back for 27x and pressing the submit with the same chosen options (Those tend to stick after the 2nd time browser-back in my, far too big, experience).

We could also try to bring the number of different testable classes in the "canary"/failing patch down, so we can have more runs of that.
But that's very likely to take (much) more time then blindly monkey-bashing the browser-back button.

Also: Big yay! for #3375959: Add a way to delay executions in test runner until terminate event completed in the child site

+1 on the commit from NR, I guess this can be a posthumous RTBC.

@Spokje that has turned out to be very important on issues where we're unskipping skipped random failures, but I think for 'quick fix' issues like this where the issue is actively failing, it's a bit less of a concern since the commit is a lot less likely to re-introduce a random failure even if it doesn't 100% fix one.