Recommend using Prophecy for mocking

[greg0ire]

Prophecy comes with phpunit since version 4.5 , and I think we should default to using it for mocking things, because it is way way more concise unless you are mocking a class with a no arguments constructor and adding no expectations.

So several questions follow this :

1. Should we allow using prophecy in tests : I vote 👍
2. Should we recommend using prophecy in new tests : I vote 👍
3. Should we refuse phpunit mocks in new tests : I vote 👎
4. Should we migrate all tests to prophecy, for consistency's sake I vote 👎
5. Should we refuse mixing Prophecy and Mocks in the same test (method or class) 👍

[chalasr]

If I can give an opinion, Prophecy is nice and people that like it should be able to use it, as it's part of phpunit.
Plus, even that needs to be investigated, I don't think that mixing classic mocks and prophecy would cause any issue, just don't mix them in the same test class for a matter of readability.

Should we allow using prophecy in tests: 👍
Should we recommend using prophecy in new tests: 👎 (mocks are still good)
Should we refuse phpunit mocks in new tests: 👎
Should we migrate all tests to prophecy: 👎
Should we refuse mixing Prophecy and Mocks in the same test (method or class): 👍

[greg0ire]

Plus, even that needs to be investigated, I don't think that mixing classic mocks and prophecy would cause any issue, just don't mix them in the same test class for a matter of readability.

In my experience it does not cause any issue, I already did it because lazyness, but totally agree with you, I would recommend not mixing them to avoid confusing people.

[soullivaneuh]

1. Should we allow using prophecy in tests : Dunno. Have to see it. But if prophecy is the new way to go, then 👍
2. Should we recommend using prophecy in new tests : Well, it's linked to 1 so dunno
3. Should we refuse phpunit mocks in new tests : 👍 if we adopt Prophecy
4. Should we migrate all tests to prophecy, for consistency's sake : 👍 Consistency is important, even for test. But this does not mean we have to migrate all the tests right now, but progressively.
5. Should we refuse mixing Prophecy and Mocks in the same test (method or class) 👍

Especially for 5, I would say this is why we should use Prophecy or Mock in the future and not both, to avoid test conflict (especially for inheritance).

[jordisala1991]

This voting is for contributors only?

In my opinion Sonata should move to prophecy because it can do the same but in a cleaner, concise and readable way...

getMock will be deprecated, and we will have to refactor at some point the getMock calls in favor of getMockBuilder or prophesize, so I think it's a good point to start thinking in which direction should the test move.

Examples of code:

    $this->admin = $this->getMockBuilder('Sonata\AdminBundle\Admin\AbstractAdmin')
        ->disableOriginalConstructor()
        ->getMock();

    $this->admin->expects($this->once())
        ->method('getModelManager')
        ->will($this->returnValue($this->modelManager));

    $this->lockExtension->preUpdate($this->admin, $this->object);

vs

    $this->admin = $this->prophesize('Sonata\AdminBundle\Admin\AbstractAdmin');

    $this->admin->getModelManager()
        ->willReturn($this->modelManager);

    $this->lockExtension->preUpdate($this->admin->reveal(), $this->object);

You end up saving 2 lines per mock if you consider one mock and one mocked method which normally are more than just one.

Another reason is that when you are using getMock, you can't mock classes, just interfaces. When you want to mock a class, you need to use getMockBuilder with the disableOriginalConstructor. With prophecy you can mock anything with the same code. Simpler.

For the questions, I would vote all of them with a 👍 but taking into account that we should try to migrate to prophecy as we implement more test or we refactor some Test classes. Trying to move all the test to prophecy at the same time could be insane.

For me, the test are just as important as the code that is being tested. And both needs to be refactor/improved at some point.

[soullivaneuh]

This voting is for contributors only?

@jordisala1991 Of course not! All opinions are welcomed! 👍

I like your comment. 😉

What about IDE completion for Prophecy?

[jordisala1991]

Thanks! @soullivaneuh. Hmm, I don't use any IDE for autocompletion. Actually using atom / sublimetext.

You mean like this?

https://github.com/maxfilatov/phpuaca

The only negative point of prophecy is that is not well documented. It is easy to learn but when you don't remember something you have to look at the github README. AFAIK there is no more documentation, maybe @greg0ire knows.

[greg0ire]

The documentation is not as good as phpunit's so you might end up reading the code in rare cases when you do advanced stuff, but the interface is more simple, more natural so you quickly don't need to read it.

[OskarStark]

but the interface is more simple, more natural so you quickly don't need to read it.

👍

[greg0ire]

I'll make one message per vote so that it is easy to count

[greg0ire]

Should we allow using prophecy in tests ?

[greg0ire]

Should we recommend using prophecy in new tests ? (SHOULD people use prophecy ?)

[greg0ire]

Should we refuse phpunit mocks in new tests ? (MUST people use prophecy ?)

[greg0ire]

Should we migrate all tests to prophecy, for consistency's sake ?

[greg0ire]

Should we refuse mixing Prophecy and Mocks in the same test method ?

[greg0ire]

About consistency, my point is : changing all the tests will be hard work, and refusing prophecy because we can't afford to change all the tests will be hard

About refusing phpunit mocks, if a contributor works hard and provides something he has written with it, not sure he will want to amend his PR to switch… which means we will loose precious code coverage.

[greg0ire]

@chalasr @jordisala1991 @sonata-project/contributors can you please cast your votes ?

[greg0ire]

Should we refuse mixing Prophecy and Mocks in the same test class (having at least two methods using different mock systems)?

[soullivaneuh]

If we decide to use Prophecy, there is my thinking:

• New test classes MUST use Prophecy
• New test method MUST use current system, because of setUp where you set your mocking logic.
• Test refactoring to use prophecy would be welcomed. But not one PR with hundreds of changed files.
• When a test is updated, if the change to Prophecy can be done, encourage to do it.

After that, I didn't test Prophecy yet. I'll give you my feedback after sonata projects refactoring.

[greg0ire]

@soullivaneuh : if you want to try it out, I suggest you try refactoring this test : https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/3.x/Tests/Datagrid/ProxyQueryTest.php

[greg0ire]

From this page :

At the moment there are no plans to deprecate the API of PHPUnit's own mocking framework. It is likely, though, that at some point in the future we will encourage developers to use Prophecy's API instead of the "classic" PHPUnit API. As you can use PHPUnit's own mocking framework and Prophecy within the same test suite (yes, you can even also use Mockery and Phake at the same time) there is no need to migrate existing tests to Prophecy's API should you decide to use it for new tests. Also note that there are some features such as test proxies in PHPUnit's own mocking framework that are outside the scope of the Prophecy project.

[codebach]

Actually I want to change my vote for this suggestion:

Should we refuse mixing Prophecy and Mocks in the same test class (having at least two methods using different mock systems)?

to "NO".

Because for some methods we can just copy tests which are coded for similar methods. This will save time and we can use Prophecy to write tests for other methods.

[soullivaneuh]

@ahmetakbn I don't fully understand your sentence. Could you please elaborate?

[codebach]

@ahmetakbn I don't fully understand your sentence. Could you please elaborate?

For example here: https://github.com/sonata-project/SonataCoreBundle/pull/272/files I copied the test for the first method from test of another bundle. And I coded the rest of the tests.

[chalasr]

@ahmetakbn If we decide to use prophecy, it means that we move to something more modern and so basic mocking would be considered as the "old" way.
Also, moving all tests using basic mocking to prophecy seems overkill, but IMHO (again only if we adopt it) using prophecy for new tests should be considered as the way to go.
If a developer is really not familiar with prophecy, we could let the possibility to use basic mocking at all (even if I'm not sure it's something good). In the same way, mixing prophecy and basic mocking would not make sense, as if you are using prophecy, you should be easily able to convert a basic mock to a prophecy one. Plus, it would be confusing to use the two in the same method or class, just choose one of them then write your tests with.

[codebach]

@chalasr My only concern was not to write the same testing logic again if I have the option of just copying it, since we have mock and prophecy in our project. But I am ok with not mixing them in the same class also.

[soullivaneuh]

@ahmetakbn Even if it's a copy, you are talking about a new test class. And we was talking about mixing method on the same test class, that should not be done.

If a developer is really not familiar with prophecy, we could let the possibility to use basic mocking at all

If a developer can't write test at all, should we accept a PR without test? No.

This is the same here. If a developer can not but want to learn, this is the good moment to learn him how to do. 👍

[chalasr]

You're right. If one becomes the good, the other needs to be softly avoided. 👍

[soullivaneuh]

@jordisala1991 I'm not sure your example will work.

The IDE completion is stuck because of the not recognized getArgument mehod.

[greg0ire]

"Tools' constraints is out of the scope here!"

[soullivaneuh]

"Tools' constraints is out of the scope here!"

If this avoid developer to code, it is not. 😉

[greg0ire]

BTW, should I close and sum up the poll? Or should we wait?

[soullivaneuh]

Just wait for the moment. I did not get time to test.

BTW, this not an emergency. 😉

[greg0ire]

Ok

[sstok]

I would say: Use whatever works, I have used both Mocks and Prophecy and both have there place.
Prophecy is great for most cases (unless you need to keep the mock class open for changes while already communicating with it) and mocks are mostly used when we need to ensure something happens in the correct order (Symfony Form type extensions-logic for example).

Telling people witch solution to choose (without a good plausible reason) will only irritate them and creates a risk for a Golden Hammer

Whenever you have a problem you look for one clear, minimal, plausible solution, every time.

Giving recommendations and tips on how to best use a solution is always possible, but never force a solution, if the solution is good it will show it's purpose if not it will be replaced with something better.

[soullivaneuh]

and mocks are mostly used when we need to ensure something happens in the

This can be also done with Prophecy, can't be?

Giving recommendations and tips on how to best use a solution is always possible, but never force a solution

Yes and no. We would like to provide TestCase to factorize test process (#116), and have both Prophecy and Mock usage on those classes would be a nightmare IMHO.

[sstok]

This can be also done with Prophecy, can't be?

Nope :) you cannot configure that you expect something to be called before something else, at least not straight forward, it requires some ugly hacks and breaks the philosophy of the Prophecy system.

[soullivaneuh]

@sstok And what about this? https://github.com/Algatux/influxdb-bundle/blob/c234180f4a7af0d1951fe36c67225570456ac5d1/tests/unit/Events/Listeners/InfluxDbEventListenerTest.php#L22-L29

Or we are not talking about the same subject...

[greg0ire]

We would like to provide TestCase to factorize test process (#116), and have both Prophecy and Mock usage on those classes would be a nightmare IMHO.

I think we should pick one and stick with it for reusable test cases, and I think classical mocks would be the best fit for that, because people are more familiar with it.

[sstok]

@sstok And what about this? https://github.com/Algatux/influxdb-bundle/blob/c234180f4a7af0d1951fe36c67225570456ac5d1/tests/unit/Events/Listeners/InfluxDbEventListenerTest.php#L22-L29

Or we are not talking about the same subject...

No I'm talking about the at() expectation.
phpspec/prophecy#130

[soullivaneuh]

So for call order. I don't think we use it under sonata packages...

[greg0ire]

@soullivaneuh can we reach a decision? I have to write some new tests, and using the basic API, even with good auto-completion, does not feel as much efficient as using Prophecy, and even if we do not resolve all decisions, knowing that using Prophecy will at least be allowed would be great. As per the vote it should be.

[core23]

@soullivaneuh Any news on this?

[soullivaneuh]

Prophecy of Mock, we should restrict to one method. Apart of the system itself, moving to Prophecy will be heavier than keeping Mock because we will have both for a (long) time.

After that, I'm not against this if a majority want it. The rules we should respect if we decide to use it:

• All new test classes must use Prophecy. Mock will be no more accepted.
• Updated test classes must use the existing system (if it's mock, let's use mock).
• Converted test classes from Mock to Prophecy PR are of course gladly accepted and encouraged.

Agree with that?

Last thing: I would like a very simple vote on this comment:

👍 For moving to Prophecy
👎 To keep mock

Please share a lot this comment to have maximum of votes.

After that, the documentation should be updated to make it official. And maybe a blog post (cc @rande)?

[greg0ire]

I know I probably shouldn't jump to conclusions too fast, but after a thorough statistical analysis of the data at hand, I think it's fairly safe to say there might be some positive response regarding this proposal.

I shall draft a new paragraph in the CONTRIBUTING.md soon, unless someone else beats me to it, in which case it would be great to give me a heads up about it.

[OskarStark]

I know I probably shouldn't jump to conclusions too fast, but after a thorough statistical analysis of the data at hand, I think it's fairly safe to say there might be some positive response regarding this proposal.

You are right @greg0ire 👍

[greg0ire]

See #198

[VincentLanglet]

How to run the test locally ? I didn't find any documentation about this
I never used prophecy in my life. And when I run vendor/bin/simple-phpunit I get an error for every tests using prophecy...

[greg0ire]

FTR tests are (for now) run with phpunit as a global phar, running them with phpunit involves telling the bridge not to remove prophecy thanks to the SYMFONY_PHPUNIT_REMOVE=symfony/yaml env variable value