-
Notifications
You must be signed in to change notification settings - Fork 54
Studio CLI: Fix Windows E2E tests #2299
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
Studio CLI: Fix Windows E2E tests #2299
Conversation
To help us diagnose which specific files or directories are causing trouble
This reverts commit 8786163.
|
|
||
| process.on( 'SIGINT', disconnect ); |
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.
The studio site list --watch command would previously not interrupt in response to Ctrl+C
|
OK, after one more round of changes, the tests are now consistently green, and I've slimmed down the diff as much as possible. |
| logger.reportStart( LoggerAction.LOAD_SITES, __( 'Loading site…' ) ); | ||
| const site = await getSiteByFolder( sitePath ); | ||
| logger.reportSuccess( __( 'Site loaded' ) ); | ||
|
|
||
| logger.reportStart( LoggerAction.START_DAEMON, __( 'Starting process daemon…' ) ); | ||
| await connect(); | ||
| logger.reportSuccess( __( 'Process daemon started' ) ); | ||
|
|
||
| logger.reportStart( LoggerAction.LOAD_SITES, __( 'Loading site…' ) ); | ||
| const site = await getSiteByFolder( sitePath ); | ||
| logger.reportSuccess( __( 'Site loaded' ) ); | ||
|
|
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 moved the connect call to the top of this function to fix a bug where triggering many site start commands in quick succession would cause only one site to start (the others would hang indefinitely).
This reverts commit 01ae1c4.
bcotrim
left a comment
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.
LGTM 👍
Amazing work not only on making E2E tests pass but ensuring the solution is robust and solid.
Related issues
Proposed Changes
This PR fixes the Windows E2E tests by making the following changes:
site list --watchcommand in response toSIGINTandSIGTERMsignals. Without this, thesite list --watchcommand couldn't be aborted with Ctrl+C and it would interfere with closing the Electron app.site stop --allcommand to kill all pm2 processes in one go, including the daemon itself.E2ESession::cleanupto not callElectronApplication::close(), but always useelectronApp.evaluate( ( { app } ) => app.quit() ).stopAllServersOnQuitchild process to finish before exiting the Electron app. This ensures all child processes have ended once the app quits, allowing auto-updates to be gracefully applied (if the bundled Node.js runtime were still used to run child processes, it would not be updatable on Windows).will-quitElectron event.rimraffor smart retry behavior.themeZipFileandpluginZipFilebeing deprecated, so I also took the opportunity to replace those keys withthemeDataandpluginDatain the test fixtures.site list --watch) and wait for thespawnevent from the child process before creating the main app window. After working on this for so long, I doubt this makes any difference, but it still seems like good practice.I also made changes related to general stability, not explicitly to the E2E tests:
pm2-connection.locklockfile to ensure that multiplesite startcommands triggered in quick sequence are still able to properly connect to the pm2 daemon. Previously, if multiple sites had anautoStartconfig, only one site would start when Studio started. The other sites would stall indefinitely.pm2.disconnectmethod takes a callback, and make alldisconnectcalls async.Testing Instructions
CI should pass.
Pre-merge Checklist