DX improvements in new monorepo #1012
Comments
thelovekesh
added
the
Infrastructure
@westonruter I think it would make more sense to include the plugin test in the plugin directory itself. It will help us in multiple ways:
Please take a look at the POC - #1013 |
|
I do like the idea of organizing the tests into the plugin's directory. It's more helpful for DX to keep the plugin-related code together than it is to keep all test-related code together. |
|
+1 on the idea to keep the plugin's source and test code in a similar location. For example, a plugin What I'm not a fan of is the separate So from my end:
|
|
@felixarntz A benefit to having a separate performance/tests/bootstrap.php Lines 37 to 90 in 082a106 The standalone plugins can still share the bootstrapping logic as I mentioned in #1013 (comment) while they each have their own |
|
@westonruter I'm not sure the value is worth the tradeoff. I can't envision a single plugin would use multiple test suites. Certainly there are a few complex plugins that may make use of that, but we haven't used it so far, and I'm not sure when or why we would. While for PHPCS I see a benefit of individual files because of minor configuration changes, for PHPUnit the config files typically look the same for every plugin. I'd say we should reevaluate this if we encounter a real need for it, but otherwise it creates overhead of duplicate files without a real benefit. |
|
@felixarntz The Embed Optimizer plugin could, for example, have an Another benefit is the DX improvement. If you're working on Speculation Rules, you could just run npm run test-php -- -- -- --testsuite=speculation-rules |
Again, IMO this is a theoretical benefit as we have no plans to make use of it at this point. If there is a need for it, we could re-evaluate. But as you're saying there may be more efficient ways to achieve it than duplicating the config files.
Would that use the correct PHPUnit version (the one locally installed via PL
npm run test-php -- -- -- --testsuite=speculation-rulesWouldn't we be able to simplify the call using our NPM wrappers? #1002 already adds a wrapper to simplify this for e.g. |
That's true. Although that can be easily resolved with a shell script that sets up the environment to work on the project. Someone could just add this to their function workhere() {
export PATH="$PATH:$(pwd)/vendor/bin"
}And then when navigating to the $ workhereAnd then the Interestingly, PhpStorm (er, IntelliJ) does this automatically for Node scripts. (VSCode does not currently implement this.) When I'm in the built-in IDE terminal, I see my
True. Although the need to pass |
TBH the
Plugin name resolution logic in bootstrap fileperformance/tests/bootstrap.php Lines 38 to 67 in 082a106
So, the tradeoff between both is making changes to the main PHPUnit config + updating tests bootstrap file + maintaining it on use cases vs keeping separate PHPUnit config which is just a one-time copy from any other plugin. |
|
@thelovekesh I'm not sure I agree with that assessment in terms of maintenance cost. I think the only difference from a maintenance perspective is the following:
Both of that isn't really different in the amount of effort. However with option 1 you end up with a lot of duplicated config, while with option 2 you don't. When something in PHPUnit changes that leads to a change in the configuration, we need to change it everywhere, instead of in a single place. That's why I think maintenance cost of individual PHPUnit config files is higher. The only way that this could be resolved without all the duplication would be if we could embed a PHPUnit config file from within the individual other PHPUnit config files (similar to how #1002 does for PHPCS). Though I'm not sure that is possible. |
|
Just so we're on the same page, what is the duplicate code in question?
|
Also, has this ever happened before? If not, perhaps it is a theoretical problem? |
@felixarntz I am like 99ish percent sure we can't do it in PHPUnit, the way we can do it for PHPCS due to the nature of both tools.
I agree there will be a duplicated config but that would be very less + it doesn't ask for any changes in most of the cases. For example: This repo uses the same approach in which the initial config is always the same in all config files but it's helping to make the test suite self-containable for every package and they just need to worry about single thing at a time. Aside, I think we should also add the maintaining overheads for maintaining scripts in npm/composer and need to know the specific test suite name within the global phpunit config to run tests for a particular plugin. |
Everything that the config contains except the
Note that only the multisite config excludes
It is not a lot of config, but it'll pile up. It's two files (single site and multisite), and that for every plugin. Why duplicate config when we don't have to? Why go the complicated effort to implement a script to sync multiple files when we could just use a single file in the first place? Just for a numeric comparison, with individual files we're talking 43 lines of config for every plugin (from both PHPUnit config, single site + multisite), whereas with centralized files (one for single site, one for multisite as is now) we're talking 8 lines of config for every plugin, and no duplication.
PHPUnit makes a lot of breaking changes between versions, so this does actually happen. For instance, unless that has been fixed by now, the current WP core PHPUnit config includes some deprecated stuff when used with the latest PHPUnit versions. So the format of the config files occasionally requires changes. It's not often probably, but it's not a theoretical problem. This is probably a pretty nit-picky discussion from both sides. Ultimately, either way will work. I personally don't see the value of having multiple config files. All the considerations about workflow appear to require other local environment setup steps that aren't bundled into the repository (i.e. unlikely to be used by many contributors), and regarding working from within the plugin folder, you'll have to pull down the main repository anyway, and it's not very large, so I'm not sure what's the value in that. Last but not least, as I said before, adding a new plugin will always require some changes in the root of the repository too (e.g. From my perspective, the only convincing argument for individual config files is if they needed to diverge between individual plugins, but there's no real use-case for that, and if we ran into one we could reconsider. In any case, I think this discussion is probably being exhausted soon. If you feel strongly about individual files though, I'm not going to block it. |
It seems like we might be able to avoid duplicating the multisite configuration. Instead, we could potentially achieve the same functionality by simply passing the WP_MULTISITE environment variable through our current setup. Of course, if there are specific use cases where ms-excluded or ms-require groups are necessary(currently I see none), we can certainly add them for those particular plugins. |
|
Personally, unless we're going for full separation of responsibilities for each plugin to maintain it's own dependencies, build process, and development tooling, I would prefer that all tests be located in the root project's One of the main benefits of keeping these in a mono-repo in the way that we are developing them is that we can gain efficiencies by maintaining a single set of tools and configurations that are shared across standalone plugins. If we do end up deciding on full separation, I think we should consider whether the performance-lab plugin should also be treated as one of the plugins in the mono-repo, and not the root of the project. |
|
I think separation can be on a need-to-go basis like we did for #1002 which is beneficial in operating the concern of text-domain for WP I18n sniffs. We could have added the text domain in the main config file but that was overkill. IMO testsuite feature should be used within the scope of a self-containable single bootstrap file. Currently, based on the testsuite we are modifying it for other use cases which doesn't seems ideal when the same can be achieved by having a different config. |
|
A couple more bits of feedback for the plugin monorepo I've noticed: First, only PHPCS for the PL plugin can have its Line 29 in 5e570fa However, for a plugin like Auto Sizes, it is explicitly set to Line 39 in 5e570fa This isn't great as env-specific overrides can't be done while also using ../../build-cs/vendor/bin/phpcsSecondly, when using PhpStorm, the presence of multiple PHPCS configs is problematic since it unfortunately doesn't support this. What this means is that if I've configured PhpStorm to use the root
I then get zero checks performed when working on a plugin, since it is configured to ignore So what I have to do is copy the root --- phpcs.xml.dist 2024-03-23 10:18:11.563329799 -0700
+++ phpcs.xml 2024-03-25 10:17:50.367229615 -0700
@@ -3,7 +3,7 @@
<description>WordPress Coding Standards for the Performance Lab Plugin</description>
<rule ref="bin/phpcs/phpcs.ruleset.xml"/>
- <config name="text_domain" value="performance-lab,default"/>
+ <config name="text_domain" value="performance-lab,default,optimization-detective,webp-uploads,speculation-rules,dominant-color-images,embed-optimizer"/>
<rule ref="WordPress.Files.FileName.NotHyphenatedLowercase">
<exclude-pattern>includes/server-timing/object-cache.copy.php</exclude-pattern>
@@ -11,6 +11,7 @@
<file>.</file>
<exclude-pattern>./node_modules/*</exclude-pattern>
- <exclude-pattern>./plugins/*</exclude-pattern>
<exclude-pattern>./vendor/*</exclude-pattern>
</ruleset>This isn't terrible, but it is something to be aware of for the DX of developers using PhpStorm. |
|
Oh, I forgot perhaps the most important thing here which is broken. Currently when attempting to make a change to a standalone plugin, lint-staged is not checking any of the PHP files in Lines 53 to 54 in 5e570fa I've opened #1089 to fix this. Aside: Perhaps the Performance Lab plugin should be moved to |
@joemcgill had suggested something along those lines before too, treat it more similar to the other plugins. I think that idea makes sense, though I think we need to be careful timing-wise because that will affect lots of other workflow aspects which will then need to be changed. We should probably not make such a change in Probably the best way to get started is to open an issue to think through what that change would entail. This issue here is good as a brainstorming starting point, but let's be mindful of not making it too broad, which would complicate discussion and code. |
|
@westonruter In #1090, I have moved lint and format scripts to |
|
Reposting here from #1065 (comment): The discussion on whether to use individual PHPUnit test suites per plugin was never resolved, so IMO it's a bit premature that #1065 now implements it. While it's possible that we won't find a solution that all of us fully agree with, we should still make an effort to facilitate / formalize a decision here before diving into the code to do it. I think we should take a step back here and instead continue the conversation in the issue. What concrete problems are we trying to solve here, and how would individual test suites per plugin address them? |
|
@joemcgill Responding to #1065 (review) with my 2 cents:
To me the big benefits are keeping the tests closer to the code being tested improves the DX. Instead of there being two
Since the plugins are standalone, is this necessary? If it is, the plugin bootstrap code for an individual plugin could still load the other plugins.
Can't this shared data simply be stored up in
I'm not sure given that the plugins are now intended to be standalone, as I shared above. It seems that them running in isolation makes more sense as the default state rather than them all running together. But it would still be good to also be able to test them all running together, but perhaps that would be enabled via some environment variable? And given that standalone plugins are indeed standalone, this makes sense to me as well that there would be a separate |
Agree, and initially, it was worked as a POC to show and tell its working. The new PR just cherry-picked those changes from POC.
I think the same is also true for running them in isolation since they are standalone. Testing these plugins with each other should be part of integration tests IMO. |
|
@westonruter @thelovekesh I agree with some of your points. Most importantly, +1 on that the PHPUnit tests should be focused on the standalone plugins, on their own (standalone). So I agree from a functionality perspective. However, that doesn't mandate a specific folder structure. One of the benefits of a monorepo is that configuration can be reused, instead of being duplicated in every single plugin. This duplication results in a maintenance burden, such as having to update all N copies of a file with the same change once something in the workflow requires an update. All the "dotfiles" (config files) that individual GitHub repos typically come with, we shouldn't try to recreate that in every single plugin folder. At this point we're defeating the purpose of a monorepo, other than not having separate repos to maintain. I don't feel strongly about where we locate the actual test directories, whether centrally in But I feel quite strongly about not duplicating the PHPUnit config files for every single plugin, as they look the same everywhere. With PHPCS, we found a good solution as its syntax allows loading from another main config. It's a bummer PHPUnit doesn't seem to support this. But so far there has not been any requirement outlined that would actually justify a duplication because of distinct changes. I think as long as that's the case, having separate config files is only a theoretical benefit, resulting in only a maintenance burden via duplicate code. |
@felixarntz I agree that we should try to reuse the config, but it also depends on whether the tools provide that flexibility. In the case of PHPCS we had that but as you mentioned PHPUnit does not provide it.
I'm having difficulty envisioning a scenario where updating all configuration files would be required. I don't think tool configurations are updated frequently. If we are talking about such cases here, then it can also be true for PHPCS config. What if PHPCS changes the way how we extend configs? What if the Also would be happy to know what type of duplication we want to eliminate from
I don't think it's fair to call it a defeat of the purpose just from the PHPUnit config POV since mono-repos are subject to complex workflows, especially with toolings. |
Probably not, but they do sometimes require updates. PHPUnit in particular moves much faster than e.g. WordPress and new versions often have breaking changes, including requirements in the config files. Anyway, as mentioned before my point is that still nowhere in this issue or the PR I have seen mentioned any concrete use-case on where those configurations need to differ. Without a real reason, I don't see why we would want to separate the config files as it only addresses a theoretical concern.
It is. But I don't think it's worth discussing that, since PHPCS fortunately provides a way for us to reference another config. Plus, for WPCS we do have a real concern, such as the
Almost the entire config is duplicated, as all config files in your PR for instance look the same, except for the plugin reference. So unless we can reference a single main config from other config files, this creates a maintenance burden. |
I think that's true for its APIs and not for configs. Particularly in https://github.com/WordPress/wordpress-develop/commits/trunk/phpunit.xml.dist, I can't find any major changes in the config schema.
Here are some specific situations where these changes could be really helpful:
These are just some examples of why these changes would be helpful.
I hold a different viewpoint on this matter. The only duplicated aspect seems to be the PHPUnit arguments, and it's improbable that they'll undergo frequent changes. In the past 11 years, the WP Core config has only added new arguments once, and that was three years ago. <phpunit
bootstrap="tests/bootstrap.php"
backupGlobals="false"
colors="true"
convertErrorsToExceptions="true"
convertNoticesToExceptions="true"
convertWarningsToExceptions="true"
defaultTestSuite="default"
>
</phpunit>The rest of the setup has been moved from the main PHPUnit configuration to the configurations for each plugin. So, any changes we make in the future would affect both the main setup and the individual plugin setups. Whether we keep them together or separate, updating them will require changes in multiple spots, making the maintenance effort similar either way. |
|
Just to note that Jetpack is also a monorepo of plugins and each plugin has its own |
|
Reading back through this conversation, I guess I have a bit of a different perspective on how we intend to use this repo and how we prioritize tradeoffs between treating each plugin as an individual unit vs thinking of these as a collection of features that are intended to be developed and used together, but happen to be published in a manner that allows individual use. My perspective is that the Performance Lab program exists primarily to allow us to support developing and testing performance enhancements for WordPress in a collective manner, rather than as individual efforts. The benefits that we get from this strategy are many—including being able to share tooling, processes, configuration, and visibility of the projects being supported by the WordPress Performance Team. The fact that we've decided to implement and publish experimental features in the form of standalone plugins is merely tactical, as a result of #618, and not a primary objective of this program. Understandably, this does lead to some confusion if the expectation is that each standalone plugin should be treated as an isolated unit that is unrelated to the rest of the repo. For example, from @westonruter's earlier comment:
This is not the primary intended development experience of this repo, IMO. Instead, the I believe the DX that should be prioritized is one where you are primarily working from the root directory of the repo, and running a development environment that is configured to automatically load all of the plugins from this repo (usually using the included As such, I think having a shared set of Unit Tests for the entire codebase gives us the opportunity to take advantage of shared test functionality and test data across the test classes that cover different individual plugins developed within this repo rather than needing to duplicate data and test helpers across different plugins. For example, we have several plugins that implement different features related to image performance. Being able to share test data and develop shared test helpers, traits, etc. makes creating tests for these types of plugins easier. Ultimately, if the preference is that we prioritize the DX for working on standalone plugins in isolation, then I think we may want to consider whether and to what extent each plugin should define its own dependencies, configuration, tooling, etc. rather than trying to manage some at the repo level and some at the plugin level. |
We can still do this even when the unit tests are located with each individual plugin, even when there are separate configs. The test helpers can be located in a root <?php
require_once __DIR__ . '/../../../tests/helpers.php';
// ... |
|
Thanks @joemcgill.
Viewed at a high level, these plugins represent performance-focused features, yet each holds its unique functionality at a granular level. For instance, WebP Uploads handles image generation, while Optimization Detective specializes in optimization tasks. Given their standalone nature, they deserve development flexibility. Integration, if required, should be managed through integration tests, maintaining both individual integrity and collaborative cohesion. At a detailed level, the flexibility introduced by #1065 encompasses all possible combinations. For instance, if plugin X relies on Y, simply include Y while testing X in its bootstrap file. This approach also facilitates testing Unit Tests for X independently and integration tests separately with the Y plugin enabled, offering comprehensive flexibility.
In any scenario, I believe that none of the Developer Experience (DX) initiatives affect this; instead, they streamline processes for improved efficiency.
There seems to be a misunderstanding regarding how we approached extended configuration for PHPCS, and this misunderstanding is being generalized to other tools, which may not apply uniformly due to their differing natures and functionalities. For instance, PHPCS is primarily designed to import rulesets, allowing CS packages to expose their sniffs to the PHPCS runner. Conversely, in PHPUnit, the configuration intended for importation, or sharing does not revolve around its XML configuration but rather involves extensions and test runner wrappers. Even though PHPUnit can share configurations when needed, we must recognize the unique features of each tool. Right now, we're using shared tooling, and we can adapt as necessary.
Our approach to single-plugin functionality within modules was effective, but the transition to a mono repo presents an opportunity to refine our performance features with a more concentrated focus. Similar to the streamlined management in a monorepo, we can now optimize our development process for greater efficiency and effectiveness. |
|
@thelovekesh @joemcgill @westonruter I have spent some time looking around at other projects in regards to the points that are being discussed here, which has reshaped my perspective a little. Sharing a few thoughts here:
It's worth noting that modules were just as much standalone as the plugins are now, this was a mandate from the beginning of the repository. So we were using shared tooling from the beginning, while the production codebases of the modules (now plugins) were not allowed to rely on anything external.
+1 to that. Some tools are more limited than others in catering for the workflows we would want to have. I think when we run into such limitations, we have three options:
With the latter, we also have to consider many other factors, such as how well another tool is maintained, familiarity of the WordPress developer community with it, how appropriate it is for the job etc. Most importantly though, I think we are getting stuck at this point in tooling discussions, but we have not even determined as a team what our developer experience should holistically look like. An example is that some of us suggest developing from a specific plugin's directory, while others suggest developing from the repository root. Those are entirely different approaches we need to discuss more. Potentially we can later find a tooling approach that works for both. What are the benefits of one or the other? Do we want to optimize for one or the other? I do agree with @joemcgill's assessment that this project and monorepo is a collective effort. So we should not just have individual plugin directories where the maintainers of said plugin can then implement anything in any way they like. A shared approach and, to some degree, shared tooling is crucial here. That said, this does not necessarily mean we can't have individual plugin specific tooling. But I think we need to think about tooling collectively. We're already doing that in some instances. Having a central PHPCS config while also having a bare minimum PHPCS config per plugin which mostly imports the central one is a great solution that caters for both the collective and individual needs. A more complicated example is PHPUnit (see #1065), where the PHPCS approach is not really feasible as the config files cannot be composed. Seeing that several other popular monorepo projects (e.g. https://github.com/symfony/symfony or https://github.com/Automattic/jetpack) are using I realize I now mentioned specific examples here, though that was mostly to clarify my overarching perspective. Let's take a step back and define which kind of developer workflows we want to encourage and optimize for, holistically, rather than discussing individual tooling concerns without having defined the bigger picture. |
If we do facilitate developing with the working directory being the plugin directory, this doesn't mean that the ability to develop from the root would have to be sacrificed. I think both should be facilitated. But IMO, it makes more sense to optimize for developing with the standalone plugin as the working directory (e.g. running |
I'm in favor of making it work both ways, as we can't mandate developers to work from a specific root or plugin directory; it's entirely their choice. Adopting the NPM script runner can address this issue promptly, ensuring equal functionality in both scenarios. By standardizing our scripts in NPM instead of Composer, we can achieve this goal. While I acknowledge that multiple scripts might result in a bloated package.json file, we can explore Joe's suggestion of consolidating commands and passing arguments to target specific plugin(s), thereby maintaining simplicity with a single command. Example:
While we can do so, is it truly necessary? Considering that we already have an executable binary for each tool in EDIT: Seems like I just deleted the previous comment history which was an edit in above commands example. |
@westonruter We can accomplish this in both scenarios by utilizing NPM as a script runner. You can refer to the examples provided above. Alternatively, those who prefer not to use an NPM run the command can execute binaries directly, such as |
|
@felixarntz Just checking if we have any updates here to unblock #1065. |
|
@thelovekesh I think we need to further discuss what we actually want here. Kind of following up on #1012 (comment), I think we should take a step back from further developing towards a solution we haven't even defined what it should look like. The major overarching question to answer here is: Is our encouraged development workflow from the repository's root directory, or from the individual plugin directories? Making a decision on that will guide our current and future engineering decisions. While ideally we would be able to cater equally for both, we're already seeing in several instances that that is either not possible or at least requiring complicated workarounds that lead to further discussions because they may seem hacky. Additionally, catering for both, in part because of those complexities, will require more time and resource investment and thus distract us from what we actually should be doing - work on performance features for the end users. Let's discuss the tradeoffs between the two development approaches. After considering these, we should make a decision on what is our encouraged and formally supported workflow. From there, we can implement accordingly. |
|
Catching up after my long leave, any updates in regards to this issue? I saw #1262 got merged, which is fair, though it seems that #1262 (comment) still needs to be addressed. Anything else that happened related to this topic but outside this issue? Curious for any updates @joemcgill @westonruter @thelovekesh |
|
@felixarntz Welcome back 🎉 A couple other PRs that are related to this issue: These make it much easier to run the tests for a single standalone plugin. Also related is: This helper trait is in the shared
The Enhanced Responsive Images (auto-sizes) plugin now bootstraps Image Prioritizer and Optimization Detective when it runs its tests since when that plugin is active it has additional integrations (#1322). The same is true for Embed Optimizer which bootstraps Optimization Detective (#1302). And of course Image Prioritizer has to bootstrap Optimization Detective since there is a plugin dependency. Other than these cases, we don't have other situations where different combinations of plugins are tested. But it seems these could be added in I think we can close this issue as completed. |
And yet, see #1433 (comment). |
|
Left a reply in #1433 (comment). I feel like generic tests for a "random" combination of our plugins active wouldn't make to much sense from a PHPUnit perspective. IMO this would be mostly about ensuring there's no fatal error from the mere presence of those plugins together. For more specific scenarios of testing a plugin with its dependency, I think those are still first and foremost testing of that plugin and therefore make more sense in the plugin's own tests directory, maybe as a second test suite with the dependency plugin active. On another note, +1 to closing this issue as completed. |
Absolutely! Ideally, if plugin Foo is designed to work with plugin Bar, it should have two test suites:
With our current setup, it would be challenging to test things this way because we only have a single bootstrap and PHPUnit configuration file. Having an independent PHPUnit config and bootstrap file for each plugin, located in its own test directory, is something to consider in such cases. |
As per #654, we are moving published modules as standalone plugins, making the current repo a mono repo for multiple plugins.
This brings a new challenge to streamline the current tooling and build setup used to maintain coding standards, testing, and publishing the plugin. With this, we are exploring integrating existing workflows in such a way that it doesn't hurt the DX and keeps the maintenance cost low.
Tasklist
phpcs.xml.distFiles for Each Plugin to Isolate Text Domains #997 (Separatephpcs.xml.distFiles for Each Plugin to Isolate Text Domains #1002)pluginsfolder for standalone plugins #934 (comment) and Introducepluginsfolder for standalone plugins #934 (comment)). Testsuite-based setup was introduced in Add test suite for Plugins folder #956.The text was updated successfully, but these errors were encountered: