Problem/Motivation

The \Drupal\Tests\node\FunctionalJavascript\TestSettingSummariesContentType class has a few problems:
1. It's wrongly named so the test is not actually run - this was noted over in #2296635-58: Evaluate performance impact of limiting test discovery to *Test.php filename suffix but that issue is trying to do way more.
2. It's slow because it has strpos('Not published', $summary) !== FALSE; in a wait an the arguments to strpos are the wrong way around...

Proposed resolution

Rename the test so it runs and fix the wait so that it is quick and will fail if it does not meet it's expectations.

Remaining tasks

None

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/a

CommentFileSizeAuthor
#2 3324901-2.patch1.54 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 3324901-2.patch1.54 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
+++ b/core/modules/node/tests/src/FunctionalJavascript/SettingSummariesContentTypeTest.php
@@ -45,10 +45,10 @@ public function testWorkflowSummary() {
+      return str_contains($summary, 'Not published');

We have symfony/s polyfil so doing this in Drupal 9 is fine.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Related issues: +#2849100: Spaces shown before commas in publishing options, +#2296635: Evaluate performance impact of limiting test discovery to *Test.php filename suffix

The test has always been named wrong since it was added in #2849100: Spaces shown before commas in publishing options

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Version: 9.5.x-dev » 10.1.x-dev
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

Ran patch #2 against 10.1.x and all green.

Changes look good and assuming this is mainly targeting 10.1 since "str_contains" is used.

  • xjm committed b962667c on 10.1.x 
    Issue #3324901 by alexpott: TestSettingSummariesContentType has a few...

  • xjm committed f22c7e1b on 10.0.x 
    Issue #3324901 by alexpott: TestSettingSummariesContentType has a few...
xjm’s picture

xjm
she/her
Primary language English
commented
Version: 10.1.x-dev » 9.5.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

I checked the verbose test output and confirmed that the test is indeed being discovered now:

13:19:59 Drupal\Tests\menu_ui\FunctionalJavascript\MenuUiJavascriptTe   1 passes                                      
13:20:01 Drupal\Tests\node\FunctionalJavascript\NodePreviewLinkTest     1 passes                                      
13:20:04 Drupal\Tests\node\FunctionalJavascript\SettingSummariesConte   1 passes                                      
13:20:05 Drupal\Tests\media_library\FunctionalJavascript\WidgetWithou   1 passes

Committed to 10.1.x and cherry-picked to 10.0.x. Thanks!

Re: 9.5.x, #3 suggests this is backportable due to D9's use of polyfills. I wasn't aware of that, so I've queued a PHP 7.3 test against 9.5.x just to be sure before backporting there.

xjm’s picture

xjm
she/her
Primary language English
commented

Fixing attribution.

  • xjm committed 55f58eae on 9.5.x 
    Issue #3324901 by alexpott: TestSettingSummariesContentType has a few...
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Patch (to be ported) » Fixed

Looks like the polyfill does indeed make this backportable as-is. Cherry-picked to 9.5.x. Thanks!

Status: Fixed » Closed (fixed)

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