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

TestCase::getMockForModel generate phpunit deprecations notices #14747

Closed
2 of 3 tasks
CauanCabral opened this issue Jun 26, 2020 · 5 comments · Fixed by #14748
Closed
2 of 3 tasks

TestCase::getMockForModel generate phpunit deprecations notices #14747

CauanCabral opened this issue Jun 26, 2020 · 5 comments · Fixed by #14748
Milestone

Comments

@CauanCabral
Copy link
Contributor

This is a (multiple allowed):

  • bug

  • enhancement

  • feature-discussion (RFC)

  • CakePHP Version: 4.1-RC2.

  • Platform and Target: phpunit 8.5.8, php 7.2

What you did

Have test cases with model linked with behaviors and tried mock some of behavior methods.

What happened

Get that message when tests are run:

Trying to set mock method "myBehaviorMethod" with onlyMethods, but it does not exist in class "App\Model\Table\ArticlesTable". Use addMethods() for methods that don't exist in the class.

What you expected to happen

Mocked methods and silent run

I found a PR on PHPUNIT about that deprecation: sebastianbergmann/phpunit#3687

So, inspecting Cake\TestSuite\TestCase::getMockForModel I can fix that message changing:

        $options += $locator->getConfig($alias);

        /** @var \Cake\ORM\Table $mock */
        $mock = $this->getMockBuilder($className)
            ->onlyMethods($methods)
            ->setConstructorArgs([$options])
            ->getMock();

to

        $options += $locator->getConfig($alias);
        $existingMethods = array_intersect($methods, get_class_methods($className));
        $nonExistingMethods = array_diff($methods, $existingMethods);

        /** @var \Cake\ORM\Table $mock */
        $mock = $this->getMockBuilder($className)
            ->onlyMethods($existingMethods)
            ->addMethods($nonExistingMethods)
            ->setConstructorArgs([$options])
            ->getMock();
@dereuromark dereuromark added this to the 4.1.0 milestone Jun 26, 2020
@dereuromark
Copy link
Member

Do you want to make a PR here?

@CauanCabral
Copy link
Contributor Author

Sure, I just don't know how to include a test for this (a test for the TestCase class?)

@othercorey
Copy link
Member

Fixed in #14748

@hakito
Copy link
Contributor

hakito commented Jul 5, 2020

This solution shifts the deprecation warning to the case when trying to mock protected methods. get_class_methods returns public methods only.

Maybe ReflectorClass should be used instead?

@CauanCabral
Copy link
Contributor Author

You are right @hakito

That way we have all methods reflected, in my codebase, worked to:

        [, $baseClass] = pluginSplit($alias);
        $options += ['alias' => $baseClass, 'connection' => $connection];
        $options += $locator->getConfig($alias);

        $reflection = new ReflectionClass($className);
        $classMethods = array_map(function ($method) { return $method->name; }, $reflection->getMethods());

        $existingMethods = array_intersect($classMethods, $methods);
        $nonExistingMethods = array_diff($methods, $existingMethods);

Have another idea?

I found another problem: if mock only proxied methods (from behaviors), PHPUNIT still generate a notice. I think its a bug in it, not a CakePHP fault, but need to build a standalone testcase to prove.

CauanCabral pushed a commit to CauanCabral/cakephp that referenced this issue Jul 14, 2020
CauanCabral pushed a commit to CauanCabral/cakephp that referenced this issue Jul 14, 2020
markstory added a commit that referenced this issue Jul 15, 2020
A correct fix to getMockForModel issue #14747
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants