Problem/Motivation

The optional $ignoreCase parameter of assertContains() is deprecated and will be removed in PHPUnit 9.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

mondrake’s picture

Status: Active » Needs review
FileSize
2.6 KB

At least in one test, the parameter seems just redundant given the usage of strlower.

Status: Needs review » Needs work

The last submitted patch, 2: 3126848-2.patch, failed testing. View results

mondrake’s picture

Status: Needs work » Needs review
FileSize
4.55 KB
daffie’s picture

Status: Needs review » Reviewed & tested by the community

All test code changes look good to me.
By removing the the 2 lines from the ignored warnings that are thrown by PHPUnit 8, we have testing that all cases of the use of the parameter are removed.
For me it is RTBC.

longwave’s picture

+++ b/core/modules/page_cache/tests/src/Functional/PageCacheTest.php
@@ -258,7 +258,7 @@ public function testPageCache() {
+    $this->assertContains('cookie', explode(',', strtolower($this->drupalGetHeader('Vary'))), 'Vary header was sent.');

@@ -274,7 +274,7 @@ public function testPageCache() {
+    $this->assertStringNotContainsStringIgnoringCase('cookie', $this->drupalGetHeader('Vary'), 'Vary: Cookie header was not sent.');

These are functionally the same except one is inverted, so I think we should pick one way and use it consistently - using assertStringContainsStringIgnoringCase() in the first seems cleaner as there is an explicit method for it.

mondrake’s picture

#6 actually there is a subtle difference - we want to check positively there is a 'cookie' substring in the main string, but also that is bound by commas, or first or last part of the string. So we explode by ',' and check there is one token which equals 'cookie'.

On the negative check, that is irrelevant since it's enough no instance of the 'cookie' subtring exists.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs a reroll... and it's more than just core/tests/Drupal/Tests/Traits/PHPUnit8Warnings.php

mondrake’s picture

FileSize
4.62 KB

Rerolled. The assertStringContainsString patch missed to figure out these were to be case insensitive. Probably because
the string under test is just emtpy, anyway.

mondrake’s picture

Status: Needs work » Reviewed & tested by the community
mondrake’s picture

Needed another reroll.

longwave’s picture

Re #7 I agree there is a subtle difference but if the exact comma separated string "cookie" is the thing under test shouldn't it be tested the same in both cases? It is probably irrelevant for the test in question and also perhaps out of scope to change here though!

alexpott’s picture

Version: 9.1.x-dev » 9.0.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 125b33ab76 to 9.1.x and c58a76feaa to 9.0.x. Thanks!

  • alexpott committed 125b33a on 9.1.x
    Issue #3126848 by mondrake, longwave, daffie: Replace usage of the $...

  • alexpott committed c58a76f on 9.0.x
    Issue #3126848 by mondrake, longwave, daffie: Replace usage of the $...

Status: Fixed » Closed (fixed)

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