Strip HTML tags when using assertEquals() to compare markup

Please find the below patch with the required changes.

I think let's wait for https://www.drupal.org/project/drupal/issues/3131281 to be fixed then we should remove t() from assertEquals

@meena.bisht Thanks for the patch.
- Updated the patch with fixes for third argument in assertEqual().
@naresh_bavaskar
- As mentioned by Naresh, have removed just the t() from assertEqual() and other patch would update it to assertEquals()
If needed will re roll the patch once https://www.drupal.org/project/drupal/issues/3131281 is marked as fixed and commited.

Fixed failing tests and missed removal of t() in assertTrue()

1. +++ b/core/modules/system/tests/src/Functional/Form/ElementsLabelsTest.php
   @@ -91,9 +91,9 @@ public function testFormLabels() {
   +    $this->assertEqual($elements[0]->getAttribute('title'), 'Checkboxes test' . ' (Required)', 'Title attribute found.');
   ...
   +    $this->assertEqual($elements[0]->getAttribute('title'), 'Radios test' . ' (Required)', 'Title attribute found.');
   

   The two strings here can be joined together rather than using the concatenation operator.

2. +++ b/core/modules/user/tests/src/Kernel/UserValidationTest.php
   @@ -125,21 +125,21 @@ public function testValidation() {
   +    $this->assertEqual(strip_tags($violations[0]->getMessage()), "{$user->getFieldDefinition('mail')->getLabel()} field is required.");
   

   Why are we using string interpolation here when in other places we use concatenation?

Here I have tried to address comment #13.

@longwave,

1. Fixed #13.1
2. Used string interpolation for all replacements

Fixed failing test in #15

Needs reroll, and a few places seem to have been missed:

core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php
core/modules/views/tests/src/Functional/Plugin/DisplayTest.php
core/modules/file/tests/src/Kernel/FileManagedUnitTestBase.php
core/modules/file/tests/src/Functional/FileManagedTestBase.php

Thanks for the review @longwave
Rerolled #17 and fixed missed files core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php
core/modules/views/tests/src/Functional/Plugin/DisplayTest.php
core/modules/file/tests/src/Kernel/FileManagedUnitTestBase.php
core/modules/file/tests/src/Functional/FileManagedTestBase.php

Fixed failing test in #20

Fixed CS issues in #22

This looks almost there, I found two more cases:

core/modules/views/tests/src/Kernel/Handler/FilterCombineTest.php
242:    $this->assertEquals(t('Field %field set in %filter is not set in display %display.', ['%field' => 'dummy', '%filter' => 'Global: Combine fields filter', '%display' => 'Master']), reset($errors['default']));
277:    $this->assertEquals(t('%display: %filter can only be used on displays that use fields. Set the style or row format for that display to one using fields to use the combine field filter.', ['%filter' => 'Global: Combine fields filter', '%display' => 'Master']), reset($errors['default']));

addressed the #24 changes. Please review

Looking at the amount of calls to strip_tags() added here, wondering if we should either include the raw HTML in the expected string, or add a helper like assertTextEquals() which calls strip_tags() on the actual value?

If #27 is making this too complicated we could split this out into two issues, one for t() that don't use % arguments (as these cases should be simple) and then handle the ones that do use % arguments separately?

This badly needs a reroll now, assertEqual() is gone.

+1 on #30, let's do first t() with no args where t() is one of the two compared variables, plus I'd say all the t() that are in the $message - these can be simply removed sometimes. The rest we can address later.

+++ b/core/modules/aggregator/tests/src/Kernel/FeedValidationTest.php
@@ -55,13 +55,9 @@ public function testValidation() {
-    $this->assertEqual($violations[0]->getMessage(), t('A feed named %value already exists. Enter a unique title.', [
-      '%value' => $feed->label(),
-    ]));

BTW things like these could IMO just stay as-is, because $violations[] will be probably an array of t() objects built by the runtime code.

Opened #3226008: Remove simple uses of t() in assertEquals() calls for #31, postponing this on that.

#33 is in, this just leaves about 75 cases that mostly use t('%something') or are actual multilingual tests of t(). Maybe we just do nothing with these remaining ones?

Regex for searching; assertEquals.*\Wt\(

Found 66 instances remaining. 23 of them were testing the t() function so those are not adjusted.
The other ones are adjusted. I compared values with the raw html instead of the strip_tags apporach, since the last one loses some of the assert testing (does not spot changed tags offcourse).

I test the patch but patch not working in 9.3.x-dev
Patch needs to re-roll

MR needs to be updated to 10.1

For some reason couldn't create a branch off 10.1.x (says it doesn't exist)

So uploading a patch for 10.1

Changed some back as they were doing variable replacements.

Think it's a random layout builder javascript failure.

Sent it for a retest, this all looks OK to me now, assuming it comes back green this time. After sitting on it for a while I think checking the full explicit string is better than using strip_tags(), we are unlikely to ever change the placeholder HTML and if we consider it then the more test fails that it catches the better.

If at least one of the expected or the actual value are a MarkupInterface object, our MarkupInterfaceComparator kicks in to evaluate the assertEquals. I wonder whether we should strip tags in the comparator instead.

I think that's a dangerous assumption to make in the comparator, we probably want to explicitly test full markup in most cases? Tests can and should be specific about what they are testing, and I think adding "magic" in comparisons leads to difficulty later in understanding and maintaining the tests.

I see your point @longwave, but we already went half way with magic and assumptions introducing our own comparator that resolves MarkupInterface objects to strings.

It all goes back to the intent of each test assertion IMHO. If the intent is to check that two texts are equal, then we should not care of HTML tags that are added in. That was the intent covered by the introduction of the comparator, and it would make sense to me to enhance the comparator to strip tags before comparing. So that we have something like:

+++ b/core/modules/comment/tests/src/Kernel/CommentValidationTest.php
@@ -101,7 +101,7 @@ public function testValidation() {
-    $this->assertEquals(t('The name you used (%name) belongs to a registered user.', ['%name' => 'test']), $violations[0]->getMessage());
+    $this->assertEquals('The name you used (test) belongs to a registered user.', $violations[0]->getMessage());

Viceversa, if the intent is to verify the actual markup, we should be stricter: use assertSame() and cast explicitly to string the actual MarkupInterface object:

+++ b/core/modules/comment/tests/src/Kernel/CommentValidationTest.php
@@ -101,7 +101,7 @@ public function testValidation() {
-    $this->assertEquals(t('The name you used (%name) belongs to a registered user.', ['%name' => 'test']), $violations[0]->getMessage());
+    $this->assertSame('The name you used (<em class="placeholder">test</em>) belongs to a registered user.', (string) $violations[0]->getMessage());

Yeah, it comes down to what "equals" means when markup is involved, and that is not clearly defined either way. However I think that stripping tags in the comparator would be a BC break? Maybe worth a test patch to see how much is broken? Or, as suggested in #27, we could add something like assertTextEquals() to strip tags first?

Here's a test patch. Let's see.

The issue with this is that we might just be comparing two empty strings in some cases now, which invalidates the test? Should we add an extra check for this?

@longwave not sure I grasp #55, can you please elaborate?

Imaginary test case:

/** @var \Drupal\Component\Render\MarkupInterface $html */
$html = $some_object->outputHtml();
$this->assertEquals('<div class="test"><img src="test.jpg" /></div>', $html);

With strip_tags() in the comparator, this is now just comparing two empty strings, so if we later break outputHtml() in some way, the test will still pass as a false positive?

Again this is really just defining what "equals" means in terms of markup, but I don't think we can assume that all existing cases should consider only the text from the markup.

We could possibly try calling strip_tags() on $actual, only if $expected contains no tags? Or only if it is a MarkupInterface object? But that seems like even more "magic" behaviour.

So the problem would be when BOTH expected and actual values contain HTML tags and we may be expecting them to be significant in the comparison. Let's try this. Note that if we now compare with stripped values, assertEquals will test successfully when differences are in the markup tags only (different tags will no longer differentiate), so we leave to the deprecation to notify the need to tighten the test.

If BOTH expected and actual are MarkupInterface though, we should not flag it... no need to test MarkupInterface itself here. So this only applies between a string and a MarkupInterface object.

I tried to fix some of the test failures and, look, I start liking the concept a lot...

I am pausing now. Waiting for feedback.

I like it! This simplifies the more basic assertions but forces us to explicitly say in the test whether markup is significant or not in more complex cases. It would also be good to add extra cases to MarkupInterfaceComparatorTest to cover this change.

Will do. Thanks!

Added CR placeholder https://www.drupal.org/node/3334057

Added tests extra cases to MarkupInterfaceComparatorTest.

spell fixes

Changes look good.

Looks great - one nitpick that could go either way:

+++ b/core/modules/field/tests/src/Kernel/EntityReference/Views/SelectionTest.php
@@ -132,7 +133,10 @@ public function testSelectionHandler() {
+      $subresults_formatted = array_map(function (MarkupInterface $markup): string {
+        return (string) $markup;
+      }, $subresults);
+      $filtered_rendered_results_formatted += $subresults_formatted;

I think it's somewhat less verbose to just use strval()?

$filtered_rendered_results_formatted += array_map('strval', $subresults_formatted);

or if you want to be more strictly typed

$filtered_rendered_results_formatted += array_map(fn(MarkupInterface $markup) => (string) $markup, $subresults_formatted);

...so while this solves the issue of assertEquals() comparing markup, it no longer solves the actual issue of removing t() in assertEquals() - there are a whole bunch of cases left:

$ rg 'assertEquals.*\Wt\('|sort
core/modules/contact/tests/src/Functional/ContactSitewideTest.php:    $this->assertEquals(t('[@label] @subject', ['@label' => $label, '@subject' => $edit['subject[0][value]']]), $mail['subject']);
core/modules/field/tests/src/Kernel/FieldCrudTest.php:    $this->assertEquals(t('%name does not accept the value @value.', ['%name' => $field_name, '@value' => -2]), $violations[0]->getMessage());
core/modules/field/tests/src/Kernel/FieldCrudTest.php:    $this->assertEquals(t('This value should be between %min and %max.', ['%min' => 0, '%max' => 32]), $violations[0]->getMessage());
core/modules/field/tests/src/Kernel/FieldCrudTest.php:    $this->assertEquals(t('This value should be between %min and %max.', ['%min' => 0, '%max' => 32]), $violations[1]->getMessage());
core/modules/field/tests/src/Kernel/FieldValidationTest.php:    $this->assertEquals(t('%name: this field cannot hold more than @count values.', ['%name' => $this->fieldTestData->field->getLabel(), '@count' => $cardinality]), $violations[0]->getMessage());
core/modules/file/tests/src/Kernel/SaveTest.php:    $this->assertEquals(t('The file %value already exists. Enter a unique file URI.', ['%value' => $uppercase_file_duplicate->getFileUri()]), $violations[0]->getMessage());
core/modules/forum/tests/src/Kernel/ForumValidationTest.php:    $this->assertEquals(t('The item %forum is a forum container, not a forum. Select one of the forums below instead.', ['%forum' => $container->label()]), $violations[0]->getMessage());
...

So the title and IS don't seem to be describing what's actually being done in this issue.

The scope of the parent meta was intended to be removing unnecessary direct uses of t() and friends in test fixture data. However, this issue's primary scope seems to be that of converting assertEquals() calls to assertSame() (and therefore adding a string cast on passed-in markup objects, where t() is already not called directly in the assertions).

If the goal is to remove use of assertEquals() with translatable strings and markup objects, then the IS should be completely rewritten and the issue retitled.

Thanks for working on this!

With such a large number of changes would it make sense to breakup?

I don't think it can be broken up easily but we might want to spin off a child issue for the changes to MarkupInterfaceComparator and then come back here to fix the assertEquals() calls.

Addressed #75.

@smustgrave, actually this issue is about the perfect size for a mostly 1:1 replacement on a single line that requires some limited thinking about each change. Edit: There is relevant text in the issue scope policy section on patch side.

Large patches with many instances of the same simple change can still be easy to create and review, while even small patches with complex new code or documentation can take longer. Good conceptual scope is more important than the size alone.

FWIW re: #77, I chatted with @longwave about this. The current scope is fine if we update the title and IS, but we would need a followup for the original scope?

#80 xposted with #76, reset status to NW.

Opened #3337295: Remove remaining uses of t() in assertEquals() calls to handle the remainder, and retitled and updated the IS of this issue.

The change record still needs work.

Fleshed the CR.

Reviewed the change record and looks great. The examples were super helpful

RTBC +1, the change record is nice and clear to me.

edit: I didn't work on this directly so could actually commit it, but would like another core committer opinion on the changes first.

+1 from me.

Committed and pushed c187cc6339 to 10.1.x. Thanks!