-
Notifications
You must be signed in to change notification settings - Fork 60
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
Simplify PHPUnit setup #520
Conversation
We added this in the past because of different PHP version requirements. Now that we require PHP 7.2+, this is no longer needed and we can simplify our testing setup.
Interesting test failures right now 🤔 |
There are still 2 failing tests, but I think now's a good time to get a second pair of eyes |
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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.
@swissspidy Mostly LGTM, just one change that I don't think we should be making, at least not unconditionally.
tests/phpunit/bootstrap.php
Outdated
tests_add_filter( | ||
'plugins_loaded', | ||
static function (): void { | ||
require_once TESTS_PLUGIN_DIR . '/plugin.php'; | ||
}, | ||
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.
This is not great, as it would be loading the plugin too late.
What problem exactly does it solve? If it's needed for a specific setup, we should make it conditional only if the more appropriate approach (directly filtering active_plugins
option) does not work. That way, if this ever leads to a problem, at least it wouldn't affect the overall tests infrastructure for everyone.
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.
IIRC I changed it mainly because this is the common way to set up plugin tests. But reverting that change actually seems to address the remaining two failures, so that's nice :)
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.
because this is the common way to set up plugin tests
Curious where you get it from - I've never seen it :)
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.
Any project that uses wp scaffold
or a derivation of that setup, see https://github.com/wp-cli/scaffold-command/blob/036c5152f85861bbd46ec63b36cde9074d786117/templates/plugin-bootstrap.mustache#L28-L35
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.
Hmm, FWIW I think this should be fixed there as it leads to problems - evidenced by this PR.
I don't see any reason not to filter the plugin option, this is something the WordPress test suite specifically allows for this kind of purpose with the wp_tests_options
global.
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.
Normally plugins wait until plugins_loaded
to run code, so I'd say PCP is the exception here :-) But feel free to open an issue there
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 just looked at the code, but it's using muplugins_loaded
- so this is different from what you had changed it to here.
Using muplugins_loaded
is okay, because that still loads the plugin early enough - even earlier than would be normal. But that's better than too late because everything that a plugin could do would still work.
I still think filtering active_plugins
is the even better solution, but since it's using muplugins_loaded
, that's fine. I was thrown off only because here the code was using plugins_loaded
.
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.
If it needs to be as early as possible, how about a new dedicated drop-in plugin file in WordPress core itself?
(Sorry if I've mentioned this idea elsewhere. Having deja vu!)
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.
@swissspidy LGTM, just one possible oversight.
I have troubles running the unit tests if the
plugin-check
folder isn't withinwp-content/plugins
of an actual WordPress installation. I don't want to change my setup because of this.build-phpunit
folder.Force_Single_Plugin_Preparation
plugin-check
repository somewhere else on the computer, not within a WordPress installation.With these changes I can now easily run tests locally using
WP_TESTS_DIR=/path/to/wordpress-develop/tests/phpunit composer test
There are still some errors and failures, particularly in
Plugin_Context_Tests
andCheck_Context_Tests
, because of plugin path differences.