test: add config vite resolver coverage#1822
Conversation
superagent-security
Bot
added
contributor:verified
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| if (hasPkgJson === undefined) | ||
| try { | ||
| await fs.stat(`${parent}/package.json`); |
There was a problem hiding this comment.
The else branch uses the original importer string for parent, which on Windows can contain backslashes. The template literal \${parent}/package.json`could produce mixed separators (e.g.,C:\Users\project/package.json). The rest of this PR consistently uses path.join` for path construction — this line should do the same for correctness on Windows.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/config/vite/relativeAliasResolver.ts
Line: 34
Comment:
**Mixed path separator in stat call**
The `else` branch uses the original `importer` string for `parent`, which on Windows can contain backslashes. The template literal `\`${parent}/package.json\`` could produce mixed separators (e.g., `C:\Users\project/package.json`). The rest of this PR consistently uses `path.join` for path construction — this line should do the same for correctness on Windows.
How can I resolve this? If you propose a fix, please make it concise.| import fs from "node:fs/promises"; | ||
| import os from "node:os"; | ||
| import path from "node:path"; | ||
| import { afterEach, beforeEach, describe, expect, it } from "vitest"; | ||
| import resolver from "./relativeAliasResolver"; | ||
|
|
||
| let tempDir: string; | ||
|
|
||
| beforeEach(async () => { | ||
| tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "cap-config-vite-")); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| await fs.rm(tempDir, { recursive: true, force: true }); | ||
| }); | ||
|
|
||
| const resolveAlias = async (source: string, importer: string) => { | ||
| const result = await resolver.customResolver?.(source, importer, {}); | ||
| if (typeof result !== "string") throw new Error("Expected string resolution"); | ||
| return result; |
There was a problem hiding this comment.
pkgJsonCache not cleared between tests
relativeAliasResolver.ts stores a module-level pkgJsonCache that persists for the lifetime of the module instance. Within a Vitest test file, module state is shared across all test cases, so entries cached in one test remain visible to subsequent tests. The current three tests are safe (unique mkdtemp paths prevent cross-contamination), but any future test that (1) asserts a path has no package.json, (2) then creates one at that path, and (3) checks again would receive a stale false from the cache. A vi.resetModules() in beforeEach, or exporting a clearCacheForTesting() helper, would make the fixture fully isolated.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/config/vite/relativeAliasResolver.test.ts
Line: 1-20
Comment:
**Module-level `pkgJsonCache` not cleared between tests**
`relativeAliasResolver.ts` stores a module-level `pkgJsonCache` that persists for the lifetime of the module instance. Within a Vitest test file, module state is shared across all test cases, so entries cached in one test remain visible to subsequent tests. The current three tests are safe (unique `mkdtemp` paths prevent cross-contamination), but any future test that (1) asserts a path has no `package.json`, (2) then creates one at that path, and (3) checks again would receive a stale `false` from the cache. A `vi.resetModules()` in `beforeEach`, or exporting a `clearCacheForTesting()` helper, would make the fixture fully isolated.
How can I resolve this? If you propose a fix, please make it concise.| describe("relativeAliasResolver", () => { | ||
| it("resolves ~/ imports from a package src directory", async () => { | ||
| const srcDir = path.join(tempDir, "pkg", "src"); | ||
| await fs.mkdir(path.join(srcDir, "components"), { recursive: true }); | ||
| await fs.writeFile(path.join(srcDir, "components", "Button.tsx"), ""); | ||
|
|
||
| await expect( | ||
| resolveAlias( | ||
| "~/components/Button", | ||
| path.join(srcDir, "pages", "index.tsx"), | ||
| ), | ||
| ).resolves.toBe(path.join(srcDir, "components", "Button.tsx")); | ||
| }); | ||
|
|
||
| it("resolves ~/ imports from the nearest package root", async () => { | ||
| const pkgDir = path.join(tempDir, "pkg"); | ||
| await fs.mkdir(path.join(pkgDir, "src", "utils"), { recursive: true }); | ||
| await fs.writeFile(path.join(pkgDir, "package.json"), "{}"); | ||
| await fs.writeFile(path.join(pkgDir, "src", "utils", "index.ts"), ""); | ||
|
|
||
| await expect( | ||
| resolveAlias("~/src/utils", path.join(pkgDir, "tests", "unit.test.ts")), | ||
| ).resolves.toBe(path.join(pkgDir, "src", "utils", "index.ts")); | ||
| }); | ||
|
|
||
| it("stops at the filesystem root when no package.json can be found", async () => { | ||
| await expect( | ||
| resolveAlias("~/missing/file", path.join(tempDir, "loose", "test.ts")), | ||
| ).rejects.toThrow("Failed to resolve import path ~/missing/file"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The PR description specifically calls out "make the resolver handle Windows-style importer paths" as a fix, and the implementation adds importer?.replace(/\\/g, "/"). None of the three tests pass a backslash-style path as importer, so the new normalization code goes untested. On Linux/macOS CI, path.join always produces forward slashes, meaning a backslash-containing string like "C:\\Users\\user\\project\\src\\pages\\index.tsx" would never be exercised. Adding one test case that uses a hardcoded Windows-style path string for the src/ branch would lock in the intended behaviour.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/config/vite/relativeAliasResolver.test.ts
Line: 23-53
Comment:
**Windows path normalization is not exercised by any test**
The PR description specifically calls out "make the resolver handle Windows-style importer paths" as a fix, and the implementation adds `importer?.replace(/\\/g, "/")`. None of the three tests pass a backslash-style path as `importer`, so the new normalization code goes untested. On Linux/macOS CI, `path.join` always produces forward slashes, meaning a backslash-containing string like `"C:\\Users\\user\\project\\src\\pages\\index.tsx"` would never be exercised. Adding one test case that uses a hardcoded Windows-style path string for the `src/` branch would lock in the intended behaviour.
How can I resolve this? If you propose a fix, please make it concise.
|
Validation follow-up for PR #1822: npx --yes pnpm@10.5.2 --dir packages/config test
# vite/relativeAliasResolver.test.ts: 4 passed
npx --yes pnpm@10.5.2 exec biome check packages/config/package.json packages/config/vite/relativeAliasResolver.test.ts packages/config/vite/relativeAliasResolver.ts packages/config/vitest.config.ts pnpm-lock.yaml
# Checked changed source/config files. No fixes applied.
git diff --check
# no outputThe only failing status I can see on the PR is the external |
|
Expanded the testing-framework slice so this PR now covers two package-level examples instead of only
Validation from this branch:
Note: my first attempt to run via |
1 participant
/claim #54
Summary:
Tests: