Problem/Motivation

$this->assertSame(FALSE, ... and $this->assertSame(TRUE, ... is no longer necessary and can be replace with assertTrue/False().

Also $this->assertSame(..., FALSE/TRUE).

See Deprecated assertTrue/False() compatibility overrides in PHPUnit Kernel, Functional and FunctionalJavascript tests

Proposed resolution

Replace assertSame(TRUE/FALSE,... with assertTrue/False().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Krzysztof Domański created an issue. See original summary.

Krzysztof Domański’s picture

Status: Active » Postponed

1. #2742585: Deprecate dangerous assertTrue/False() compatibility overrides in PHPUnit Kernel, Functional and FunctionalJavascript tests should be fixed first.

2. See also #3082287: \Drupal\user\Plugin\views\access\Role::access() does not conform to the base class documentation.

--- a/core/modules/user/tests/src/Functional/Views/AccessRoleTest.php
+++ b/core/modules/user/tests/src/Functional/Views/AccessRoleTest.php
@@ -50,8 +50,8 @@ public function testAccessRole() {
     $this->assertTrue($access_plugin instanceof Role, 'Make sure the right class got instantiated.');
 
     // Test the access() method on the access plugin.
-    $this->assertFalse($executable->display_handler->access($this->webUser));
-    $this->assertTrue($executable->display_handler->access($this->normalUser));
+    $this->assertSame(FALSE, $executable->display_handler->access($this->webUser));
+    $this->assertSame(TRUE, $executable->display_handler->access($this->normalUser));
Krzysztof Domański’s picture

Title: Replace assertSame(TRUE/FALSE,... with assertTrue/False in PHPUnit tests » Replace assertSame(TRUE/FALSE,... with assertTrue/False() in PHPUnit tests
Issue summary: View changes
Parent issue: » #2742585: Deprecate dangerous assertTrue/False() compatibility overrides in PHPUnit Kernel, Functional and FunctionalJavascript tests
Related issues: +#3082287: \Drupal\user\Plugin\views\access\Role::access() does not conform to the base class documentation
Krzysztof Domański’s picture

mondrake’s picture

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

mondrake’s picture

Status: Postponed » Needs review

Parent committed, unpostponing.

mondrake’s picture

There are also assertEquals(true|false... and assertEqual(true|false... calls that probably need love, not sure if here or in a separate issue.

mondrake’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
Krzysztof Domański’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
45.23 KB
2.58 KB

1. Re-rolled and fixed additional changes added in the meantime (see #2).

2. I agree that we can also change assertEqual(s) (see #8) but we need a separate issue.

Replacing assertEqual with assertFalse/True is not the same as replacing assertSame with assertFalse/True.

assertEqual -> $value == FALSE
assertSame -> $value === FALSE
assertFalse -> $value === FALSE

mondrake’s picture

Status: Needs review » Needs work

Couple nits:

1.

+++ b/core/modules/migrate/tests/src/Kernel/MigrateEmbeddedDataTest.php
@@ -48,7 +48,7 @@ public function testEmbeddedData() {
       // The plugin should not mark any rows as stubs. We need to use
       // assertSame() here because assertFalse() will pass falsy values (e.g.,
       // empty arrays).
-      $this->assertSame(FALSE, $row->isStub());
+      $this->assertFalse($row->isStub());
 
       $data_row = $row->getSource();

The comment here should be changed according to the change in the assert.

2. There are a couple of hacky use of assertSame remaining, in rest/test/src/Functional/ResourceTestBase.php and same in jsonapi/test/src/Functional/ResourceTestBase.php:

    // Expected cache tags: X-Drupal-Cache-Tags header.
    $this->assertSame($expected_cache_tags !== FALSE, $response->hasHeader('X-Drupal-Cache-Tags'));
    ...

    // Expected cache contexts: X-Drupal-Cache-Contexts header.
    $this->assertSame($expected_cache_contexts !== FALSE, $response->hasHeader('X-Drupal-Cache-Contexts'));
   ...

this could be changed to assertNotFalse

Krzysztof Domański’s picture

Status: Needs work » Needs review
FileSize
45.33 KB
730 bytes

11.1. Deleted redundant comment.

11.2. The $expected_cache_tag is a function parameter.

* @param string[]|false $expected_cache_tags
*   (optional) The expected cache tags in the X-Drupal-Cache-Tags response
*   header, or FALSE if that header should be absent. Defaults to FALSE.

I think we should not change anything here.

// Expected cache tags: X-Drupal-Cache-Tags header.
$this->assertSame($expected_cache_tags !== FALSE, $response->hasHeader('X-Drupal-Cache-Tags'));

The code above is a shorter form of this:

if ($expected_cache_tags !== FALSE) {
  $this->assertTrue($response->hasHeader('X-Drupal-Cache-Tags'));
}
else {
  $this->assertFalse($response->hasHeader('X-Drupal-Cache-Tags'));
}
mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for #12.2 - I'd rather go for your longer form then because I feel it's not good to combine conditional logic with assertions, but that's definitely not for here. RTBC for me.

alexpott’s picture

Version: 8.9.x-dev » 9.0.x-dev
Status: Reviewed & tested by the community » Needs work

I think we should only do this on Drupal 9 once we've removed the overrides. This is because if the test is marked @legacy then the deprecations won't matter and we might not be testing what we think we're testing.

Let's make this a Drupal 9 issue that removes the deprecations and overridden methods and then we're good.

mondrake’s picture

Title: Replace assertSame(TRUE/FALSE,... with assertTrue/False() in PHPUnit tests » Replace assert(Not)Same/Identical on booleans with assert(Not)True/False() in PHPUnit tests

It's not just assertSame, but actually assertSame, assertNotSame, assertIdentical, assertNotIdentical. Also, not nuking them, since something like assertSame(FALSE, strpos($x, $y)) should be replaced by assertStringNotContainsString for example - separate issues for that.

mondrake’s picture

Title: Replace assert(Not)Same/Identical on booleans with assert(Not)True/False() in PHPUnit tests » Replace assert(Not)Same/Identical() on booleans with assert(Not)True/False() in PHPUnit tests
Status: Needs work » Needs review
FileSize
67.64 KB
110.02 KB

Rerolled and addressing #15.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

I applied the patch and can only find a couple of stragglers with grep - I guess these probably want to be assertNotNull() anyway, which is out of scope? If so these can be done elsewhere and this is RTBC.

core/modules/field/tests/src/Kernel/Migrate/d6/MigrateFieldWidgetSettingsTest.php:    $this->assertIdentical(FALSE, is_null($form_display), "Form display node.story.default loaded with config.");
core/modules/node/tests/src/Kernel/Migrate/d6/MigrateViewModesTest.php:    $this->assertIdentical(FALSE, is_null($view_mode), 'Preview view mode loaded.');
mondrake’s picture

#17 exactly, there are cases where more appropriate assertions can be used.

catch’s picture

Could we open a follow-up for #17?

jungle’s picture

  • catch committed 5132c83 on 9.1.x
    Issue #3082415 by Krzysztof Domański, mondrake, alexpott, longwave,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5132c83 and pushed to 9.1.x, cherry-picked to 9.0.x. Thanks!

  • catch committed 84f4007 on 9.0.x
    Issue #3082415 by Krzysztof Domański, mondrake, alexpott, longwave,...

Status: Fixed » Closed (fixed)

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