Skip to content
This repository was archived by the owner on Feb 20, 2023. It is now read-only.

As of 3.2.4 some mocked objects now through an undefined index message failing the unit test #321

Closed
benmatselby opened this issue Aug 17, 2016 · 15 comments
Labels

Comments

@benmatselby
Copy link

As mentioned on twitter, still not sure if we are doing something odd that is now exposed as of 3.2.4 or if it's a bug in this repo

    /**
     * Tests if the Statsd::get returns the same object when it has been set
     *
     * @covers Emis\Monitor\Statsd::set
     * @covers Emis\Monitor\Statsd::get
     */
    public function testGetWhenSet()
    {
        $statsD = $this
            ->getMockBuilder('Statsd')
            ->setMethods(['send'])
            ->disableOriginalConstructor()
            ->getMock();

        $statsD
            ->expects($this->once())
            ->method('send');

        Statsd::set($statsD);
        $this->assertEquals($statsD, Statsd::get());
    }

Worked in 3.2.3 but now we get

1) Emis\Tests\StatsdTest::testGetWhenSet
Undefined index: send

/Users/Ben/git/apps/intranet/tests/libs/Emis/Monitor/StatsdTest.php:169
@benmatselby
Copy link
Author

It looks to be an ordering issue.. From another test failing

        $dbAdapter = $this
            ->getMockBuilder('MysqliManager')
            ->setMethods(['quote', 'query', 'fetchByAssoc',])
            ->getMock();

We get

2) Emis\Tests\Mq\QueueTest::testGetFailedQueueMessages
Undefined index: quote

If we change the order of the array in set methods to

        $dbAdapter = $this
            ->getMockBuilder('MysqliManager')
            ->setMethods(['query', 'fetchByAssoc', 'quote',])
            ->getMock();

We then get

2) Emis\Tests\Mq\QueueTest::testGetFailedQueueMessages
Undefined index: query

@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
Copy link
Owner

Just to be sure: this issue is not present in phpunit-mock-objects 3.2.3, right?

@benmatselby
Copy link
Author

@sebastianbergmann Aye, no worries, just in a meeting but will get the minimal test for you.

And yes, 3.2.3 works fine if I checkout that tag

@benmatselby
Copy link
Author

@sebastianbergmann It is our code..

We do not hit phpunit-mock-objects/src/Framework/MockObject/Generator.php:806

        if (isset($class)) {

So we get the error in

            foreach ($methods as $methodName) {
                if ($this->canMockMethod($methodReflections[$methodName])) {
                    $mockedMethods .= $this->generateMockedMethodDefinitionFromExisting(
                        $templateDir,
                        $methodReflections[$methodName],
                        $cloneArguments,
                        $callOriginalMethods
                    );
                }
            }

As $methodName is not in $methodReflections

It is because we actually have to specify the full path to the class even though we have a use statement at the top:

$statsD = $this
            ->getMockBuilder('\Emis\Monitor\Statsd')
            ->setMethods(['send'])
            ->disableOriginalConstructor()
            ->getMock();

Sorry for hassling

@benmatselby
Copy link
Author

Actually, just looking at a test where it isn't namespaced

@sebastianbergmann
Copy link
Owner

Do you mean that this can be closed because it's a problem in your code?

@benmatselby
Copy link
Author

Let me check this one that isn't a namespace, then will close or report back :)

@benmatselby
Copy link
Author

Closing as it is our code.. There is a subtle change here, but it's our code that needs to deal with it.

Thanks

@greg0ire
Copy link

greg0ire commented Aug 23, 2016

@sebastianbergmann : the doctrine/dbal project builds mock from some classes that do not exist . I'm replacing all these class names with stdClass, but maybe there is a better solution you would like to recommend? See https://github.com/doctrine/dbal/pull/2479/files

@nubs
Copy link

nubs commented Aug 23, 2016

I am also running into this issue. You used to be able to mock a class that didn't exist, but that is no longer possible. I'll put together a simple reproduction case, but in the meantime you can see nubs/sensible#29 for what I did to get around it for now.

@nubs
Copy link

nubs commented Aug 23, 2016

https://gist.github.com/nubs/ddb081e1d1b09541d7f7a1d93d009533 is a simple reproduction case. Didn't take too long to write up :)

@sebastianbergmann
Copy link
Owner

At first glance, what @greg0ire and @nubs mention in their comments looks unrelated. Please open new, separate tickets and I will have a look.

@greg0ire
Copy link

greg0ire commented Aug 24, 2016

At first glance, what @greg0ire and @nubs mention in their comments looks unrelated. Please open new, separate tickets and I will have a look.

Thanks! See #322

@benmatselby
Copy link
Author

@sebastianbergmann FYI, what @nubs and @greg0ire are saying was exactly the issue I had.. Before this release it was silently allowing you to mock an non-existent object (Rightly or wrongly!).

Not sure if it either needs to be more clear you are mocking a non-existent object or not, as developers will make mistakes.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants