assert equals on cloned mocks

[fd8s0]

The following works on phpunit 3.6 but it doesn't work on 3.7.

$a = $this->getMockBuilder('stdClass')->getMock();
$b = clone $a;
$this->assertEquals($a, $b);

Fails because of the __phpunit_id which was added.

When you mock a dependency and the code you are testing clones it, doing assertEquals() before was working. Maybe assertEquals() should ignore this attribute, since if we wanted to know if it is exactly the same object we'd use assertSame().

[sebastianbergmann]

Does sebastianbergmann/phpunit@b82e74c solve the issue for you?

[fd8s0]

Yes, it does, cherry picked the commits, tests passing. That fixes the problem.

[sebastianbergmann]

Probably causes other issues, though. We need to further investigate this.

[edorian]

So a quick writeup of the current status because I currently don't have time.

The issue is caused by a combination of the id and this 3a294a1.

Initially it wasn't intended that clones should get a new "internal" id as they are clones and should match ===.

During debugging this is what I've collected as cases that should work / fail. The last test is supposed to fail.

When removing the "new id on clone" behavior only test 4 fails and

$mock2->expects($this->once())->method('foo')->with($this->equalTo($x));
        $mock2->foo($y);

reports

1) Issue100Test::testShouldWork4
Expectation failed for method name is equal to <string:foo> when invoked 1 time(s).
Parameter 0 for invocation mocking::foo(Mock_stdClass_08dd4529 Object (...)) does not match expected value.
Failed asserting that two objects are equal.

which I need to look into.

The point of the whole thing is to support two notions.

One is that generating two mocks should result in objects that are not equal as this might lead to falsely succeeding tests ( See #85 ) and the other is that cloning the object in "userland" should still work for state equality (==).

I'll came back an take another look when I'm not in a hurry

---

class Issue100Test extends PHPUnit_Framework_TestCase {

    public function testShouldWork() {
        $a = $this->getMockBuilder('stdClass')->getMock();
        $b = clone $a;
        $this->assertEquals($a, $b);
    }

    public function testShouldWork0() {
        $x = $this->getMock('stdClass');

        $y = clone $x;

        $this->assertEquals($x, $y);
        $this->assertNotSame($x, $y);
    }

    public function testShouldWork2() {
        $a = $this->getMockBuilder('stdClass')->getMock();
        $b = clone $a;

        $this->assertEquals($a, $b);
        $this->assertNotSame($a, $b);
    }

    public function testShouldWork3() {
        $x = $this->getMock('stdClass');

        $y = clone $x;

        $mock = $this->getMock('mocking');
        $mock->expects($this->once())->method('foo')->with($x);
        $mock->foo($x);
    }

    public function testShouldWork4() {
        $x = $this->getMock('stdClass');
        $y = clone $x;

        $mock2 = $this->getMock('mocking');
        $mock2->expects($this->once())->method('foo')->with($this->equalTo($x));
        $mock2->foo($y);
    }

    public function testShouldWork5() {
        $mock1 = PHPUnit_Framework_MockObject_Generator::getMock('stdClass');
        $mock2 = PHPUnit_Framework_MockObject_Generator::getMock('stdClass');
        $this->assertNotEquals($mock1, $mock2);
    }

    public function testShouldFail() {
        $x = $this->getMock('stdClass');
        $y = clone $x;

        $mock3 = $this->getMock('mocking');
        $mock3->expects($this->once())->method('foo')->with($this->identicalTo($x));
        $mock3->foo($y);
    }

}

[fd8s0]

Hmm, equals compares types and values. I think two mocks of the same class with the same values should be "equal".

The following also passes in 3.6 and 3.7, and I deem it correct.

$a = $this->getMock('stdClass');
$b = $this->getMock('stdClass');
$this->assertEquals($a,$b);

#85 speaks about expectations being made over one particular object, it doesn't negate the equality between them, if a mechanism was needed to work with the expectations, then a way to stop it interfering the other features should be worked on as well.

[edorian]

@fd8s0 The issue with allowing this:

$a = $this->getMock('stdClass');
$b = $this->getMock('stdClass');
$this->assertEquals($a,$b);

is that you can have tests that don't fail when they should.

class Foo {
     public function bar($a) { return $a; }
}

class FooTest extends PHPUnit_Framework_TestCase {

    public function testFoo() {
         $a = $this->getMock('stdClass');
         $otherObject = $this->getMock('stdClass');

         $this->assertEquals($a, $foo->bar($otherObject));
    } 

}

which is technically correct but not really what people are trying to test I'd say

[fd8s0]

I think it is what people are trying to test and that test should definitely pass, as I said in the original post, if we really wanted to test that it was the same object we would use $this->assertSame().

Refer to example 4.27 in http://www.phpunit.de/manual/current/en/writing-tests-for-phpunit.html

And the definition of equal objects of PHP. I don't see why this behaviour needs changing.

[edorian]

sigh Five release candidates and extensive testing with all major open framework to make sure that wasn't an issue for people to have that discussion now, after relase, on 4 channels with people arguing one way or the other.

---

The thing is that $this->getMock('stdClass'); doesn't really create the same object as the mocks can have different behaviors and, if phpunit would do some speed up caching, would be the same class. (Hmm, maybe making them a differnt class (Mock_StdClass_464743 vs Mock_StdClass_234265) would be another option to do this).

And there are a good amount of folks that expect different mock objects to not compare and i can see the point.

[fd8s0]

Hey, I agree is not the best way, I wouldn't do it myself that way, but there's lots of people with lots of tests written, and sadly, most of us will only start trying the new versions once they become stable. I still think I'm correct, though it is arguable that two mocks are not equal, what about a clone of such mock, should that not be equal?
In here, the problems come from cloning, there might be a case were people want to test two completely different mocks generated by phpunit, but I can't picture a scenario right now.
But if I'm not correct, or it doesn't matter because it will stay this way, at least there should be some upgrading notes at http://www.phpunit.de/manual/current/en/installation.html about this I believe.

[fd8s0]

What's the status, I still don't understand why this https://github.com/sebastianbergmann/phpunit-mock-objects/blob/master/PHPUnit/Framework/MockObject/Generator/mocked_clone.tpl.dist needs to be there.

[edorian]

Current status:

I've switched jobs this month and am busy hence I didn't get a quite 2 hours I need to tackel this without making things worse :)

What i want to make happen:

$a = $this->getMockBuilder('stdClass')->getMock();
$b = clone $a;
$this->assertEquals($a, $b);

This has to be true; Userland cloning should not change the object state in a way that comparing the objects with == fails.

Weighing the cases against each other it seems to make more sense to me that

$a = $this->getMock('stdClass');
$b = $this->getMock('stdClass');
$this->assertEquals($a,$b);

will fail as the objects are not connected to each other in a meaningful way and the false positives are way worse than the cases where it would make sense if they match.

---

I think thats the current state but I just did a quick braindump.

[Seldaek]

I just took a look at it and from what you describe, I think #104 is a fix that would do the job? I don't know the internals of the mock builder or phpunit much, so maybe I'm overlooking something, but if you have a minute to merge it and try it locally it'd be nice :)

[fd8s0]

I think the entire method should be removed, not just the id. But in doing this, I think that'd achieve it.

[bskendig]

For this code:

$a = $this->getMock('stdClass');
$b = $this->getMock('stdClass');

It seems to me that $this->assertSame($a, $b) should fail, but $this->assertEquals($a, $b) should pass. Why should they not be equal, if they are of the same class and they have been given the same values on the same properties?

I have a large project with a lot of tests like this: create mocked object $a and set a lot of properties on it; call the code I'm testing and get mocked object $b from it; and then test $this->assertEquals($a, $b). Having these objects no longer be considered equal will likely prevent me from upgrading beyond PHPUnit 3.6, as it would require too much change at this point in my project.

What you refer to as "false positives" seem to me to be confusion between assertEquals() and assertSame(), and this change in MockObjects means that (for mocks) assertEquals is now the same thing as assertSame().

Just my two cents. I appreciate all the work y'all do!

[logistiker]

The fix for this failed to address why the id was added in the first place. When you clone a mocked object in phpunit, you have no way to uniquely identify that object. I need to be able to uniquely identify that cloned object so when the expected method is called, it can call the property associated to the cloned object and not the original object. Here's a use case:

$this->endDateTime = $this->getMock('MyDateTime',
array('setDateTime','getDateTime'), array(),
'', true, true, false, false);
$this->endDateTime->dateTime = new DateTime('now', new DateTimeZone('America/New_York'));
$this->endDateTime->dateTime = $dateTime->modify('1 day');
$that = $this;
$this->endDateTime->expects($this->any())->method('setDateTime')
->will($this->returnCallback(function ($time, $timezone) use ($that) {
$that->endDateTime->dateTime = new DateTime($time, $timezone);
})
);
$this->endDateTime->expects($this->any())->method('getDateTime')
->will($this->returnCallback(function () use ($that) {
return $that->endDateTime->dateTime;
})
);

$foo = new foo($this->endDateTime);

class foo {
function __construct($endDateTime)
{
$nowDateTime = clone $endDateTime;
$nowDateTime->setDateTime('now');
var_dump($endDateTime->getDateTime(), $nowDateTime->getDateTime());
}
}

When var_dump outputs, $endDateTime and $nowDateTime will be the same. The reason for this is when $endDateTime gets cloned, it takes $that with it so as a result, it's still operating on the original dateTime property from $endDateTime. I need a unique id for the mock object and each of its clones so when it calls the setDateTime method, it sets the id to the dateTime property passed in with $that as an associative array key. Then when I try to retrieve the same object again with getDateTime, I would examine the id of $that->endDateTime and look for the key in the dateTime property array and retrieve the associated value so dateTime property would have something like this in it to identify the property values of each mocked object: array('mockid1' => origDateTime, 'mockid2' => newClonedDateTime)

[logistiker]

Perhaps you could reinstate the behavior of 3a294a1 but make it optional (off by default) so that people who need this id functionality can use it and the others who for some reason must check if a cloned object is equal have that as the default behavior?

[logistiker]

Nevermind. I'll just check the number of calls to a method instead since there doesn't seem to be any way to verify if a mocked object was cloned.