Problem/Motivation

Spinning off from #3227033: Remove Quick Edit from core. There are a handful of places where we're currently leaning on quickedit data attributes as selectors for various test assertions in places that have nothing to do with testing quick edit functionality.

The main culprit are tests of stable / stable9, where there are no classes to target something specific. For example:

core/modules/node/tests/src/Functional/NodeDisplayConfigurableTest.php

It's trying to do this in assertNodeHtml():

$assert->elementTextContains('css', $created_selector, \Drupal::service('date.formatter')->format($node->getCreatedTime()));

So we need a selector to find exactly where the timestamp is appearing. But without quickedit enabled, the markup for this node in stable and stable9 looks something like this:

<article role="article" about="/drupal-9_1/node/1">
      <footer>
      <article typeof="schema:Person" about="/drupal-9_1/user/2">
  <div class="js-form-item form-item js-form-type-item form-item- js-form-item- form-no-label">
        <h4 class="label">Member for</h4> 24 seconds
        </div>
</article>
      <div>
        Submitted by <span><a title="View user profile." href="/drupal-9_1/user/2" lang="" about="/drupal-9_1/user/2" typeof="schema:Person" property="schema:name" datatype="">eMHCSGYOuJAkD2</a></span>
 on <span>Fri, 08/06/2021 - 11:09</span>
      </div>
    </footer>
  <div>
            <div><p>vJUMtKdL7e4hG5MdElsfGh0ucpCuPhDY</p>
</div>
  </div>
</article>

I'm not sure how we're supposed to find this needle: <span>Fri, 08/06/2021 - 11:09</span> in that haystack, without any classes or anything to distinguish it from every other <span>.

Proposed resolution

Make test assertions less specific when testing themes without field classes.

The following tests need help:

  • core/modules/node/tests/src/Functional/NodeDisplayConfigurableTest.php
    • Works for all core themes that provide field classes.
    • Needs a solution for stable + stable9
  • core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php
  • core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayTestBase.php

Remaining tasks

  1. Come up with a solution.
  2. Implement it.
  3. Confirm tests are all still passing.
  4. Reviews / refinements.
  5. RTBC.
  6. Commit.
  7. Unblock parent and friends.

User interface changes

n/a

API changes

n/a

Data model changes

n/a

Release notes snippet

n/a

CommentFileSizeAuthor
#3 3227161-2.data-quickedit-todo.txt1.93 KBdww

Issue fork drupal-3227161

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:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww created an issue. See original summary.

dww opened merge request !1029

dww’s picture

dww
we/he/they
CreditAttribution: dww at TEN7 commented
Issue summary: View changes
Status: Active » Needs work
FileSize
3227161-2.data-quickedit-todo.txt1.93 KB

Cherry picked the proof-of-concept commit about NodeDisplayConfigurableTest from the parent issue into this issue's fork. Still @todo to get that test working again for stable + stable9.

Also grabbed the commit about MediaEmbedFilterDisabledIntegrationsTest. Still @todo: that test is now a Kernel test with a data provider that only provides a single case. Refactor?

Grep says these are the only places we reference 'data-quickedit' in any tests that are neither provided by quickedit module itself, nor explicit tests for quickedit integration (e.g. core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php):

  • core/modules/media/tests/src/Kernel/MediaEmbedFilterDisabledIntegrationsTest.php
  • core/modules/node/tests/src/Functional/NodeDisplayConfigurableTest.php
  • core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php
  • core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayTestBase.php

grep output attached for reference.

This is actually a smaller set of tests to deal with than I first feared. ;)

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

MediaEmbedFilterDisabledIntegrationsTest I think is actually an integration test with quickedit. But it is testing code in quickedit_entity_view_alter(), so maybe we can just move that test into the quickedit module to make it easier to move into contrib.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Priority: Normal » Major

Posted #3228634: Move tests for integrations between QuickEdit and other modules into QuickEdit so that it can more easily be moved into contrib to better address @Berdir's feedback.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +Drupal 10
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue summary: View changes

Also updated the IS to remove the Media test from the scope.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Sorry for the noise; cherry-picked something from a related issue to the wrong branch.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented

I'm not sure how we're supposed to find this needle: Fri, 08/06/2021 - 11:09 in that haystack, without any classes or anything to distinguish it from every other .

I did not find any solution for it as well. What is a possible solution?

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Parent issue: #3227033: Remove Quick Edit from core » #3228986: [Meta] Tasks to deprecate Quick Edit
dww’s picture

dww
we/he/they
CreditAttribution: dww at TEN7 commented
Issue summary: View changes
Status: Needs work » Needs review

Here's a solution that at least gets the tests passing locally:

commit 0d9a9f5738b4cca779f2309411113762d3052bd4
Author: Derek Wright <git@dwwright.net>
Date:   Thu Aug 19 19:21:25 2021 -0700

    Issue #3227161: Make the assertions less specific for themes that don't have field classes.

Not sure that's going to fly, but it's the best I could come up with that maintains as much test coverage as possible.

If @xjm is cool with that, we can turn our attention elsewhere.

I did a quick skim of core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTray*. Those look more like actual tests of Quick edit, and are maybe more appropriately moved over to the scope of #3228634: Move tests for integrations between QuickEdit and other modules into QuickEdit so that it can more easily be moved into contrib instead of trying to deal with them here?

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Needs review » Needs work

This needs merge conflicts resolved -- auto-rebasing through the UI failed.

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

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Needs review

Rebased and force pushed; deleted the commits that were later reverted.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: Stop using quickedit data attributes as selectors in tests that are not testing quickedit » Stop using Quick Edit data attributes as selectors in tests that are not testing Quick Edit
Status: Needs review » Needs work

Review posted. It makes sense that the two fields whose values cannot be precisely asserted anymore are the name and created base fields — back in 2012 when I was working on Quick Edit that were precisely the two I was unable to reliably target across themes as well, precisely because they were rendered in non-standard ways. 9 years later, that's still the case 🤓

Those look more like actual tests of Quick edit, and are maybe more appropriately moved over to the scope of #3228634: Move tests for integrations between QuickEdit and other modules into QuickEdit so that it can more easily be moved into contrib

+1

… But … #3228634: Move tests for integrations between QuickEdit and other modules into QuickEdit so that it can more easily be moved into contrib has since then been committed and did not do that. That's wrong, because:

  • \Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testEditModeEnableDisable() even has it in its method name: its literally testing Quick Edit's "edit mode".
  • \Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testBlocks() cals ::doTestBlocks() which does:
        $link = $web_assert->waitForElement('css', "$block_selector .contextual-links li a");
    $this->assertEquals('Quick edit', $link->getHtml(), "'Quick edit' is the first contextual link for the block.");
  • \Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testValidationMessages() does $this->enableEditMode();
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

> that were precisely the two I was unable to reliably target across themes as well, precisely because they were rendered in non-standard ways. 9 years later, that's still the case

FWIW, this is actually a "new" problem, as we aggressively removed all classes from most markup in the minimalist themes in Drupal 8. So that's kind of by design and a feature(?).

This is off topic of course, but I'm wondering if that's really still a thing that enough people care about? I feel like a a lot of people that want that level of control moved to decoupled and other approaches, and there could still be a contrib theme like that. As mentioned, the whole thing always felt very strange to me, someone who doesn't want default classes will also not want double-divs on single-value fields with no class, imho that's even less desired than divs with default classes, but maybe that's just me. Anyway, might be worth to revisit that topic?

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

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

I feel like a a lot of people that want that level of control moved to decoupled and other approaches, and there could still be a contrib theme like that.

Very interesting question! 🤔

As mentioned, the whole thing always felt very strange to me, someone who doesn't want default classes will also not want double-divs on single-value fields with no class, imho that's even less desired than divs with default classes, but maybe that's just me. Anyway, might be worth to revisit that topic?

I think it's to empower the Site Builder persona?

@lauriii & @dww: thanks, that addressed all of my concerns! :)

I'd RTBC, but the last 3 bullets of #15 are not yet addressed. I'm fine with moving that to a follow-up too.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

#15 seems scope creep given that this issue was opened specifically to handle Quick Edit data attributes used in arbitrary tests. Maybe we need a follow-up for #3228634: Move tests for integrations between QuickEdit and other modules into QuickEdit so that it can more easily be moved into contrib?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +Needs followup

+1

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

(RTBC as soon as the follow-up exists.)

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented
Issue tags: -Needs followup

Here is a follow-up issue: #3238626: [Follow-up] Move tests for integrations between QuickEdit and other modules into QuickEdit

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented
Status: Needs work » Reviewed & tested by the community

Moving to RTBC per #21

  • catch committed f93d9a3 on 9.3.x 
    Issue #3227161 by dww, xjm, longwave, Wim Leers, paulocs, lauriii,...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed f93d9a3 and pushed to 9.3.x. Thanks!

Status: Fixed » Closed (fixed)

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