-
Notifications
You must be signed in to change notification settings - Fork 110
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
Move PL plugin under plugins/performance-lab
#1182
Conversation
16605b4
to
5f9587e
Compare
* Description: The Performance Monorepo is not a plugin rather a collection of performance features as plugins. Download <a href="https://wordpress.org/plugins/performance-lab/" to install performance features instead. | ||
* Author: WordPress Performance Team | ||
* Author URI: https://make.wordpress.org/performance/ | ||
* Text Domain: performance-lab |
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.
Initially, I kept it performance
but tests have performance-lab
text domain so switched it. But keeping it performance-lab
doesn't seem right given it's already taken by the PL plugin.
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 like the idea of providing some feedback to anyone who tries to install the whole repo. I think using the performance-lab
text domain is likely fine. I wonder if we could even automatically load the root file for the actual Performance Lab plugin if someone ends up in this situation?
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.
@joemcgill For users utilizing wp-env, it should function as expected. However, for those with custom setups, we should include documentation on how to work with the monorepo. Simply activating the PL plugin isn't sufficient since we need to provide references to other plugins as well. It could also be confusing if one plugin from the /plugins
directory is loaded while others are not.
@westonruter Any thoughts 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.
We're currently lacking such documentation for users who want to work on the standalone plugins as well, so I don't see this as a blocker.
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've updated notices in 8cbac0c with a link to the handbook page. We can add documentation there for how to handle contributing when not using wp-env.
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.
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 think this is a good step. Thanks.
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.
Looking good!
tests/bootstrap.php
Outdated
}, | ||
1 | ||
); | ||
if ( 'performance-lab' === $plugin_name ) { |
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.
Doesn't the --testsuite
need to always be supplied now? If $plugi_bane
is empty, shouldn't an error occur?
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 have set the default test suite to performance-lab
. See 7a5dea4
29e4657
to
5c11950
Compare
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. |
Let's prioritize merging this right after the 3.1.0 release. |
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 dig this idea and it's looking really good. I've added some feedback inline.
.github/workflows/deploy-plugins.yml
Outdated
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.
We can't test this workflow in debug mode until after this PR is merged, so let's make sure to check that this works once after merging.
* Description: The Performance Monorepo is not a plugin rather a collection of performance features as plugins. Download <a href="https://wordpress.org/plugins/performance-lab/" to install performance features instead. | ||
* Author: WordPress Performance Team | ||
* Author URI: https://make.wordpress.org/performance/ | ||
* Text Domain: performance-lab |
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 like the idea of providing some feedback to anyone who tries to install the whole repo. I think using the performance-lab
text domain is likely fine. I wonder if we could even automatically load the root file for the actual Performance Lab plugin if someone ends up in this situation?
7a5dea4
to
af1b602
Compare
c9f44d0
to
0596b06
Compare
In my opinion, the contents of |
@thelovekesh Agreed. Let's eliminate all of these lines: Lines 3 to 43 in be4c00a
|
- plugins | ||
- tests | ||
- performance.php | ||
- plugins/performance-lab/load.php |
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 line is redundant since it is already inside the plugins
directory now:
- plugins/performance-lab/load.php |
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.
@westonruter If I remove this, I got the following error:
------ ------------------------------------------------------------------------------------------------
Line tests/plugins/performance-lab/load-tests.php
------ ------------------------------------------------------------------------------------------------
:35 Call to method PHPUnit\Framework\Assert::assertFalse() with int will always evaluate to false.
:50 Call to method PHPUnit\Framework\Assert::assertFalse() with int will always evaluate to false.
:73 Call to method PHPUnit\Framework\Assert::assertFalse() with int will always evaluate to false.
:144 Call to method PHPUnit\Framework\Assert::assertFalse() with int will always evaluate to false.
------ ------------------------------------------------------------------------------------------------
I think it has something to do with the file loading order.
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.
Interesting. I don't get that error with that line removed.
$ npm run phpstan
> phpstan
> composer phpstan
> phpstan analyse --memory-limit=2048M
Note: Using configuration file /home/westonruter/repos/performance/phpstan.neon.dist.
107/107 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
[OK] No errors
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.
Interesting. Which version are you on?
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.
1.11.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.
Can you try with --debug
flag once since it disables the cache? Also, it was failing in CI as well - https://github.com/WordPress/performance/actions/runs/9111313232/job/25048167246
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.
Humm. No errors when passing --debug
either.
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.
🤔
No configs override with phpstan.neon
in the root?
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.
No 😞
$ npm run phpstan -- -- --debug
> phpstan
> composer phpstan -- --debug
> phpstan analyse --memory-limit=2048M '--debug'
! [NOTE] The Xdebug PHP extension is active, but "--xdebug" is not used.
! The process was restarted and it will not halt at breakpoints.
! Use "--xdebug" if you want to halt at breakpoints.
Note: Using configuration file /home/westonruter/repos/performance/phpstan.neon.dist.
/home/westonruter/repos/performance/plugins/optimization-detective/class-od-data-validation-exception.php
...
/home/westonruter/repos/performance/performance.php
[OK] No errors
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.
Failing in wp-env
as well with and without XDebug.
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.
Great work and great changes! 🎉
Co-authored-by: Weston Ruter <westonruter@google.com>
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 think we should go ahead and merge this. I think there's some follow-up work we need to do to make it easier to run PHPUnit locally, but that can be handled in a follow-up issue.
Summary
Fixes #1165
Relevant technical choices