test: add unit tests for milady-root utility#295
Conversation
Creates a comprehensive Vitest suite in `src/utils/milady-root.test.ts` to validate the logic for resolving the milady package root directory. Uses deterministic file system mocks to verify the correct resolution logic for resolving package.json across various scenarios including the happy path, missing file errors (ENOENT), different package names, `moduleUrl` fallback logic, and `argv1` configurations without leaking state or attempting real file I/O.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
|
|
||
| describe("resolveMiladyPackageRoot", () => { | ||
| it("returns null when no package.json is found", async () => { | ||
| vi.mocked(fs.readFile).mockRejectedValue(new Error("ENOENT")); |
There was a problem hiding this comment.
The mock for fs.readFile uses new Error("ENOENT"), but Node.js typically throws an error object with a code property (e.g., { code: 'ENOENT' }). If the implementation checks err.code, this test may not accurately simulate the real error, potentially leading to false positives.
Recommended solution:
Mock the error as follows:
const err = new Error("ENOENT");
err.code = "ENOENT";
vi.mocked(fs.readFile).mockRejectedValue(err);Apply a similar pattern for all error mocks simulating file-not-found.
| describe("resolveMiladyPackageRootSync", () => { | ||
| it("returns null when no package.json is found", () => { | ||
| vi.mocked(fsSync.readFileSync).mockImplementation(() => { | ||
| throw new Error("ENOENT"); |
There was a problem hiding this comment.
The synchronous mock for fsSync.readFileSync throws new Error("ENOENT"), but Node.js errors for missing files include a code property (e.g., { code: 'ENOENT' }). If the implementation checks err.code, this test may not accurately simulate the real error, risking false positives.
Recommended solution:
Throw an error with a code property:
const err = new Error("ENOENT");
err.code = "ENOENT";
throw err;Update all similar error mocks in the sync tests.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request adds a new test file to validate the logic of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive suite of unit tests for the resolveMiladyPackageRoot and resolveMiladyPackageRootSync utility functions. The tests effectively use mocks for the file system to validate different scenarios, including success paths and error conditions. My review focuses on improving test coverage and ensuring cross-platform compatibility. I've suggested adding test cases for malformed package.json files and fixing an issue where moduleUrl tests would fail on Windows. Overall, this is a solid addition for ensuring the reliability of the package root resolution logic.
| ); | ||
| const result = await resolveMiladyPackageRoot({ cwd: "/unknown/path" }); | ||
| expect(result).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
The test suite is comprehensive, but it's missing a case for a malformed package.json. The current implementation correctly handles this by returning null due to the try...catch block around JSON.parse, but it would be good to have an explicit test case for this scenario to prevent future regressions.
You could add a test like this after the current one:
it("returns null when package.json is malformed", async () => {
vi.mocked(fs.readFile).mockResolvedValue("not a valid json");
const result = await resolveMiladyPackageRoot({ cwd: "/unknown/path" });
expect(result).toBeNull();
});| it("finds root from moduleUrl", async () => { | ||
| const mockDir = path.resolve("/app/milady"); | ||
| // Use file:/// schema for moduleUrl | ||
| const moduleUrl = `file://${path.join(mockDir, "src/utils/test.ts")}`; |
There was a problem hiding this comment.
Manually constructing a file:// URL with path.join is not cross-platform compatible and will likely fail on Windows because path.join uses backslashes (\) as separators, resulting in an invalid URL. The fileURLToPath function in the code under test expects a correctly formatted file URL.
To ensure the test runs correctly on all platforms, you should use pathToFileURL from the node:url module to generate the URL. You'll need to add pathToFileURL to your imports from node:url at the top of the file.
| const moduleUrl = `file://${path.join(mockDir, "src/utils/test.ts")}`; | |
| const moduleUrl = pathToFileURL(path.join(mockDir, "src/utils/test.ts")).href; |
| ); | ||
| const result = resolveMiladyPackageRootSync({ cwd: "/unknown/path" }); | ||
| expect(result).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
Similar to the async version, it would be beneficial to add a test case for a malformed package.json to ensure the synchronous function also handles this gracefully.
You could add a test like this after the current one:
it("returns null when package.json is malformed", () => {
vi.mocked(fsSync.readFileSync).mockReturnValue("not a valid json");
const result = resolveMiladyPackageRootSync({ cwd: "/unknown/path" });
expect(result).toBeNull();
});|
|
||
| it("finds root from moduleUrl", () => { | ||
| const mockDir = path.resolve("/app/milady"); | ||
| const moduleUrl = `file://${path.join(mockDir, "src/utils/test.ts")}`; |
There was a problem hiding this comment.
Similar to the async test, manually constructing a file:// URL with path.join is not cross-platform compatible. Please use pathToFileURL from node:url to ensure this test works correctly on Windows.
| const moduleUrl = `file://${path.join(mockDir, "src/utils/test.ts")}`; | |
| const moduleUrl = pathToFileURL(path.join(mockDir, "src/utils/test.ts")).href; |
1 participant
Creates a comprehensive Vitest suite in
src/utils/milady-root.test.tsto validate the logic for resolving the milady package root directory.
Uses deterministic file system mocks to verify the correct resolution
logic for resolving package.json across various scenarios including
the happy path, missing file errors (ENOENT), different package names,
moduleUrlfallback logic, andargv1configurations without leakingstate or attempting real file I/O.
PR created automatically by Jules for task 347443979378213741 started by @Dexploarer