Workflows: Performance: Run suite atop latest stable major WordPress version #32244
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +427 B (0%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
|
(thanks to @priethor for spotting typos in my Bash-inside-JS snippet!) |
|
I'm not sure we need this 😬 I don't think that the Core version set in As @youknowriad said in Slack,
Aside, setting the Core version to |
mcsf
force-pushed
the
fix/wp-env-base-wp-install
branch
from
1831ec7 to
ef363d0
Compare
mcsf
changed the title
mcsf
changed the title
|
I like the current approach of this PR (an option to define the base WP to use) |
In that case, and also based on your comment in #32277, I've marked it ready for review. :) |
mcsf
force-pushed
the
fix/wp-env-base-wp-install
branch
from
ef363d0 to
8836392
Compare
... by patching the temporary test environment's `.wp-env.json` config.
mcsf
force-pushed
the
fix/wp-env-base-wp-install
branch
from
35ef36e to
9d58030
Compare
priethor
reviewed
May 28, 2021
ockham
reviewed
May 28, 2021
ockham
reviewed
May 28, 2021
bin/plugin/commands/performance.js
Outdated
Comment on lines
272
to
273
| // Only use the major version, i.e. 5.7.2 becomes 5.7. | ||
| const major = options.wpVersion.split( '.' ).slice( 0, 2 ).join( '.' ); |
There was a problem hiding this comment.
How about leaving this to the caller? I think it's arguably not this script's responsibility (plus we're not really documenting it in bin/plugin/cli.js).
There was a problem hiding this comment.
This was me taking a shortcut because of how ZIP files are named on the server. See 1217cff, in which I did things correctly. :)
ockham
reviewed
May 28, 2021
bin/plugin/commands/performance.js
Outdated
Comment on lines
276
to
278
| await runShellScript( | ||
| `sed -I '' -e '/"core":/s/WordPress\\/WordPress/${ zipUrl }/' "${ environmentDirectory }/.wp-env.json"` | ||
| ); |
There was a problem hiding this comment.
We could use jq -- or just plain JS -- to modify the JSON file here 😬
There was a problem hiding this comment.
or just plain JS
True, true. 😅 How about now?
ockham
reviewed
May 31, 2021
Comment on lines
+266
to
+272
| // In order to match the topology of ZIP files at wp.org, remap .0 | ||
| // patch versions to major versions: | ||
| // | ||
| // 5.7 -> 5.7 (unchanged) | ||
| // 5.7.0 -> 5.7 (changed) | ||
| // 5.7.2 -> 5.7.2 (unchanged) | ||
| const zipVersion = options.wpVersion.replace( /^(\d+\.\d+).0/, '$1' ); |
There was a problem hiding this comment.
IMO, we could drop this altogether and make it the caller's concern to choose the right version format (mostly thinking YAGNI here, since our only use case is guaranteed to use the correct format). Not a big deal tho, I don't mind having it here.
There was a problem hiding this comment.
True, though in this case I feel it would open the door for mistakes, because someone operating the --wp-version flag doesn't need to know that the flag results in a specific ZIP file being pulled later by wp-env. --wp-version 5.7.0 should be an accepted value, so let's ensure it doesn't break.
ockham
approved these changes
May 31, 2021
7 tasks
Thanks for the feedback! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Potential alternative to #32277
Description
Right now our Run performance test GitHub Action is failing, presumably because it is trying to run Gutenberg's performance test suite on top of WordPress trunk. Since we have just ported all WP5.8-related pieces to WordPress trunk (#31322), the Gutenberg plugin is temporarily in a state of conflict with WordPress trunk.
This PR simply changes the default setting in our.wp-env.jsonconfiguration so that it will prefer to run on whichever is the latest stable release of WordPress (as of today,5.7.x). I thought this might be a saner and more stable configuration anyway, regardless of performance testing.This PR:
--wp-versionoption to theperformance-testsCLI command--wp-version.How has this been tested?
Checklist:
*.native.jsfiles for terms that need renaming or removal).