-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Feature similar to withConsecutive(), but without checking order #4026
Comments
@dbrekelmans @jaapio Sorry for dropping the ball on this. Are you able to send a pull request with an implementation for this in time for PHPUnit 10 (February 5, 2021)? |
No sorry, I had to spend more time on my own projects. So I didn't have the time to design something for this. Unless I find a way to focus a longer period on the mocks. I don't think I will be able to provide a patch for this. |
Not likely within the timeframe. I can look into it, but I don't know how much time I'll have to work on this and I've never worked on the PHPUnit codebase before so it'll take more time to figure out a solution. |
Thank you for getting back to me so quickly. I'll see what I can do. |
Any update on this? Thanks! |
Not from my side. The best thing to do from my point of view would be to remove phpunit internal mocking system. This is really a project on its own. When I first dived into this it seemed to be feasible within a short period of time to resolve this problem. But the more I looked at it, the more I realized that this would be way too much work for me. I can recommend you to re-evaluate whether you want to keep mocks as part of phpunit. Knowing that phpunit mocks do have their limits and quirks. But also that prophecy and mockery are a very good replacement. Which both fixed the problem as described. To be honest I try to avoid the problem right now or use the earlier mentioned mock frameworks. |
My workaround for this must-have feature was a |
As of right now, I do not think that we should create a replacement for |
Thanks @sebastianbergmann for the blogpost. I agree with your vision. Although in legacy projects you do not always have the time/capability/motivation to implement this correctly, it is not the responsibility of phpunit to fix this problem :) |
Where does this leave us on setting up expectations on a method that should return different values based on the arguments it's called with? Previously this was doable (albeit in a brittle way) via Having a feature like this would be a good solution. For example, say you're using doctrine and you do FWIW, mockery takes the expectation's arguments into consideration when determining which one to use. I.e. this is valid, which is a pretty clean way to go about it. $this
->em
->shouldReceive('getRepository')
->with(User::class)
->andReturn($userRepository)
->once();
$this
->em
->shouldReceive('getRepository')
->with(Article::class)
->andReturn($articleRepository)
->once(); Has it been considered doing something similar to that? |
The problem is not that nobody want's this feature. But nobody has time to implement it. There are alternatives like mockery and prophecy which are working perfectly with phpunit. However, I'm absolutely sure that any contribution regarding this ticket will be accepted. For me this was the mean reason to choose to spend my time on other more important projects. |
I'm late to the party but just saw this was deprecated when upgrading from 9.5 to 9.6. I guess I'm confused why something would be deprecated without a replacement if a replacement is desired. That's like saying I want to bake something but don't know how so I guess I'll remove the oven. That's irrational thinking. I don't believe saying things like "use mockery or prophecy" are valid solutions. Some of us don't want to install tons of bloated additional libraries to do a simple thing that has been available for 16+ years. It's a shame that one or two people complaining about not understanding or remembering how |
Please take in mind that phpunit was build by a single person, lucky it's maintained by a group of people nowadays.Direction of the project is always chosen by the maintainers. Not by me or any other community members. Your opinion on this topic might be different, that's perfectly fine. But you cannot force the maintainers of a project in a direction. They chose wisely based on input and their own thoughts. Phpunit drops support for methods every release, which moves the project in a certain direction. Given the fact that the project exists for so many years, I think we can only be thankful that this group of people are spending there time and passion on this lovely project. |
The blog post you linked is dead @sebastianbergmann |
https://thephp.cc/articles/do-not-mock-what-you-do-not-own EDIT: Thanks for all the downvotes. I just wanted to post the new link to Sebastian's article since the old link from above was broken. I personally also don't agree with this decission. |
Do you understand that I feel a bit offended by the way you are talking about my try to improve the mocking framework? The final decision to remove the method was made months after I opened the issue. Please be careful with the way you position others in your opinion. I think nobody here was trying to remove any use case. We are all trying to improve the project with contributions. |
For anyone reading this in 2023, you can solve the issue with $mock->expects($this->exactly(2))
->method('set')
->willReturnCallback(fn (string $property, int $value) => match (true) {
$property === 'foo' && $value > 0,
$property === 'bar' && $value > 0 => $mock->$property = $value,
default => throw new LogicException()
}); I'm writing a package to help migrate from Prophecy, but most of the time |
This is my temporary solution to still support <?php
declare(strict_types=1);
use PHPUnit\Framework\Constraint\Callback;
use PHPUnit\Framework\Constraint\Constraint;
use function array_column;
use function count;
trait PHPUnitHelper
{
/**
* @param array<mixed> $firstCallArguments
* @param array<mixed> ...$consecutiveCallsArguments
*
* @return iterable<Callback<mixed>>
*/
public static function withConsecutive(array $firstCallArguments, array ...$consecutiveCallsArguments): iterable
{
foreach ($consecutiveCallsArguments as $consecutiveCallArguments) {
self::assertSameSize($firstCallArguments, $consecutiveCallArguments, 'Each expected arguments list need to have the same size.');
}
$allConsecutiveCallsArguments = [$firstCallArguments, ...$consecutiveCallsArguments];
$numberOfArguments = count($firstCallArguments);
$argumentList = [];
for ($argumentPosition = 0; $argumentPosition < $numberOfArguments; $argumentPosition++) {
$argumentList[$argumentPosition] = array_column($allConsecutiveCallsArguments, $argumentPosition);
}
$mockedMethodCall = 0;
$callbackCall = 0;
foreach ($argumentList as $index => $argument) {
yield new Callback(
static function (mixed $actualArgument) use ($argumentList, &$mockedMethodCall, &$callbackCall, $index, $numberOfArguments): bool {
$expected = $argumentList[$index][$mockedMethodCall] ?? null;
$callbackCall++;
$mockedMethodCall = (int) ($callbackCall / $numberOfArguments);
if ($expected instanceof Constraint) {
self::assertThat($actualArgument, $expected);
} else {
self::assertEquals($expected, $actualArgument);
}
return true;
},
);
}
}
}
class Test extends TestCase
{
public function testWithConsecutive(): void
{
// PHPUnit < 10
$mock->withConsecutive(
['arg1_1', 'arg1_2'],
['arg2_1', 'arg2_2'],
);
// Using the trait
$mock->with(
...self::withConsecutive(
['arg1_1', 'arg1_2'],
['arg2_1', 'arg2_2'],
)
);
}
}
|
Also, |
Just stumbled on this while working on a legacy project that was still using 9.5 and looking for guidance on the issue of withConsecutive being evaluated twice. Can someone explain to me how the "Do Not Mock What You Do Not Own" concept has anything to do with this feature being removed? So far, I've relied on this to make assertions against code that I do own, and not 3rd party code. |
Exactly this. And at least provide some real life migration guide how to resolve this issue. |
@sebastianbergmann could we re-introduce this function in v10? It's bothering a lot of people, it creates a lot of frictions and there is no happy path to upgrade. On top of that it's not exactly clear (at least for me) to understand why it was removed, even after reading the blog. |
I am sorry, but I do not have the time to explain the problems of the implementation of The only thing I can offer is this: I will consider a pull request that implements what you (I am refering to everyone who commented on this issue) need. But it must be very clear what problem(s) it aims to solve and must use a name different than |
I propose to add support for multiple independent invocation mockers for a method. From user perspective supplement This would allow to set expectations for each combination of parameters independently, including number of invocations per combination. Example: $response = (new \Laminas\Diactoros\Response())
->withStatus(200)
->withAddedHeader('Set-Cookie', 'foo=bar')
->withAddedHeader('Set-Cookie', 'bar=baz')
->withAddedHeader(
'Set-Cookie',
'baz=qux; Domain=somecompany.co.uk; Path=/; Expires=Wed, 09 Jun 2021 10:18:14 GMT; Secure; HttpOnly'
)
->withAddedHeader('Set-Cookie', 'ss1=foo1; SameSite=Strict')
->withAddedHeader('Set-Cookie', 'ss2=foo2; SameSite=strict')
->withAddedHeader('Set-Cookie', 'ss3=foo3; SameSite=Lax')
->withAddedHeader('Set-Cookie', 'ss4=foo4; SameSite=lax')
->withAddedHeader('Set-Cookie', 'ss5=foo5; SameSite=None')
->withAddedHeader('Set-Cookie', 'ss6=foo6; SameSite=none');
$swooleResponse = $this->createMock(\Swoole\Http\Response::class);
$swooleResponse
->expects($this->once())
->method('status')
->with(200);
$swooleResponse
->expects($this->never())
->method('header')
->with('Set-Cookie', $this->anything());
$expectedCookieCalls = [
['foo', 'bar', 0, '/', '', false, false, ''],
['bar', 'baz', 0, '/', '', false, false, ''],
['baz', 'qux', 1_623_233_894, '/', 'somecompany.co.uk', true, true, ''],
// SameSite cookies
['ss1', 'foo1', 0, '/', '', false, false, 'Strict'],
['ss2', 'foo2', 0, '/', '', false, false, 'Strict'],
['ss3', 'foo3', 0, '/', '', false, false, 'Lax'],
['ss4', 'foo4', 0, '/', '', false, false, 'Lax'],
['ss5', 'foo5', 0, '/', '', false, false, 'None'],
['ss6', 'foo6', 0, '/', '', false, false, 'None'],
];
$actualCookieCalls = [];
$swooleResponse
->expects($this->exactly(9))
->method('cookie')
->willReturnCallback(static function (
string $name,
string $value = '',
int $expires = 0,
string $path = '/',
string $domain = '',
bool $secure = false,
bool $httponly = false,
string $samesite = ''
) use (&$actualCookieCalls): bool {
/** @psalm-var array $actualCookieCalls */
$actualCookieCalls[] = [$name, $value, $expires, $path, $domain, $secure, $httponly, $samesite];
return true;
});
$this->assertTrue($this->emitter->emit($response));
// sort since order does not match
$this->assertEqualsCanonicalizing(
$expectedCookieCalls,
$actualCookieCalls,
'Expected header calls do not matter'
); We the proposed interface it would looks like this: $response = (new \Laminas\Diactoros\Response())
->withStatus(200)
->withAddedHeader('Set-Cookie', 'foo=bar')
->withAddedHeader('Set-Cookie', 'bar=baz')
->withAddedHeader(
'Set-Cookie',
'baz=qux; Domain=somecompany.co.uk; Path=/; Expires=Wed, 09 Jun 2021 10:18:14 GMT; Secure; HttpOnly'
)
->withAddedHeader('Set-Cookie', 'ss1=foo1; SameSite=Strict')
->withAddedHeader('Set-Cookie', 'ss2=foo2; SameSite=strict')
->withAddedHeader('Set-Cookie', 'ss3=foo3; SameSite=Lax')
->withAddedHeader('Set-Cookie', 'ss4=foo4; SameSite=lax')
->withAddedHeader('Set-Cookie', 'ss5=foo5; SameSite=None')
->withAddedHeader('Set-Cookie', 'ss6=foo6; SameSite=none');
$swooleResponse = $this->createMock(\Swoole\Http\Response::class);
$swooleResponse
->expects($this->once())
->method('status')
->with(200);
$swooleResponse
->expects($this->never())
->method('header')
->with('Set-Cookie', $this->anything());
$expectedCookieCalls = [
['foo', 'bar', 0, '/', '', false, false, ''],
['bar', 'baz', 0, '/', '', false, false, ''],
['baz', 'qux', 1_623_233_894, '/', 'somecompany.co.uk', true, true, ''],
// SameSite cookies
['ss1', 'foo1', 0, '/', '', false, false, 'Strict'],
['ss2', 'foo2', 0, '/', '', false, false, 'Strict'],
['ss3', 'foo3', 0, '/', '', false, false, 'Lax'],
['ss4', 'foo4', 0, '/', '', false, false, 'Lax'],
['ss5', 'foo5', 0, '/', '', false, false, 'None'],
['ss6', 'foo6', 0, '/', '', false, false, 'None'],
];
foreach ($expectedCookieCalls as $cookieCallParams) {
$swooleResponse
// each variant must be invoked and invoked exactly once
->expects($this->once())
// Cookie name is a denominator for the invocation mock selection.
->methodMatching('cookie', $this->identicalTo($cookieCallParams[0]))
// actual assertions for parameters passed, only applied when this method mock is matched to invocation
->with(...$cookieCallParams)
->willReturn(true);
}
$this->assertTrue($this->emitter->emit($response)); |
There is no match in php7.4 so the alternative is to opt into a deprecated method, upgrade to php8+, or use 3rd party library. |
Or write the same code without using The real problem is that |
Another gist with workaround: https://gist.github.com/oleg-andreyev/85c74dbf022237b03825c7e9f4439303 |
Nice! This seems to be the most elegant solution I saw for this so far! Also, it shouldn't be that hard to modify in a way that ignores the order. |
I agree that phpunit has become a hassle to upgrade in large code bases in the last years. And this time it seems to be because of a personal philosophical viewpoint, not a technical one, which makes it extra frustrating. |
I put a workaround into a composer repository. Feel free to use it: https://github.com/nopenopenope/phpunit-consecutive-params |
Thanks a lot for that. The fact that there is so much discussion and snippets/packages to try to solve this issue is quite sad. I really hope that we will have an happy upgrade path for next majors. |
I think this should be a locked issue. @sebastianbergmann has given his reasons why this is no longer supported. Whether we agree with those or not, that's the case. We don't need to use this mocker, there are other mocking solutions out like Mockery that can be used, and people can make forks of this project if they really want. |
It literally is a closed issue... |
Agree with all the above, but sometimes, when the old application is being migrated to the latest PHP and PHPUnit, have come up with the following workaround (in order to avoid refactoring the old app for a quite trivial place, like logging. ->with(self::callback(function (string $message) {
static $i = 0;
return match (++$i) {
1 => $message === 'Removing audio files started.',
2 => $message === "Removing audio files that older than '2018-01-01 00:00:00'.",
};
})); |
In case it's useful to someone, I opted for this solution that allows me to use combine different assertions and manage multiple parameters easily:
|
The deprecation of The workarounds, that were presented here, are great. However, some of them have a So, imagine you have a SUT. class SUT
{
public function adore(string $arg): void
{
}
public function love(string $first, string $second): void
{
}
} One argument solutionThe solution for the one-argument class SUTTest extends TestCase
{
/** @test */
public function it_can_test_one_argument_consecutive()
{
$expectedArguments = [ // the consecutive arguments
'PHPUnit',
'pest',
];
$sutMock = $this->createMock(SUT::class);
$matcher = $this->exactly(count($expectedArguments)); // a matcher with a number of invocations
$sutMock->expects($matcher)
->method('adore')
->with(
$this->callback(function ($param) use ($expectedArguments, $matcher) {
$needle = $expectedArguments[$this->resolveInvocations($matcher) - 1]; // retrieves an argument
$this->assertStringContainsString($needle, $param); // performs assertion on the argument
return true;
})
);
$sutMock->adore('PHPUnit');
$sutMock->adore('pest');
}
private function resolveInvocations(\PHPUnit\Framework\MockObject\Rule\InvocationOrder $matcher): int
{
if (method_exists($matcher, 'numberOfInvocations')) {
return $matcher->numberOfInvocations();
}
if (method_exists($matcher, 'getInvocationCount')) {
return $matcher->getInvocationCount();
}
$this->fail('Cannot count the number of invocations.');
}
} Two arguments solutionThe solution for the two-arguments class SUTTest extends TestCase
{
/** @test */
public function it_can_test_two_arguments_consecutive()
{
$expectedArguments = [ // the consecutive arguments
['PHPUnit', 'pest'],
['laravel', 'symfony'],
];
$sutMock = $this->createMock(SUT::class);
$matcher = $this->exactly(count($expectedArguments)); // a matcher with a number of invocations
$sutMock->expects($matcher)
->method('love')
->with(
$this->callback(function ($param) use ($expectedArguments, $matcher) {
$arguments = $expectedArguments[$this->resolveInvocations($matcher) - 1]; // retrieves arguments
$this->assertStringContainsString($arguments[0], $param); // performs assertion on the argument
return true;
}),
$this->callback(function ($param) use ($expectedArguments, $matcher) {
$arguments = $expectedArguments[$this->resolveInvocations($matcher) - 1]; // retrieves arguments
$this->assertStringContainsString($arguments[1], $param); // performs assertion on the argument
return true;
}),
);
$sutMock->love('PHPUnit', 'pest');
$sutMock->love('laravel', 'symfony');
}
private function resolveInvocations(\PHPUnit\Framework\MockObject\Rule\InvocationOrder $matcher): int
{
if (method_exists($matcher, 'numberOfInvocations')) { // PHPUnit 10+ (including PHPUnit 12)
return $matcher->numberOfInvocations();
}
if (method_exists($matcher, 'getInvocationCount')) { // before PHPUnit 10
return $matcher->getInvocationCount();
}
$this->fail('Cannot count the number of invocations.');
}
} As you can see, both examples use the |
It's been one and half year since PHPUnit 10 is out and there is still more PHPUnit 9 install than 10 or 11. It's even stable, which shows the difficulties to upgrade. We are still stuck with many deprecations like this one and its taking years to migrate big codebases. Source: https://packagist.org/packages/phpunit/phpunit/stats |
We're in the same boat as @BafS . Far too many affected tests for Product to OK so much tech-debt-related work. |
I've put one of the solutions from this issue into this little repo which is by today already downloaded over 130k times - it will help you to progress to PhpUnit 10. 👍 |
IMHO, until a better (and working) solution can be added to PHPUnit itself, |
There are around ~10 variations of how PHPUnit Instead of creating answer that fits only one specific quesition, I've put down an article that explains, how to upgrade all kind of combinations. See: Hopefully you'll find it useful. It includes PHP 7.* ready code (no |
@TomasVotruba You propose using the method If you're already annoyed that dealing with the Your article doesn't even warn users from this, which might lead other users into the wrong direction, unknowingly replacing the inital frustration about the |
@jaapio Yes, the method is I'll update the post to mention this, good point 👍 Thanks |
@TomasVotruba we just use a simple static $invocationCount = 0;
$invocationCount++; at the top of the callback. It keeps the function self-contained, no internals, and avoids making the readability of the |
In the docs we find this example for
withConsecutive()
:If your class calls method
set
with parameterfoo
first and with parameterbar
second, the test will pass. However, if you call it withbar
first andfoo
second the test will fail. This leads to brittle tests (as is mentioned in a note under theat()
docs).It would be lovely to have a way to check
method 'foo' is called exactly twice. Once with parameter 'x' and once with parameter 'y'
, without checking the order.A concrete example is testing that certain events are dispatched when it doesn't matter in what order they are dispatched (of course sometimes you do care about the order and in that case you can use
withConsecutive()
).The text was updated successfully, but these errors were encountered: