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

Fix phpunit tests #10

Merged
merged 9 commits into from
Feb 1, 2022
Merged

Fix phpunit tests #10

merged 9 commits into from
Feb 1, 2022

Conversation

otsch
Copy link
Contributor

@otsch otsch commented Jan 15, 2022

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Related tickets
Documentation if this is a new feature, link to pull request in https://github.com/php-http/documentation that adds relevant documentation
License MIT

What's in this PR?

I wanted to make a pull request for another thing, that I'll prepare next. I checked out the project and had a look at the readme where it says that you can run composer test, but the command didn't exist, so I added the "test": "@php vendor/bin/phpunit" script. Also when running tests I saw the warning:

1) Http\Adapter\Guzzle7\Tests\PromiseTest::testNonDomainExceptionIsHandled
PHPUnit\Framework\TestCase::prophesize() is deprecated and will be removed in PHPUnit 10. Please use the trait provided by phpspec/prophecy-phpunit.

Which relates to sebastianbergmann/phpunit#4141, so I did as recommended require phpspec/prophecy-phpunit and use the trait from there to fix the warning.

Why?

  • composer test command/script was missing.
  • warning when running phpunit tests.

Checklist

  • Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
    • I want to do another PR for a bugfix and add changelog there if that's OK?

Add the `composer test` command as mentioned in the readme file.
And as phpunit's prophesize method is deprecated from version 9 on and
will be removed in phpunit 10, use the trait from
phpspec/prophecy-phpunit instead as recommended.
Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

seems we had some copy-paste but not completely copy-pasted things. thanks for the fix!

can you please also remove the diff-format parameter in the static.yml workflow to get the cs-fixer to run again?

composer.json Show resolved Hide resolved
composer.json Show resolved Hide resolved
phpspec/prophecy-phpunit v2 which contains the needed trait isn't
available for PHP 7.2, so bump the version requirement to 7.3.
Also use composer test instead of phpunit directly in the ci config and
remove running the tests on version 7.2.
Finally also remove --diff-format to get cs running again.
@otsch
Copy link
Contributor Author

otsch commented Jan 17, 2022

@dbu I just pushed:

  • bump PHP version requirement to 7.3 and remove it from ci.yml
  • run composer test directly in ci.yml
  • remove diff-format parameter in static.yml

Also added a changelog entry. Wasn't sure if you want to actually tag it already or maybe just merge it and leave it in the changelog as "unreleased" until the next change actually relevant for users?

.github/workflows/static.yml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@dbu
Copy link
Contributor

dbu commented Jan 17, 2022

i changed the changelog to "unreleased", i don't think we need a release as no functional changes happened.

it seems the cs fixer has not been run in a long time... shall i take over or are you okay to also rename (and if necessary adjust) the cs-fixer configuration file?

@otsch
Copy link
Contributor Author

otsch commented Jan 17, 2022

Perfectly fine with you taking over, thx 🙂👌🏻

New php cs fixer config file name.
@otsch
Copy link
Contributor Author

otsch commented Jan 31, 2022

@dbu The output of the github action for the cs fixer said:
Configuration file .php_cs.dist is outdated, rename to .php-cs-fixer.dist.php.
Which I did now. Maybe it works now.

CHANGELOG.md Outdated
### Fixed
- Add missing `composer test` command and use it in CI
- Fix deprecated usage of prophesize in phpunit tests
- Drop support for PHP version 7.2
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Drop support for PHP version 7.2
- Drop support for PHP version 7.2

please move this point up above the ### Fixed heading - it is not something we fixed, just changed ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍🏻

@dbu
Copy link
Contributor

dbu commented Feb 1, 2022

thanks for picking this up again. i did not yet find time to look into it.

afaik the format of the cs fixer configuration also changed. an example of the new format would be https://github.com/FriendsOfSymfony/FOSHttpCache/blob/master/.php-cs-fixer.dist.php

@otsch
Copy link
Contributor Author

otsch commented Feb 1, 2022

I just changed the changelog and tried to fix the cs fixer config file 🤞🏻

setFinder() isn't static.
@otsch
Copy link
Contributor Author

otsch commented Feb 1, 2022

🥳😅

Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

yay, thanks a lot!

@dbu dbu merged commit 75f02c5 into php-http:master Feb 1, 2022
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.

2 participants