Remove uses of FormattableMarkup in kernel test assertions

nikolay shapovalov → made their first commit to this issue’s fork.

I was not able to fix merge conflicts, and create new MR.
Used changes from Dave's MR.

Current status:

grep FormattableMarkup -r -l | grep Kernel

tests/Drupal/KernelTests/Core/Routing/ExceptionHandlingTest.php (single comment)
tests/Drupal/KernelTests/Core/Theme/TwigMarkupInterfaceTest.php (markup tests)
tests/Drupal/KernelTests/Core/Test/Comparator/MarkupInterfaceComparatorTest.php (markup tests)
tests/Drupal/KernelTests/AssertContentTrait.php (not sure to fix)
tests/Drupal/KernelTests/Component/Render/FormattableMarkupKernelTest.php (test FormattableMarkup)
modules/system/tests/src/Kernel/Theme/FunctionsTest.php (not sure to fix)
modules/ckeditor5/tests/src/Kernel/SmartDefaultSettingsTest.php (don't know how to fix)

Todo:
Decide if should be removed from

• tests/Drupal/KernelTests/AssertContentTrait.php
• modules/system/tests/src/Kernel/Theme/FunctionsTest.php

And fix, I don't have a clue what will be the best way, looking for your suggestions.

• modules/ckeditor5/tests/src/Kernel/SmartDefaultSettingsTest.php

smustgrave → closed merge request !1042

Tried comparing the original MR against 9.3.x, which has more changes. Checking the file changes missing it appears the FormattableMarkup() has already been done.

But also notice that a number of the tests are now using HTML:escape which didn't see in the original MR either?

Think the issue summary should be updated if the scope is being altered from the original.

Thanks a lot.
You are right, code base changed significantly.
I used changes made by @longwave, but also add some fixes from my side.
Scope was not changed because there was no direct tasks, I added grep command to get current list of Tests files that used FormattableMarkup.

Set status to NW, because I have no solution for SmartDefaultSettingsTest.php .

Since those tests don't appear to be testing translation or anything could the parts just be added to make a single string to compare?

Or we can just compare message string and variable array without convert it to string itself.

      $variables = unserialize($log->variables);
      $message = strtr($log->message, $variables);
      $db_logs[$type_to_status[$log->severity]][] = (string) $message;

Then we don't even need to unserialize variables.

I was able to remove FormattableMarkup from SmartDefaultSettingsTest.php.
IS updated. MR updated.
Does anyone have idea what should be done with these files:

• tests/Drupal/KernelTests/AssertContentTrait.php
• modules/system/tests/src/Kernel/Theme/FunctionsTest.php

I glanced at the MR and the size is well past that recommended in the Merge Request size → section of the scope guidelines. I suggest that this be split into smaller issues that are easier to review. Say, creating an MR for a group of changes that are implementing the same style of fix. I also noticed what could be inconsistent use of sprintf in the MR, that is, sometimes it is used and sometimes it is not. Can this be explained.

Thanks for your feedback.
I removed sprintf, and keep it only at core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php to keep code style consistent.
I create 2 new MRs and move changes from first MR.

1. MR 10434 - Changes in FileManagedUnitTestBase doesn't look obvious.
2. MR 10433 - Changes at data provider for SmartDefaultSettingsTest is huge.

Should I create new issue for each MR, or it's possible to keep several MRs at current issue?

With regards to #19 and (hopefully) to help answer #22. New tickets should be created. For each issue it should have 1 MR to review to make it super clear what's being fixed. Sometimes reviewers or committers may need the top level view of things and if multiple MRs it could be hard to review.

Let me know if that makes sense.

nikolay shapovalov → closed merge request !10433

nikolay shapovalov → closed merge request !10434

Create 2 new child issues

1. 📌 Remove FormattableMarkup from SmartDefaultSettingsTest Active
2. 📌 Remove FormattableMarkup from FileManagedUnitTestBase Active

Close 2 MRs. Update IS.

Thanks for breaking out, these looks like good replacements.

Left some comments on the MR, we can simplify the date formatter test bit more now - there's some dead code

Thanks for review. I simplified the date formatter test. And apllied one more suggestion.
Changes at 📌 Fix 'Drupal.Semantics.FunctionT.NotLiteralString' coding standard Active caused MR conflict, I fixed conflict.

Ready for one more review round.

Feedback appears to be addressed.

Step 3 of the remaining tasks has not been resolved yet. I didn't see discussion of those files in the comments, apologies if I missed that. Do they need a separate issue or can that be discussed here? I am not sure.

The title here should probably change to something like "Remove remaining uses of FormattableMarkup in kernel test assertions" to help indicate this is not all Kernel tests tests.

I also think it would be helpful to those who come after us to add a class comment in these files to explicitly state why FormattableMarkup is used in those tests. That can be a separate issue. If the text of the comment is supplied that would be a suitable novice issue.

• tests/Drupal/KernelTests/Core/Routing/ExceptionHandlingTest.php (single comment)
• tests/Drupal/KernelTests/Core/Theme/TwigMarkupInterfaceTest.php (markup tests)
• tests/Drupal/KernelTests/Core/Test/Comparator/MarkupInterfaceComparatorTest.php (markup tests)
• tests/Drupal/KernelTests/Component/Render/FormattableMarkupKernelTest.php (test FormattableMarkup)

Thanks for the feedback.
+1 for new title, so I update the title.
I believe we still need to decide, if listed tests still need FormattableMarkup, before we can proceed with MR review.
I already made my suggestion, because I create this list, and think we can keep it there. I am looking for someone to also check these files and provide their feedback.

I like the idea to add comments to files that will use FormattableMarkup, explaining the reason. This can be done at follow up.
But decision about keeping or removing should be done at this issue.

Not sure what status to set for now, leave it NW.

Set to NR to get some feedback.

I made changes to ConfigImporterMissingContentTest::log().

Appears to need rebase

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

Rebased.

Believe feedback has been addressed.

@smustgrave's feedback in #14 wasn't addressed afaict, it's not clear why Html::escape() is used in some of the assertions and that seems unnecessary.

@catch HTML::escape used only for variables with special characters.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased and grep currently shows

$ grep FormattableMarkup -r -l | grep Kernel
core/tests/Drupal/KernelTests/AssertContentTrait.php
core/tests/Drupal/KernelTests/Component/Render/FormattableMarkupKernelTest.php
core/tests/Drupal/KernelTests/Core/Test/Comparator/MarkupInterfaceComparatorTest.php
core/tests/Drupal/KernelTests/Core/Routing/ExceptionHandlingTest.php
core/tests/Drupal/KernelTests/Core/Theme/TwigMarkupInterfaceTest.php
core/modules/system/tests/src/Kernel/Theme/FunctionsTest.php

And modified to remove the files where FormattableMarkup is to remain;

$ grep FormattableMarkup -r -l | grep -v tests/Drupal/KernelTests/Core/Routing/ExceptionHandlingTest.php | grep -v tests/Drupal/KernelTe
sts/Core/Theme/TwigMarkupInterfaceTest.php | grep -v tests/Drupal/KernelTests/Core/Test/Comparator/MarkupInterfaceComparatorTest.php | grep -v tests/Drupal/KernelTests/Component/Render/FormattableMarkupKernelTest.ph | grep Kernel
core/tests/Drupal/KernelTests/AssertContentTrait.php
core/modules/system/tests/src/Kernel/Theme/FunctionsTest.php

For consideration, I removed the use of FormattableMarkup to build the message in AssertContentTrait.php. This is only done for methods that are not deprecated.

appears to have merge conflicts

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

Thanks for the bump.

Rebased with several conflicts in several files

Found one issue in the MR. Please fix.

Thanks, LGTM.

Committed aeccef9 and pushed to 11.x. Thanks!

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