Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

returnValueMap return null when key to match is object #4008

Closed
karborator opened this issue Jan 8, 2020 · 4 comments
Closed

returnValueMap return null when key to match is object #4008

karborator opened this issue Jan 8, 2020 · 4 comments
Labels
feature/test-doubles Test Stubs and Mock Objects status/waiting-for-feedback Waiting for feedback from original reporter type/backward-compatibility Something will be/is intentionally broken type/bug Something is broken

Comments

@karborator
Copy link

karborator commented Jan 8, 2020

Q A
PHPUnit version PHPUnit 8.5.0
PHP version 7.3
Installation Method Composer

Summary

This is not related to default null arguments.

I found same issue using it even when method doesn't have defaults null

    $dummyAccountID = new ID();
    $dummyClientID = new ID();
    $dummySiteID = new ID();

  $this->expectMultipleGetLegacyIDCalls(6, [
            [$dummyAccountID , 1],
            [$dummyClientID , 2],
            [$dummySiteID , 3],
        ]);

    private function expectMultipleGetLegacyIDCalls($numCalls, $returnValueMap) {
        $this->mock
            ->expects($this->exactly($numCalls))
            ->method('getLegacyID')
            ->will($this->returnValueMap($returnValueMap));
    }

 public function getLegacyID(ID $id){ ... }

Previous Behavior

On phpuni4 the method "getLegacyID" was returning proper integers from the map instead null

   [$dummyAccountID , 1],
            [$dummyClientID , 2],
            [$dummySiteID , 3],

Current Behavior

On phpuni8 the method "getLegacyID" returns null per dummyAccountID , dummyClientID , dummySiteID instead the given integers from the map

How to reproduce

Create map from new instance of object and integers to return then pass it to
->will($this->returnValueMap($returnValueMap));

  [$dummyAccountID , 1],
            [$dummyClientID , 2],
            [$dummySiteID , 3],
@karborator karborator added type/backward-compatibility Something will be/is intentionally broken type/bug Something is broken labels Jan 8, 2020
@sebastianbergmann
Copy link
Owner

Thank you for your report.

Please provide a minimal, self-contained, reproducing test case that shows the problem you are reporting.

Without such a minimal, self-contained, reproducing test case I will not be able to investigate this issue.

@sebastianbergmann sebastianbergmann added the status/waiting-for-feedback Waiting for feedback from original reporter label Jan 8, 2020
@karborator
Copy link
Author

karborator commented Jan 8, 2020

While trying to provide test I found whats the issue.
I'm not using the original method createMock instead I have proxy to phpunit4 getMock

 public function getMock(...$args) {
            if (!$this->mockGenerator) {
                $this->mockGenerator = new MockGenerator;
            }

            $args[4] = false;//phpunit4 $callOriginalConstructor default is true but phpunit8 createMock is "disableOriginalConstructor"
            $mockObject = $this->mockGenerator->getMock(
                ...$args
            );

            $this->registerMockObject($mockObject);

            return $mockObject;
		}

In my opinion its related to cloning the original object:
Invocation.php:83: $this->parameters[$key] = $this->cloneObject($value);

Test is:

class A {
    public function test(\stdClass $obj)
    {
        //
    }
}

    public function testInvocationClone()
    {
        $dummyAccountID = new \stdClass();
        $dummyClientID = new \stdClass();
        $dummySiteID = new \stdClass();

        $returnValueMap = [
            [$dummyAccountID, 1],
            [$dummyClientID, 2],
            [$dummySiteID, 3],
        ];

        $mock = $this->getMock(A::class,array(), array(), '',
            false, false, false);
        $mock->expects($this->any())
            ->method('test')
            ->will($this->returnValueMap($returnValueMap));

        $this->assertEquals(1, $mock->test($dummyAccountID));
        $this->assertEquals(2, $mock->test($dummyClientID));
        $this->assertEquals(3, $mock->test($dummySiteID));
    }

PHPUnit 8.5.0 by Sebastian Bergmann and contributors.

Warning:       Invocation with class name is deprecated

F                                                                   1 / 1 (100%)

Time: 1.21 seconds, Memory: 32.16 MB

There was 1 failure:

1) LogicNow\Alerting\Mappers\AgentlessOutagesByDeviceMapperTest::testInvocationClone
Failed asserting that null matches expected 1.

/var/www/vhosts/server/classlib/Alerting/Test/Mappers/AgentlessOutagesByDeviceMapperTest.php:104

FAILURES!
Tests: 1, Assertions: 1, Failures: 1.

And when I change to use createMock

class A {
    public function test(\stdClass $obj)
    {
        //
    }
}


 public function testInvocationClone()
    {
        $dummyAccountID = new \stdClass();
        $dummyClientID = new \stdClass();
        $dummySiteID = new \stdClass();

        $returnValueMap = [
            [$dummyAccountID, 1],
            [$dummyClientID, 2],
            [$dummySiteID, 3],
        ];

        $mock = $this->createMock(A::class);
        $mock->expects($this->any())
            ->method('test')
            ->will($this->returnValueMap($returnValueMap));

        $this->assertEquals(1, $mock->test($dummyAccountID));
        $this->assertEquals(2, $mock->test($dummyClientID));
        $this->assertEquals(3, $mock->test($dummySiteID));
    }


.                                                                   1 / 1 (100%)

Time: 1.26 seconds, Memory: 32.16 MB

OK (1 test, 3 assertions)
```

Feel free to close this if this is the case, thanks

@sebastianbergmann sebastianbergmann added the feature/test-doubles Test Stubs and Mock Objects label Feb 11, 2020
@jaapio
Copy link
Contributor

jaapio commented May 30, 2020

The problem is probably in the \PHPUnit\Framework\MockObject\Stub\ReturnValueMap this class is doing the following check:

            if ($invocation->getParameters() === $map) {
                return $return;
            }

in the provided example testcase the $map contains [$dummyAccountID]

php will do a strict check on the objects. === means that the object should be the same. not equal so using ReturnValueMap will not work in this case. The only way I know currently is using a combination of withConsecutive with willReturnOnConsecutiveCalls but this will require the calls in a certain order.

I think this is related to #4255 and #4026.

@sebastianbergmann
Copy link
Owner

No feedback, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/test-doubles Test Stubs and Mock Objects status/waiting-for-feedback Waiting for feedback from original reporter type/backward-compatibility Something will be/is intentionally broken type/bug Something is broken
Projects
None yet
Development

No branches or pull requests

3 participants