Expand EntityEmbedFilter's test coverage: test many more edge cases, change to kernel test

Many thanks, both of you!

#3:

I know, unit tests are hard when doing rendering!

1. +++ b/tests/src/Kernel/EntityEmbedFilterMediaTest.php
   @@ -0,0 +1,164 @@
   +class EntityEmbedFilterMediaTest extends KernelTestBase {
   

   Great start, thanks!

2. +++ b/tests/src/Kernel/EntityEmbedFilterMediaTest.php
   @@ -0,0 +1,164 @@
   +    // \Drupal\entity_embed\EntityEmbedDisplay\FieldFormatterEntityEmbedDisplayBase::build()
   +    // has an undeclared dependency on the "Node" module.
   +    'node',
   

   Reported independently, and being fixed over at #3054873: Hidden dependency on node module :)

#4: You may be right. I have no strong preference about how we get there. As long as we get there :)

I see what you're saying with the painful assertions. I encountered the same problem. That's why as part of #2752253-42: Document correct filter_html_image_secure + filter_align + filter_caption + entity_embed filters order, write tests, and ensure correct by default, I started converting those $this->assertRaw('LONG HTML STRING') calls to something like $this->assertSession()->elementExists('css', 'figure.caption-drupal-entity.align-left div.embedded-entity[data-entity-embed-display="entity_reference:entity_reference_entity_view"][data-entity-embed-display-settings="full"][data-entity-type="node"][data-entity-uuid="' . $this->node->uuid() . '"][data-view-mode="some-invalid-view-mode"][data-langcode="en"]');. I like how you're taking it even further.

Does either of you have time to continue this in the next days or week?

Woah, massive progress since I last reviewed this! 🥳 💪

#12:

With reducing runtime being the goal of a Kernel test, I'll combine them.

Kernel tests already run much faster than functional tests. Let's not combine them, because then we will eventually end up with a single multi-thousand line method that is impossible to understand. Let's keep them scoped as before.
EDIT: this actually will probably become irrelevant if we start using a @dataProvider like I suggested below :)

#13: you're right. Ignore that bullet :) Removed it.

1. +++ b/tests/src/Functional/EntityEmbedFilterTest.php
   @@ -2,8 +2,11 @@
   @@ -27,6 +30,41 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
   
   @@ -27,6 +30,41 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
   +    $manager = $this->container->get('plugin.manager.filter');
   +    $bag = new FilterPluginCollection($manager, []);
   +    $this->filter = $bag->get('entity_embed');
   +    $this->filterCaption = $bag->get('filter_caption');
   +  }
   

   This has not yet been converted to a kernel test. Even though much of the setUp() method is already prepared for operating in a kernel test: it's very similar to \Drupal\Tests\filter\Kernel\FilterKernelTest::setUp(), where \Drupal\Tests\filter\Kernel\FilterKernelTest::testAlignFilter() and \Drupal\Tests\filter\Kernel\FilterKernelTest::testCaptionFilter() are good examples to consider.

2. +++ b/tests/src/Functional/EntityEmbedFilterTest.php
   @@ -34,181 +72,388 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
        // Tests entity embed using entity ID and view mode.
   ...
   +    $content = $this->getEmbedCode([
   +      'data-entity-type' => 'node',
   +      'data-entity-id' => $this->node->id(),
   +      'data-view-mode' => 'teaser',
   +    ]);
   ...
   +    $this->assertNotContains('drupal-entity data-entity-type="node" data-entity', $output);
   +    $this->assertNotContains('This placeholder should not be rendered.', $output);
   ...
        // Tests that embedded entity is not rendered if not accessible.
   

   This is now ideally prepared to be converted to a test using a @dataProvider. The quoted comment would be the array key (describing a particular test case) and the parameters for getEmbedCode() would be the first array value, plus additional array values for each of the expectations below.

   So something like

   /**
    * @dataProvider filterData
    */
   function testFilter(array $attributes, array $expected_html_present, array $expected_html_absent, $expected_attributes) {
     // assertions here
   }
   
   function filterData() {
     return [
       'the minimum: entity type, ID and view mode'=> [
         ['data-entity-type' => 'node', 'data-entity-id' => 3, 'data-view-mode' => 'teaser'],
         ['NODE BODY VALUE: hello world!'],
         [
           'drupal-entity data-entity-type="node" data-entity', 
           'This placeholder should not be rendered.',
         ],
         […]
       ],
       'not rendered if not accessible' => […],
       …
     ];
   }
   

   This will:
   A) allow each test case to run independently, meaning that if one of the earlier cases fails, the others are still evaluated, which makes the test suite MUCH more useful
   B) therefore it becomes 100x easier to debug regressions
   C) make it much easier to get a sense of the various test cases that exist

   Good examples of this pattern: \Drupal\Tests\big_pipe\Unit\Render\Placeholder\BigPipeStrategyTest and \Drupal\Tests\layout_builder\Kernel\DefaultsSectionStorageTest::providerTestAccess().

If you want, I'm happy to explain it more detail. :) Or start doing this for a subset of it.

🥳👍

Yeah I was in your place a few years ago, I first wasn't entirely convinced by its value, but once I got the hang of it, MAN IS IT AWESOME! Seems superfluous now, but wait until you run into a bug that breaks just this one edge case. It's such a time saver then.

WRT converting to kernel test: I'll try to do that today.

#18 is looking excellent already! 👏

Here is interim feedback, this is heading the right direction — thanks!

1. +++ b/tests/src/Functional/EntityEmbedFilterTest.php
   @@ -76,11 +76,78 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
   +   * Tests the entity_embed filter.
   

   Nit: Already in the class doc, can be removed :)

2. +++ b/tests/src/Functional/EntityEmbedFilterTest.php
   @@ -76,11 +76,78 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
   +   * @throws \Drupal\Core\Entity\EntityStorageException
   

   Nit: We don't add @throws in test methods :)

3. +++ b/tests/src/Functional/EntityEmbedFilterTest.php
   @@ -76,11 +76,78 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
   +  public function testFilter($embed_attributes, $contains, $not_contains, $expected_attributes, $published, $view_unpublished) {
   

   Nit: $embed_attributes is an array, so should be typehinted array.

4. +++ b/tests/src/Functional/EntityEmbedFilterTest.php
   @@ -76,11 +76,78 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
   +      $this->webUser->addRole('access_unpublished');
   

   Nit: $this->webUser can be a local variable, it's not reused anywhere.

5. +++ b/tests/src/Functional/EntityEmbedFilterTest.php
   @@ -76,11 +76,78 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
   +    if (!$published && !$view_unpublished) {
   

   The naming confused me; I think $is_published and $allowed_to_view_unpublished are clearer.

6. +++ b/tests/src/Functional/EntityEmbedFilterTest.php
   @@ -76,11 +76,78 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
   +    $this->assertNotContains('This placeholder should not be rendered.', $output);
   
   @@ -174,115 +241,163 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
   +        'not_contains' => 'This placeholder should not be rendered.',
   ...
   +        'not_contains' => 'This placeholder should not be rendered.',
   

   These test case-specific not_contains cases are already tested automatically/generically, so there's no need to repeat them here.

7. +++ b/tests/src/Functional/EntityEmbedFilterTest.php
   @@ -76,11 +76,78 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
   +      if (\is_array($contains)) {
   ...
   +      if (\is_array($not_contains)) {
   

   Nit: the leading \ is unnecessary.

8. +++ b/tests/src/Functional/EntityEmbedFilterTest.php
   @@ -174,115 +241,163 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
   +      'Test deprecated \'default\' Entity Embed Display plugin.' => [
   

   Übernit: \' in there: you could use either backticks or wrap it in double quotes.

9. +++ b/tests/src/Functional/EntityEmbedFilterTest.php
   @@ -174,115 +241,163 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
   +      if (\is_array($contains)) {
   +        foreach ($contains as $needle) {
   +          $this->assertContains($needle, $output);
   +        }
   +      }
   +      else {
   +        $this->assertContains($contains, $output);
   +      }
   

   This is in ::testInvalidEntity() and is extremely similar to the above test method. Perhaps a helper method would simplify this. Or perhaps one of the test*() methods can call the other?

10. +++ b/tests/src/Functional/EntityEmbedFilterTest.php
    @@ -174,115 +241,163 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
    +  /**
    +   * Test that tag of container element is not replaced when it's not
    +   * <drupal-entity>.
    +   */
    +  public function testContainerTagElementWhenNot($tag = "drupal-entity") {
    ...
    +    $content = str_replace($tag, "not-{$tag}", $content);
    

    Why is there a parameter if this is not a data provider?

11. +++ b/tests/src/Functional/EntityEmbedFilterTest.php
    @@ -174,115 +241,163 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
    +    /** @var \DOMNode $figure_wrapper */
    

    Outdated annotation :)

12. +++ b/tests/src/Functional/EntityEmbedFilterTest.php
    @@ -174,115 +241,163 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
    +    $this->assertNotEmpty($untouched);
    

    What is this asserting? How can it be not empty?

1. +++ b/tests/src/Functional/EntityEmbedFilterTest.php
   @@ -101,39 +96,21 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
        // Clear caches.
   -    drupal_flush_all_caches();
   +    // drupal_flush_all_caches();
   

   AWESOME! Tests still passing with this commented out. So let's remove it now :)

2. +++ b/tests/src/Functional/EntityEmbedFilterTest.php
   @@ -348,13 +322,13 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
   +    $content = str_replace('drupal-entity', "not-drupal-entity", $content);
   

   Perhaps it'd be clearer to not use <not-drupal-entity> but $this->randomMachineName()? :)

3. +++ b/tests/src/Functional/EntityEmbedFilterTest.php
   @@ -38,199 +48,490 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
   +        'not_contains' => NULL,
   ...
   +        'not_contains' => NULL,
   ...
   +    if (!empty($not_contains)) {
   +      if (is_array($not_contains)) {
   +        foreach ($not_contains as $needle) {
   +          $this->assertNotContains($needle, $output);
   +        }
   +      }
   +      else {
   +        $this->assertNotContains($not_contains, $output);
   +      }
   

   This is actually fairly confusing. Because we allow NULL or array, not only are the test cases more difficult to grok ("what is the meaning of NULL here?"), it also makes for more complex assertion logic. The assertion logic first needs to check the type of the argument, whereas if it'd just always be array, we could do this:

   foreach ($not_contains as $needle) {
     $this->assertNotContains(…);
   }
   

   That'd be it!

   In the test cases, we'd then have [] instead of NULL, which is more clear: there are no strings whose non-existence needs to be asserted.

4. I will continue with refactoring later this morning.

   How is this going? :)

Been caught up in meetings all day. Reviewed & committed two CKEditor-related patches today; this is my top priority for tomorrow 🥳

Interestingly, I have some fails locally, with PHP 7.2.14:

Testing Drupal\Tests\entity_embed\Functional\EntityEmbedFilterTranslationTest
.....                                                               5 / 5 (100%)

Time: 44.86 seconds, Memory: 6.00MB

OK (5 tests, 48 assertions)

Testing Drupal\Tests\entity_embed\Functional\EntityEmbedFilterTest
..........SEEE..                                                  16 / 16 (100%)

Time: 1.97 minutes, Memory: 6.00MB

There were 3 errors:

1) Drupal\Tests\entity_embed\Functional\EntityEmbedFilterTest::testDisplayPluginPreference
Error: Call to undefined method DOMText::getAttribute()

/Users/wim.leers/Work/d8/modules/entity_embed/tests/src/Functional/EntityEmbedFilterTest.php:668
/Users/wim.leers/Work/d8/modules/entity_embed/tests/src/Functional/EntityEmbedFilterTest.php:398

2) Drupal\Tests\entity_embed\Functional\EntityEmbedFilterTest::testAttributesWhenUsingImageFormatter
Error: Call to undefined method DOMText::getAttribute()

/Users/wim.leers/Work/d8/modules/entity_embed/tests/src/Functional/EntityEmbedFilterTest.php:668
/Users/wim.leers/Work/d8/modules/entity_embed/tests/src/Functional/EntityEmbedFilterTest.php:451

3) Drupal\Tests\entity_embed\Functional\EntityEmbedFilterTest::testEmbedSettingsCompatibility
Error: Call to undefined method DOMText::getAttribute()

/Users/wim.leers/Work/d8/modules/entity_embed/tests/src/Functional/EntityEmbedFilterTest.php:668
/Users/wim.leers/Work/d8/modules/entity_embed/tests/src/Functional/EntityEmbedFilterTest.php:492

This is caused by >firstChild returning a "text" DOM node for whitespace.

I wonder if this is specific to PHP 7.2, so I queued a test for it: https://www.drupal.org/pift-ci-job/1324414. That passed too. So … I don't know what's going on here, but let's just work around the problem by making the test coverage more robust :)

Actually, to make iterating on this faster, I spent a few hours today on making this a kernel test. The additional set of changes is minimal. It's mostly in ::setUp(). Which means it's much easier to review and understand than all of the work that has happened on this issue so far.

The difference is quite big:

Before

Testing Drupal\Tests\entity_embed\Functional\EntityEmbedFilterTest
..........S.....                                                  16 / 16 (100%)

Time: 1.77 minutes, Memory: 6.00MB

OK, but incomplete, skipped, or risky tests!
Tests: 16, Assertions: 256, Skipped: 1.

Testing Drupal\Tests\entity_embed\Functional\EntityEmbedFilterTranslationTest
.....                                                               5 / 5 (100%)

Time: 35.85 seconds, Memory: 6.00MB

OK (5 tests, 48 assertions)

After

PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\entity_embed\Functional\EntityEmbedFilterTest
..........S.....                                                  16 / 16 (100%)

Time: 19.85 seconds, Memory: 6.00MB

OK, but incomplete, skipped, or risky tests!
Tests: 16, Assertions: 302, Skipped: 1.

PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\entity_embed\Functional\EntityEmbedFilterTranslationTest
.....                                                               5 / 5 (100%)

Time: 6.91 seconds, Memory: 6.00MB

OK (5 tests, 53 assertions)

Next up: extracting common logic in a base test class for all filter kernel tests. And then actually reviewing the changes.

In converting this to kernel tests, a few assumptions in the functional tests were made more explicit, which is an expected and welcome consequence 🙂

Before extracting a base test class: moving to the right location.

This then extracts a base class doing the absolute minimum. We don't want the base test class to be coupled to all the potential things we want to have tests for; we want each subclass to install the additional modules needed and set up the additional configuration needed. That way, it remains very clear what the dependencies are, and adding new capabilities then also is forced to provide correspondingly clear test coverage.

EDIT: oh wow, @oknate continued working on this too. My bad, I should've assigned to myself. No big deal though — I'll incorporate his changes :) That means only #56 will largely be wasted. Assigning to myself, as I should've done before.

To be crystal clear: the attached interdiff is relative to my last patch (#48).

• #51 + #53: see attached interdiff-ported-51_and_53.txt.
• #54: see attached interdiff-ported-54.txt. Also: 👏👏 — that's exactly what I was going to suggest to @Wannes DR in a next iteration :)
• #55: Thanks 😊
• #56: see #61, where I already did the same, but in a more strict fashion (bare minimum base test class). I see you moved a bunch of the helpers into the base class. I did not yet do that. I prefer to hold off on doing that until there is a proven need for it across multiple test subclasses.
• #60: I already took care of this in #61, where I also added a comment explaining why this is necessary. Your guess at the root cause in #60 is reasonable, but not correct. The real answer lies in \Drupal\user\Entity\User::hasPermission() 🤓

The current test coverage is specifically testing Classy's markup and its subthemes. We shouldn't do that.

(It is currently not assuming core/modules/filter/templates/filter-caption.html.twig but core/themes/classy/templates/content-edit/filter-caption.html.twig.)

This test refactor is currently leading us into a kinda awkward place. We're not testing @Filter=entity_embed in isolation: we're testing it together with @Filter=filter_caption. Ideally we'd test it in isolation. But

In HEAD:

    $this->assertSession()->elementExists('css', 'figure.caption-drupal-entity.align-left div.embedded-entity[alt="This is alt text"][data-entity-embed-display="image:image"][data-entity-type="file"][data-entity-uuid="' . $image->uuid() . '"][title="This is title text"][data-langcode="en"] img[src][alt="This is alt text"][title="This is title text"]');

In this patch, that is converted to:

    $rendered_embed = $xpath->query('//figure[@role="group"]/div[contains(@class, "embedded-entity")]')[0];
    $this->assertHasAttributes($rendered_embed, [
      'alt' => 'This is alt text',
      'data-align' => 'left',
      'data-entity-embed-display' => 'image:image',
      'data-entity-type' => 'file',
      'data-entity-uuid' => $image->uuid(),
      'title' => 'This is title text',
      'data-langcode' => 'en',
    ]);
    $figcaption = $xpath->query('//figcaption')[0];
    $this->assertNotEmpty($figcaption);
    $this->assertEquals('test caption', $figcaption->textContent);

This is asserting the results for @Filter=filter_caption, but not for @Filter=filter_align. Having integration tests makes sense.

Note that this also lost the test coverage that asserts whether the alt and title on <drupal-entity> have been persisted onto the generated <img> tag. And it in fact is not testing anything on the <img> tag at all anymore, which git blame shows was the goal in #2712111: Embed specific attributes are cached along with the entity, where this test coverage was originally added.

I also have my doubts about whether this is truly clearer. I wrote in #5 how I like that you're taking it further, but it never was taken all the way, and hence is currently resulting in a net reduction in test coverage. This particular bit of test coverage was lost in #7's interdiff.

I'll continue to evaluate this as I review the rest of the patch.

Here is another concrete example of where the conversion is wrong: \Drupal\Tests\entity_embed\Kernel\EntityEmbedFilterTest::testDisplayPluginPreference(). This is porting the test coverage added by https://git.drupalcode.org/project/entity_embed/commit/b9a4f65c88b75cf3d.... It's asserting the presence of data- attributes continuing to exist that we set ourselves (which is pointless, of course they continue to exist), and is testing caption filter integration (which is not the edge case this is aiming to test). Reducing this back to the original scope make this much clearer.

And another such example: testEmbedSettingsCompatibility(). This used to and still should be testing that no <a> was created, since that's what the legacy data-embed-settings attribute is specifying. But that's not being asserted at all anymore.

(This was introduced by #2760801: Rename data-entity-embed-settings attribute — see https://git.drupalcode.org/project/entity_embed/commit/72a93e2d5efa7a760....)

These $this->assertNotContains('This placeholder should not be rendered.', $output); assertions all over the place were annoying, distracting and confusing in HEAD, and they still are. Let's make that better.

#68: indeed! An unavoidable painful process to go through when taking code with a lot of history but without clear test coverage. None of this is a criticism on either you or the original code contributors — we're all bound to deadlines, we all learn along the way, we all would like to do better. I just am fortunate enough to be given the task to bring this to core and have gone through stabilization processes like this many times before :) Please continue to double-check what I'm doing, and let me know when you disagree (or when you're happy with a change!).

Looks like there was a PHP 7.2-specific assertion.

Just like #65 and #66, here's another correction, this time for testAttributesWhenUsingImageFormatter(). This is the one I was talking about at length in #64.

Let's remove the dependency of our kernel test on entity_embed_test, which installs boatloads of configuration, so it's not at all clear what exactly we're testing with.

(entity_embed_test sort of became a dumping ground, which was fine for fast iteration, but if we want to bring this to core, we'll need more rigor.)

#75 allows us to remove editor and ckeditor from the modules being installed. We're getting to a place where dependencies are actually clear! 🥳

Related to #74: the cacheability-related assertions in #2712111: Embed specific attributes are cached along with the entity are actually covered much better by #3058872: Entity Embed render array shouldn't cache separately from host field, which introduced testOverridesAndRenderCaching().

That being said, testAttributesWhenUsingImageFormatter() is still valuable to keep: it asserts that overrides are working for File entities plus @EntityEmbedDisplay=image:image, whereas testOverridesAndRenderCaching() is testing Media entities plus @EntityEmbedDisplay=view_mode:*.

I think it makes sense to test both of those "override data of the embedded entity" scenarios in a single test class. #75 was a necessary step to make that feasible, because we don't want to be changing test logic and moving in a single interdiff, that makes it much to hard to track.

So this interdiff moves the two aforementioned test methods to a new \Drupal\Tests\entity_embed\Kernel\EntityEmbedFilterOverridesTest class, renames the methods and improves their docs, but doesn't change anything about their code. A happy (and very much intentional) consequence is that the file, image and media modules no longer need to be installed for EntityEmbedFilterTest, and almost no configuration needs to be installed anymore! 💪 That means that just like the name indicates, this is now providing the foundational tests only, without testing integration with lots of other modules, entity types, and whatnot.

Simultaneously, I've moved assertHasAttributes() and drupalCreateRole() into the base class, because there are now multiple test classes using them.

--- a/tests/src/Functional/EntityEmbedTestBase.php
+++ b/tests/src/Functional/EntityEmbedTestBase.php

The changes made to this test class are now irrelevant. Let's simply drop those hunks from the patch.

Having looked at the override tests in detail for #77, I spotted a few problems:

1. we're not yet testing what happens when there's no override
2. there's a lot of attributes irrelevant to the test coverage, which make it needlessly hard to understand what the test is testing
3. there's a useless drupalCreateNode()

Fixed all.

And another problem in that same test, but which I wanted to post separately to keep scope clear: in one of the two test methods, we're using the existing trait, but wrapping it in a helper of our own. In the other test method, we're copying an existing file in core. We should make this consistent and less complex.

I spotted $this->assertNotContains('drupal-entity data-entity-type="node" data-entity', $output); in a few places, this should be merged into assertEntityEmbedFilterHasRun(). That assertion method was added in #67 and this addition makes it much more clear what it does. It also helps that we're now consistently asserting this, instead of only in two places — that made it extra confusing!

An assortment of clarity nitpicks:

• data-align and data-caption where they're not needed
• $rendered_embed = $xpath->query('//div[contains(@class, "embedded-entity")]') where that is not the rendered embed, but the container of the rendered embed — renamed all of them to embedded_entity_container

With these changes, I consider \Drupal\Tests\entity_embed\Kernel\EntityEmbedFilterOverridesTest core-worthy now.

I'll continue iterating on EntityEmbedFilterTest because that definitely still needs more love. But I'm approaching my end of day. @oknate or @Wannes, feel free to port the @Filter=filter_caption and @Filter=filter_align test coverage from before that is currently missing. The testFigCaption() placeholder that Wannes added would be a great place to start to see those appear in :)

I started working on this again. Started the process of comparing the old test coverage with the refactored test coverage, to ensure we don't lose any coverage.

Updating testMissingEntityPlaceholder() to not create its own node but reuse the existing one, and to maximally use assertHasAttributes() rather than using that and doing raw string presence assertions.

There is a lot of repetition of a few lines of code. Let's introduce a helpful helper and let it absorb assertEntityEmbedFilterHasRun():

-    /** @var \Drupal\filter\FilterProcessResult $filter_result */
-    $filter_result = $this->filter->process($content, 'en');
-    $output = $filter_result->getProcessedText();
-    $this->assertEntityEmbedFilterHasRun($output);
+    $output = $this->applyFilter($content);

#88: yep, that's just a new comment and adding one assertion to be more explicit about what it's testing. That's the only case that cannot be converted precisely because it is not testing <drupal-entity> :)

If you liked #87, I think you'll like this too :)

We have a lot of parsing all over the place. So much of this:

    $dom = Html::load($output);
    $xpath = new \DOMXPath($dom);
    $this->assertSame(0, $xpath->query('//div[contains(@class, "embedded-entity")]/a')->length);

But \Drupal\KernelTests\AssertContentTrait is already used by KernelTestBase, so using cssSelect() is within reach!

This means assertHasAttributes() has to change slightly, because AssertContentTrait unfortunately uses SimpleXML for parsing, not DOMDocument. But I think that's okay. This again makes the tests simpler to understand.

Fixed a few nits, plus:

1. +++ b/tests/src/Kernel/EntityEmbedFilterTest.php
   @@ -0,0 +1,459 @@
   +        'not_contains' => [
   +          '<drupal-entity data-entity-type="node" data-entity',
   +        ],
   

   As of #81, this is handled automatically by applyFilter().

2. +++ b/tests/src/Kernel/EntityEmbedFilterTest.php
   @@ -0,0 +1,459 @@
   +    $this->assertNotContains('Deleted content encountered, site owner alerted', $output);
   

   Rather than asserting the absence of this content, it's better to assert the absence of the placeholder element, because further in the test case we assert the presence of that placeholder element (after deleting the embedded entity).

#87 updated almost everything, with the exception of EntityEmbedFilterTranslationTest::testFilterTranslations() Let's update that too.

So #92 did quite a bit of this:

+++ b/tests/src/Kernel/EntityEmbedFilterTranslationTest.php
@@ -47,12 +47,10 @@ class EntityEmbedFilterTranslationTest extends EntityEmbedFilterTestBase {
-    $this->assertContains($expected_title, $output);
...
+    $this->assertRaw($expected_title);

That pattern of swapping assertContains() for assertRaw() can be done in more places. And when that's done, applyFilter() no longer needs to return the resulting string.

This does that.

#94: That's from \Drupal\KernelTests\AssertContentTrait.

Following up on #92: #27 introduced $assert_title_from_source, $assert_title_lang, $assert_extra and friends, and I think @Wannes DR perhaps thought it's more complicated than it is — this can be simplified a lot :)

I've also finished comparing the test coverage with this patch applied compared to HEAD's test coverage. Nothing is missing except for the implicit integration test coverage with the @Filter=filter_align and @Filter=filter_caption filters. Hurray! 🥳

Thanks to both @oknate and @Wannes DR, I was able to approach this was a fresher perspective than I otherwise would have. Both of your suggestions, ideas and contributions are still present and will continue to live on. Hopefully you've both appreciated my taking the time to post piece-by-piece improvements. I am still all ears to hear suggestions from you too 🙂

I will continue working on those in a few hours.

#97: That's wonderful to hear :) Made my day — thanks! 🥳🤓

One of the crucial things that are still missing (and are also missing from HEAD) is asserting the presence of the expected bubbleable metadata. The additional assertions are trivial, but the code to properly run filters and collect bubbleable metadata (cacheability & attachments) is not.

What I didn't tell you in #99 is that this also happens to be the infrastructure I need to be able to add caption filter integration tests 🤫😇

This test is pretty rough, with lots of copy/pasted bits, but they'll soon evaporate! 🌞

+++ b/tests/src/Kernel/EntityEmbedFilterTest.php
@@ -0,0 +1,496 @@
+    $result = $this->applyFilter($content);
...
+    $this->applyFilter($content);
...
+    $filter_result = $this->filter->process($content, 'en');
+    $output = $filter_result->getProcessedText();
+    $this->setRawContent($output);

Three different calls, because three different needs. That's pretty awkward.

Fortunately #99 gave us the infrastructure to make this better 🥳

+++ b/tests/src/Kernel/EntityEmbedFilterTestBase.php
@@ -98,14 +84,10 @@ abstract class EntityEmbedFilterTestBase extends KernelTestBase {
-      'uuid' => 'e7a3e1fe-b69b-417e-8ee4-c80cb7640e63',
+      'uuid' => static::EMBEDDED_ENTITY_UUID,

This hunk shouldn't have been in there, that's for a future iteration…

Part 1 of #102's promised evaporation delivered.

1. Polish caption filter integration test coverage.
2. Remove $this->renderer which is pointless as of a few patches ago.

Part 2 of #102's promised evaporation delivered.

Now we have integration test coverage for filter_align, filter_caption and both simultaneously. 👍

data-entity-id is deprecated, but the tests would make you think otherwise. This is a problem for future maintenance.

(Part of this already snuck in #105 and hence the failure.)

It's very late. Time to wrap up my day. I'll continue tomorrow and expect to be able to commit this tomorrow afternoon my time.

(There is still some test refactoring that I think needs to happen. But the end is near!)

Added missing tests I'm aware of to the issue summary.

#42:

Great!

I can think of some edge cases to add after the last round of bug fixes.

• caption
• caption with link
• align left
• align center
• align right
• It seems there are some legacy properties, such as data-view-mode and data-entity-id. It would be great if that were documented, as far as named test cases marked as legacy support.
• The data-entity-embed-display-settings property doesn't seem to apply to view mode derived embeds. It would be good to get some clarity as to which supercedes which. I haven't tested, but I would assume that if you're using a view mode derived formatter, there are basically no settings, and therefore, no settings added there should take affect.

Perhaps it's redundant, but we could also test the alt and title overrides here.

• ✅
• Irrelevant here; that's a concern of the caption filter.
• See next.
• ✅ — testing with either of the three is sufficient, everything else is a concern of the align filter.
• ✅ on data-entity-id, I actually just did that 😀Finally, data-view-mode definitely is not deprecated. Why did you think that?
• Hm, can you clarify this?

If anybody wants to work on this in the mean time, feel free to. I'll continue in ~9 hours.