Remove all usage of FormattableMarkup in tests apart from explicit tests of that API

Just posting my progress, curious if I missed any placeholders.
This is still unfinished.

This should fix the test fails for now.
Still a work in progress, anyone feel free to work on this, I'll assign myself when i'm doing so.

So how would be convert this? From BookTest.php

+++ b/core/modules/book/src/Tests/BookTest.php
@@ -266,8 +265,9 @@ function checkBookNode(EntityInterface $node, $nodes, $previous = FALSE, $up = F
-      $text = "<b>‹</b> {$previous->label()}";
-      $this->assertRaw(\Drupal::l($text, $url), 'Previous page link found.');
+      $unrendered_text = ['#markup' => '<b>‹</b> ' . $previous->label()];
+      $rendered_text = \Drupal::service('renderer')->renderRoot($unrendered_text);
+      $this->assertRaw(\Drupal::l($rendered_text, $url), 'Previous page link found.');

@alexpott, i fixed that like this, does that seem acceptable?

Looks like it needs a re-roll.

Rerolled.

@@ -166,21 +166,21 @@ protected function doTestMultilingualProperties($entity_type) {
+    $this->assertEqual($default_langcode, $field->getLangcode(), "$entity_type: The field object has the expect langcode.");

s/expect/expected/g

Tempted to fix this here. Is it allowed or out of scope?

In scope imo. It's in a changed hunk so won't effect other patches more than what we are changing.

Just pointing out that there still are a lot of occurrences of SafeMarkup::format() and format_string() that need replacing. My patch in #7 was just a work in progress.

Since this is improving only tests. this can be done during RC. https://www.drupal.org/core/d8-allowed-changes#rc

Fixed errors, fixed typos mentioned in #13, and removed a few more usages.

Fix some mistakes that I added in #18.

1.) Some of these strings are displayed in the browser when viewing test results. Why wouldn't those messages need to use SafeMarkup? Are they getting escaped later? I think they are auto-escpaped, but just making sure.
2.) Maybe we can split this issue up since there are over 100 instances.

Thank you very mush @lokapujya!

To anwser your questions I think that variables in tests can be considered as safe as they are never taken from user inputs. Even if one of these variables were not safe, tests are not likely run from the UI on a production server so there would be no impact to an XSS injection in the tests results.

About the size of this patch I think that core maintainers would like to have some smaller changes to review but I don't know how they'd like them to be splitted. One should ask them what they prefer.

Finally, I have a remark about some changes introduced by your patch. For example in:

+++ b/core/modules/aggregator/src/Tests/AggregatorRenderingTest.php
@@ -66,7 +64,7 @@ public function testBlockLinks() {
-    $this->assert(isset($links[0]), format_string('Link to href %href found.', array('%href' => $href)));
+    $this->assert(isset($links[0]), "Link to href $href found.");

The fact is that %href get transformed in something like <em>$href</em>. So, your patch make us loose something. As <em>something</em> is painful while reading tests results in command line, I'd suggest to add double quotes around the variable to keep the value separated from the other part of the message:

"Link to href \"$href\" found."

Thanks for reviewing!

The fact is that %href get transformed in something like $href. So, your patch make us loose something.

Good point. Maybe the <em> is important for messages?

As <em>something</em> is painful while reading tests results in command line

I don't know if we should worry about this part. One could argue that is command line's problem. Command line could eventually support interpreting <em> tag.

Why don't we add a trait onto WebTestbase for a ::format() method that essentially does the same thing, but w/o the safe string handling? That should allow for nearly one-to-one replacement in tests?

@mpdonadio I like your idea. Ready to provide a patch? :)

Sure. Probably not tomorrow, but I'll have something ready on Monday or Tuesday.

So the current patch is changing both assertions themselves, and assertion messages. These are two separate scopes actually and I think we should split them into two separate issues. The former (removing it from non-messages) is important to make sure our tests are testing the right thing. The latter (removing format_string() from assertion messages) does not affect the integrity of the test and is a separate discussion. At first @alexpott had suggested doing so, but then said once "actually SimpleTest result display sanitization works differently than I thought" and so said it would need to be considered more carefully. There may be a different issue for that somewhere.

Also, I'm not actually in favor of adding additional assertion methods for the string formatting.

Finally, we should actually consider first off which of these assertion messages are unneeded. Often with an assertText() or assertRaw() the custom assertion message is not actually adding any additional information.

So my suggestion would be:

1. One patch (this issue) that removes any format_string() and friends that are not on the message parameter.
2. Take what's left over, and look at what messages are simply redundant that also use format_string(), and just remove the parameter so only the default message is used.
3. Take what's left over from this patch after that, and then look into whether the message needs to be sanitized or not for output.

reroll + beginning #27-1. So, this only includes what we already discovered in the previous patch. Many more may have been added.

I think I've identified all of them. Left some as @todos. Do we still want to do this? If so, then if anyone wants to finish it off, go ahead.

+++ b/core/modules/contact/src/Tests/ContactSitewideTest.php
@@ -130,7 +130,7 @@ function testSiteWideContact() {
-    $this->assertText(format_string('Machine-readable name cannot be longer than @max characters but is currently @exceeded characters long.', array('@max' => $max_length, '@exceeded' => $max_length_exceeded)));
+    $this->assertText('Machine-readable name cannot be longer than ' . $max_length . ' characters but is currently @' . $max_length_exceeded . ' characters long.');

remove the @

Just some quick feedback.

+++ b/core/modules/book/src/Tests/BookTest.php
@@ -271,8 +271,9 @@ function checkBookNode(EntityInterface $node, $nodes, $previous = FALSE, $up = F
+      $unrendered_text = ['#markup' => '<b>‹</b> ' . $previous->label()];
+      $rendered_text = \Drupal::service('renderer')->renderRoot($unrendered_text);

a renderPlain would be enough here. In general it feels weird though that we require to do that, but yeah this more or less how its expected to work.

I feel responsible for fixing the errors.

What would be the ideal branch to commit this?

Rerolled to 8.2.x.

@xjm in #27 might disagree, but we could preserve the effort here by reviewing and committing, add a follow-up to split things out for assertion strings vs. message strings, and maybe even scope for format_string() only, and one for SafeMarkup::format().

After this patch, NetBeans says: Found 581 matches of format_string in 158 files and Found 179 matches of SafeMarkup::format in 63 files. That's searching through files that end in *Test.php.

Custom test seemed to pass but it's stuck. @Mile23 mind uploading that again?

I see this in the jenkins log:

23:52:14 Completed publish:gather_artifacts
23:52:14 Executing publish:junit_xmlformat
23:52:16 Reformatted test results written to /var/lib/drupalci/web/jenkins-default-179672/artifacts/xml/testresults.xml
23:52:16 Completed publish:junit_xmlformat
23:52:16 Completed publish
23:52:17 Checking console output
23:52:17 Recording test results
23:52:18 Finished: SUCCESS

But I'll re-up anyway.

Ewps.

Patch still applies to 8.3.x. Re-running tests.

The patch in @28 basically filters the changes down to what was suggested in the #27.1 comment. So patch #20 has a lot more conversions in it.

Reroll of #43.

There are still a lot more usages. We might finish up here with the remainder of either SafeMarkup or format_string, and then do a follow-up for the other one. For the sake of reviewability.

format_string() in *Test.php: 156 files.

SafeMarkup::format() in *Test.php: 63 files.

Let's finish out SafeMarkup::format() here when this patch passes.

It could be closed, no more usage found in core

@andypost Well, no, the original scope wasn't only about removing deprecated API use; it was the followup to getting rid of t() in tests. The equivalent of the safe string list is still being polluted by whatever's left in tests.

Rerolled patch for 9.1.x and removed FormattableMarkup and t() from tests, please review.

Fixed PHPLint issues in #66, please review.

Working on failed tests

1. Ran grep -r "FormattableMarkup" * and could find FormattableMarkup still in many places.
2. Also found t() being used in many asserts.

We have a separate meta for t() as there are thousands of these to replace. This issue should be for FormattableMarkup only.

@longwave in #63, it was requested for.

Moved to back to needs work and working on failed tests.

Fixed failing tests in #70. Also removes t() from many places.
I second #74. It would be better to have a separate issue from removal of t().

That issue already exists: #3133726: [meta] Remove usage of t() in tests not testing translation

There are 933 instances of "new FormattableMarkup" in tests, is this too much to fix in one issue? As with the t() issue, should we convert this to a meta and try and break this down into smaller patches that are easier to write and review?

@longwave, I think it would be better not to pollute the scope of this issue. We can group all issues for removal of t() under one meta and then address it from there. Also want to know @xjm's view on this as she mentioned that the scope also includes removal of t() in #63.

Moving to Needs review and expecting comments form @longwave and @xjm

We are almost done with removing t() in tests, there are two child issues of #3133726: [meta] Remove usage of t() in tests not testing translation open and then we should be done.

There are roughly just under 1000 references to FormattableMarkup in tests:

$ rg FormattableMarkup|grep -i test|wc -l
995

Some of those will be use statements and comments, or tests of FormattableMarkup itself. If we cut this down into just assertions there are about 600:

$ rg assert.*Formattable|wc -l
599

If we then break those down into kernel and functional tests, those seem manageable amounts to do in separate child issues:

$ rg assert.*Formattable|grep Kernel|wc -l
359

$ rg assert.*Formattable|grep Functional|wc -l
235

Does that seem OK as a rescoping of this problem? We can then clean up any final leftovers in this issue.

Based on #84, it looks like this needs work?