Convert \Drupal\comment\Tests\CommentNewIndicatorTest to JavascriptTestBase and/or BrowserTestBase

Updated the issue summary as I think you copied the wrong issue number. You meant that this is a follow-up from #2866513: Convert web tests to browser tests for comment module not #2809467: Convert \Drupal\comment\Tests\CommentPagerTest to BrowserTestBase

First of all, I've exactly the same errors like it was discussed here: #2795049: Convert web tests to browser tests for history module.

The request gets 403, it is happening because the Guzzle request is from an unauthenticated (anon) user.

As a solution, I used the same code for getHttpClient. I'm not sure if there is a better solution for now. Any ideas?

1. +++ b/core/modules/comment/tests/src/Functional/CommentNewIndicatorTest.php
   @@ -127,8 +120,8 @@ public function testCommentNewCommentsIndicator() {
   +    $this->assertEquals(200, $response->getStatusCode());
   
   @@ -138,15 +131,30 @@ public function testCommentNewCommentsIndicator() {
   +    $this->assertEquals(404, $response->getStatusCode());
   ...
   +    $this->assertEquals(403, $response->getStatusCode());
   ...
   +    $this->assertEquals(403, $response->getStatusCode());
   

   Let's use assertSame here

2. +++ b/core/modules/comment/tests/src/Functional/CommentNewIndicatorTest.php
   @@ -138,15 +131,30 @@ public function testCommentNewCommentsIndicator() {
   +   *   The client with BrowserTestBase configuration.
   +   */
   +  protected function getHttpClient() {
   +    // Similar code is also employed to test CSRF tokens.
   +    // @see \Drupal\Tests\system\Functional\CsrfRequestHeaderTest::testRouteAccess()
   +    $domain = parse_url($this->getUrl(), PHP_URL_HOST);
   +    $session_id = $this->getSession()->getCookie($this->getSessionName());
   +    $this->cookies = CookieJar::fromArray([$this->getSessionName() => $session_id], $domain);
   +    return $this->getSession()->getDriver()->getClient()->getClient();
   

   Do we want to wait on #2911915: Add getHttpClient() to BrowserTestBase?

Fixed #6.1, the second point is not something I think we should wait for.

Here is updated patch.

Changes:
1) Make right order of method getHttpClient
2) use BTB getHttpClient instead of $this->getSession()->getDriver()->getClient()->getClient()

I think that's all what can we do here.

Looks great, I don't think I can RTBC this as I've also supplied a patch.

+++ b/core/modules/comment/tests/src/Functional/CommentNewIndicatorTest.php
@@ -24,40 +25,47 @@ class CommentNewIndicatorTest extends CommentTestBase {
+  /**
+   * Obtain the HTTP client and set the cookies.
+   *
+   * {@inheritdoc}
+   */
+  protected function getHttpClient() {
+    // Similar code is also employed to test CSRF tokens.
+    // @see \Drupal\Tests\system\Functional\CsrfRequestHeaderTest::testRouteAccess()
+    $domain = parse_url($this->getUrl(), PHP_URL_HOST);
+    $session_id = $this->getSession()->getCookie($this->getSessionName());
+    $this->cookies = CookieJar::fromArray([$this->getSessionName() => $session_id], $domain);
+
+    return parent::getHttpClient();
+  }

Is there a reason we don't move this to the parent method? It feels sensible to inherit the cookies automatically

let's wait when #2983504: Add a way to easily set the cookies in a request done using the Guzzle client lands and update the patch again

Yay, it lands. Patch is much cleaner now.

+++ b/core/modules/comment/tests/src/Functional/CommentNewIndicatorTest.php
@@ -30,34 +30,21 @@ class CommentNewIndicatorTest extends CommentTestBase {
+    $url = Url::fromRoute('comment.new_comments_node_links')
+      ->setAbsolute()
+      ->toString();

Nitpick: You should be able to call $this->buildUrl, it should prepare the url into a string which can be used.

It is overall much nicer, I agree.

Nice tip. Thanks.

I've updated a patch.

This looks so much cleaner then the webtest version, very nice.

Committed and pushed f45132741a to 8.7.x and 99de45529d to 8.6.x. Thanks!