Problem/Motivation
Remove use of Search module from tests not in the Search module.
- core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderDisableInteractionsTest.php
- core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php
- core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathTestBaseFilledTest.php
- core/modules/system/tests/src/Kernel/Entity/ConfigEntityImportTest.php
- core/modules/toolbar/tests/src/Functional/ToolbarAdminMenuTest.php
- core/tests/Drupal/FunctionalTests/Theme/OliveroTest.php
- core/tests/Drupal/KernelTests/Core/Recipe/RecipeTest.php
- core/modules/views/tests/modules/views_test_config/test_views/views.view.test_search.yml
- core/modules/views/tests/modules/views_test_config/test_views/views.view.test_argument_dependency.yml
Steps to reproduce
Proposed resolution
Remove Search, it was was removed from the example recipe used in this test
- core/tests/Drupal/KernelTests/Core/Recipe/RecipeTest.php
Remove and change a test string
- core/modules/toolbar/tests/src/Functional/ToolbarAdminMenuTest.php
Replace Search with a test module
- core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderDisableInteractionsTest.php
- core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php
Views tests still referencing search
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_search.yml is still needed so moved to under a new search_test_views module under search tests.core/modules/views/tests/modules/views_test_config/test_views/views.view.test_argument_dependency.yml referenced search but it does not actually seem to need search
No change here, do when Search is removed as part of #3565783: [meta] Tasks to remove the Search module
- core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathTestBaseFilledTest.php
- core/modules/system/tests/src/Kernel/Entity/ConfigEntityImportTest.php
- core/tests/Drupal/FunctionalTests/Theme/OliveroTest.php
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
LLM disclosure
LLM was used to discover and resolve the views portion of the issue.
Show commands
changes
Comments
Comment #3
quietone commentedComment #4
quietone commentedComment #5
quietone commentedComment #6
quietone commentedcore/modules/system/tests/src/Functional/UpdateSystem/UpdatePathTestBaseFilledTest.php can be done later, when the module is removed.
Not sure how best to change these:
1. core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderDisableInteractionsTest.php
Doesn't pass tests locally on main, so can't really do anything.
2. core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php
What to do here, this test is currently skipped due to random failers.
3. core/tests/Drupal/FunctionalTests/Theme/OliveroTest.php
Don't know
Comment #8
mstrelan commentedThis is a good start, and I've pushed some more changes.
1.
LayoutBuilderDisableInteractionsTestpasses locally for me. I've removed the search form and replaced it with a form block from a test module. We just need to test that the form cannot be interacted with.2.
SettingsTrayBlockFormTestis skipped, but it passes sometimes. I removed the skips temporarily and applied the same treatment as #1. The test passed, and I restored the skips. There is a small possibility that switching out the form reduced the flakiness of this test, but I don't we should unskip it here.3.
OliveroTestneeds to keep testing for the search block form until it is removed fromconfig/optional. When we remove that config, the test will start failing and we can address it then.Comment #9
mstrelan commentedI actually don't think it makes sense to move the ConfigEntityImportTest changes to search module. AFAICT we're not testing anything specifically related to search, it's actually testing the config entity import system. I think we should find a replacement here, or leave it out.
Comment #10
quietone commented@mstrelan, thanks for working on this. Nice that you could work on the functionaljavascript tests here! I was hoping to done what you did but without the tests passing for me I was stuck. And even though I changed ConfigEntityImportTest.php I thought it was a mistake.
You mention that that form in LayoutBuilderDisableInteractionsTest needs to be tested that it can be interacted with. But isn't that already done in assertLinksFormIframeNotInteractive?
I have restored ConfigEntityImportTest.php and updated the issue summary
Comment #11
mstrelan commentedYes, I just meant the test needs some form to test, and that's what I provided by swapping out the search form. I can see how what I wrote sounds like it there was more to do.
Comment #12
dcam commentedI found one issue where search* wasn't removed from a modules array. I left a suggestion for it. Please check what I found.
I wasn't able to find any other instances of search being used by other modules' tests aside from what's mentioned here.
Comment #13
quietone commented@dcam, thanks for spotting that error.
All fixed and tests are passing
Comment #15
gábor hojtsyFound two views in the views module that still used search.
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_search.ymlwhich was needed so moved to under a newsearch_test_viewsmodule under search tests.core/modules/views/tests/modules/views_test_config/test_views/views.view.test_argument_dependency.ymlreferenced search but that seems to be leftover from prior things as it did not use it anymore.Added proposed solutions to these.
LLM was used to accelerate resolving this.
Added to issue summary too.
Comment #16
gábor hojtsyComment #17
gábor hojtsyFix issue summary note.
Comment #18
gábor hojtsyThis is still not passing on the views test. After much back and forth it seems like the prior pass was accidental.
ViewExecutableTestdoesn't install search in its$modules. Therefor the search argument plugin class did not exist at runtime, so the keys argument was broken (went to plugin not found). It hit the$argument->broken()check, and was then skipped entirely without substitutions. That's why only uid appeared in the expected array in the code prior to the MR.It does not seem like the test is intended to test the broken argument does it? Without more info on it, this seems like an open question left.
Comment #19
gábor hojtsyOk I finally found https://git.drupalcode.org/project/drupal/-/commit/26a5a2ab8a4a69298d006... which is where this test method was added (the view itself was used before too). Looks like the goal was to do that one substitution, so that one argument should be the only one subtituted :)
Comment #20
smustgrave commentedKernel failures seem related to the changes to core/modules/views/tests/modules/views_test_config/test_views/views.view.test_argument_dependency.yml