Skip to content

Fix unit tests #1631

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

Closed
wants to merge 1 commit into from
Closed

Conversation

vincentchalamon
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #1580 (comment)
License MIT
Doc PR

@soyuka
Copy link
Member

soyuka commented Jan 8, 2018

It's only failing on master?

@vincentchalamon
Copy link
Contributor Author

vincentchalamon commented Jan 8, 2018

@soyuka I don't think so.
It's related to this: symfony/symfony@0917c4c#diff-9d63a61ac1b3720a090df6b1015822f2
Maybe should I target to 2.0?

@soyuka
Copy link
Member

soyuka commented Jan 8, 2018

Maybe should I target to 2.0?

Yes

@vincentchalamon vincentchalamon changed the base branch from master to 2.0 January 8, 2018 16:04
@vincentchalamon
Copy link
Contributor Author

@soyuka It's rebased on 2.0

@soyuka
Copy link
Member

soyuka commented Jan 8, 2018

still failing with some deprecations :|

@vincentchalamon
Copy link
Contributor Author

@soyuka Those deprecations are out of this PR scope. Was it working earlier? Should I target it on 2.1?

{
$request = new Request();

$requestProphecy = $this->prophesize(Request::class);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Objects (without related interfaces) from external libraries should not be mocked, especially when its a value object like this one.

@dunglas dunglas mentioned this pull request Jan 8, 2018
@dunglas
Copy link
Member

dunglas commented Jan 8, 2018

Thanks @vincentchalamon, fixed in #1634.

@dunglas dunglas closed this Jan 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants