SummaryLengthTest just tests the character count for rendering one node teaser, so instead of logging in a user, rendering the whole node teaser listing, etc., the test can just render the teaser with the API to check for the correct length. This makes things easier for #1806334: Replace the node listing at /node with a view and should also reduce the overhead of running SummaryLengthTest slightly.

CommentFileSizeAuthor
#11 node-1847554-11.patch2.28 KBxjm
#11 interdiff.txt1.29 KBxjm
#3 node-1847554-3.patch2.05 KBxjm
#3 interdiff.txt1.89 KBxjm
#1 interdiff.txt1.28 KBdawehner
#1 drupal-1847554-1.patch1.67 KBdawehner
summary-test.patch1.64 KBxjm

Comments

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new drupal-1847554-1.patch1.67 KB
new interdiff.txt1.28 KB

The patch is looking great in general.

+++ b/core/modules/node/lib/Drupal/node/Tests/SummaryLengthTest.phpundefined
@@ -46,8 +47,12 @@ function testSummaryLength() {
+    // Attempt to access the front page again and check if the summary is now
+    // only 200 characters in length.

Adapted the comment, as we don't render it directly anymore.

+++ b/core/modules/node/lib/Drupal/node/Tests/SummaryLengthTest.phpundefined
@@ -46,8 +47,12 @@ function testSummaryLength() {
+    $this->drupalSetContent(drupal_render($content));

That's a great idea!

xjm’s picture

xjm
she/her
Primary language English
commented

Thanks @dawehner! (This is what happens when I roll patches after midnight!)

xjm’s picture

xjm
she/her
Primary language English
commented
StatusFileSize
new interdiff.txt1.89 KB
new node-1847554-3.patch2.05 KB

The comments and assertion messages were also a bit misleading, so attached fixes that.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Oh this much better explains what's going on.

+++ b/core/modules/node/lib/Drupal/node/Tests/SummaryLengthTest.phpundefined
@@ -33,21 +33,28 @@ function testSummaryLength() {
+    // Render the node as a teaser again and check that the summary is now only
+    // 200 characters in length and so does not include 'What is a Drupalism?'.

Couldn't we check that the actual length is not longer then 200 chars as well?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, node-1847554-3.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs work » Needs review

Couldn't we check that the actual length is not longer then 200 chars as well?

Ah yeah, might as well since we already have the render array.

xjm’s picture

xjm
she/her
Primary language English
commented

Oh, but actually, it will not be exactly 200 or 600 characters... just less than those, based on the trim algorithm. So I think this is okay as is. Ish.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

The test failure is a bot issue; it's in the process of retesting already.

lars toomre’s picture

lars toomre commented

Is there a test to check that the teaser is not longer than X characters? Presumably there is and that would catch that the teaser is no more than 200 characters. If such a check of functionality does not exist, it should be added in the correct place.

xjm’s picture

xjm
she/her
Primary language English
commented

@Lars Toomre, did you read the patch or the test it modifies? That is what the test does implicitly, because the specific string that it's checking for is more than 200 characters into the summary.

Anyway, this patch is not intended to add test coverage for this feature, just to decouple the test from the node frontpage.

xjm’s picture

xjm
she/her
Primary language English
commented
StatusFileSize
new interdiff.txt1.29 KB
new node-1847554-11.patch2.28 KB

This might be a bit less fragile.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, node-1847554-11.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs work » Reviewed & tested by the community

Bot issue.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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