Skip to content

Conversation

@dcalhoun
Copy link
Member

@dcalhoun dcalhoun commented Apr 30, 2024

Related to #4 (comment).

Proposed Changes

  • Reintroduce directory reset when site creation fails to simplify retrying.
  • Refactor manual mocks to share foundational module mocks.
  • Refactor fs mocks to support reading different files.
  • Run main process tests in the Node.js environment for more realistic results
    and faster runs.

Testing Instructions

In addition to the following, exploratory/regression testing around site creation is welcome.

Failed site creation resets the directory

  1. Checkout the proposed changes.
  2. Apply the below intentional error diff.
  3. npm start
  4. Click "Add site" in the bottom-left corner.
  5. Click "Add site" in the modal.
  6. Verify a helpful error message is displayed.
  7. Verify the failed site directory is moved to the OS trash.
  8. Click the "local path" input and select the default Studio directory.
  9. Click "Add site" in the modal.
  10. Verify the site is successfully created and server started.
Intentional error diff
diff --git a/src/site-server.ts b/src/site-server.ts
index 4ac8ab3..563926b 100644
--- a/src/site-server.ts
+++ b/src/site-server.ts
@@ -16,6 +16,8 @@ import { getSiteThumbnailPath } from './storage/paths';
 
 const servers = new Map< string, SiteServer >();
 
+let forceFailure = true;
+
 export async function createSiteWorkingDirectory(
 	path: string,
 	wpVersion = 'latest'
@@ -28,6 +30,11 @@ export async function createSiteWorkingDirectory(
 	await purgeWpConfig( wpVersion );
 	await recursiveCopyDirectory( getWordPressVersionPath( wpVersion ), path );
 
+	if ( forceFailure ) {
+		forceFailure = false;
+		throw new Error( 'Simulated failure' );
+	}
+
 	return true;
 }
 

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@dcalhoun dcalhoun self-assigned this Apr 30, 2024
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like other new files in src/__mocks__, these mocks were hoisted here to simplify sharing these foundational module mocks across various test files.

Comment on lines +12 to +14
fs.__setFileContents = ( path: string, fileContents: string | string[] ) => {
mockFiles[ path ] = fileContents;
};
Copy link
Member Author

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.

Comment on lines +1 to +3
/**
* @jest-environment node
*/
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the node testing environment should provide a more realistic environment for main process tests. Also, the tests should run faster.

import fs from 'fs';
import { loadUserData } from '../user-data';

jest.mock( 'fs' );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to mock Node's built-in modules (e.g.: fs or path), then explicitly calling e.g. jest.mock('path') is required, because built-in modules are not mocked by default.

Jest Manual Mocks documentation

@dcalhoun dcalhoun marked this pull request as ready for review April 30, 2024 14:20
@dcalhoun dcalhoun requested a review from a team April 30, 2024 14:20
dcalhoun added 5 commits May 2, 2024 08:29
Simplify mocking foundational modules by placing them in a shared
location.
Simplify mocking foundational modules by placing them in a shared
location.
Simplify mocking foundational modules by placing them in a shared
location.
Simplify retrying site creation by removing all files generated during
the process.
The Node.js environment is more applicable than the default `jsdom`
environment. This should make tests more realistic and faster.
@dcalhoun dcalhoun force-pushed the fix/failed-site-creation-resets-the-directory branch from e5df727 to 5266363 Compare May 2, 2024 12:31
Copy link
Contributor

@kozer kozer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it and worked as expected:

  1. Originally I got the error
    2024-05-02T16:51:58,668709238+03:00
  2. Verified the delete site
    2024-05-02T16:52:59,996871993+03:00
  3. The second time the site created successfully.

Also note, that I run all tests to verify that everything work as expected
2024-05-02T16:53:31,552600014+03:00

Nice work @dcalhoun !

@dcalhoun dcalhoun merged commit b20c711 into trunk May 2, 2024
@dcalhoun dcalhoun deleted the fix/failed-site-creation-resets-the-directory branch May 2, 2024 14:03
Copy link
Member

@sejas sejas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!, The change looks great! 👍
Now users can re-try after a new site creation fails.
Thank you!

Comment on lines +132 to +139
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;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!, thanks for introducing the logic!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants