Skip to content

phpunit update #446

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

Merged
merged 5 commits into from
Oct 19, 2016
Merged

phpunit update #446

merged 5 commits into from
Oct 19, 2016

Conversation

kokspflanze
Copy link
Contributor

removed unit test shouldNotUpdateHookWithoutName for #442
AbstractApiTest cleanup with ReflectionMethod
createMock removed in ResultPagerTest like in #438 comment written

AbstractApiTest cleanup with ReflectionMethod
createMock removed in ResultPagerTest like in #438 comment written
@acrobat
Copy link
Collaborator

acrobat commented Oct 16, 2016

👍 this should fix the tests! If the build is restarted it should pass as php-http/client-common 1.3 is released

@Nyholm
Copy link
Collaborator

Nyholm commented Oct 18, 2016

Instead of duplicating the reflection code you could use the NSA lib: https://packagist.org/packages/nyholm/nsa

The code gets more readable with it.

@cursedcoder
Copy link
Contributor

Can you please take care of conflicts?

@kokspflanze
Copy link
Contributor Author

conflicts fixxed

about https://packagist.org/packages/nyholm/nsa it will be a new dependency, with just PHPUnit 4.0 support, which will be not realy helpful in both ways.

@cursedcoder cursedcoder merged commit d03d708 into KnpLabs:master Oct 19, 2016
@cursedcoder
Copy link
Contributor

Thank you!

@Nyholm
Copy link
Collaborator

Nyholm commented Oct 19, 2016

Yes, NSA would be a new dev dependency. I know it is just a matter of taste but using NSA would remove about half the code you added here AND make it more readable.

What dev dependencies NSA has does not matter for us. We would only care about the hard requirements for our dependencies.

Anyhow, Good work with this PR. I'll make a new PR with NSA and we could discuss it there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants