-
Notifications
You must be signed in to change notification settings - Fork 101
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
Add / #639 Standalone Built Plugin Tests #671
Add / #639 Standalone Built Plugin Tests #671
Conversation
…uild prior to unit testing of each.
…dPress plugin assets
…s fail accordingly
…gnal success to test pipeline, although should not be too necessary it is a safety net
…idate test pipeline failure
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.
Thanks @10upsimon, Great start. Left some nit-pick feedback.
@@ -0,0 +1,58 @@ | |||
name: Standalone Plugin Integration Testing | |||
|
|||
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.
Does this workflow only work when the plugins.json
file is updated?
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.
@mukeshpanchal27 for now, it runs at the same time php unit tests run, but this will change once I have approval on the process itself. Thank you!
Thanks @10upsimon for the thorough writeup of the approach you've chosen to take here. There's a lot to dig into, so before getting too deep into details, I'd like to share some initial observations and questions.
|
@joemcgill many thanks for your early review feedback, to address your points:
I believe we should be running tests in 2x scenarios:
To be clear,
This is not the case. This approach builds a plugin string of all plugins that reside in the Regarding running separate unit tests for all plugins, this is currently handled by the PL plugin which runs tests against each module. Will it not always be the case that we'll maintain the PL plugin and handle standalone plugins as builds therefrom, i.e via
I could not get it to work correctly without having the PL plugin active as well (which already sets up the prerequisites for tests) and I think forcing the presence of the PL plugin is the incorrect path to take. That being said, I could attempt to simply run Thanks for your thoughtful feedback @joemcgill , much appreciated! |
Thanks for the update, @10upsimon, I had definitely misunderstood part of how this was working, in terms of spinning up separate environments for each plugin. I've done some local testing and things are much more clear. I'll consolidate any additional feedback in a proper code review soon. |
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.
@10upsimon This looks great to me! Three nit-picks below, I only have one broader point of feedback for consideration: I think the approach of reading .wp-env.json
and replacing the plugins
entry in it is a creating solution, however it may be simpler to create a static .wp-env.json
for this purpose in another location (e.g. in plugin-tests/.wp-env.json
) with the plugins already in it, since the list of plugins we already know from the beginning (see the plugins.json
file), so I don't think we technically have to "detect" them from the build
directory. Your approach here would allow to e.g. only manually build one plugin and then the test code would only test that one - which may be useful, but at the same time I wonder whether that'll ever be used, given that we would always want to have standalone plugins available for all plugins in the plugins.json
configuration.
I'm curious what you think about it and what your rationale for the approach was. Definitely not saying that we have to change it, I'm just pointing out an alternative solution that we can consider.
.gitattributes
Outdated
@@ -30,3 +30,5 @@ | |||
|
|||
/.gitattributes export-ignore | |||
/.gitignore export-ignore | |||
|
|||
/plugin-tests export-ignore |
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.
Nit-pick, maybe put this exclude together with the other directories that contain tooling code (lines 8-10 above).
bin/plugin/commands/test-plugins.js
Outdated
// Buffer .wp-env.json content var. | ||
let wpEnvFileContent = ''; | ||
|
||
try { | ||
wpEnvFileContent = fs.readFileSync( wpEnvFile, 'utf-8' ); | ||
} catch ( e ) { | ||
log( formats.error( `Error reading file "${ wpEnvFile }": "${ e }"` ) ); | ||
|
||
// Return with exit code 1 to trigger a failure in the test pipeline. | ||
process.exit( 1 ); | ||
} | ||
|
||
// If the contents of the file were incorrectly read or exception was not captured and value is blank, abort. | ||
if ( '' === wpEnvFileContent ) { | ||
log( | ||
formats.error( | ||
`File content for "${ wpEnvFile }" is empty, aborting.` | ||
) | ||
); | ||
|
||
// Return with exit code 1 to trigger a failure in the test pipeline. | ||
process.exit( 1 ); | ||
} | ||
|
||
// If we do not have a match on the wp-env enabled plugins regex, abort. | ||
if ( ! wpEnvPluginsRegex.test( wpEnvFileContent ) ) { | ||
log( | ||
formats.error( | ||
`Unable to find plugins property/key in WP Env config file: "${ wpEnvFile }". Please ensure that it is present and try agagin.` | ||
) | ||
); | ||
|
||
// Return with exit code 1 to trigger a failure in the test pipeline. | ||
process.exit( 1 ); | ||
} | ||
|
||
// Let the user know we're re-writing the .wp-env.json file. | ||
log( formats.success( `Rewriting plugins property in ${ wpEnvFile }` ) ); | ||
|
||
// Attempt replacement of the plugins property in .wp-env.json file to match built plugins. | ||
try { | ||
// Create plugins property from built plugins. | ||
wpEnvPluginsRegexReplacement = `"plugins": [ "${ builtPlugins | ||
.map( ( item ) => `${ builtPluginsDir }${ item }` ) | ||
.join( '", "' ) }" ],`; | ||
|
||
fs.writeFileSync( | ||
wpEnvFile, | ||
wpEnvFileContent.replace( | ||
wpEnvPluginsRegex, | ||
wpEnvPluginsRegexReplacement | ||
) | ||
); | ||
} catch ( e ) { | ||
log( | ||
formats.error( | ||
`Error replacing content in ${ wpEnvFile } using regex "${ wpEnvPluginsRegex }": "${ e }"` | ||
) | ||
); | ||
|
||
// Return with exit code 1 to trigger a failure in the test pipeline. | ||
process.exit( 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.
Maybe move this code into its own function? The handler function is extremely long, and I think especially this here is pretty much one logical step that could be broken apart into its own function unit.
bin/plugin/commands/test-plugins.js
Outdated
} ); | ||
|
||
// Run tests per plugin. | ||
log( formats.success( `About to execute standalone plugins tests.` ) ); |
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 output is probably unnecessary :)
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.
Thanks @10upsimon, I left some findings and feedback.
bin/plugin/commands/test-plugins.js
Outdated
* External dependencies | ||
*/ | ||
const fs = require( 'fs-extra' ); | ||
const { execSync, spawnSync } = require( 'node:child_process' ); |
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.
const { execSync, spawnSync } = require( 'node:child_process' ); | |
const { execSync, spawnSync } = require( 'child_process' ); |
When i run command in local environment it show me Error: Cannot find module 'node:child_process'
error. After applying the changes it working for me.
bin/plugin/commands/test-plugins.js
Outdated
process.exit( 1 ); | ||
} | ||
|
||
// Only try to read plugin dirs if build ir exists. |
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.
// Only try to read plugin dirs if build ir exists. | |
// Only try to read plugin dirs if build is exists. |
bin/plugin/commands/test-plugins.js
Outdated
|
||
// Copy over test files. | ||
try { | ||
fs.copySync( pluginTestAssets, `./build/${ 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.
fs.copySync( pluginTestAssets, `./build/${ plugin }/`, { | |
fs.copySync( pluginTestAssets, `${ builtPluginsDir }${ plugin }/`, { |
Use already define variable.
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.
A couple small suggestions, but after reading all this again, I think the approach makes sense and is looking good. One other thing I'd like to confirm is whether running these tests during a deployment workflow (see: #638) has the potential of polluting the build artifacts that would be uploaded to the SVN repo, given that we're copying certain files into the plugin folder in order to run these tests.
bin/plugin/commands/test-plugins.js
Outdated
// Attempt replacement of the plugins property in .wp-env.json file to match built plugins. | ||
try { | ||
// Create plugins property from built plugins. | ||
wpEnvPluginsRegexReplacement = `"plugins": [ "${ builtPlugins | ||
.map( ( item ) => `${ builtPluginsDir }${ item }` ) | ||
.join( '", "' ) }" ],`; | ||
|
||
fs.writeFileSync( | ||
wpEnvFile, | ||
wpEnvFileContent.replace( | ||
wpEnvPluginsRegex, | ||
wpEnvPluginsRegexReplacement | ||
) | ||
); | ||
} catch ( e ) { | ||
log( | ||
formats.error( | ||
`Error replacing content in ${ wpEnvFile } using regex "${ wpEnvPluginsRegex }": "${ e }"` | ||
) | ||
); |
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.
Rather than overwriting the wp-env.json
file, we can create a new wp-env.override.json
file, which we could also optionally add to the .gitignore
so anyone running this locally won't accidentally have changes in their working branch.
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.
Thanks @joemcgill , taking a look at this as it seems a necessary change to me 👍
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 this has been implemented in a recent commit.
plugin-tests/tests/bootstrap.php
Outdated
$GLOBALS['wp_tests_options'] = array( | ||
'active_plugins' => array( basename( TESTS_PLUGIN_DIR ) . '/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.
Do we also need to optionally load the main Plugin Lab plugin as well, in order to ensure there are no fatals due to both being loaded at the same time, or is that out of scope for this specific issue?
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 I figured this was out of the scope of this task/issue, but given that we now have a working ability to run them side by side, maybe we should add this test case?
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.
Yes I think that it's a good idea that we have the ability to do both, but let's handle that in a separate issue so it doesn't block progress 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.
@joemcgill this has been handled in a recent commit.
plugin-tests/composer.json
Outdated
"require-dev": { | ||
"dealerdirect/phpcodesniffer-composer-installer": "^0.7", | ||
"phpcompatibility/php-compatibility": "^9.3", | ||
"phpunit/phpunit": "^4|^5|^6|^7|^8|^9", |
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.
Fun fact: PHPUnit doesn't need to be required if we're using yoast/phpunit-polyfills
, because it requires PHPUnit as a dependency itself and handles proper versioning for you.
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 have removed, thanks. We should probably consider this rule for the main plugin, as that is where I borrowed the file from.
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.
Agreed! It's some thing that we could check for in all of our plug-ins that use the polyfills library
…Press/performance into feature/creating-standalone-plugins
…alone-built-plugin-tests
…d excluded from version control
@joemcgill @felixarntz @mukeshpanchal27 this is ready for a final review I believe. I've requested a re-review from all 3 of you. Notable changes:
The only outstanding item is a full and proper implementation of the GitHub workflow rules for the changed files. If we're happy with the rest of the code, I can work on that as a final deliverable. Thank you. |
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.
Thanks Simon! Looks like it's working now. Left a couple additional comments, but nothing blocking.
// Create an array of plugins from entries in plugins JSON file. | ||
builtPlugins = Object.keys( pluginsJsonFileContentAsJson ) | ||
.filter( ( item ) => { | ||
if ( ! fs.pathExistsSync( `${ settings.builtPluginsDir }${ pluginsJsonFileContentAsJson[ item ].slug }` ) ) { |
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.
Do we need to add any handling here in case the builtPluginsDir
value does not end in a slash?
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 I do not think this is absolutely necessary, as this is not a dynamically defined value, we hard define it as part of the settings object we pass to doRunStandalonePluginTests()
. We can potentially handle this in a future PR if the need arises?
bin/plugin/commands/test-plugins.js
Outdated
process.exit( 1 ); | ||
} | ||
|
||
// Copy the newly modified ./plugins-tests/.wp-env.json file to the root level. |
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.
Why not write directly to this file rather than writing to the original and then copying?
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 we have to copy it regardless the first time, but I agree that a check for file existing and then a copy over and final write is probably best, as if it's existing already, we may as well write to it. To be clear, it won't exist already as we need to create this overrides file, so I am not actually sure how much of a difference this makes, but I am going to effect this in case this os ever enhanced to handle multiple writes (which it is currently but you hav further feedback in this review to change that).
`Re-running standalone plugin integration tests with WPP plugin active.` | ||
) | ||
); | ||
// Add the root level WPP plugin to the built plugins array. |
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 works, but we could probably speed this up a bit by doing the first run with the WPP plugin active and then using a WP CLI command to disable it for the second run. That way we don't have to worry about restarting the environment.
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 suggestion, I am in the process of changing this.
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 this has been refactored in the latest commit at the time of writing, please see my general update comment further down.
… and activate plugins ability as part of running tests to optimize the overal process
@joemcgill @mukeshpanchal27 I've pushed a round of changes, all of which can be seen in the commit at f485ccf I've refactored a few areas, but high level takeaways are:
|
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.
Nice updates @10upsimon , this is looking good, and I can confirm that everything is working as expected locally. The only thing that I noticed while testing this command locally that does not exceed affect our GitHub workflow is that the performancePluginSlug
value may be incorrect if somebody has cloned this repo to a folder named something other than "performance". Unless that's an easy fix here, I think we could dress that has an enhancement later.
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.
@10upsimon Code-wise this looks pretty much good to go, however I found two things we should address, both related to running locally. We need to also make sure this works smoothly locally, not only in the CI workflow.
Nit-pick, one thing in your PR description is probably out of date now? See this:
HOWEVER, be aware that this command does change the
plugins
property of the.wp-env.json
file to reference the built plugins, and removes the reference to the top levelperformance
plugin.
I believe this is no longer accurate, maybe from a previous iteration of the PR? We can probably remove it now that the code here uses the .wp-env.override.json` file.
…nv overrides file as part of cleanup operations.
@joemcgill @felixarntz thanks for the review, I've addressed final pieces of feedback in e36d516
|
…be executed immediately after spinning up 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.
Approving again. This is working great locally now too. 🎉
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.
Thank you @10upsimon for the last updates, great work!
Summary
Fixes #639
Relevant technical choices
Introduction of a new
test-plugins
cli commandThis CLI command is an extension of the existing
./bin/plugin/cli.js
entry point and can be execute in one of two manners;npm run test-plugins
for tests in a single site environmentnpm run test-plugins-multisite
for tests in a multisite environment./bin/plugin/cli.js test-plugins
for single site site testing./bin/plugin/cli.js test-plugins --sitetype=multi
for multisite testingThe former 2 is the preferred means, and the means used by the Github workflow that executes standalone plugin integration testing.
Why a
test-plugins
cli command as opposed to just Github Workflow set of steps?Authors may want to validate standalone plugins locally following changes to one or more of the modules. A cli command off of the existing
./bin/plugin/cli.js
seemed like a logical and accessible choice and flows naturally from the recently introducedbuild-plugins
command.test-plugins
logic overviewThe logical flow undertaken by the script is as follows:
.wp-env.json
file is performed as this is critical to the logic that follows. If one is not present, a suitable error is produced and an exit with code1
is performed to ensure that the test fails.build
directory is checked. If nobuild
directory is found, a suitable error is produced and an exit with code1
is performed to ensure that the test pipeline fails. Users should runnpm run build-plugins
as a prerequisite to the standalone plugin integration tests. This step is however performed as part of the Github workflow introduced in this PR.build
dir is then looped over and the steps that follow are performed.plugin-tests
folder are copied to each standalone plugin directory. These include a suitablecomposer.json
file to install necessary testing dependencies, atests
folder containing a single test suite used for all standalone plugins andphpunit.xml.dist
used for all standalone plugins, that defines the test(s) to execute per plugin. All plugins perform the same tests, so updating the tests within theplugin-tests/tests
folder will have bearing on all built plugins.composer install --no-interaction
is performed against each plugin directory within thebuild
directory. This is handled by setting the--working-dir
argument of the command to reference the suitable plugin within the loop..wp-env.json
file withinplugin-test
is update and itsplugins
property update to an array reflective of the plugins inside of thebuild
directory. It is then copied to the root directory. This ensures that when we start the wp-env environment, all built plugins are already active. This array of plugins within the.wp-env.json
file does not include the top levelperformance
plugin.wp-env
environment is started. This command is executed synchronously.npm run pretest-php
is executed in order for the applicable packages at a top level to be installed.wp-env run phpunit 'phpunit -c /var/www/html/wp-content/plugins/${ plugin }/phpunit.xml.dist --verbose --testdox'
where${plugin}
is the applicable plugin in the loop. This command is executed synchronously.1
, the process is halted and an overall exit with code1
is returned, forcing a fail on the test pipeline.wp-env
environment is stopped and an exit with code0
is performed. This signals a successful execution of the test cycle. At this stage, the integration tests for all plugins would have passedOK
.Why does the
test-plugins
command handlewp-env start
?Because we need to first ensure that the
.wp-env.json
file contains valid plugin pointers to the plugins within thebuild
directory or else there would be no way to activate them as the environment would be unaware of their existence. Further to this, at time of authoring this code it is not possible to run the standalone plugins alongside the performance plugin.What if there are I/O operation errors?
All I/O operations are executed synchronously and with suitable exception catching. Exits with code
1
are performed, therefore producing a suitable error message, and failing the test pipeline.Can I test plugin manually outside of the Github Workflow?
Yes, you're encouraged to. As long as you've run
npm run build-plugins
first, runningnpm run test-plugins
should result in a successful execution of the integration test suite for all standalone plugins inbuild
. HOWEVER, be aware that this command does change theplugins
property of the.wp-env.json
file to reference the built plugins, and removes the reference to the top levelperformance
plugin.For example, your
.wp-env.json
filesplugins
property may change from something like this:To this:
Simply reinstate it to
[ "." ]
and re-runwp-env start
in order to resume a WordPress environment with just theperformance
plugin active.Introduction of a new
Standalone Plugin Integration Testing / PHP
Github workflowNOTE: This workflow currently runs against
push
andpull
actions againsfeature
,release
andtrunk
branches. Once the process of the checks are confirmed via this PR, the workflow will be suitably updated to be more concise and specific.This workflow executes the following operations:
npm ci
agains the rootpackage.json
filenpm run build-plugins
command to generate standalone plugins, as these are not part of the repository sourcenpm run test-plugins
following a successful step above, and continues to run integration tests for all built plugins as described further above, in a single site environment.npm run test-plugins-multisite
following a successful step above, and continues to run integration tests for all built plugins as described further above, in a multisite environment.Checklist
[Focus]
orInfrastructure
label.[Type]
label.no milestone
label.