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

Throw exception on non unique mocked method #4258

Closed

Conversation

jaapio
Copy link
Contributor

@jaapio jaapio commented Jun 1, 2020

This patch will make a mock throw an exception when multiple matchers
can be applied to a invoke. When allowing this, the results of tests are
not predictable.

In preparation on #4255 I found that a test case like the one below would never pass. Because the written test is just wrong. However, the mocking framework does allow this. But does not handle it properly. From now on an exception will be thrown.

        $mock->expects($this->any())
            ->method($this->stringStartsWith('foo'))
            ->with('foo')
            ->willReturn('result');

        $mock->expects($this->any())
            ->method('foo')
            ->with('bar')
            ->willReturn('result');

refs #4255

@jaapio jaapio force-pushed the feature/non-unique-method-mock branch from 1ce8bfb to d4b906b Compare June 1, 2020 08:19
@codecov
Copy link

codecov bot commented Jun 1, 2020

Codecov Report

Merging #4258 into master will decrease coverage by 0.04%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4258      +/-   ##
============================================
- Coverage     84.26%   84.22%   -0.05%     
- Complexity     4526     4527       +1     
============================================
  Files           218      218              
  Lines         11101    11103       +2     
============================================
- Hits           9354     9351       -3     
- Misses         1747     1752       +5     
Impacted Files Coverage Δ Complexity Δ
src/Framework/MockObject/InvocationHandler.php 69.84% <66.66%> (-7.21%) 27.00 <6.00> (+1.00) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61f7c75...e860ff4. Read the comment docs.

@sebastianbergmann sebastianbergmann added feature/test-doubles Test Stubs and Mock Objects type/enhancement A new idea that should be implemented labels Jun 1, 2020
This patch will make a mock throw an exception when multiple matchers
can be applied to a invoke. When allowing this, results of tests are
not predictable.

refs sebastianbergmann#4255
@jaapio jaapio force-pushed the feature/non-unique-method-mock branch from d4b906b to e860ff4 Compare June 1, 2020 19:29
@jaapio jaapio marked this pull request as ready for review June 1, 2020 19:42
@sebastianbergmann
Copy link
Owner

Cherry-picked into 9.2 and merged from there. Thanks!

@jaapio jaapio deleted the feature/non-unique-method-mock branch June 3, 2020 14:59
@GrahamCampbell
Copy link
Contributor

This change broke laravel/framework's test suite.

@GrahamCampbell
Copy link
Contributor

1) Illuminate\Tests\Database\DatabaseConnectionTest::testBeginTransactionMethodRetriesOnFailure
Non unique mocked method invocation: PDO::beginTransaction

/home/runner/work/framework/framework/src/Illuminate/Database/Concerns/ManagesTransactions.php:120
/home/runner/work/framework/framework/src/Illuminate/Database/Concerns/ManagesTransactions.php:100
/home/runner/work/framework/framework/tests/Database/DatabaseConnectionTest.php:158

@driesvints
Copy link

No worries about the above. We fixed the test which probably was incorrect to begin with. Thanks for your work!

@jaapio
Copy link
Contributor Author

jaapio commented Jun 5, 2020

This was definitely a broken test. The mocked method in question was called to many times internally. Although phpunit didn't report this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/test-doubles Test Stubs and Mock Objects type/enhancement A new idea that should be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants