Great work, I suppose this also takes care of random failures in tests that actually need to use State for testing like \Drupal\Tests\system\Functional\System\CronRunTest::testCronUI?
This is an example where we can't just switch to KeyValue, since State is an integral part of running cron.
So this is a test that doesn't scale well when ran multiple times.
With a concurrency of 5 I just manage to squeeze out a 100x run, which is far too low to hit the random failure.
Unsure how to proceed here.
@mstrelan: Meh! Are the new additions all in the var/param docs region? Or were there a lot of additions to the baseline during that period?
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.
@mondrake Agreed, thanks!
Ah, I see the MR doesn't cleanly apply to 11.0.x and lower, due to the OOP-ing of hooks.
We've been there before in other issues, and if it's anything similar as those, the 11.0.x MR will backport down to 10.3.x without issues.
To keep in line ay want to backport these random-state-failures as far back as 10.3.x?
We have been doing that we all the other fixes that landed so far.
I'm not getting anywhere with the remark in the MR, so throwing it out in the open for other, bigger brains.
Just my EUR0.02: I can see an, almost always considerable, rise in the failure rate when it's more busy on Drupal CI.
All runs here have taken place in the Christmas/New Years period, in which it's very quiet around here.
If possible, I would like to see a few runs from monday January 6th onward, when normal business is restored and see if that affects the numbers drastically.
If that would be too long of a wait, Im OK with this as-is, if stuff starts breaking down (more) on Monday we can act then as needed.
HEAD fixed, MR for 11.0.x is up
Tried to answer in thread
Looks like HEAD broken on 11.0.x: https://www.drupal.org/project/drupal/issues/3490710#comment-15922094 🐛 Catch potential exception when calling Request::create() in PathBasedBreadcrumbBuilder Needs review , so we're stalled here.
Seems to have broken HEAD of 11.0.x: https://git.drupalcode.org/project/drupal/-/pipelines/382966/test_report...
Should be fixed with the commit of 🐛 [random test failure] LayoutBuilderBlocksTest::testBlockPlaceholder failing Active
so it would be useful to not commit this for 24-48 hours just to get an extra few test runs on that issue.
I can see the usefulness, but I also see numerous MRs failing on this (and others).
We _could_ commit this and in your test-MR reverse the changes made here, which leaves you with exactly the same failing MR as before?
spokje → changed the visibility of the branch 3496529-multi-run-as-is to active.
spokje → changed the visibility of the branch 3496529-multi-run-as-is to hidden.
By changing \Drupal\Tests\language\Functional\LanguageNegotiationInfoTest::testInfoAlterations we also change \Drupal\Tests\comment\Functional\CommentLanguageTest, so we need to include both in our multiple runs.
MR !10733 (which is basically \Drupal\Tests\language\Functional\LanguageNegotiationInfoTest::testInfoAlterations and \Drupal\Tests\comment\Functional\CommentLanguageTest and some pipeline changes to make both, and only both, run 3000 times) gives us 2b fails out of the 3000 runs.
MR !10734 shows that changing \Drupal::state()->set()/get() to Drupal::keyValue()->set()/get(); passes a 3000 times + 5000 times run without errors. (The fail rate as so low that I've ran it twice)
MR !10731 contains the changes we want committed after review.
spokje → created an issue. See original summary → .
MR !10728 (which is basically \Drupal\Tests\editor\Functional\EditorSecurityTest::testEditorXssFilterOverride and some pipeline changes to make it, and only it, run 7500 times) gives us 325 fails out of the 10000runs.
MR !10729 shows that changing \Drupal::state()->set()/get() to Drupal::keyValue()->set()/get(); passes a 10000 times run without errors.
MR !10731 contains the changes we want committed after review.
spokje → changed the visibility of the branch 3477586-2500x-LayoutBuilderBlocksTest-testBlockPlaceholder-invalidate-cache-minus-10 to hidden.
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...
spokje → changed the visibility of the branch 3487371-2500x-ImageStylesPathAndUrlTest-as-is to hidden.
Hating to be that guy, but hey, I _am_ that guy...
I'm very doubtful if 7500 runs will fit into the current cutoff time of 1 hour.
@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?
🐛
Race conditions/bad lock logic in CacheCollector/State
Active
Whilst we're only trying to fix CommentPreviewTest::testCommentPreview here, this means making changes in \Drupal\user_hooks_test\Hook\UserHooksTest.
In turn, those changes affect the following tests:
\Drupal\Tests\comment\Functional\CommentPreviewTest::testCommentPreview
\Drupal\Tests\user\Kernel\UserEntityLabelTest::testLabelCallback
\Drupal\Tests\user\Functional\UserEditTest::testUserEdit
\Drupal\Tests\user\Functional\UserTokenReplaceTest::testUserTokenReplacement
So I think we have to run our multiple-run tests on all of them before and after the chage.
MR !10711 Has some pipeline changes to make those tests, and only those tests, run 2500 times and gives us 28 fails out of the 2500 runs. Those fails are all in \Drupal\Tests\comment\Functional\CommentPreviewTest::testCommentPreview
MR !10712 shows that changing \Drupal::state()->set()/get() to Drupal::keyValue()->set()/get(); passes a 2500 times run on all four mentioned tests without errors.
*cough*
Testbot really doesn't like it...
*cough*
spokje → changed the visibility of the branch 3496259-random-test-failure to active.
MR !10707 (which is Drupal\Tests\layout_builder\Functional\LayoutSectionTest::testLayoutSectionFormatterAccess and some pipeline changes to make it, and only it, run 7500 times) gives us 102 fails out of the 17500 runs.
MR !10708 shows that changing \Drupal::state()->set()/get() to Drupal::keyValue()->set()/get(); passes a 7500 times run without errors.
spokje → changed the visibility of the branch 3496259-random-test-failure to hidden.
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.
spokje → created an issue. See original summary → .
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.
spokje → changed the visibility of the branch 3487371-random-test-failure to hidden.
spokje → changed the visibility of the branch 3487371-debug-failing-test to hidden.
MR !10698 (which is basically \Drupal\Tests\block\Functional\BlockCacheTest trimmed down to only contain testCachePermissions() and some pipeline changes to make it, and only it, run 2500 times) gives us 8 fails out of the 2500 runs.
MR !10699 shows that changing \Drupal::state()->set()/get() to Drupal::keyValue()->set()/get(); as suggested in the Slack thread passes a 2500 times run without errors.
Which shows to me that when 🐛 [random test failure] LayoutBuilderBlocksTest::testBlockPlaceholder failing Active lands, this issue should be solved.
Re #7, 🐛 [random test failure] LayoutBuilderBlocksTest::testBlockPlaceholder failing Active also makes changes for that method, so the random failures there also should be gone.
@berdir:
\Drupal\Core\Cache\DatabaseBackend::invalidateAll() set the expiration to request time - 10 => MR!1069553 failures out of 2500 runs.
\Drupal\Core\Cache\DatabaseBackend::invalidateAll() set the expiration to 1 => MR!1069682 failures out of 2500 runs.
So roughly the same as the unchanged 2500 run.
I'm going to set this to NR for #22 🐛 [random test failure] LayoutBuilderBlocksTest::testBlockPlaceholder failing Active .
MR !10694 shows that changing \Drupal::state()->set()/get() to Drupal::keyValue()->set()/get(); as suggested in the Slack thread passes a 2500 times run without errors.
Since this involves making changes in core/modules/block/tests/modules/block_test/src/Plugin/Block/TestCacheBlock.php, this means more tests are affected.
@godotislate was already miles ahead of me and created MR !10689, which is the MR to be committed.
Should be solved by 🐛 [random test failure] LayoutBuilderBlocksTest::testBlockPlaceholder failing Active
spokje → changed the visibility of the branch 3468830-random-test-failure to hidden.
MR !10692 (which is basically \Drupal\Tests\layout_builder\Functional\LayoutBuilderBlocksTest trimmed down to only contain testBlockPlaceholder() and some pipeline changes to make it, and only it, run 2500 times) gives us 69 fails out of the 2500 runs.
spokje → changed the visibility of the branch 3477586-random-test-failure to hidden.
Thanks @godotislate.
There's a Slack thread about this here: https://drupal.slack.com/archives/C079NQPQUEN/p1735214972315009
Ok, so there seems to be, what;looks like, a delay for setting a value in State.
A lot of the currently randomly failing tests are using State to trigger some kind of behaviour change in the middle of a test.
Which, if the above statement is true, would make them also randomly fail, which seems to happen.
Time for some nitty-gritty proof why I think this is this case, dug up from the killing fields of test runs I had on this particular issue:
Not quite sure how to handle the new stronger typing, also some/many methods can be removed from our code and reference the behat-ones in 12.0.0.
Seems 📌 When using drupalGet(), provide an associative array for $headers Fixed went into a different direction as #3 intended, there are still six TODO's left pointing to this issue.
These TODO's are all about deprecating passing non-string values to string-only parameters.
This issue will add a deprecation for removal in 12.0.0 to the current BC-layer.
Thanks @ptmkenny and @mondrake: All changes make sense => RTBC,
larowlan → credited spokje → .
Added a first draft for a CR.
Hmmm,
If I search core for "varchar" I get ~1200 results, so I would expect those to fail in the core GitLab CI, which they don't.
So that makes me think it's a GitLab (config of cspell-run) problem, and not a specific cspell version problem?
And as a side-note: This MR also seems to bump cspell from version "8.16.1" to version "8.17.1".
Perhaps by deleting the yarn.lock before doing a yarn install?
I tried this but also got shipwrecked on the Selenium session bit, probably some new config option we have to pass somewhere?
Oh, and all the resolutions we all love to hate in the package.json are there because of Nightwatch holding at 2.4.2.
If we get it higher, we can drop all of them AFAICT.
Two test-failures, one is a known random, the image one I haven't seen before myself, but I would find it _very_ weird if that could be cause by tuf.
Marking as RTBC.
MINIMUM_STABILITY should be set to 'stable' in core/tests/Drupal/BuildTests/Composer/Template/ComposerProjectTemplatesTest.php.
Not to sure we want to change the composer.json files.
I agree, basically by default, with @mondrake.
Adding the current errors to the baseline would, as already stated by @mondrake, flag any new ones that pop up.
If we added them to be ignored by default with the parameters: ignoreErrors: "magic" new ones would also be ignored without warning us and thus not give us the chance to fix them, or at the very least give it an attempt.
So I'm on #TeamMondrake for adding them to the baseline.
NW is probably the status for this now.
No clue either, but I'm officially calling it quits on trying to update any JS dependency.
It's far too tedious with rerolls, canary-in-coal-mine for broken HEADs and multiple branches.
I think we should put time in researching something like Renovate or whatever shiny stuff works on GitLab these days.
IMHO it shouldn't take the roughly 10-15hrs I've spent this time around.
This is by no means a rant against core committers, who can somehow juggle multiple branches almost perfectly, but for mere mortals like me, this is _very_ frustrating and something that looks like it can be achieved with automation far easier than a/this human banging its forehead on a keyboard :)
After:
$ yarn outdated --no-links
yarn outdated v1.22.22
info Color legend :
"<red>" : Major Update backward-incompatible updates
"<yellow>" : Minor Update backward-compatible features
"<green>" : Patch Update backward-compatible bug fixes
Package Current Wanted Latest Package Type
eslint 8.57.0 8.57.1 9.17.0 devDependencies
glob 10.3.5 10.3.5 11.0.0 devDependencies
jackspeak 2.1.1 2.1.1 4.0.2 resolutionDependencies
nightwatch 2.4.2 2.4.2 3.9.0 devDependencies
postcss 8.4.38 8.4.49 8.4.49 devDependencies
postcss-preset-env 9.5.11 9.6.0 10.1.2 devDependencies
semver 7.5.4 7.5.4 7.6.3 resolutionDependencies
shepherd.js 10.0.1 10.0.1 14.3.0 devDependencies
TBH I've ran out of steam for fancy diffs and linking issues for stuff that can't be done.
Instead here's the before:
$ yarn outdated
yarn outdated v1.22.22
info Color legend :
"<red>" : Major Update backward-incompatible updates
"<yellow>" : Minor Update backward-compatible features
"<green>" : Patch Update backward-compatible bug fixes
Package Current Wanted Latest Package Type URL
@floating-ui/dom 1.6.5 1.6.12 1.6.12 devDependencies https://floating-ui.com
chokidar 3.6.0 3.6.0 4.0.1 devDependencies https://github.com/paulmillr/chokidar
dotenv 16.4.5 16.4.7 16.4.7 devDependencies https://github.com/motdotla/dotenv#readme
eslint 8.57.0 8.57.1 9.17.0 devDependencies https://eslint.org
eslint-plugin-import 2.29.1 2.31.0 2.31.0 devDependencies https://github.com/import-js/eslint-plugin-import
eslint-plugin-no-jquery 3.0.2 3.1.0 3.1.0 devDependencies https://github.com/wikimedia/eslint-plugin-no-jquery#readme
eslint-plugin-prettier 5.1.3 5.2.1 5.2.1 devDependencies https://github.com/prettier/eslint-plugin-prettier#readme
eslint-plugin-yml 1.14.0 1.16.0 1.16.0 devDependencies https://ota-meshi.github.io/eslint-plugin-yml/
glob 10.3.5 10.3.5 11.0.0 devDependencies https://github.com/isaacs/node-glob#readme
jackspeak 2.1.1 2.1.1 4.0.2 resolutionDependencies https://github.com/isaacs/jackspeak#readme
jquery-ui 1.14.0 1.14.1 1.14.1 devDependencies https://jqueryui.com
jsdom 24.0.0 24.1.3 25.0.1 devDependencies https://github.com/jsdom/jsdom#readme
nightwatch 2.4.2 2.4.2 3.9.0 devDependencies https://nightwatchjs.org
postcss 8.4.38 8.4.49 8.4.49 devDependencies https://postcss.org/
postcss-preset-env 9.5.11 9.6.0 10.1.2 devDependencies https://github.com/csstools/postcss-plugins/tree/main/plugin-packs/postcss-preset-env#readme
prettier 3.2.5 3.4.2 3.4.2 devDependencies https://prettier.io
semver 7.5.4 7.5.4 7.6.3 resolutionDependencies https://github.com/npm/node-semver#readme
shepherd.js 10.0.1 10.0.1 14.3.0 devDependencies https://shepherdjs.dev
sortablejs 1.15.2 1.15.6 1.15.6 devDependencies https://github.com/SortableJS/Sortable#readme
terser 5.31.0 5.37.0 5.37.0 devDependencies https://terser.org
terser-webpack-plugin 5.3.10 5.3.11 5.3.11 devDependencies https://github.com/webpack-contrib/terser-webpack-plugin
tua-body-scroll-lock 1.5.0 1.5.3 1.5.3 devDependencies https://github.com/tuax/tua-body-scroll-lock#readme
underscore 1.13.6 1.13.7 1.13.7 devDependencies https://underscorejs.org
webpack 5.96.1 5.97.1 5.97.1 devDependencies https://github.com/webpack/webpack
They all we're needed to pass the stylelint job, see https://git.drupalcode.org/issue/drupal-3487817/-/pipelines/359246/codeq...
Nice,
The only failing test now is an annoying one, which always gets me.
In core/tests/PHPStan/composer.json the version of phpstan/phpstan should match the version that is in composer.json.
Thanks for the hard work @ptmkenny
The first two errors you mentioned are caused by there being text after the
So far example <code>core/tests/Drupal/Tests/Core/State/StateTest.php /**
* Tests both get() & delete() method.
*
* Here testing the key and value after deleting the key's value.
*
* @covers ::get
* @covers ::delete
*
* Ensure that deleting clears some static cache.
*/
So if we put that last line before the @covers ::get PHPStan should be much happier.
The missingType.generics are much harder to fix and we _could_ suppress them, so the behaviour on those would be the same as in PHPStan 1.x, with:
parameters:
ignoreErrors:
-
identifier: missingType.generics
https://github.com/phpstan/phpstan/blob/2.0.x/UPGRADING.md#removed-optio...