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

Command icon 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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm created an issue. See original summary.

xjm’s picture

Issue tags: +Drupal 10
xjm’s picture

Issue summary: View changes
xjm’s picture

Assigned: Unassigned » xjm

Starting this now.

xjm’s picture

Status: Active » Needs review
xjm’s picture

Some unused use statements after the move indicate they probably need to have namespaces fixed:

FILE: ...it/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 8 | WARNING | [x] Unused use statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------
FILE: .../quickedit/tests/src/FunctionalJavascript/QuickEditImageTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 7 | WARNING | [x] Unused use statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...ore/modules/quickedit/tests/src/Kernel/EditorIntegrationTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 11 | WARNING | [x] Unused use statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Removing them is not the right course of action as the tests will likely fatal anyway. ;)

xjm’s picture

Actually 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.

xjm’s picture

Issue summary: View changes
xjm’s picture

xjm’s picture

Assigned: xjm » Unassigned

Unassigning in case anyone else wants to work on this overnight-for-me.

Spokje made their first commit to this issue’s fork.

xjm’s picture

Assigned: Unassigned » xjm

Nice 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.

xjm’s picture

Assigned: xjm » Unassigned

So 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!

xjm’s picture

Status: Needs review » Needs work
Issue tags: -Needs release manager review +Needs change record

Discussed 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.

xjm’s picture

Assigned: Unassigned » xjm
xjm’s picture

Issue summary: View changes
xjm’s picture

Assigned: xjm » Unassigned
Status: Needs work » Needs review

Cool, 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.

xjm’s picture

xjm’s picture

Issue summary: View changes

paulocs made their first commit to this issue’s fork.

xjm’s picture

xjm’s picture

Just pushing that last commit to the test MR as well to verify that the test is still running after being renamed.

xjm’s picture

Which it is. 👍

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

The 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.

xjm credited Lendude.

xjm’s picture

Adding credit for @Lendude who gave helpful feedback on the MR on the GitLab side.

  • catch committed e4ce993 on 9.3.x
    Issue #3228634 by Spokje, xjm, paulocs, tim.plunkett, Lendude: Move...

  • catch committed d2da6f2 on 9.2.x
    Issue #3228634 by Spokje, xjm, paulocs, tim.plunkett, Lendude: Move...
catch’s picture

Version: 9.3.x-dev » 9.2.x-dev
Status: Reviewed & tested by the community » Fixed

Looks great here too. Committed/pushed to 9.3.x, thanks!

Cherry-picking to 9.2.x too to keep the tests in sync.

Lendude’s picture

  • catch committed a3f40ba on 9.3.x
    Revert "Issue #3228634 by Spokje, xjm, paulocs, tim.plunkett, Lendude:...

  • catch committed 33e6832 on 9.2.x
    Revert "Issue #3228634 by Spokje, xjm, paulocs, tim.plunkett, Lendude:...
catch’s picture

Status: Fixed » Needs work

Rolled back for now.

tim.plunkett’s picture

Uhhh 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

  • catch committed 02bf753 on 9.3.x
    Issue #3228634 by Spokje, xjm, paulocs, tim.plunkett, Lendude: Move...

  • catch committed e06d49b on 9.2.x
    Issue #3228634 by Spokje, xjm, paulocs, tim.plunkett, Lendude: Move...
catch’s picture

Status: Needs work » Fixed

I committed the fail merge request - clicked the wrong link to get the .diff.

Trying again.

xjm’s picture

Published the CR.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.