-
Notifications
You must be signed in to change notification settings - Fork 54
Implement studio go for creating preview sites #1130
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
Conversation
fredrikekelund
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.
I have yet to test this, but the code is looking good overall and the structure is easy to follow 👍
My biggest comment is that I'd suggest following a "classical" error handling pattern where exceptions are thrown instead of returned. Implementing a custom error class that extends Error would make this easier because we could pass a logger action as a prop on the error.
| async function runCommand( siteFolder: string, outputFormat?: OutputFormat ): Promise< void > { | ||
| const archivePath = path.join( | ||
| os.tmpdir(), | ||
| `${ path.basename( siteFolder ) }-${ Date.now() }.zip` | ||
| ); | ||
| const logger = new Logger< LoggerAction >( outputFormat ); | ||
|
|
||
| logger.reportStart( LoggerAction.VALIDATE, 'Validating...' ); | ||
| const isValidSiteFolder = validateSiteFolder( siteFolder ); | ||
| if ( isValidSiteFolder instanceof Error ) { | ||
| logger.reportError( LoggerAction.VALIDATE, isValidSiteFolder.message ); | ||
| return; | ||
| } | ||
| const token = await getAuthToken(); | ||
| if ( ! token ) { | ||
| logger.reportError( | ||
| LoggerAction.VALIDATE, | ||
| 'Authentication required. Please run the Studio app and authenticate first.' | ||
| ); | ||
| return; | ||
| } | ||
| logger.reportSuccess( LoggerAction.VALIDATE, 'Validation successful' ); | ||
|
|
||
| logger.reportStart( LoggerAction.ARCHIVE, 'Creating archive...' ); | ||
| const archive = await createArchive( siteFolder, archivePath ); | ||
| if ( archive instanceof Error ) { | ||
| logger.reportError( LoggerAction.ARCHIVE, archive.message ); | ||
| return; | ||
| } | ||
| logger.reportSuccess( LoggerAction.ARCHIVE, 'Archive created' ); | ||
|
|
||
| logger.reportStart( LoggerAction.UPLOAD, 'Uploading archive...' ); | ||
| const uploadResponse = await uploadArchive( archivePath, token ); | ||
| if ( uploadResponse instanceof Error ) { | ||
| logger.reportError( LoggerAction.UPLOAD, uploadResponse.message ); | ||
| return; | ||
| } | ||
| if ( ! uploadResponse.site_url || ! uploadResponse.site_id ) { | ||
| logger.reportError( LoggerAction.UPLOAD, 'Failed to upload archive' ); | ||
| return; | ||
| } | ||
| logger.reportSuccess( LoggerAction.UPLOAD, 'Archive uploaded' ); | ||
|
|
||
| logger.reportStart( LoggerAction.READY, 'Creating preview site...' ); | ||
| const isSiteReady = await waitForSiteReady( uploadResponse.site_id, token ); | ||
| if ( ! isSiteReady ) { | ||
| logger.reportError( LoggerAction.READY, 'Failed to create preview site' ); | ||
| return; | ||
| } | ||
| cleanup( archivePath ); | ||
| logger.reportSuccess( | ||
| LoggerAction.READY, | ||
| `Preview site available at: https://${ uploadResponse.site_url }` | ||
| ); | ||
| } |
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.
| async function runCommand( siteFolder: string, outputFormat?: OutputFormat ): Promise< void > { | |
| const archivePath = path.join( | |
| os.tmpdir(), | |
| `${ path.basename( siteFolder ) }-${ Date.now() }.zip` | |
| ); | |
| const logger = new Logger< LoggerAction >( outputFormat ); | |
| logger.reportStart( LoggerAction.VALIDATE, 'Validating...' ); | |
| const isValidSiteFolder = validateSiteFolder( siteFolder ); | |
| if ( isValidSiteFolder instanceof Error ) { | |
| logger.reportError( LoggerAction.VALIDATE, isValidSiteFolder.message ); | |
| return; | |
| } | |
| const token = await getAuthToken(); | |
| if ( ! token ) { | |
| logger.reportError( | |
| LoggerAction.VALIDATE, | |
| 'Authentication required. Please run the Studio app and authenticate first.' | |
| ); | |
| return; | |
| } | |
| logger.reportSuccess( LoggerAction.VALIDATE, 'Validation successful' ); | |
| logger.reportStart( LoggerAction.ARCHIVE, 'Creating archive...' ); | |
| const archive = await createArchive( siteFolder, archivePath ); | |
| if ( archive instanceof Error ) { | |
| logger.reportError( LoggerAction.ARCHIVE, archive.message ); | |
| return; | |
| } | |
| logger.reportSuccess( LoggerAction.ARCHIVE, 'Archive created' ); | |
| logger.reportStart( LoggerAction.UPLOAD, 'Uploading archive...' ); | |
| const uploadResponse = await uploadArchive( archivePath, token ); | |
| if ( uploadResponse instanceof Error ) { | |
| logger.reportError( LoggerAction.UPLOAD, uploadResponse.message ); | |
| return; | |
| } | |
| if ( ! uploadResponse.site_url || ! uploadResponse.site_id ) { | |
| logger.reportError( LoggerAction.UPLOAD, 'Failed to upload archive' ); | |
| return; | |
| } | |
| logger.reportSuccess( LoggerAction.UPLOAD, 'Archive uploaded' ); | |
| logger.reportStart( LoggerAction.READY, 'Creating preview site...' ); | |
| const isSiteReady = await waitForSiteReady( uploadResponse.site_id, token ); | |
| if ( ! isSiteReady ) { | |
| logger.reportError( LoggerAction.READY, 'Failed to create preview site' ); | |
| return; | |
| } | |
| cleanup( archivePath ); | |
| logger.reportSuccess( | |
| LoggerAction.READY, | |
| `Preview site available at: https://${ uploadResponse.site_url }` | |
| ); | |
| } | |
| class CommandError extends Error { | |
| action: LoggerAction; | |
| constructor( action: LoggerAction, message: string ) { | |
| super( message ); | |
| this.action = action; | |
| } | |
| } | |
| async function runCommand( siteFolder: string, outputFormat?: OutputFormat ): Promise< void > { | |
| const archivePath = path.join( | |
| os.tmpdir(), | |
| `${ path.basename( siteFolder ) }-${ Date.now() }.zip` | |
| ); | |
| const logger = new Logger< LoggerAction >( outputFormat ); | |
| try { | |
| logger.reportStart( LoggerAction.VALIDATE, 'Validating...' ); | |
| validateSiteFolder( siteFolder ); | |
| const token = await getAuthToken(); | |
| logger.reportSuccess( LoggerAction.VALIDATE, 'Validation successful' ); | |
| logger.reportStart( LoggerAction.ARCHIVE, 'Creating archive...' ); | |
| await createArchive( siteFolder, archivePath ); | |
| logger.reportSuccess( LoggerAction.ARCHIVE, 'Archive created' ); | |
| logger.reportStart( LoggerAction.UPLOAD, 'Uploading archive...' ); | |
| const uploadResponse = await uploadArchive( archivePath, token ); | |
| if ( ! uploadResponse.site_url || ! uploadResponse.site_id ) { | |
| throw new CommandError( LoggerAction.UPLOAD, 'Failed to upload archive' ); | |
| } | |
| logger.reportSuccess( LoggerAction.UPLOAD, 'Archive uploaded' ); | |
| logger.reportStart( LoggerAction.READY, 'Creating preview site...' ); | |
| const isSiteReady = await waitForSiteReady( uploadResponse.site_id, token ); | |
| if ( ! isSiteReady ) { | |
| throw new CommandError( LoggerAction.READY, 'Failed to create preview site' ); | |
| } | |
| cleanup( archivePath ); | |
| logger.reportSuccess( | |
| LoggerAction.READY, | |
| `Preview site available at: https://${ uploadResponse.site_url }` | |
| ); | |
| } catch ( error ) { | |
| if ( error instanceof CommandError ) { | |
| logger.reportError( error.action, error.message ); | |
| } | |
| } | |
| } |
This is probably subjective, but given this async / await ES6+ world we live in, I would love to embrace classical JS error handling with exceptions that actually throw instead of potentially returning errors.
Implementing custom error classes (like CommandError in my suggestion) and using them across the different CLI modules would help make this more convenient.
| function isWordPressDirectory( projectPath: string ): boolean { | ||
| return ( | ||
| fs.existsSync( path.join( projectPath, 'wp-content' ) ) && | ||
| fs.existsSync( path.join( projectPath, 'wp-includes' ) ) && | ||
| fs.existsSync( path.join( projectPath, 'wp-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 is an interesting example of logic that would ideally be shared across Studio and the CLI, but that is small enough to not warrant implementing its own CLI command.
I would actually consider replacing this with:
import { isWordPressDirectory } from 'src/lib/fs-utils';Webpack/TS can tree shake the code just fine, and we should take advantage of the fact that the CLI code lives right next to the Studio code while we can.
cli/commands/preview/lib/auth.ts
Outdated
| const userData = JSON.parse( fs.readFileSync( appDataPath, 'utf8' ) ); | ||
| return userData.authToken?.accessToken || null; |
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.
Let's add a zod schema to validate the authToken.accessToken part of the config file.
| const archive = archiver( 'zip', { | ||
| zlib: { level: ZIP_COMPRESSION_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.
Not something we have to look into now, but there was a recent request to follow symlinks when creating preview site archives. This makes a lot of sense to me, and if there's an easy way to solve it with archiver, we might as well solve it now.
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.
For reference, here's the relevant issue STU-309
cli/commands/preview/lib/api.ts
Outdated
| const response = await wpcom.req.post< CreateSiteResponse >( { | ||
| path: '/jurassic-ninja/create-new-site-from-zip', | ||
| apiNamespace: 'wpcom/v2', | ||
| formData, | ||
| } ); |
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'd add a zod schema here.
cli/commands/preview/lib/api.ts
Outdated
| const response = await wpcom.req.get< StatusResponse >( '/jurassic-ninja/status', { | ||
| apiNamespace: 'wpcom/v2', | ||
| site_id: siteId, | ||
| } ); |
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'd add a zod schema here.
cli/commands/preview/lib/api.ts
Outdated
| export interface StatusResponse { | ||
| status: SnapshotStatus; | ||
| domain_name: string; | ||
| atomic_site_id: number; | ||
| is_deleted: string; | ||
| } | ||
|
|
||
| export enum SnapshotStatus { | ||
| Pending = '0', | ||
| Processing = '1', | ||
| Active = '2', | ||
| } |
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 enum usage 👍
fredrikekelund
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.
I've tested this and it's working great overall 👍 I pushed two small commits to fix a deprecation warning from an outdated descendant dependency and to fix how the --output-format option is parsed.
The only issue I encountered is that if I pass the --output-format=json option, the command exits early with the following error:
/Users/fredrik/Code/studio/dist/cli/main.js:43174
throw new Error('Cannot report success for an action that is not currently in progress');
^
Error: Cannot report success for an action that is not currently in progress
at Logger.reportSuccess (/Users/fredrik/Code/studio/dist/cli/main.js:43174:19)
at /Users/fredrik/Code/studio/dist/cli/main.js:42846:16
at Generator.next (<anonymous>)
at fulfilled (/Users/fredrik/Code/studio/dist/cli/main.js:42805:58)
at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
Node.js v22.9.0
| "license": "GPL-2.0-or-later", | ||
| "main": "index.js" | ||
| } | ||
| } |
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 you use some tooling that strips out newlines from the end of files, @bcotrim?
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.
Nothing besides Cursor 🤔
|
One more thing: we should write the newly created preview site to the Studio appdata file. |
Fixes a deprecation warning that was previously generated
3cc1ce4 to
554b065
Compare
fredrikekelund
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.
Shaping up nicely 👍
I left a number of comments on the code. Functionality-wise, I'm still getting errors when passing the --output-format=json option.
cli/logger.ts
Outdated
| if ( this.currentAction !== error.action ) { | ||
| throw new Error( 'Cannot report error for an action that is not currently in progress' ); | ||
| } |
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 really need this? It seems like removing it would greatly reduce the number of times we pass around LoggerAction references.
cli/commands/preview/lib/appdata.ts
Outdated
| const result = UserDataSchema.safeParse( userData ); | ||
|
|
||
| if ( ! result.success ) { | ||
| throw new LoggerError( `Invalid appdata format. Please run the Studio app again.`, action ); | ||
| } |
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 result = UserDataSchema.safeParse( userData ); | |
| if ( ! result.success ) { | |
| throw new LoggerError( `Invalid appdata format. Please run the Studio app again.`, action ); | |
| } | |
| const result = UserDataSchema.parse( userData ); |
Since we already have an extensive catch clause here, it seems like we could use the zod.parse method.
cli/commands/preview/lib/appdata.ts
Outdated
| userData.version = 1; | ||
| } | ||
|
|
||
| // Create a deep copy to avoid reference issues |
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.
Did you encounter any issues like 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.
I did, but I re-tested and I didn't find the same issue, so I think it's safe to remove now.
cli/commands/preview/lib/appdata.ts
Outdated
| const dataToSave = JSON.parse( JSON.stringify( userData ) ); | ||
| const fileContent = JSON.stringify( dataToSave, null, 2 ) + '\n'; | ||
|
|
||
| // Write the file |
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.
| // Write the file |
cli/commands/preview/lib/appdata.ts
Outdated
| const UserDataSchema = z.object( { | ||
| version: z.number().optional(), | ||
| sites: z.array( z.any() ).optional(), | ||
| snapshots: z.array( SnapshotSchema ).optional(), | ||
| authToken: z.any().optional(), | ||
| locale: z.string().optional(), | ||
| } ); |
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 applaud this initiative, but this schema is missing a couple of top-level items (sentryUserId and lastSeenVersion, for example).
The best-case scenario is that we have a shared schema for the user-config file between Studio and the CLI. However, to avoid blowing the scope of this PR, I suggest keeping a schema that validates only the snapshots array in the user config. All other properties should be left untouched between reading and writing the file.
cli/commands/preview/lib/api.ts
Outdated
| // Validate the response against our schema | ||
| const result = CreateSiteResponseSchema.safeParse( rawResponse ); | ||
|
|
||
| if ( ! result.success ) { | ||
| throw new LoggerError( 'Invalid API response', action ); | ||
| } | ||
|
|
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.
| // Validate the response against our schema | |
| const result = CreateSiteResponseSchema.safeParse( rawResponse ); | |
| if ( ! result.success ) { | |
| throw new LoggerError( 'Invalid API response', action ); | |
| } | |
| const result = CreateSiteResponseSchema.parse( rawResponse ); |
Again, we have an extensive catch clause already, so I don't see the need for using safeParse
cli/commands/preview/lib/api.ts
Outdated
| const result = StatusResponseSchema.safeParse( rawResponse ); | ||
|
|
||
| if ( ! result.success ) { | ||
| return false; | ||
| } | ||
|
|
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 result = StatusResponseSchema.safeParse( rawResponse ); | |
| if ( ! result.success ) { | |
| return false; | |
| } | |
| const result = StatusResponseSchema.parse( rawResponse ); |
cli/commands/preview/lib/api.ts
Outdated
| ]; | ||
|
|
||
| try { | ||
| const rawResponse = await wpcom.req.post< CreateSiteResponse >( { |
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 rawResponse = await wpcom.req.post< CreateSiteResponse >( { | |
| const rawResponse = await wpcom.req.post( { |
The default response type is unknown, which is actually accurate. We can't guarantee anything about the data type until it's parsed by zod.
cli/commands/preview/lib/api.ts
Outdated
| const wpcom = new WPCOM( token ); | ||
|
|
||
| try { | ||
| const rawResponse = await wpcom.req.get< StatusResponse >( '/jurassic-ninja/status', { |
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 rawResponse = await wpcom.req.get< StatusResponse >( '/jurassic-ninja/status', { | |
| const rawResponse = await wpcom.req.get( '/jurassic-ninja/status', { |
Again, no guarantees until we've parsed the data with zod.
cli/commands/preview/lib/api.ts
Outdated
| export type CreateSiteResponse = z.infer< typeof CreateSiteResponseSchema >; | ||
| export type StatusResponse = z.infer< typeof StatusResponseSchema >; | ||
|
|
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.
| export type CreateSiteResponse = z.infer< typeof CreateSiteResponseSchema >; | |
| export type StatusResponse = z.infer< typeof StatusResponseSchema >; |
Let's remove these per my other comments about not assuming the response types without validating.
| it( 'should throw LoggerError for invalid API response', async () => { | ||
| const invalidResponse = { | ||
| // Missing domain_name | ||
| atomic_site_id: mockSiteId, | ||
| }; | ||
|
|
||
| const mockWpcom = { | ||
| req: { | ||
| post: jest.fn().mockResolvedValue( invalidResponse ), | ||
| }, | ||
| }; | ||
| ( wpcom as jest.Mock ).mockReturnValue( mockWpcom ); | ||
|
|
||
| await expect( uploadArchive( mockArchivePath, mockToken, mockAction ) ).rejects.toThrow( | ||
| LoggerError | ||
| ); | ||
| await expect( uploadArchive( mockArchivePath, mockToken, mockAction ) ).rejects.toMatchObject( | ||
| { | ||
| message: 'Invalid API response', | ||
| action: mockAction, | ||
| } | ||
| ); | ||
| } ); | ||
|
|
||
| it( 'should throw LoggerError for response with wrong types', async () => { | ||
| const invalidResponse = { | ||
| domain_name: '', // Empty string, should fail validation | ||
| atomic_site_id: 'not-a-number', // Should be a number | ||
| }; | ||
|
|
||
| const mockWpcom = { | ||
| req: { | ||
| post: jest.fn().mockResolvedValue( invalidResponse ), | ||
| }, | ||
| }; | ||
| ( wpcom as jest.Mock ).mockReturnValue( mockWpcom ); | ||
|
|
||
| await expect( uploadArchive( mockArchivePath, mockToken, mockAction ) ).rejects.toThrow( | ||
| LoggerError | ||
| ); | ||
| await expect( uploadArchive( mockArchivePath, mockToken, mockAction ) ).rejects.toMatchObject( | ||
| { | ||
| message: 'Invalid API response', | ||
| action: mockAction, | ||
| } | ||
| ); | ||
| } ); |
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.
Seems like these two cases are testing more or less the same thing.
| it( 'should handle validation errors', async () => { | ||
| const errorMessage = 'Validation failed'; | ||
| ( validateSiteFolder as jest.Mock ).mockImplementation( () => { | ||
| throw new LoggerError( errorMessage, 'validate' ); | ||
| } ); | ||
|
|
||
| const { registerCommand } = await import( '../create' ); | ||
| registerCommand( program ); | ||
|
|
||
| await program.parseAsync( [ 'node', 'test', 'go', mockFolder ] ); | ||
|
|
||
| expect( mockLogger.reportError ).toHaveBeenCalled(); | ||
| expect( mockErrorData ).toHaveProperty( 'message', errorMessage ); | ||
| expect( mockErrorData ).toHaveProperty( 'action', 'validate' ); | ||
| expect( createArchive ).not.toHaveBeenCalled(); | ||
| } ); | ||
|
|
||
| it( 'should handle authentication errors', async () => { | ||
| const errorMessage = | ||
| 'Authentication required. Please run the Studio app and authenticate first.'; | ||
| ( getAuthToken as jest.Mock ).mockImplementation( () => { | ||
| throw new LoggerError( errorMessage, 'validate' ); | ||
| } ); | ||
|
|
||
| const { registerCommand } = await import( '../create' ); | ||
| registerCommand( program ); | ||
|
|
||
| await program.parseAsync( [ 'node', 'test', 'go', mockFolder ] ); | ||
|
|
||
| expect( mockLogger.reportError ).toHaveBeenCalled(); | ||
| expect( mockErrorData ).toHaveProperty( 'message', errorMessage ); | ||
| expect( mockErrorData ).toHaveProperty( 'action', 'validate' ); | ||
| expect( createArchive ).not.toHaveBeenCalled(); | ||
| } ); | ||
|
|
||
| it( 'should handle archive creation errors', async () => { | ||
| const errorMessage = 'Archive creation failed'; | ||
| ( createArchive as jest.Mock ).mockImplementation( () => { | ||
| throw new LoggerError( errorMessage, 'archive' ); | ||
| } ); | ||
|
|
||
| const { registerCommand } = await import( '../create' ); | ||
| registerCommand( program ); | ||
|
|
||
| await program.parseAsync( [ 'node', 'test', 'go', mockFolder ] ); | ||
|
|
||
| expect( mockLogger.reportError ).toHaveBeenCalled(); | ||
| expect( mockErrorData ).toHaveProperty( 'message', errorMessage ); | ||
| expect( mockErrorData ).toHaveProperty( 'action', 'archive' ); | ||
| expect( uploadArchive ).not.toHaveBeenCalled(); | ||
| } ); | ||
|
|
||
| it( 'should handle upload errors', async () => { | ||
| const errorMessage = 'Upload failed'; | ||
| ( uploadArchive as jest.Mock ).mockImplementation( () => { | ||
| throw new LoggerError( errorMessage, 'upload' ); | ||
| } ); | ||
|
|
||
| const { registerCommand } = await import( '../create' ); | ||
| registerCommand( program ); | ||
|
|
||
| await program.parseAsync( [ 'node', 'test', 'go', mockFolder ] ); | ||
|
|
||
| expect( mockLogger.reportError ).toHaveBeenCalled(); | ||
| expect( mockErrorData ).toHaveProperty( 'message', errorMessage ); | ||
| expect( mockErrorData ).toHaveProperty( 'action', 'upload' ); | ||
| expect( waitForSiteReady ).not.toHaveBeenCalled(); | ||
| } ); | ||
|
|
||
| it( 'should handle errors gracefully', async () => { | ||
| const mockError = new Error( 'Test error' ); | ||
| ( fs.createWriteStream as jest.Mock ).mockImplementation( () => { | ||
| throw mockError; | ||
| it( 'should handle site readiness errors', async () => { | ||
| const errorMessage = 'Failed to create preview site'; | ||
| ( waitForSiteReady as jest.Mock ).mockImplementation( () => { | ||
| throw new LoggerError( errorMessage, 'ready' ); | ||
| } ); | ||
|
|
||
| const { registerCommand } = await import( '../create' ); | ||
| registerCommand( program ); | ||
|
|
||
| await program.parseAsync( [ 'node', 'test', 'go', mockFolder ] ); | ||
|
|
||
| expect( mockLogger.reportError ).toHaveBeenCalled(); | ||
| expect( mockErrorData ).toHaveProperty( 'message', errorMessage ); | ||
| expect( mockErrorData ).toHaveProperty( 'action', 'ready' ); | ||
| expect( addPreviewSiteToAppdata ).not.toHaveBeenCalled(); | ||
| } ); | ||
|
|
||
| it( 'should handle appdata errors', async () => { | ||
| const errorMessage = 'Failed to save to appdata'; | ||
| ( addPreviewSiteToAppdata as jest.Mock ).mockImplementation( () => { | ||
| throw new LoggerError( errorMessage, 'appdata' ); | ||
| } ); | ||
|
|
||
| const { registerCommand } = await import( '../create' ); | ||
| registerCommand( program ); | ||
|
|
||
| await program.parseAsync( [ 'node', 'test', 'go', mockFolder ] ); | ||
|
|
||
| expect( mockLogger.reportError ).toHaveBeenCalled(); | ||
| expect( mockErrorData ).toHaveProperty( 'message', errorMessage ); | ||
| expect( mockErrorData ).toHaveProperty( 'action', 'appdata' ); | ||
| } ); | ||
|
|
||
| it( 'should always clean up archive file even on error', async () => { | ||
| ( uploadArchive as jest.Mock ).mockImplementation( () => { | ||
| throw new LoggerError( 'Upload failed', 'upload' ); | ||
| } ); | ||
|
|
||
| const { registerCommand } = await import( '../create' ); | ||
| registerCommand( program ); | ||
|
|
||
| await program.parseAsync( [ 'node', 'test', 'go', mockFolder ] ); | ||
|
|
||
| expect( cleanup ).toHaveBeenCalledWith( mockArchivePath ); | ||
| } ); | ||
|
|
||
| it( 'should handle unexpected errors', async () => { | ||
| const errorMessage = 'Unexpected error'; | ||
| const unexpectedError = new Error( errorMessage ); | ||
| ( validateSiteFolder as jest.Mock ).mockImplementation( () => { | ||
| throw unexpectedError; | ||
| } ); | ||
|
|
||
| const { registerCommand } = await import( '../create' ); | ||
| registerCommand( program ); | ||
|
|
||
| await program.parseAsync( [ 'node', 'test', 'go', mockFolder ] ); | ||
|
|
||
| expect( mockLogger.reportError ).toHaveBeenCalledWith( mockError.message ); | ||
| expect( mockLogger.reportError ).toHaveBeenCalled(); | ||
| expect( mockErrorData ).toHaveProperty( 'message', errorMessage ); | ||
| expect( mockErrorData ).toHaveProperty( 'action', 'validate' ); | ||
| expect( createArchive ).not.toHaveBeenCalled(); | ||
| } ); |
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.
Not wrong per se, but this strikes me as an excessive set of test cases. I assume this test code was written by AI, and it should be easy enough to modify with AI, too. Still, I think we can reduce this to two or three test cases and it would give us the same assurances in practice.
| throw new LoggerError( `Folder not found: ${ siteFolder }`, action ); | ||
| } | ||
|
|
||
| if ( ! isWordPressDirectory( siteFolder ) && ! hasWpContentDirectory( siteFolder ) ) { |
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 is never going to be true, since isWordPressDirectory checks if the folder contains a wp-content folder.
Related issues
Proposed Changes
gocommand to create a preview site in WordPress.comNote: As discussed on Slack, the way we fetch the user auth token is hard coded and will be adjusted in the a follow-up PR.
Testing Instructions
In the project root folder:
npm run cli:buildnode dist/cli/main.js go [folder]Some suggested test scenarios:
Pre-merge Checklist