Cast safe string objects to string or use assertEqual() in some assertIdentical() comparisons

@stefan.r there may be a chance here that we are making a test less reliable to detect changes. I usually prefer identical if I can get away with it.

It would be nice to see how many of these are still failing and remove the unnecessary changes, what do you think?

1. +++ b/core/modules/breakpoint/src/Tests/BreakpointDiscoveryTest.php
   @@ -193,6 +193,7 @@ public function testBreakpointGroups() {
   +    $this->castSafeStrings($expected, $breakpoint_groups);
   

   Is this still needed?

2. +++ b/core/modules/system/src/Tests/Common/FormatDateTest.php
   @@ -87,12 +87,12 @@ function testAdminDefinedFormatDate() {
   -    $this->assertIdentical(format_date($timestamp, 'custom', 'l, d-M-y H:i:s T', 'America/Los_Angeles', 'en'), 'Sunday, 25-Mar-07 17:00:00 PDT', 'Test all parameters.');
   ...
   +    $this->assertEqual(format_date($timestamp, 'custom', 'l, d-M-y H:i:s T', 'America/Los_Angeles', 'en'), 'Sunday, 25-Mar-07 17:00:00 PDT', 'Test all parameters.');
   

   Like all the format_date ones shouldn't need this right?

+++ b/core/modules/ckeditor/src/Tests/CKEditorAdminTest.php
@@ -81,6 +81,7 @@ function testExistingFormat() {
+    // Begin expected settings array:

@@ -111,7 +112,8 @@ function testExistingFormat() {
+    // ^ End expected settings array.

We never do this, and it's really rather pointless, because that's already what the variable name indicates?

The changes in CKEditor look fine — I manually checked: https://3v4l.org/ei98N. I dislike that we're making tests less strict though, but if it's done for The Better Good overall, then +1.

Its sad that we need that, but hey, at least it adds some semanticness into our tests, which makes it clearer what is going on!
+1 for introducing a helper assertion.

+++ b/core/modules/simpletest/src/TestBase.php
--- a/core/modules/system/src/Tests/Common/FormatDateTest.php
+++ b/core/modules/system/src/Tests/Common/FormatDateTest.php

+++ b/core/modules/system/src/Tests/Common/FormatDateTest.php
@@ -88,10 +88,10 @@ function testAdminDefinedFormatDate() {
-    $this->assertIdentical(format_date($timestamp, 'custom', 'l, d-M-y H:i:s T', 'America/Los_Angeles', self::LANGCODE), 'domingo, 25-Mar-07 17:00:00 PDT', 'Test translated format.');
...
+    $this->assertIdenticalWithSafeStrings(format_date($timestamp, 'custom', 'l, d-M-y H:i:s T', 'America/Los_Angeles', self::LANGCODE), 'domingo, 25-Mar-07 17:00:00 PDT', 'Test translated format.');

My test results https://www.drupal.org/pift-ci-job/32267
Show this is not needed.

This removes some unnecessary changes. Here's the whole patch green #2465399-82: IGNORE: Patch testing issue

• +++ b/core/modules/views/src/Tests/ModuleTest.php
  @@ -157,40 +157,40 @@ public function testLoadFunctions() {
  -    $this->assertIdentical(array_keys($all_views), array_keys(Views::getAllViews()), 'Views::getAllViews works as expected.');
  +    $this->assertIdenticalWithSafeStrings(array_keys($all_views), array_keys(Views::getAllViews()), 'Views::getAllViews works as expected.');
  ...
  -    $this->assertIdentical(array_keys($expected_enabled), array_keys(Views::getEnabledViews()), 'Expected enabled views returned.');
  +    $this->assertIdenticalWithSafeStrings(array_keys($expected_enabled), array_keys(Views::getEnabledViews()), 'Expected enabled views returned.');
  ...
  -    $this->assertIdentical(array_keys($expected_disabled), array_keys(Views::getDisabledViews()), 'Expected disabled views returned.');
  +    $this->assertIdenticalWithSafeStrings(array_keys($expected_disabled), array_keys(Views::getDisabledViews()), 'Expected disabled views returned.');
  ...
  -    $this->assertIdentical(array_keys($all_views), array_keys(Views::getViewsAsOptions(TRUE)), 'Expected option keys for all views were returned.');
  +    $this->assertIdenticalWithSafeStrings(array_keys($all_views), array_keys(Views::getViewsAsOptions(TRUE)), 'Expected option keys for all views were returned.');
  

  Why is these necessary? Array key can never be an object..

• +++ b/core/modules/simpletest/src/TestBase.php
  @@ -750,6 +750,38 @@ protected function assertIdentical($first, $second, $message = '', $group = 'Oth
  +   * @return
  +   *   TRUE if the assertion succeeded, FALSE otherwise.
  +   *
  

  This should be @return bool

Fixes #24 thanks @lauriii

This is good for me. I tested all the changed tests locally with the fix given in #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list and a little custom logic so that it would fail if assertIdenticalWithSafeStrings is called for no reason.

1. +++ b/core/modules/simpletest/src/TestBase.php
   @@ -654,7 +654,7 @@ protected function assertNotNull($value, $message = '', $group = 'Other') {
   -   * @return
   +   * @return bool
   
   @@ -750,6 +750,38 @@ protected function assertIdentical($first, $second, $message = '', $group = 'Oth
   +   * @return
   

   I guess bool was added to wrong place. Can be fixed on commit

2. +++ b/core/modules/locale/src/Tests/LocaleImportFunctionalTest.php
   @@ -238,8 +238,10 @@ public function testLanguageContext() {
   +    // We cast the return value of t() to string so as to retrieve the translated
   +    // value, rendered as a string.
   

   This is over 80chars. Can be also fixed on commit.

Whoops, fixed that too. Thanks @lauriii

Changes looks good so still RTBC :)

Unrelated test fail. Drupal\user\Tests\UserCancelTest has GET http://ec2-54-191-110-230.us-west-2.compute.amazonaws.com/checkout/user/... returned 0 (0 bytes).

I'm not sure about this. Especially adding a method calling assertIdenticalAnything that actually doesn't assert identical-ness.

Discussed quickly with @xjm who said "it is a hack for bad tests"

I'm trying to think of alternatives.

I just had a look at many of those examples ...

+++ b/core/modules/block_content/src/Tests/BlockContentListViewsTest.php
@@ -45,10 +45,10 @@ public function testListing() {
     $expected_items = [t('Block description'), t('Block type'), t('Updated'), t('Operations')];
     foreach ($elements as $key => $element) {
       if ($element->xpath('a')) {
-        $this->assertIdentical(trim((string) $element->xpath('a')[0]), $expected_items[$key]);
+        $this->assertIdenticalWithSafeStrings(trim((string) $element->xpath('a')[0]), $expected_items[$key]);
       }
       else {
-        $this->assertIdentical(trim((string) $element[0]), $expected_items[$key]);
+        $this->assertIdenticalWithSafeStrings(trim((string) $element[0]), $expected_items[$key]);
       }
     }

So for example this case. Why do we care about identicalness at all? IMHO we care about equalness, in which case you can define a comparator for your own, in which the t() and ((string) t()) are equal.

I think we can fix TestBase::castSafeStrings() to help us. And make it the responsibility of the test to call it. Also we should replace assertIdentical where it just is not necessary - for example - single element arrays where order is just not an issue.

This is blocking #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list

1. +++ b/core/modules/block_content/src/Tests/BlockContentListViewsTest.php
   @@ -42,7 +42,7 @@ public function testListing() {
   -    $expected_items = [t('Block description'), t('Block type'), t('Updated'), t('Operations')];
   +    $expected_items = ['Block description', 'Block type', 'Updated', 'Operations'];
   

   +1, I mean we don't have locale module enabled on those tests anyway

2. +++ b/core/modules/config/src/Tests/ConfigImportRenameValidationTest.php
   @@ -112,7 +112,7 @@ public function testRenameValidation() {
   -      $this->assertIdentical($expected, $this->configImporter->getErrors());
   +      $this->assertEqual($expected, $this->configImporter->getErrors());
   
   @@ -155,7 +155,7 @@ public function testRenameSimpleConfigValidation() {
   -      $this->assertIdentical($expected, $this->configImporter->getErrors());
   +      $this->assertEqual($expected, $this->configImporter->getErrors());
   

   Isn't the point that this works simply because we don't have a translation wrapper for t() in place yet? @see core/lib/Drupal/Core/Config/ConfigImporter.php:871 for example

3. +++ b/core/modules/node/src/Tests/Migrate/d6/MigrateNodeBuilderTest.php
   @@ -34,8 +34,8 @@ class MigrateNodeBuilderTest extends MigrateDrupal6TestBase {
   -    $this->assertIdentical($id, $migration->Id());
   ...
   +    $this->assertEqual($id, $migration->id());
   

   Nitpick: I think we don't need this particular change, right?

4. +++ b/core/modules/simpletest/src/TestBase.php
   @@ -691,37 +690,26 @@ protected function assertNotEqual($first, $second, $message = '', $group = 'Othe
   +  protected function castSafeStrings($value) {
   

   What about expanding the unit test coverage here? Ideally we would have that particular method available as trait, so the other tests could profit from it as well

2. assertEqual() ought to be fine here, right?

Ah you are totally right about that.

Had a review through the test changes and everything seems copacetic with me.

Casting seems a bit more explicit which is nice and leaves most of the identicals alone.

1. +++ b/core/modules/breakpoint/src/Tests/BreakpointDiscoveryTest.php
   @@ -193,7 +193,7 @@ public function testBreakpointGroups() {
        $breakpoint_groups = \Drupal::service('breakpoint.manager')->getGroups();
        // Ensure the order is as expected. Should be sorted by label.
   -    $this->assertIdentical($expected, $breakpoint_groups);
   +    $this->assertIdentical($expected, $this->castSafeStrings($breakpoint_groups));
    
   

   While this for example certainly works, it feels wrong, because do we actually care about the identicalness (this is just one random example), or are actually just interested in the equalness. I totally agree that this is NOT the default of this patch, but I think we need to think more about these things. Some code probably just has it because it is more strict, without thinking whether its actually needed.

2. +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
   @@ -20,6 +20,7 @@
    use Drupal\simpletest\AssertContentTrait;
   +use Drupal\simpletest\AssertionHelperTrait;
    use Drupal\simpletest\RandomGeneratorTrait;
   

   Thank you

+++ b/core/modules/simpletest/src/AssertionHelperTrait.php
@@ -0,0 +1,38 @@
+/**
+ * @file
+ * Contains \Drupal\simpletest\AssertionHelperTrait.
+ */
+
+namespace Drupal\simpletest;
+
+/**
+ * Provides helper methods for assertions.
+ */
+trait AssertionHelperTrait {
...
+    if ($value instanceof SafeStringInterface) {

No use for SafeStringInterface so this does not work. Also AssertContentTrait and AssertionHelperTrait seems messy so renamed this to AssertHelperTrait. Added unit tests too.

Changes in #49 looks good. Also +1 for the new trait and the new version of castSafeStrings().

Okay fixed.

+++ b/core/modules/simpletest/src/TestBase.php
@@ -659,9 +659,11 @@ protected function assertNotNull($value, $message = '', $group = 'Other') {
+    $first = $this->castSafeStrings($first);
+    $second = $this->castSafeStrings($second);
     return $this->assert($this->castSafeStrings($first) == $this->castSafeStrings($second), $message ? $message : SafeMarkup::format('Value @first is equal to value @second.', array('@first' => var_export($first, TRUE), '@second' => var_export($second, TRUE))), $group);

Shouldn't we remove the second castSafeStrings call from the assertion?

Back to RTBC

That approach seems much better to me too.

This looks great! And much better than earlier approaches indeed. Nothing to remark.

I dislike that we're making tests less strict though, but if it's done for The Better Good overall, then +1.

I agree with both counts. In general, I don't like assertEqual(). PHPUnit's assertEquals() is better in that at least it corrects for 0 == 'foo' problems, but Simpletest's assertEqual(), which other than fixing for safe strings comes with all of PHP's == faults, is icky.

But, I looked over all the ones in this patch and they all seem fine, so in the grand scheme of things, definitely better to unblock #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list, so I'll commit this shortly. First though, checking the credit boxes for @lauriii and @dawehner for their substantive reviews.

If anyone at any point is inspired to open issues to change any of these or other assertEqual() assertions to something stricter, please go for it.

Pushed to 8.0.x. Thanks for the great work on this and the careful reviews!

I added this issue to the issues for https://www.drupal.org/node/2564451 -- can we update the third item there with a few different examples?