Restore the PHPUnit listener #638
Conversation
The listener has been removed in ede1431 because it is called too late in the test lifecycle. This marks tests incorrectly as risky, if there are no other assertions in the test. Simply removing the listener silently breaks existing testsuites for PHPUnit <5.5.6, which will not complain about missing listeners. The listener added in this commit will no longer verify mock assertions. Instead, the listener will mark tests as failed if they don't verify the mock assertions themselves. This will surface the change for existing users of the listener and help them migrate to the trait or base class. Is also serves as an opt-in safefail for new Mockery users. Fixes mockery#636
| * @license http://github.com/padraic/mockery/blob/master/LICENSE New BSD License | ||
| */ | ||
|
|
||
| class Mockery_Adapter_Phpunit_TestListenerTest extends PHPUnit_Framework_TestCase |
There was a problem hiding this comment.
Let me know if you want me to refactor to namespaces. The existing tests didn't have any, so I stuck to that style.
There was a problem hiding this comment.
I wouldn't worry about that :)
| // the container is closed already will do. | ||
| \Mockery::self(); | ||
| } catch (\LogicException $_) { | ||
| return; |
There was a problem hiding this comment.
If there are is a better way to check this I'm all ears.
There was a problem hiding this comment.
I think it will suffice. I guess you could argue that you might be using Mockery for stubs or spies and post verification isn't required, therefore this would scream at you without justification, but I think we'll worry about that if someone mentions it.
| __NAMESPACE__ | ||
| )); | ||
| $result = $test->getTestResultObject(); | ||
| $result->addFailure($test, $e, $time); |
There was a problem hiding this comment.
This should probably only happen if there are expectations to begin with, right?
There was a problem hiding this comment.
Ah, I guess my reply to the other question is more relevant here. We can make a choice, throw anyway as a way of warning, or try and be clever and call Mock::close(), work on the result of that call. I don't really mind either way and will go with your judgement.
|
@pschultz replied to all comments, I'm happy to merge as is, please make changes regarding my comments if you would like to, otherwise let me know and I'll merge. |
|
Let's merge it as is. I'm updating one of our test suites now and it looks like tests will not fail if Mockery isn't involved. I'll dig into Mockery's internals later to see if the listener can be improved. I've only ever used mocks, never spies and have to become familiar with the differences. |
|
Actually hold on. There may be a bug. Let me finish migration our test suite. Then I'll be much more confident that this works correctly. |
|
This is definitely broken. There are lots of cases where assertPostConditions isn't called but the listener is, for instance skipped and incomplete tests. |
|
TLDR for spies, you don't set up expectations on them, they record all interactions and you manually verify against them afterwards, e.g. |
|
Good to go now. I decided to not check unsuccessul tests at all. As far as I can see, when using only spies it is not strictly necessary to call close(). But it also doesn't hurt so I agree to wait for complaints before getting clever in the listener. The fact that assertPostConditions isn't called for skipped, incomplete and failing tests is actually a problem. I will create a new issue for that. |
|
|
||
| public function testSuccessOnClose() | ||
| { | ||
| $mock = $this->container->mock('foo'); |
There was a problem hiding this comment.
Using 'foo' here is causing some flaky tests to fail when running the full suite, just calling mock() for these purposes should be fine. Same on lines 56 and 71.
Edit: missed the word "fail"
|
Let me know if you want to rebase into a single commit. |
|
Thanks, great work. |
The listener has been removed in ede1431 because it is called too
late in the test lifecycle. This marks tests incorrectly as risky, if
there are no other assertions in the test.
Simply removing the listener silently breaks existing testsuites for
PHPUnit <5.5.6, which will not complain about missing listeners.
The listener added in this commit will no longer verify mock assertions.
Instead, the listener will mark tests as failed if they don't verify the
mock assertions themselves.
This will surface the change for existing users of the listener and help them
migrate to the trait or base class. Is also serves as an opt-in safefail for
new Mockery users.
This is how it looks in action:
Fixes #636