Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Several modules, including Media, have integration tests for QuickEdit, but the module is slated to be removed from core and moved to contrib.
List of known tests:
core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
core/modules/layout_builder/tests/src/Functional/LayoutBuilderQuickEditTest.php
core/modules/inline_form_errors/tests/src/FunctionalJavascript/FormErrorHandlerQuickEditTest.php
core/modules/image/tests/src/FunctionalJavascript/QuickEditImageEditorTestTrait.php
core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php
core/modules/image/tests/src/Functional/QuickEditImageControllerTest.php
core/modules/settings_tray/tests/src/FunctionalJavascript/QuickEditIntegrationTest.php
core/modules/editor/tests/src/Functional/QuickEditIntegrationLoadingTest.php
core/modules/editor/tests/src/Kernel/QuickEditIntegrationTest.php
core/modules/media/tests/src/Kernel/MediaEmbedFilterDisabledIntegrationsTest.php
Proposed resolution
Move these tests (or the appropriate test cases from them) into QuickEdit's test namespace to facilitate the core removal.
Remaining tasks
NR
API changes
QuickEditImageEditorTestTrait
is moved from the Image module namespace to the QuickEdit module namespace.
Data model changes
N/A
Release notes snippet
Not needed per release managers.
Issue fork drupal-3228634
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:
- 3228634-move-qe-tests changes, plain diff MR !1077
- 3228634-FAIL changes, plain diff MR !1085
Comments
Comment #2
xjmComment #3
xjmComment #4
xjmStarting this now.
Comment #6
xjmComment #7
xjmSome unused use statements after the move indicate they probably need to have namespaces fixed:
Removing them is not the right course of action as the tests will likely fatal anyway. ;)
Comment #8
xjmActually it turned out they were all from a QuickEdit test base class in the same directory, so they were actually safe to remove. I'm expecting at least one other fail due to a namespace problem but we'll see.
Comment #9
xjmComment #10
xjmComment #11
xjmUnassigning in case anyone else wants to work on this overnight-for-me.
Comment #13
xjmNice work @Spokje! Also doh at forgetting to take out the data provider once and the parameter twice.
I'm going to open a second test MR and branch based on this one, just to make it clear that all nine tests are still being discovered.
Comment #15
xjmSo we expect the FAIL branch to fail nine tests, to prove those tests are all still being discovered. So long as that happens, I think we are good here!
Comment #16
xjmDiscussed with @catch. We agreed that moving a test trait does not need a BC layer, just a CR and issues for contrib projects affected. It appears no contrib uses this trait: http://grep.xnddx.ru/search?text=QuickEditImageEditorTestTrait&filename=
If it were production code we would be more careful about BC since we never know what custom code is doing, but at worst we'll break their CI with a clean fatal error.
Comment #17
xjmComment #18
xjmComment #19
xjmCool, that's what we're looking for. Nine fails on the test-only MR to prove all nine tests are running, and green on the actual MR.
Comment #20
xjmAdded a draft CR: https://www.drupal.org/node/3228826
Comment #21
xjmComment #23
xjmComment #24
xjmJust pushing that last commit to the test MR as well to verify that the test is still running after being renamed.
Comment #25
xjmWhich it is. 👍
Comment #26
tim.plunkettThe patch looks great. The tests are either already well-named or have been renamed, and the only real change is to MediaEmbedFilterDisabledIntegrationsTest, which also looks correct.
Comment #28
xjmAdding credit for @Lendude who gave helpful feedback on the MR on the GitLab side.
Comment #31
catchLooks great here too. Committed/pushed to 9.3.x, thanks!
Cherry-picking to 9.2.x too to keep the tests in sync.
Comment #32
LendudeSeems to have broken HEAD
https://www.drupal.org/pift-ci-job/2161579
Comment #35
catchRolled back for now.
Comment #36
tim.plunkettUhhh what are those $this->fail(); calls in the commit? They are not in the https://git.drupalcode.org/project/drupal/-/merge_requests/1077.diff or the MR when I check it out...
EDIT: Seems to be fd3500a3c10c4eeeab8d1670d0ed448e41db62b4 but there's something wonky with gitlab where that commit is NOT in the branch when you check it out, or in the generated diff, but it showed up in the squash merge
Comment #39
catchI committed the fail merge request - clicked the wrong link to get the .diff.
Trying again.
Comment #40
xjmPublished the CR.