-
Notifications
You must be signed in to change notification settings - Fork 43
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
Enhancement/140 Bump WordPress and PHP minimums #143
Conversation
Warning: Your XML configuration validates against a deprecated schema. Suggestion: Migrate your XML configuration using "--migrate-configuration"!
> PHPUnit 9.5.21 #StandWithUkraine > EEEEEEEEEEEEEEEEEEEEEEEEEEEEEE 30 / 30 (100%)
- Root composer.json requires php >=7.4 but your php version (7.1.33) does not satisfy that requirement.
The PHP Unit tests are failing with PHPUnit
@cadic I saw you mentioned about this somewhere recently, any suggestions here would be helpful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@faisal-alvi thank you for this update, I left couple questions about PHP versions which are still below the current minimum.
composer.json
Outdated
@@ -19,7 +19,7 @@ | |||
"source": "https://github.com/10up/simple-local-avatars" | |||
}, | |||
"require": { | |||
"php": ">=5.6" | |||
"php": ">=7.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we put 7.4 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had put 7.4
but as mentioned in the comment above, it upgrades the PHPUnit to 9.5.21
and all tests failed. Hence I reverted it back to 7.1
(0712ba0) that fixed this issue. Any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that the PHPUnit failure was due to the fix required in the upstream and the PR is already raised 10up/wp_mock#164.
Upgraded to 7.4
(c46a733)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getAnnotations() error is fixed by changing parent::setUp()
to \WP_Mock::setUp()
in the test declarations 2b8e160
But there are still 3 tests fail after this fix
Tests: 30, Assertions: 270, Errors: 1, Failures: 2.
.github/workflows/test.yml
Outdated
@@ -31,9 +31,6 @@ jobs: | |||
strategy: | |||
fail-fast: false | |||
matrix: | |||
# We claim to support from 5.6+ but WP Mock only supports 7.1+ | |||
# Also an issue with WP Mock not working on PHPUnit 9.5+, which PHP 7.3+ needs. | |||
# php: [ '5.6', '7.0', '7.4', '8.0', '8.1' ] | |||
php: [ '7.1', '7.2' ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7.1 and 7.2 looks like a deprecated support, needs to be updated to 7.4 and 8.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should, but need to fix the PHPUnit test failure issue first as mentioned above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, c46a733
…essed in the upstream: 10up/wp_mock#164
As mentioned in #143 (comment), the PHPUnit failure is due to the fix required in the upstream, and the PR is already raised 10up/wp_mock#164, so we can ignore the PHPUnit failures in the Github action(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check 3 tests are still failing after fixing the getAnnotation error
1) SimpleLocalAvatarsNetworkTest::test_is_network Error: Call to undefined method SimpleLocalAvatarsNetworkTest::getAnnotations() https://github.com/10up/simple-local-avatars/runs/7919270614?check_suite_focus=true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description of the Change
Bumped the minimum WordPress and PHP minimums.
Closes #140.
How to test the Change
Changelog Entry
Credits
Props @vikrampm1 @faisal-alvi @cadic
Checklist: