Replace usages of AssertLegacyTrait::assertResponse(), which is deprecated

@mondrake,

I have reviewed the patch and found one change which might lead to patch re-roll in the future if the other issue (#3131900) gets resolved before this.

diff --git a/core/modules/views/tests/src/Functional/Plugin/DisplayPageWebTest.php b/core/modules/views/tests/src/Functional/Plugin/DisplayPageWebTest.php
index 87c6c0f6..3cdc9a21 100644
--- a/core/modules/views/tests/src/Functional/Plugin/DisplayPageWebTest.php
+++ b/core/modules/views/tests/src/Functional/Plugin/DisplayPageWebTest.php
@@ -169,10 +169,10 @@ public function assertPagePath($path) {
     $this->container->get('router.builder')->rebuild();
     // Check if we successfully changed the path.
     $this->drupalGet($path);
-    $success = $this->assertResponse(200);
+    $success = $this->assertSession()->statusCodeEquals(200);
     // Check if we don't get any error on the view edit page.
     $this->drupalGet('admin/structure/views/view/test_page_display_path');
-    return $success && $this->assertResponse(200);
+    return $success && $this->assertSession()->statusCodeEquals(200);
   }
 
 }

Since another issue (Patch: #3131900.15) is already RTBC'd. You should avoid making changes in the method assertPagePath.

@mondrake,

I notice this same piece of change in other patch as well. So just pointed it out. Agreeing to your point here we need a re-roll any ways.

I will review the patch #7 after completion of testing job.

Thanks!

All occurrences of $this->assertResponse(XXX); have been replaced by $this->assertSession()->statusCodeEquals(XXX);.
The method was already deprecated in 8.2.
Test has been added for deprecation testing.
The supression of the deprecation message has been removed.
The method Drupal\FunctionalTests\Bootstrap\UncaughtExceptionTest::assertResponse() has been given a proper docblock instead of the @inheritdoc it had.
All code changes look good to me.
For me it is RTBC.

Adding the "Deprecated assertions" tag to add this into the kanban board https://contribkanban.com/board/Deprecatedassertions

Thanks for this. I confirmed with git diff --color-words that, with one exception, the change is 1:1 replacements of the method call, and that the only grep occurrences of assertResponse( outside the legacy code are in UncaughtExceptionTest.

Even though UncaughtExceptionTest caused a problem here, we should have filed a separate issue to address that. Especially with a patch of this size, it should be an entirely scannable set of one exact pattern of change with git diff --color-words. See https://www.drupal.org/core/scope#context. I almost marked the issue NW for this.

I think it would have been better to file an issue for UncaughtExceptionTest first to actually rename its methods to avoid this confusion. As it is, let's do that as a followup.

Let's make backport patches that replace the method calls but do not un-silence the deprecation. Thank you!

@mondrake My point is mostly that the added docs should not be included in this issue. :) So I would have filed an issue when I noticed the test fail, something like:

Document UncaughtExceptionTest::assertResponse() and decide whether to rename it.

Edit: That doesn't affect the backport; backports are backports. Just a general practice that's increasingly important as we do more of this work.

On this.

Backported patch for 8.8.x branch.

Re-uploading patch caught one error while self-testing. Changes done in the wrong file.

Changed patch to fix failed tests.

SimpleTestBrowserTest extends WebTestBase which have its own assertResponse method.

Filed #3142906: simpletest WebTestBase overrides of AssertLegacyTrait methods and decide whether to rename them

Thanks @sja112. Given the size of this patch, I would recommend backporting it one branch at a time. The version here is just to show which branches allow the change. It does not mean creating a patch that only works for 8.8.x when it has only been committed to 9.1.x. You can test the patch against a different branch than the version selector by selecting the correct version next to the upload button when you submit the patch. Edit: Also, you should just try to apply the patch to the relevant branches locally before uploading. :)

NW to start with a working 9.0.x patch.

Oh and adding the reminder to the title that it's a backport.

Patch for D9.0 and D8.9

1. As per #17 need follow up issue has been created in #18 for uncaughtExceptionTest::assertResponse()
2. Backport patches created for D8.8, D8.9, and D9.0
3. Checked all three branches for any occurrence of $this->assertResponse() and all are replaced by $this->assertSession()->statusCodeEquals()
All code changes look good to me.
For me it is RTBC.

Thanks @sja112 and @ketikagrover. I reviewed locally with git diff --color-words (which was very hypnotic; I almost dozed off a couple times!) and also checked 9.0.x for other usages:

[ibnsina:maintainer | Thu 13:59:36] $ grep -r "assertResponse(" * | grep -v "~"
core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php:    $this->assertResponse(500);
core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php:    $this->assertResponse(500);
core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php:    $this->assertResponse(500);
core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php:    $this->assertResponse(418);
core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php:    $this->assertResponse(500);
core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php:    $this->assertResponse(418);
core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php:    $this->assertResponse(500);
core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php:    $this->assertResponse(500);
core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php:    $this->assertResponse(500);
core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php:    $this->assertResponse(500);
core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php:  protected function assertResponse($code) {
core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php:  protected function assertResponse($code) {

That's the same test again (with the followup) plus the legacy method.

I cherry-picked it to 8.9.x as well. 8.9.x has a number of other matches:

[ibnsina:maintainer | Thu 14:04:03] $ grep -rl "assertResponse(" * | grep -v "~"
core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php
core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
core/tests/Drupal/FunctionalTests/Installer/InstallerCustomConfigDirectoryCreateTest.php
core/tests/Drupal/FunctionalTests/Installer/InstallerExistingSettingsMismatchProfileTest.php
core/modules/file/tests/src/Functional/Update/FileUsageTemporaryDeletionConfigurationUpdateTest.php
core/modules/views_ui/src/Tests/UITestBase.php
core/modules/aggregator/src/Tests/AggregatorTestBase.php
core/modules/system/tests/src/Functional/Update/UpdatePathRC1TestBaseFilledTest.php
core/modules/system/tests/src/Functional/Render/AjaxPageStateTest.php
core/modules/system/src/Tests/Entity/EntityWithUriCacheTagsTestBase.php
core/modules/content_translation/src/Tests/ContentTranslationUITestBase.php
core/modules/rest/src/Tests/RESTTestBase.php
core/modules/block/tests/src/Functional/Update/BlockContextMappingUpdateTest.php
core/modules/simpletest/src/WebTestBase.php
core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php

At least some of those are SimpleTests which should not be changed, but not all of them. That said, the main goal of backporting this is to keep the branches in sync so that other cherry-picks are successful. Therefore, there's not too much value in fixing additional cases that are D8-only, so I pushed the 8.9.x cherry-pick to avoid anything going stale there.

I'll review 8.8.x. in a bit. Thanks!

#26 does not apply anymore, so NW for that to be rerolled.

Re-rolled patch for the D8.8 branch.

1. Patch re-rolled for D8.8 branch
2. One change I observed in this patch that some inline comments are added below is the example,

--- a/core/modules/basic_auth/tests/src/Functional/BasicAuthTest.php
+++ b/core/modules/basic_auth/tests/src/Functional/BasicAuthTest.php
@@ -47,30 +47,36 @@ public function testBasicAuth() {
+    // Ensure we can log in with valid authentication details.
     $this->basicAuthGet($url, $account->getAccountName(), $account->pass_raw);

After debugging I noticed that changes are already present D8.9 and above branches.
as we are keeping all the branches the same and its good to have the appropriate information with the code this change is valid in my view.

Patch looks good to me.
Thanks!

Thanks @ketikagrover for describing how you reviewed this; that made me realize something that was affecting the 8.8.x backport. Example:

+++ b/core/modules/comment/tests/src/Functional/CommentAdminTest.php
@@ -159,9 +159,9 @@ public function testApprovalNodeInterface() {
     // Approve comment.
     $this->drupalLogin($this->adminUser);
     $this->drupalGet('comment/1/approve');
-    $this->assertResponse(403, 'Forged comment approval was denied.');
+    $this->assertSession()->statusCodeEquals(403);
     $this->drupalGet('comment/1/approve', ['query' => ['token' => 'forged']]);
-    $this->assertResponse(403, 'Forged comment approval was denied.');
+    $this->assertSession()->statusCodeEquals(403);

This is not being fixed correctly as it was in #3132964: assertResponse() does not actually support a $message parameter, so stop passing one.

I think what we should do is work on a backport of that patch first, and get that committed to 8.8.x. Then, we can reroll @sja112's patch on top of that. It should mostly be a clean rebase since @sja112's fixes have followed the same patterns, but a few hunks might need to be fixed manually. Then, this patch will be easier to review and commit as well. Thanks!

Note that the final commit freeze for 8.8.x is on June 2 at 1200 UTC, so if it gets too close to that, there's a chance we might not be able to commit the backport. That said, the review is easier when all the comment changes are happening in the separate issue, so that should help.