PhpUnit 7 integration and drop support for php70 #44

Giuffre · 2018-02-04T15:00:34Z

To catch up the roadmap of PHPUnit (https://github.com/sebastianbergmann/phpunit/milestones, https://phpunit.de/) we must drop the support to PHP 7.0.

As phpunit/phpunit is not a required dependency to moka but phpunit/phpunit-mock-objects is and follows phpunit development, we must update.

…) that is private

….php

Giuffre · 2018-02-04T17:15:47Z

@Jean85 or @xzhavilla consider you my workaround properly in this context?

To provide a full integration with PHPUnit\Framework\TestCase we attach automatically the generated mock object with the method registerMockObject and test it through https://github.com/facile-it/moka/blob/feature/phpunit7/tests/MokaTest.php. Until PHPUnit 6.* can be done by calling directly the method verifyMockObjects but from 7.* this method has changed the visibility from protected to private and to call it we need to do some magic with reflection.

Jean85

The workaround was probably the only one possible. I don't know whole Moka's architecture, the only bothering stuff could be performance if that is called many times.

BTW, it's still a workaround, so you could try to find a completely different approach anyway...

Giuffre · 2018-02-07T09:01:14Z

@Jean85 I agree. Maybe we can resolve it with another pull request.

It's not worth using that convenience method now that we have to invoke it via reflection. As a very big plus, only catch the very specific expections thrown on failed expectations, thus making tests much more robust.

xzhayon · 2018-02-13T10:35:15Z

src/Generator/Template/ClassTemplate.php

@@ -45,7 +46,7 @@ public function __get(%s $name)
    ';

    /**
-     * @param \ReflectionClass $class
+     * @param \Reflector $class


Why this change? \ReflectionClass implements \Reflector.

xzhayon · 2018-02-13T10:35:31Z

src/Generator/Template/MethodTemplate.php

        %s %s function %s(%s)%s
        {
            %s $this->__call("%s", func_get_args());
        }
    ';

    /**
-     * @param \ReflectionMethod $method
+     * @param \Reflector|\ReflectionMethod $method


Same as above.

Because we call a specific method of the concrete class implementation.

That's why the docblock had \ReflectionMethod instead of \Reflector. I mean that there shouldn't be any \Reflector type hint in the docblock.

According to @Giuffre it's a matter of IDE warnings... Resolving.

xzhayon · 2018-02-13T10:35:44Z

src/Generator/Template/ParameterTemplate.php


    /**
-     * @param \ReflectionParameter $parameter
+     * @param \Reflector|\ReflectionParameter $parameter


Same as above.

xzhayon · 2018-02-13T10:35:56Z

src/Generator/Template/PropertyInitializationTemplate.php

            unset(%s%s);
    ';

    /**
-     * @param \ReflectionProperty $property
+     * @param \Reflector|\ReflectionProperty $property


Same as above.

Because we call a specific method of the concrete class implementation.

xzhayon · 2018-02-13T10:36:45Z

src/Generator/Template/ParameterTemplate.php

@@ -32,7 +32,7 @@ protected static function doGenerate(\ReflectionParameter $parameter): string
                : 'null';

            $defaultValue = '=' . $defaultValue;
-        } catch (\ReflectionException $exception) {
+        } catch (\Exception $exception) {


Is it really safe silencing all the exceptions?

xzhayon · 2018-02-13T10:38:05Z

src/Generator/Template/PropertyTemplate.php

        %s %s $%s;
    ';

    /**
-     * @param \ReflectionProperty $property
+     * @param \Reflector|\ReflectionProperty $property


Same as above.

Because we call a specific method of the concrete class implementation.

xzhayon · 2018-02-13T10:38:33Z

src/Generator/Template/ReturnTypeTemplate.php


    /**
-     * @param \ReflectionMethod $method
+     * @param \Reflector|\ReflectionMethod $method


Same as above.

xzhayon · 2018-02-13T10:46:09Z

src/Tests/MokaMockingStrategyTestCase.php

@@ -195,11 +237,12 @@ public function testBuildWithPHP71Class()
            $this->strategy->get($this->mock)->getInt()
        );

-        $this->expectException(\Exception::class);
-        $this->expectExceptionMessage($this->namesWithValues['throwException']->getMessage());


Why did you remove this? It's meant to test explicitly which exception is thrown, it is important for it to be that way.

xzhayon · 2018-02-13T10:52:00Z

tests/Strategy/IncompleteMockingStrategy.php

@@ -14,10 +14,13 @@ class IncompleteMockingStrategy extends AbstractMockingStrategy

    protected function doBuild(string $fqcn)
    {
-        throw new \Exception();
+        throw new \BadMethodCallException(sprintf(


This is just a test class, a specific exception is not serving any need.

xzhayon · 2018-02-13T11:03:20Z

src/Factory/buildProxy.php

+ */
+function buildProxy(string $fqcn, MockingStrategyInterface $mockingStrategy): ProxyInterface
+{
+    return (new ProxyGenerator($mockingStrategy))->get($fqcn);


Why aren't we caching proxy generators anymore?

Because I've noticed that we have only n proxy generator where n is the number of strategies and probably the caching is useless for improving performance.

What if we're creating multiple mock objects in a test? Without a cache we're also creating multiple generators.

xzhayon · 2018-02-13T11:06:49Z

The workaround was fine to me, yet not needed. I replaced the call to the private method with calls to specific mock methods. Furthermore, I made the test catch only specific exceptions, to make it more robust.

This reverts commit e2cf8aa.

xzhayon · 2018-04-03T09:02:01Z

@Giuffre: I've reverted the change which removed the ProxyGeneratorFactory, to get generator cache back. Apart from that, I cleaned up some stuff and fixed a couple of tests.

I do not like all these functions, but, if it's fine for you, this changeset can go in!

Giuffre added 2 commits February 4, 2018 16:24

Update dependencies requirements

67ff554

Intercept \Error instead of \Exception

790545f

Giuffre force-pushed the feature/phpunit7 branch from f3743da to 790545f Compare February 4, 2018 15:24

Giuffre added 4 commits February 4, 2018 16:26

Fix typo

af863c6

Catch right throwable

edc9f10

Allow php72

5f19c10

Drop support for php70

fe2f83c

Giuffre changed the title ~~PhpUnit 7 integration~~ PhpUnit 7 integration and drop support for php70 Feb 4, 2018

Giuffre requested a review from xzhayon February 4, 2018 15:37

Giuffre self-assigned this Feb 4, 2018

Giuffre added the enhancement label Feb 4, 2018

Giuffre added 5 commits February 4, 2018 17:50

Delete PluginHelper and introduce plugin.helper.functions

9f34324

Use Reflection to call PHPUnit\Framework\TestCase::verifyMockObjects(…

0353b0d

…) that is private

Fix coding standard

ea99e36

Reset visibility of PHPUnit\Framework\TestCase::verifyMockObjects()

42a8093

Add to autoload the functions file src/Plugin/plugin.helper.functions…

48ed265

….php

Giuffre requested a review from Jean85 February 4, 2018 17:09

Giuffre added 12 commits February 4, 2018 18:26

Do not call __callStatic inside class, protected getProxy implemented

43595da

Remove duplication and apply PHP71 hints

5218bbc

Fix return types

c568258

Follow some phpstorm hints

8624836

Encapsulate in a private context the function 'generateFQCn'

b709ba7

Autoload internal functions with single file

a766bee

Declare strict_types

da7fc57

Fix docblock of throwned exceptions

e61a7e3

Replace StubFactory with hof buildStubs

45266ea

Split correctly buildStub and buildStubs

840bcd1

Completely split StubHelper into many hof

4d0a511

Rename correctly stripNameAndValidate and stripName

a7d6738

Giuffre added 2 commits February 6, 2018 23:05

Remove StubHelper references

d86be6d

Introduce buildProxy

e2cf8aa

Jean85 reviewed Feb 7, 2018

View reviewed changes

Remove TestCase::verifyMockObjects() workaround

f5e3edc

It's not worth using that convenience method now that we have to invoke it via reflection. As a very big plus, only catch the very specific expections thrown on failed expectations, thus making tests much more robust.

xzhayon requested changes Feb 13, 2018

View reviewed changes

xzhayon added 2 commits February 13, 2018 12:04

Remove comments from test class

81b3f5d

Cleanup style

be89213

xzhayon added 2 commits February 13, 2018 12:25

Add missing type hint back

946171d

Apply PHP CS fxer

ceb29a8

Giuffre added this to the v2.2.0 milestone Mar 1, 2018

xzhayon added 5 commits April 3, 2018 10:24

Test exception message

20a7002

Fix language

1878632

Revert "Introduce buildProxy"

6b21a80

This reverts commit e2cf8aa.

Remove return type breaking failing test

6f202a3

Cleanup generic stuff

7e31c2e

xzhayon approved these changes Apr 3, 2018

View reviewed changes

xzhayon merged commit 5369e8a into master Apr 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PhpUnit 7 integration and drop support for php70 #44

PhpUnit 7 integration and drop support for php70 #44

Giuffre commented Feb 4, 2018 •

edited

Giuffre commented Feb 4, 2018

Jean85 left a comment

Giuffre commented Feb 7, 2018 •

edited

xzhayon Feb 13, 2018

xzhayon Feb 13, 2018

Giuffre Feb 13, 2018

xzhayon Feb 13, 2018

xzhayon Feb 13, 2018

xzhayon Feb 13, 2018

xzhayon Feb 13, 2018

Giuffre Feb 13, 2018

xzhayon Feb 13, 2018

xzhayon Feb 13, 2018

Giuffre Feb 13, 2018

xzhayon Feb 13, 2018

xzhayon Feb 13, 2018 •

edited

xzhayon Feb 13, 2018

xzhayon Feb 13, 2018

Giuffre Feb 13, 2018

xzhayon Feb 13, 2018

xzhayon commented Feb 13, 2018

xzhayon commented Apr 3, 2018

PhpUnit 7 integration and drop support for php70 #44

PhpUnit 7 integration and drop support for php70 #44

Conversation

Giuffre commented Feb 4, 2018 • edited

Giuffre commented Feb 4, 2018

Jean85 left a comment

Choose a reason for hiding this comment

Giuffre commented Feb 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xzhayon Feb 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xzhayon commented Feb 13, 2018

xzhayon commented Apr 3, 2018

Giuffre commented Feb 4, 2018 •

edited

Giuffre commented Feb 7, 2018 •

edited

xzhayon Feb 13, 2018 •

edited