Replace assertEquals() overrides in PHPUnit Kernel, Functional and FunctionalJavascript tests with a MarkupInterfaceComparator implementation

1. In the new class MarkupInterfaceComparator the assertEquals method contains an additional parameter ($processed) that should be described. Dock block for assertEquals cannot be inherited.

+  /**
+   * {@inheritdoc}
+   */
+  public function assertEquals($expected, $actual, $delta = 0.0, $canonicalize = false, $ignoreCase = false, array &$processed = []) {

2. Coding standards.

--- lib/Drupal/Core/Test/Comparator/MarkupInterfaceComparator.php
+++ PHP_CodeSniffer
@@ -39,7 +39,7 @@
   /**
    * {@inheritdoc}
    */
-  public function assertEquals($expected, $actual, $delta = 0.0, $canonicalize = false, $ignoreCase = false, array &$processed = []) {
+  public function assertEquals($expected, $actual, $delta = 0.0, $canonicalize = FALSE, $ignoreCase = FALSE, array &$processed = []) {

+++ b/core/lib/Drupal/Core/Test/Comparator/MarkupInterfaceComparator.php
@@ -0,0 +1,65 @@
+    // When either expected or actual are MarkupInterface objects, we take over
+    // and convert to strings before comparing.

I think it is slightly more accurate to say something like "If at least one argument is a MarkupInterface object" here, as we also handle the case when both are objects?

Otherwise this looks good to me and will help with #3097892: Remove all @deprecated code in test base classes

+++ b/core/lib/Drupal/Core/Test/Comparator/MarkupInterfaceComparator.php
@@ -0,0 +1,65 @@
+use SebastianBergmann\Comparator\Comparator;

Do we need an explicit dev dependency on sebastian/comparator now?

No, but do we need to pin a version/set of known working versions? Otherwise Comparator 4.0 may be released and it could be backward incompatible with what we are doing here?

Thanks for the explanation, I think that is OK - in which case this looks good to go to me.

Hmmm... the container thing is a bit nasty. I wonder if we can add the comparator in a different place. For example, can we add it in the base test classes. I dunno.

This might work?

Fixed up the test data provider comment.

edit: @returns should be @return, that can be fixed on next patch or commit

@longwave nice one! TIL about \PHPUnit\Framework\TestCase::registerComparator() :)

Thanks for review, addressed #30 and #32.

As a followup I wonder if we will be able to refactor away Drupal\Tests\AssertHelperTrait::castSafeStrings() as the code is suspiciously similar (although it also deals with arrays of MarkupInterfaces)

What's also super interesting is that I think we only need this comparator when both sides are MarkupInterface objects - otherwise the regular scalar comparator is going to do the work for us. And yes I think a follow-up to see if this allows us to deprecate Drupal\Tests\AssertHelperTrait::castSafeStrings() is a good idea. But that might mean that we need to do something for Unit tests.

Let's try your hypothesis from #37.

Yeah I think we should go back to #33 unfortunately.

+++ b/core/lib/Drupal/Core/Test/Comparator/MarkupInterfaceComparator.php
@@ -0,0 +1,47 @@
+  /**
+   * Casts MarkupInterface objects into strings.
+   *
+   * @param mixed $value
+   *   The value to act on.
+   *
+   * @return mixed
+   *   The input value, with MarkupInterface objects cast to string.
+   */
+  private function safeCastString($value) {
+    return $value instanceof MarkupInterface ? (string) $value : $value;
+  }

Let's drop this in favour of doing

$expected = (string) $expected;
$actual = (string) $actual;

Less methods and simpler.

I think #33 is better in order to deal with the int vs MarkupInterface case, as otherwise this would trigger NumericComparator which doesn't support objects, but I think this is a valid thing to try and do in a test.

In #42 I was just trying to say that I think the #33 approach is both valid and correct, and we need to handle more than just the case where both sides are MarkupInterfaces, but I did not explain that very well :)

+++ b/core/lib/Drupal/Core/Test/Comparator/MarkupInterfaceComparator.php
@@ -0,0 +1,34 @@
+    $expected_safe = $expected instanceof MarkupInterface ? (string) $expected : $expected;
+    $actual_safe = $actual instanceof MarkupInterface ? (string) $actual : $actual;

I'm pretty sure that we can cast to string here without the $expected instanceof MarkupInterface

What is fixing #45 is now we stringify when both are MarkupInterface objects.

@alexpott That blew up the test when I tried it as neither array nor stdClass can be cast to string, but I guess that part of the test is invalid anyway, as it will never be called with these arguments. It does however mean int is cast to string but I suppose that doesn't ultimately matter?

I think that looks OK, a clever way of testing the three possible states that I hadn't thought of. Tentatively marking RTBC even though I worked on some of this, hope that's OK.

+++ b/core/tests/Drupal/KernelTests/Core/Test/Comparator/MarkupInterfaceComparatorTest.php
@@ -0,0 +1,148 @@
+      $this->assertNull($this->comparator->assertEquals($expected, $actual));

On second thoughts I think we need to assert that $equals_result is TRUE after this, if no exception was thrown?

Nice work, the changes are all good improvements.

+++ b/core/lib/Drupal/Core/Test/Comparator/MarkupInterfaceComparator.php
@@ -0,0 +1,34 @@
+namespace Drupal\Core\Test\Comparator;

Just realised this in the main core namespace. Let's put this in the test tools. There's no need for this to be autoloadable by the non test core autoloader.

Also this will make our life simpler if as probable Comparator changes signature in different PHPUnit versions.

Back to RTBC.

So we should favour composition over inheritance :)

Committed and pushed 398a61b7e0 to 9.0.x and 8d8cff8854 to 8.9.x. Thanks!

Great! Raised #3101247: Deprecate AssertHelperTrait::castSafeStrings() in favour of MarkupInterfaceComparator as followup.

@mondrake I don't think it is worth it. We're not going to make major changes to 8.8.x now. And there's enough to think about with 9.0.x and 8.9.x