Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 72 additions & 0 deletions src/utils/__tests__/storage.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"
import * as fs from "fs/promises"
import * as path from "path"
import * as os from "os"
import { getStorageBasePath } from "../storage"

// Mock VSCode
vi.mock("vscode", () => ({
workspace: {
getConfiguration: vi.fn(),
},
window: {
showErrorMessage: vi.fn(),
},
}))

// Mock i18n
vi.mock("../../i18n", () => ({
t: vi.fn((key: string) => key),
}))

describe("getStorageBasePath", () => {
let tempDir: string

beforeEach(async () => {
tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "storage-test-"))
vi.clearAllMocks()
})

afterEach(async () => {
await fs.rm(tempDir, { recursive: true, force: true })
vi.restoreAllMocks()
})

it("should handle concurrent storage validations without race conditions", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Excellent test coverage for the race condition scenario! For future enhancements, consider adding test cases for:

  • Custom path becoming inaccessible during runtime
  • Invalid/non-writable custom path handling
  • Fallback to default path when custom path fails

These would provide even more comprehensive coverage of edge cases.

const customPath = path.join(tempDir, "custom-storage")
const defaultPath = path.join(tempDir, "default-storage")

// Mock VSCode configuration to return custom path
const mockConfig = {
get: vi.fn().mockReturnValue(customPath),
has: vi.fn().mockReturnValue(true),
inspect: vi.fn(),
update: vi.fn(),
} as any

const vscode = await import("vscode")
vi.mocked(vscode.workspace.getConfiguration).mockReturnValue(mockConfig)

// Create the custom storage directory
await fs.mkdir(customPath, { recursive: true })

// Run multiple concurrent storage validations
const concurrentCalls = Array(10)
.fill(null)
.map(() => getStorageBasePath(defaultPath))

// All should succeed and return the custom path
const results = await Promise.all(concurrentCalls)

// Verify all calls succeeded
expect(results).toHaveLength(10)
results.forEach((result) => {
expect(result).toBe(customPath)
})

// Verify no leftover test files (all should be cleaned up with unique names)
const dirContents = await fs.readdir(customPath)
const testFiles = dirContents.filter((file) => file.startsWith(".write_test"))
expect(testFiles).toHaveLength(0)
})
})
5 changes: 3 additions & 2 deletions src/utils/storage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,9 @@ export async function getStorageBasePath(defaultPath: string): Promise<string> {
// Ensure custom path exists
await fs.mkdir(customStoragePath, { recursive: true })

// Test if path is writable
const testFile = path.join(customStoragePath, ".write_test")
// Test if path is writable (use unique filename to avoid race conditions)
Copy link
Contributor

Choose a reason for hiding this comment

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

Great fix! Consider making the comment slightly more detailed to explain why we need the unique suffix, e.g.:

const uniqueSuffix = `${Date.now()}-${Math.random().toString(36).slice(2, 11)}`
Copy link
Contributor

Choose a reason for hiding this comment

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

The unique suffix generation pattern works well. As a minor enhancement for future consideration, this could be extracted to a small utility function if similar unique ID generation is needed elsewhere in the codebase. But for a single use case, the current implementation is perfectly fine.

const testFile = path.join(customStoragePath, `.write_test_${uniqueSuffix}`)
await fs.writeFile(testFile, "test")
await fs.rm(testFile)

Expand Down
Loading