Update PHPUnit tests infrastructure

[thelovekesh]

Summary

Previously #1013
Fixes task 2 of #1012

Relevant technical choices

• Move tests to their plugin directories and move any helper files to the tests/data directory.
• Add separate phpunit.xml.dist to plugin directories. The current setup also allows to override of PHPUnit config per plugin e.g. put a phpunit.xml file in the plugin directory and PHPUnit will override phpunit.xml.dist.
• Move test commands from composer to npm scripts. NPM allows to run scripts from any directory and its config feature helps provide required args from CLI to npm commands out of the box. See: https://docs.npmjs.com/cli/v10/using-npm/config#command-line-flags

[github-actions]

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: thelovekesh <thelovekesh@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>
Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>
Co-authored-by: joemcgill <joemcgill@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: adamsilverstein <adamsilverstein@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[thelovekesh]

Failing tests are unrelated. It wasn't discovered previously as something was going wrong in the wp-env based setup.
See: #1013 (comment) and #1013 (comment)

[thelovekesh]

@felixarntz @westonruter Should I skip the failing tests and fix them in a new follow-up PR? Seems like to fix this we need to make some changes in the dominant-color-images plugin to specify which image editor needs to be loaded if both gd and imagick extensions are present and we want to use a specific extension.

[felixarntz]

@thelovekesh There is a lot of changes to review here. I'd like for us to take a step back.

This PR makes several changes, and I'm not sure all of them are justified. It would be great if you could provide more context on what your goal is with this PR and why/how those changes help us.

For instance, I see the tests are being moved to the individual plugin folders. That generally makes sense to me. However, I also see the wp-env setup is replaced with install-wp-tests.sh. That to me doesn't make sense because having that install-wp-tests.sh script here is far worse from a maintenance perspective than using a setup like wp-env which is supposed to "just work".

Let's discuss that more. Generally, I would prefer that we make one change at a time, to avoid such very difficult-to-review PRs.

[thelovekesh]

@felixarntz Most of the changes are related to moving test files from tests/plugins to their respective directory and it was previously worked at #1013 and discussed at #1012

However, I also see the wp-env setup is replaced with install-wp-tests.sh. That to me doesn't make sense because having that install-wp-tests.sh script here is far worse from a maintenance perspective than using a setup like wp-env which is supposed to "just work".

There are many reasons to not use wp-env in the CI. With this setup, the tests are being executed in half of the time wp-env takes to start the environment. Also, we identified a few issues in the setup itself which are mentioned in #973 (comment). Also from the maintenance perspective, I have used and seen plugins like AMP, and Web Stories using it for years and it doesn't require any maintenance so far.

Generally, I would prefer that we make one change at a time, to avoid such very difficult-to-review PRs.

If that's the case, I would love to break this PR into chunks. Just LMK your opinion.

[westonruter]

There are many reasons to not use wp-env in the CI. With this setup, the tests are being executed in half of the time wp-env takes to start the environment. Also, we identified a few issues in the setup itself which are mentioned in #973 (comment). Also from the maintenance perspective, I have used and seen plugins like AMP, and Web Stories using it for years and it doesn't require any maintenance so far.

If contributors have a blessed way to run tests without wp-env this may prove advantageous given many developers now are unable to use Docker given the licensing change to Docker Desktop.

[felixarntz]

@thelovekesh @westonruter While there are certainly different ways to set up CI workflows and tests, I feel quite strongly about using wp-env, for various reasons:

• It's an established project in the WordPress ecosystem, which allows us not to have to worry about all of this ourselves.
• It's used by both WordPress core and Gutenberg, the other two key projects that contributors to our repository are likely familiar with.
• Generally, it "just works", whether in CI or locally. You need Docker, run npm install, and the wp-env command, and that's it. You don't need to set up anything else. Plugins like Web Stories point to requirements of a local development environment, which may look different for everyone and requires more setup that is not documented as part of the repo and thus more complicated and more error-prone. Other than repositories using wp-env, I have not seen any plugin repositories on GitHub that were trivial to set up the development environment. We shouldn't move towards a model that makes getting started more complicated than it has to be.

So while based on the above, I'm opposed to using something custom in favor of wp-env, of course we can discuss this. But that should happen in an issue of its own. If you break that part out of this PR, we can focus on the other changes, which make more sense to me.

[thelovekesh]

Generally, it "just works", whether in CI or locally.

TBH I don't think so after what's discovered in #1013 (comment) and that's why dominant-color-images tests are failing.

It's used by both WordPress core and Gutenberg, the other two key projects that contributors to our repository are likely familiar with.

And they can use it locally even for running their tests. But in CI, I don't think it will affect any user workflow. Rather it will act as a source of truth that tests are being run as expected in an expected environment. In the current setup, I don't think we are even able to validate the environment configuration apart from PHP version.

[felixarntz]

Generally, it "just works", whether in CI or locally.

TBH I don't think so after what's discovered in #1013 (comment) and that's why dominant-color-images tests are failing.

I was referring to the development environment generally "just working", as in not requiring any external setup that is not part of the repository's documentation. I completely get your point though, but I would argue this kind of thing may happen and we can try to fix it upstream in https://github.com/WordPress/gutenberg/tree/trunk/packages/env.

Rather it will act as a source of truth that tests are being run as expected in an expected environment. In the current setup, I don't think we are even able to validate the environment configuration apart from PHP version.

I'm not sure I understand what you mean. As far as I know, we can customize how wp-env is configured to a certain degree. And if anything, using it should imply a consistent environment better than if we were requiring any WordPress development environment to be set up. What concretely is wp-env lacking for CI that a custom implementation could achieve? And would it make sense to potentially add those things to the upstream package?

[thelovekesh]

I'm not sure I understand what you mean.

I referred to the logging of environment details like PHP, WP, and PHPUnit versions.

I would argue this kind of thing may happen and we can try to fix it upstream

And how do we identify such problems? If we hadn't run our test suite with WP Tests setup we wouldn't have identified this issue in the first place. WP Tests setup is also from the WP ecosystem and used widely(just came to know site-kit also uses install-wp-tests 🤪).

Maybe we can bring this discussion to #973 and discuss it widely?

[felixarntz]

@thelovekesh We can consider enhancing wp-env to give us the logging that we're looking for. wp-env is an established solution that works out of the box, and is part of the WordPress project, so I don't think it's a blocker. We can contribute to it upstream to cater for our problems. Unless we find that our proposed changes don't make it into the product, I think it's inefficient to replicate similar functionality with our own solution, worse from a maintenance perspective.

Regarding install-wp-tests.sh, I know this is established too, but I really don't think it's a great practice for this file to just be copied around. It seems every repository is including it, there's no one place to include it from, which is not great from a maintenance perspective. You could almost argue it's not even an established approach, as everyone has their own versions of this file which may differ.

Overall, I completely agree this should be discussed elsewhere. Though I think replacing wp-env with something else should be discussed in its own issue, as it has wide-reaching implications. #973 doesn't look right for this as it's merely focused on expanding capabilities of our workflows. Let's not discuss too many different things in the same issue, as that ends up very difficult to navigate. For wp-env, we will need to carefully consider why it may not work for us and how an alternative solution would work better.

[thelovekesh]

We can consider enhancing wp-env to give us the logging that we're looking for. wp-env is an established solution that works out of the box, and is part of the WordPress project, so I don't think it's a blocker. We can contribute to it upstream to cater for our problems.

We can always contribute upstream to make it a better tool for local development while I think fitting it into CI is a bit overkill, and I would say install-wp-tests.sh is also a part of the WordPress project designed specifically for testing purposes.

I know this is established too, but I really don't think it's a great practice for this file to just be copied around. It seems every repository is including it, there's no one place to include it from, which is not great from a maintenance perspective.

I think this is also the case with wp-env where we have to add a workaround to make it work in CI by adding a wp-env override config so a little maintenance burden will be on both sides.

As mentioned above, to discuss it more widely I have opened #1085 and I will limit the scope of this PR to organizing test files and improving DX in testing.

[felixarntz]

@thelovekesh Sounds good. Let me know once you've updated the PR.

[mukeshpanchal27]

Should be reverted as trunk now have latest dependancy.

[mukeshpanchal27]

Can we utilize log from require( '../plugin/lib/logger' )?

Suggested change

	// eslint-disable-next-line no-console
	console.error(
	formats.error(
	`The plugin ${ WPP_PLUGIN } is not a valid plugin managed as part of this project.`
	)
	);
	log(
	formats.error(
	`The plugin ${ WPP_PLUGIN } is not a valid plugin managed as part of this project.`
	)
	);

[westonruter]

I think better to use console.error() as it goes to STDERR instead of STDOUT.

Otherwise, this could be added to logger as an alias in the same way that was done for console.log.

[mukeshpanchal27]

Suggested change

	/*
	* Last resort: see if this is a typical WP-CLI scaffold command set-up where a subset of
	* the WP test files have been put in the system temp directory.
	*/
	/*
	* Last resort: see if this is a typical WP-CLI scaffold command set-up where a subset of
	* the WP test files have been put in the system temp directory.
	*/

[mukeshpanchal27]

Suggested change

	/*
	* If neither of the constants was set, check whether the plugin is installed
	* in `src/wp-content/plugins`. In that case, this file would be in
	* `src/wp-content/plugins/performance/tests`.
	*/
	/*
	* If neither of the constants was set, check whether the plugin is installed
	* in `src/wp-content/plugins`. In that case, this file would be in
	* `src/wp-content/plugins/performance/tests`.
	*/

[felixarntz]

@thelovekesh Unrelated to the ongoing discussion in #1012: If we implement new JS tooling scripts like this, I think they should be incorporated into the existing foundation in bin/plugin. It uses commander for a full CLI suite, which has some benefits like built-in help / documentation functionality and error handling infrastructure. This could be reused rather than implementing standalone commands like this.

[adamsilverstein]

do we need tiff and bmp for tests? what about webp/avif?

[adamsilverstein]

TBH I don't think so after what's discovered in #1013 (comment) and that's why dominant-color-images tests are failing.

@thelovekesh if the issue for tests passing is with the Imagick implementation, we may be able to fix this by adding a filter to the test to have it use GD instead?

[adamsilverstein]

Overall this looks great @thelovekesh - a few moire points from other commenters to address then this should be good to go. Thanks for limiting the scope here to reorganization of files.