-
Notifications
You must be signed in to change notification settings - Fork 50
fix: Failed site creation resets the directory #68
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
Changes from all commits
884317a
8e86f02
17374c6
92dda3f
5266363
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| export const addBreadcrumb = jest.fn(); | ||
| export const captureException = jest.fn(); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| export const app = { | ||
| getFetch: jest.fn(), | ||
| getPath: jest.fn( ( name ) => `/path/to/app/${ name }` ), | ||
| getName: jest.fn( () => 'App Name' ), | ||
| getPreferredSystemLanguages: jest.fn( () => [ 'en-US' ] ), | ||
| }; | ||
|
|
||
| export const BrowserWindow = { | ||
| fromWebContents: jest.fn( () => ( { | ||
| webContents: { | ||
| send: jest.fn(), | ||
| }, | ||
| } ) ), | ||
| }; | ||
|
|
||
| export const shell = { | ||
| openExternal: jest.fn(), | ||
| trashItem: jest.fn(), | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| type Fs = typeof import('fs'); | ||
| interface MockedFs extends Fs { | ||
| __setFileContents: ( path: string, fileContents: string | string[] ) => void; | ||
| } | ||
|
|
||
| const fs = jest.createMockFromModule< MockedFs >( 'fs' ); | ||
| const fsPromises = jest.createMockFromModule< typeof import('fs/promises') >( 'fs/promises' ); | ||
|
|
||
| fs.promises = fsPromises; | ||
|
|
||
| const mockFiles: Record< string, string | string[] > = {}; | ||
| fs.__setFileContents = ( path: string, fileContents: string | string[] ) => { | ||
| mockFiles[ path ] = fileContents; | ||
| }; | ||
|
|
||
| ( fs.promises.readFile as jest.Mock ).mockImplementation( | ||
| async ( path: string ): Promise< string > => { | ||
| const fileContents = mockFiles[ path ]; | ||
|
|
||
| if ( typeof fileContents === 'string' ) { | ||
| return fileContents; | ||
| } | ||
|
|
||
| return ''; | ||
| } | ||
| ); | ||
|
|
||
| ( fs.promises.readdir as jest.Mock ).mockImplementation( | ||
| async ( path: string ): Promise< Array< string > > => { | ||
| const dirContents = mockFiles[ path ]; | ||
|
|
||
| if ( Array.isArray( dirContents ) ) { | ||
| return dirContents; | ||
| } | ||
|
|
||
| return []; | ||
| } | ||
| ); | ||
|
|
||
| module.exports = fs; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -129,7 +129,14 @@ export async function createSite( | |
| } | ||
|
|
||
| if ( ( await pathExists( path ) ) && ( await isEmptyDir( path ) ) ) { | ||
| await createSiteWorkingDirectory( path ); | ||
| try { | ||
| await createSiteWorkingDirectory( path ); | ||
| } catch ( error ) { | ||
| // If site creation failed, remove the generated files and re-throw the | ||
| // error so it can be handled by the caller. | ||
| shell.trashItem( path ); | ||
| throw error; | ||
| } | ||
|
Comment on lines
+132
to
+139
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice!, thanks for introducing the logic! |
||
| } | ||
|
|
||
| const details = { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,6 @@ | ||
| /** | ||
| * @jest-environment node | ||
| */ | ||
|
Comment on lines
+1
to
+3
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using the |
||
| import { app } from 'electron'; | ||
| import { createI18n } from '@wordpress/i18n'; | ||
| import { getLocaleData, getSupportedLocale } from '../locale'; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,12 @@ | ||
| /** | ||
| * @jest-environment node | ||
| */ | ||
| // To run tests, execute `npm run test -- src/storage/user-data.test.ts` from the root directory | ||
| import fs from 'fs'; | ||
| import { loadUserData } from '../user-data'; | ||
|
|
||
| jest.mock( 'fs' ); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| const mockUserData = { | ||
| sites: [ | ||
| { name: 'Tristan', path: '/to/tristan' }, | ||
|
|
@@ -11,23 +16,12 @@ const mockUserData = { | |
| snapshots: [], | ||
| }; | ||
|
|
||
| jest.mock( 'fs', () => ( { | ||
| promises: { | ||
| readFile: async () => JSON.stringify( mockUserData ), | ||
| }, | ||
| existsSync: () => true, | ||
| } ) ); | ||
|
|
||
| jest.mock( 'electron', () => ( { | ||
| app: { | ||
| getFetch: jest.fn(), | ||
| getPath: jest.fn(), | ||
| getName: jest.fn(), | ||
| }, | ||
| } ) ); | ||
| jest.mock( 'path', () => ( { | ||
| join: jest.fn(), | ||
| } ) ); | ||
| ( fs as MockedFs ).__setFileContents( | ||
| '/path/to/app/appData/App Name/appdata-v1.json', | ||
| JSON.stringify( mockUserData ) | ||
| ); | ||
| // Assume each site path exists | ||
| ( fs.existsSync as jest.Mock ).mockReturnValue( true ); | ||
|
|
||
| describe( 'loadUserData', () => { | ||
| test( 'loads user data correctly and sorts sites', async () => { | ||
|
|
@@ -41,7 +35,7 @@ describe( 'loadUserData', () => { | |
| } ); | ||
|
|
||
| test( 'Filters out sites where the path does not exist', async () => { | ||
| fs.existsSync = jest.fn( ( path ) => path === '/to/lancelot' ); | ||
| ( fs.existsSync as jest.Mock ).mockImplementation( ( path ) => path === '/to/lancelot' ); | ||
| const result = await loadUserData(); | ||
| expect( result.sites.map( ( sites ) => sites.name ) ).toEqual( [ 'Lancelot' ] ); | ||
| } ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| /** | ||
| * @jest-environment node | ||
| */ | ||
| import { shell, IpcMainInvokeEvent } from 'electron'; | ||
| import fs from 'fs'; | ||
| import { createSite } from '../ipc-handlers'; | ||
| import { isEmptyDir, pathExists } from '../lib/fs-utils'; | ||
| import { SiteServer, createSiteWorkingDirectory } from '../site-server'; | ||
|
|
||
| jest.mock( 'fs' ); | ||
| jest.mock( '../lib/fs-utils' ); | ||
| jest.mock( '../site-server' ); | ||
|
|
||
| ( SiteServer.create as jest.Mock ).mockImplementation( ( details ) => ( { | ||
| start: jest.fn(), | ||
| details, | ||
| updateSiteDetails: jest.fn(), | ||
| updateCachedThumbnail: jest.fn( () => Promise.resolve() ), | ||
| } ) ); | ||
| ( createSiteWorkingDirectory as jest.Mock ).mockResolvedValue( true ); | ||
|
|
||
| const mockUserData = { | ||
| sites: [], | ||
| }; | ||
| ( fs as MockedFs ).__setFileContents( | ||
| '/path/to/app/appData/App Name/appdata-v1.json', | ||
| JSON.stringify( mockUserData ) | ||
| ); | ||
| // Assume the provided site path is a directory | ||
| ( fs.promises.stat as jest.Mock ).mockResolvedValue( { | ||
| isDirectory: () => true, | ||
| } ); | ||
|
|
||
| const mockIpcMainInvokeEvent = {} as IpcMainInvokeEvent; | ||
|
|
||
| describe( 'createSite', () => { | ||
| it( 'should create a site', async () => { | ||
| ( isEmptyDir as jest.Mock ).mockResolvedValue( true ); | ||
| ( pathExists as jest.Mock ).mockResolvedValue( true ); | ||
| const [ site ] = await createSite( mockIpcMainInvokeEvent, '/test', 'Test' ); | ||
|
|
||
| expect( site ).toEqual( { | ||
| adminPassword: expect.any( String ), | ||
| id: expect.any( String ), | ||
| name: 'Test', | ||
| path: '/test', | ||
| running: false, | ||
| } ); | ||
| } ); | ||
|
|
||
| describe( 'when the site path started as an empty directory', () => { | ||
| it( 'should reset the directory when site creation fails', () => { | ||
| ( isEmptyDir as jest.Mock ).mockResolvedValue( true ); | ||
| ( pathExists as jest.Mock ).mockResolvedValue( true ); | ||
| ( createSiteWorkingDirectory as jest.Mock ).mockImplementation( () => { | ||
| throw new Error( 'Intentional test error' ); | ||
| } ); | ||
|
|
||
| createSite( mockIpcMainInvokeEvent, '/test', 'Test' ).catch( () => { | ||
| expect( shell.trashItem ).toHaveBeenCalledTimes( 1 ); | ||
| expect( shell.trashItem ).toHaveBeenCalledWith( '/test' ); | ||
| } ); | ||
| } ); | ||
| } ); | ||
| } ); |
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.
Sourced from the Jest documentation, implemented to allow simultaneously mocking file contents from different locations.