Use assertText() instead of t()/SafeMarkup::format() in NodeEditFormTest [#2567851]

Problem/Motivation

Follow-up to #2506445-182: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests

From @xjm on #2506445-185: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests

+++ b/core/modules/node/src/Tests/NodeEditFormTest.php
@@ -77,8 +78,8 @@ public function testNodeEdit() {
     // Check that the title and body fields are displayed with the correct values.
-    $active = '<span class="visually-hidden">' . t('(active tab)') . '</span>';
-    $link_text = t('!local-task-title!active', array('!local-task-title' => t('Edit'), '!active' => $active));
+    $active = SafeMarkup::format('<span class="visually-hidden">@active_tab</span>', array('@active_tab' => t('(active tab)')));
+    $link_text = t('@local-task-title@active', array('@local-task-title' => t('Edit'), '@active' => $active));
     $this->assertText(strip_tags($link_text), 0, 'Edit tab found and marked active.');
     $this->assertFieldByName($title_key, $edit[$title_key], 'Title field displayed.');
     $this->assertFieldByName($body_key, $edit[$body_key], 'Body field displayed.');

Oh, I also agree with @justAChris that this is kinda a WTF. It'd be better to remove both the format() and the t(), and just do a plain simple assertText(). Let's split this one off into its own issue too.

Proposed resolution

Remaining tasks

User interface changes

API changes

None.

Comment	File	Size	Author
#9	interdiff.txt	843 bytes	dawehner
#9	2567851-9.patch	1.18 KB	dawehner
#9	8.0.x: PHP 5.5 & MySQL 5.5 13,818 pass
#6	2567851-6.patch	1.18 KB	dawehner
#6	8.0.x: PHP 5.5 & MySQL 5.5 13,817 pass, 1 fail

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

13 September 2015 at 22:10

joelpittet created an issue. See original summary.

Comment #2

joelpittet

English

Vancouver

CreditAttribution: joelpittet commented 13 September 2015 at 22:12

Issue summary:

View changes

Comment #3

xjm

she/her

English

CreditAttribution: xjm at Acquia commented 13 September 2015 at 22:57

Title:	Use assertText() instead of t()/SafeMarkup::format() on non-URLs in tests with !placeholder	» Use assertText() instead of t()/SafeMarkup::format() in NodeEditFormTest
Status:	Needs review	» Active

Narrowing the scope a bit.

Comment #4

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 14 September 2015 at 01:12

Assigned:

Unassigned

» larowlan

Comment #5

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 14 September 2015 at 01:15

Assigned:	larowlan	» Unassigned
Status:	Active	» Postponed

Postponed until #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests lands

Comment #6

dawehner

German

CreditAttribution: dawehner as a volunteer commented 14 September 2015 at 05:22

Status:

Postponed

» Needs review

File	Size
2567851-6.patch	1.18 KB
8.0.x: PHP 5.5 & MySQL 5.5 13,817 pass, 1 fail

I'm sorry, but the other patch does not touches this particular file at all.

We are certainly overpostponing from time to time.

Comment #7

14 September 2015 at 05:49

Status:

Needs review

» Needs work

The last submitted patch, 6: 2567851-6.patch, failed testing.

Comment #8

14 September 2015 at 06:02

The last submitted patch, 6: 2567851-6.patch, failed testing.

Comment #9

dawehner

German

CreditAttribution: dawehner as a volunteer commented 14 September 2015 at 06:34

Status:

Needs work

» Needs review

File	Size
2567851-9.patch	1.18 KB
8.0.x: PHP 5.5 & MySQL 5.5 13,818 pass
interdiff.txt	843 bytes

IMHO we want to test that the active tab is actually there, so we would need an assertRaw.

Comment #10

catch

he/him

English

CreditAttribution: catch commented 14 September 2015 at 08:46

Status:

Needs review

» Reviewed & tested by the community

Looks right to me.

Comment #11

14 September 2015 at 08:47

alexpott committed 6905340 on 8.0.x

Issue #2567851 by dawehner, joelpittet: Use assertText() instead of t()/...

Comment #12

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Chapter Three commented 14 September 2015 at 08:47

Status:

Reviewed & tested by the community

» Fixed

Committed 6905340 and pushed to 8.0.x. Thanks!

Comment #13

xjm

she/her

English

CreditAttribution: xjm at Acquia commented 14 September 2015 at 14:38

+++ b/core/modules/node/src/Tests/NodeEditFormTest.php
@@ -77,9 +77,10 @@ public function testNodeEdit() {
-    $active = '<span class="visually-hidden">' . t('(active tab)') . '</span>';
-    $link_text = t('!local-task-title!active', array('!local-task-title' => t('Edit'), '!active' => $active));
-    $this->assertText(strip_tags($link_text), 0, 'Edit tab found and marked active.');
+    // As you see the expected link text has no HTML, but we are using
+    $link_text = 'Edit<span class="visually-hidden">(active tab)</span>';
+    // @todo Ideally assertLink would support HTML, but it doesn't.
+    $this->assertRaw($link_text, 'Edit tab found and marked active.');

Why are we assigning $link_text to a variable at all? It's never used again.

Comment #14

cilefen CreditAttribution: cilefen commented 14 September 2015 at 15:34

#2568287: NodeEditFormTest::testNodeEdit() sets $link_text but uses it only once

Comment #15

cilefen CreditAttribution: cilefen commented 14 September 2015 at 19:26

+++ b/core/modules/node/src/Tests/NodeEditFormTest.php
@@ -77,9 +77,10 @@ public function testNodeEdit() {
+    // As you see the expected link text has no HTML, but we are using
+    $link_text = 'Edit<span class="visually-hidden">(active tab)</span>';

I find this comment as a segue in to a line of code odd (is that what it is?). It makes the followup issue of removing $link_text hard to do. Is comment the reason $link_text exists.