As in https://github.com/sebastianbergmann/comparator/issues/44 mentioned the ArrayComparator ignore the order of two arrays, also if the attribute $canonicalize = false is set.

1. Required version of PHP moved up, but code not updated yet(partially fixes by PR #95)
2. DateTimeComparator::assertEquals() calls setTimezone(), but DateTimeInterface not declare this method

This PR mainly solves #21.

I want to point to a few aspects of my changes:

1. As you can see, I removed

$document->formatOutput = true;
$document->normalizeDocument();

Because tests not failed and I don't see why it needed. I could return it add appropriate tests if you explain me reason of this code. 2. I removed a lot of checks instanceof DOMDocument because in fact there is always DOMDocument in the $node in case if canonicalize is always true. 3. I removed $canonicalize because in fact it is always true and the unused argument just make the code maintenance harder. Especially without corresponding unit-tests. 4. I made nodeToText public to be able to test it. 5. ~~I see the possible error with the $ignoreCase argument (it ignores case when set to false) and I could fix it in this PR or in the different. What do you think?~~ Fixed in the #53

I'll be happy to receive any criticism/suggestions about the appearance of my changes and tests.

According to docs of ScalarComparator:

Compares scalar or NULL values for equality.

equality, do we consider it as an equality operator == of PHP ?

for that, I would propose to put extra assertion that compares not only if expected/actual matches provided expectation, but also are they following contract and mimics == equality.

I expect Travis to fail and show when contract of == is violated

ref: https://github.com/sebastianbergmann/phpunit/issues/3185 ref: https://github.com/sebastianbergmann/comparator/pull/58/files

Hi @sebastianbergmann,

I hit an issue with the signature of DOMNodeComparator::assertEquals() and PHP 7.2 nightly found by Travis CI, it seems the arguments don't match with ObjectComparator::assertEquals():

Fatal error: Declaration of
SebastianBergmann\Comparator\DOMNodeComparator::assertEquals($expected, $actual, $delta = 0, $canonicalize = false, $ignoreCase = false)
must be compatible with
SebastianBergmann\Comparator\ObjectComparator::assertEquals($expected, $actual, $delta = 0, $canonicalize = false, $ignoreCase = false, array &$processed = Array)
in phar:///home/travis/.phpenv/versions/master/bin/phpunit/sebastian-comparator/DOMNodeComparator.php
on line 0

PHP and Composer versions:

$ php --version
PHP 7.2.0-dev (cli) (built: Jan 28 2017 22:46:30) ( ZTS )
Copyright (c) 1997-2017 The PHP Group
Zend Engine v3.2.0-dev, Copyright (c) 1998-2017 Zend Technologies
    with Zend OPcache v7.2.0-dev, Copyright (c) 1999-2017, by Zend Technologies
$ composer --version
Composer version 1.3.2 2017-01-27 18:23:41

• Fixed the validity of the composer file
• Changed constraint to be bound (see https://github.com/composer/composer/pull/2321 for an explanation why it is better)

needed for https://github.com/sebastianbergmann/phpunit/pull/4972

fixed EPSILON was problematic and small values like 1E-100 and 1E-200 were considered the same

this PR removes the DoubleComparator completely, if number is supposed to be compared unexactly, delta can be passed to NumericComparator

With the changes made in 408d0d190976d08de44f19a8e502195a423b1281, comparison for DateTimeImmutable objects is broken, as the clone is no longer modified but rather returned as the result of the setTimezone and add calls. This pull requests uses the fluid interface defined for DateTime and DateTimeImmutable to ensure the modified clones are compared.

While making this change, I discovered indentation issues in the comparator test. If you'd rather not have whitespace fixes along with this pull request, let me know and I'll drop the commit.

Thanks for this great library, Sebastian. I'm going to get PHPSpec to use the ArrayComparator. I think I'm missing something because it's not built-in but seems so essential... can't be just me?

Anyway, there are these lines in ArrayComparator:

try {
    $comparator = $this->factory->getComparatorFor($value, $actual[$key]);
    $comparator->assertEquals($value, $actual[$key], $delta, $canonicalize, $ignoreCase, $processed);

    $expectedAsString .= \sprintf(
            "    %s => %s\n",
            $this->exporter->export($key),
            $this->exporter->shortenedExport($value)
            );

    $actualAsString .= \sprintf(
            "    %s => %s\n",
            $this->exporter->export($key),
            $this->exporter->shortenedExport($actual[$key])
            );
} catch (ComparisonFailure $e) {
    $expectedAsString .= \sprintf(
            "    %s => %s\n",
            $this->exporter->export($key),
            $e->getExpectedAsString() ? $this->indent($e->getExpectedAsString()) : $this->exporter->shortenedExport($e->getExpected())
            );

    $actualAsString .= \sprintf(
            "    %s => %s\n",
            $this->exporter->export($key),
            $e->getActualAsString() ? $this->indent($e->getActualAsString()) : $this->exporter->shortenedExport($e->getActual())
            );

    $equal = false;
}

I'm wondering why the second (failure) set of $actualAsString and $expectedAsString assignments aren't decorated so that the output highlights which key/values aren't matching? Something like:

Array (
    'a' => 'b'
--->'c' => 'e'
)

I'm happy to try a PR, but I'm thinking this is too obvious to be an idea that hasn't been considered already...

Fixes:

• https://github.com/sebastianbergmann/phpunit/issues/3208
• https://github.com/phpspec/phpspec/issues/887

Problem:

In certain scenarios mocking DateTime and DateTimeImmutable instances might be required. Currently any comparison between such mocks will fail because some methods are invoked by the comparator and it comes as a surprise to everyone involved.

Solution:

We might mock the methods, however, even if we expect the methods to be called in the mocks, there's no guarantee the implementation won't change in the next version of the library.

In most cases if we are mocking an object, we would probably be comparing it to the very same instance later on in the unit test. I figured, we could use spl_object_hash() to validate whether the actual and the expected values reference the same instance and skip comparing any other properties if so.

I wrote a unit test to validate that mocks of both DateTime and DateTimeImmutable pass. I am willing to write one that asserts that a clone of a mock would fail, but am not sure how to catch the error shown.

Do you think there are drawbacks to this approach? Let me know what your thoughts are.

The following assertion produces blank output: $this->assertEquals(1, 0);

Output There was 1 failure:

1. Test: Run Test Test.php:testRun - Expected | + Actual

Expected output There was 1 failure:

1. Test: Run Test Test.php:testRun - Expected | + Actual -1 +0

ComparisonFailure exception should be provided with expectedAsString and actualAsString values (https://github.com/sebastianbergmann/comparator/blob/master/src/NumericComparator.php#L61) but it isn't. As a result ComparisonFailure::getDiff() produces blank output: https://github.com/sebastianbergmann/comparator/blob/master/src/ComparisonFailure.php#L114

This covers issue #77 which is also the cause for PHPUnit Issues #4455.

The first change is in "src/ArrayComparator.php" where we first detect if the expected array has any non-integer keys. If that is the case, we treat both arrays as associative and sort them with ksort() instead of sort().

The second change is test coverage of the above logic in "tests/ArrayComparatorTest.php".

The issue #87 points out that in some edge cases the C14N can fail. It is not a common case, even psalm don't recognize that c14n() can return false.

This problem can be reproduced by setting a namespace uri value as a defined xml entity.

<!DOCTYPE foo SYSTEM "foo.dtd" [
  <!ENTITY ns_foo "http://uri.tld/foo">
]>
<i:foo xmlns:i="&ns_foo;"/>

I append two cases to tests when C14N fails (using current providers):

• Identicals documents must assert equal
• Different documents must throw ComparisonFailure

To solve it, I separate @$document->loadXML($node->C14N()) into two steps: $c14n = @$node->C14N() and @$document->loadXML($c14n). If $c14n === false then retry node to text conversion but without using canonicalization.

Observations:

• This is an edge case, C14N failing is very uncommon.
• DOMNodeComparator::nodeToText is always called with $canonicalize = true, DOMNodeComparator::assertEquals ignored the $canonicalize argument.
• The comparison on this cases (when C14N fails) is more strict, since canonicalization is not performed.
• Didn't remove the silent operator, and extended it to c14n() call. This is preserving the same behavior as before.

If you think is necessary I can try other approaches:

• Node canonicalization and xml loading capturing libxml errors, then throw an exception. This will require to add another exception that contains the libxml_get_errors() array, or, implode the errors into a single message and use the current SebastianBergmann\Comparator\RuntimException.

• If convert the node to text fails then throw a ComparisonFailure. This is like saying that $expected is different than $actual because the comparator is unable to perform the comparison.

• Change DOMNodeComparator::acept() and only return true if objects are DOMNode and the canonicalization don't fail. I don't like this approach because canonicalization can be very expensive.

I have been experimenting false positives on asserting that two XML are equal on <4.0.1, and TypeError on >=4.0.1. The change on >=4.0.1 is the declare(strict_types=1).

$xml = <<<EOT
<?xml version="1.0" encoding="utf-8"?>
<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd" [
  <!ENTITY ns_ai "http://ns.adobe.com/AdobeIllustrator/10.0/">
]>
<!-- This is a reduction of a much larger SVG file from Adobe Illustrator 15.0.0, SVG Export Plug-In. -->
<svg xmlns="http://www.w3.org/2000/svg" xmlns:i="&ns_ai;" foo="foo"/>
EOT;

// just change something to illustrate the problem
$modified = str_replace('foo="foo"', 'foo="bar"', $xml);

// on < 4.0.1 this is false positive
$this->assertXmlStringEqualsXmlString($xml, $modified);

// on => 4.0.1 this is a TypeError, but xmls contents are the same
$this->assertXmlStringEqualsXmlString($xml, $xml);

On >=4.0.1 the declare(strict_types=1) was set on DOMNodeComparator and it raises a TypeError.

TypeError : DOMDocument::loadXML() expects parameter 1 to be string, bool given
path-of-my-test.php:99

I trace down the error to DOMNodeComparator::nodeToText where it try to always canonicalize the XML by doing @$document->loadXML($node->C14N());. The problem is when DOMNode::C14N() fails and return false.

• On <4.0.1 it just try lo load an empty string. In case that both, $actual and $expected fails, then it gives the false positive.

• On >=4.0.1 because of strict types it throws TypeError, wich is good, but not explicit.

I think its possible to do something like: the error silence operator: the users of DOMNodeComparator will know that they are providing some object impossible to canonicalize and compare. The failing point will be raised by $node->C14N() returning false.

• Separate and silence @$node->C14N(), but check the return value on false and fail.

• Throw an exception that inform what the problem is instead of letting the TypeError to raise.

Would you like me to take care of this issue with a PR?

Would you suggest a better approach?

I think there need create comporator for strings and move code cor string compare form Scalar to separate class. Then will possible make string comparations with canonicalizing

Main issue in phpunit: https://github.com/sebastianbergmann/phpunit/issues/4049