comment_empty_title_test has invalid hook

+++ b/core/modules/comment/src/Tests/CommentTitleTest.php
@@ -34,8 +34,7 @@ public function testCommentEmptyTitles() {
-    // Tests that markup is not generated for the comment without header.

We've lost this comment.

Since this issue was created, the tests were refactored into core/modules/comment/tests/src/Functional/CommentTitleTest.php but the issue still remains.

Issue
Test regex checks that title is missing and passes but simpletest output shows the title is actually on the page
Reason
Hook is incorrectly implement

drupal-comment-empty-title-2886615-11-D8__test_fail.patch
Updates test to cause it to correctly fail without changing the invalid hook

drupal-comment-empty-title-2886615-11-D8.patch
Fixes the invalid hook

Back to needs review since second patch was meant to fail to prove test is now working (where it was previously a false positive)

Great find!

Nice find!

+++ b/core/modules/comment/src/Tests/CommentTitleTest.php
@@ -34,8 +34,7 @@ public function testCommentEmptyTitles() {
-    // Tests that markup is not generated for the comment without header.
-    $this->assertNoPattern('|<h3[^>]*></h3>|', 'Comment title H3 element not found when title is an empty string.');

I think this test is also trying to assert that no <h3></h3> is being adding to the page. Now we're not covering that case. I think we should still assert that there are no H3s empty on the page. So let's leave this assert in there. But we need the new assert to prove that the hook is working.

Makes sense, thanks for the review! I have updated the test to ensure there is no empty element. On my local I got the following error with the original code:
Remaining deprecation notices (1) 1x: assertNoPattern() is deprecated and scheduled for removal in Drupal 9.0.0. Use $this->assertSession()->responseNotMatches($pattern) instead. See https://www.drupal.org/node/2864262. 1x in CommentTitleTest::testCommentEmptyTitles from Drupal\Tests\comment\Functional
So I followed that recommendation and the check for no empty element uses the recommended responseNotMatches().

Thanks!

Local testing showed that the test is not doing what the comments say it is. There are two errors here, one is the name of the hook in the test module is wrong and therefor never executes and the other is that the regex for finding 'h3' tags wasn't actually finding them.

This patch adds a test to verify that the regex for finding 'h3' tags works. Then it installs the test module and runs the test as before, with the exception of using the new regex for finding 'h3'.

Looks good, we have the positive assertion that the regex is testing what we want and the positive assert that the comment is rendered after posing the second comment, so in that way it looks acceptable to end on the negative assertions.

Committed and pushed d7082acf60 to 10.0.x and dd103d843e to 9.4.x and aeb8420a93 to 9.3.x. Thanks!