mirror of
https://github.com/modelcontextprotocol/servers.git
synced 2026-02-19 11:54:58 -05:00
Address PR review comments: fix UNC path handling, improve test coverage, remove problematic test files
- Fixed UNC path handling bug in normalizePath function to preserve leading double backslashes - Added comprehensive test coverage for drive letter capitalization and UNC paths - Removed file-operations.test.ts and core-functionality.test.ts as they were testing their own code rather than actual server functionality - All path-utils tests now pass with 100% coverage of the actual utility functions
This commit is contained in:
Ola Hungerford
@@ -1,119 +0,0 @@
|
||||
import { describe, it, expect, beforeEach, afterEach } from '@jest/globals';
|
||||
import fs from 'fs/promises';
|
||||
import path from 'path';
|
||||
import os from 'os';
|
||||
|
||||
describe('Core Filesystem Functionality', () => {
|
||||
let tempDir: string;
|
||||
let testFile: string;
|
||||
let testDir: string;
|
||||
|
||||
beforeEach(async () => {
|
||||
// Create a temporary directory for testing
|
||||
tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'filesystem-core-test-'));
|
||||
testFile = path.join(tempDir, 'test.txt');
|
||||
testDir = path.join(tempDir, 'testdir');
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
// Clean up temporary files
|
||||
try {
|
||||
await fs.rm(tempDir, { recursive: true, force: true });
|
||||
} catch (error) {
|
||||
// Ignore cleanup errors
|
||||
}
|
||||
});
|
||||
|
||||
describe('Core File Operations', () => {
|
||||
it('should read a simple text file', async () => {
|
||||
const content = 'Hello, world!\nThis is a test file.';
|
||||
await fs.writeFile(testFile, content);
|
||||
|
||||
const readContent = await fs.readFile(testFile, 'utf-8');
|
||||
expect(readContent).toBe(content);
|
||||
});
|
||||
|
||||
it('should write and create a file', async () => {
|
||||
const content = 'This is test content for writing.';
|
||||
|
||||
// Write file
|
||||
await fs.writeFile(testFile, content);
|
||||
|
||||
// Verify it was written correctly
|
||||
const readContent = await fs.readFile(testFile, 'utf-8');
|
||||
expect(readContent).toBe(content);
|
||||
|
||||
// Verify file exists
|
||||
const stats = await fs.stat(testFile);
|
||||
expect(stats.isFile()).toBe(true);
|
||||
});
|
||||
|
||||
it('should create a directory', async () => {
|
||||
// Create directory
|
||||
await fs.mkdir(testDir);
|
||||
|
||||
// Verify directory was created
|
||||
const stats = await fs.stat(testDir);
|
||||
expect(stats.isDirectory()).toBe(true);
|
||||
});
|
||||
|
||||
it('should list directory contents', async () => {
|
||||
// Create test directory and files
|
||||
await fs.mkdir(testDir);
|
||||
await fs.writeFile(path.join(testDir, 'file1.txt'), 'content1');
|
||||
await fs.writeFile(path.join(testDir, 'file2.txt'), 'content2');
|
||||
await fs.mkdir(path.join(testDir, 'subdir'));
|
||||
|
||||
// List directory contents
|
||||
const entries = await fs.readdir(testDir, { withFileTypes: true });
|
||||
|
||||
expect(entries.length).toBe(3);
|
||||
|
||||
const names = entries.map(e => e.name).sort();
|
||||
expect(names).toEqual(['file1.txt', 'file2.txt', 'subdir']);
|
||||
|
||||
// Verify types
|
||||
const files = entries.filter(e => e.isFile()).map(e => e.name).sort();
|
||||
const dirs = entries.filter(e => e.isDirectory()).map(e => e.name);
|
||||
|
||||
expect(files).toEqual(['file1.txt', 'file2.txt']);
|
||||
expect(dirs).toEqual(['subdir']);
|
||||
});
|
||||
|
||||
it('should get file info and metadata', async () => {
|
||||
const content = 'Test content for file info';
|
||||
await fs.writeFile(testFile, content);
|
||||
|
||||
const stats = await fs.stat(testFile);
|
||||
|
||||
expect(stats.isFile()).toBe(true);
|
||||
expect(stats.isDirectory()).toBe(false);
|
||||
expect(stats.size).toBe(content.length);
|
||||
expect(stats.mtime).toBeDefined();
|
||||
expect(stats.birthtime).toBeDefined();
|
||||
expect(typeof stats.mtime.getTime()).toBe('number');
|
||||
expect(typeof stats.birthtime.getTime()).toBe('number');
|
||||
});
|
||||
});
|
||||
|
||||
describe('Error Handling', () => {
|
||||
it('should handle reading non-existent file', async () => {
|
||||
const nonExistentFile = path.join(tempDir, 'does-not-exist.txt');
|
||||
|
||||
await expect(fs.readFile(nonExistentFile, 'utf-8')).rejects.toThrow();
|
||||
});
|
||||
|
||||
it('should handle writing to invalid path', async () => {
|
||||
// Try to write to a path that doesn't exist (parent directory missing)
|
||||
const invalidPath = path.join(tempDir, 'nonexistent', 'subdir', 'file.txt');
|
||||
|
||||
await expect(fs.writeFile(invalidPath, 'content')).rejects.toThrow();
|
||||
});
|
||||
|
||||
it('should handle listing non-existent directory', async () => {
|
||||
const nonExistentDir = path.join(tempDir, 'does-not-exist');
|
||||
|
||||
await expect(fs.readdir(nonExistentDir)).rejects.toThrow();
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -1,252 +0,0 @@
|
||||
import { describe, it, expect, beforeEach, afterEach } from '@jest/globals';
|
||||
import fs from 'fs/promises';
|
||||
import path from 'path';
|
||||
import os from 'os';
|
||||
|
||||
// Test the actual functions by importing them directly
|
||||
// We'll create a minimal version to test the core logic
|
||||
|
||||
describe('File Operations', () => {
|
||||
let tempDir: string;
|
||||
let testFile: string;
|
||||
let testDir: string;
|
||||
|
||||
beforeEach(async () => {
|
||||
// Create a temporary directory for testing
|
||||
tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'filesystem-test-'));
|
||||
testFile = path.join(tempDir, 'test.txt');
|
||||
testDir = path.join(tempDir, 'testdir');
|
||||
|
||||
// Create test directory
|
||||
await fs.mkdir(testDir);
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
// Clean up temporary files
|
||||
try {
|
||||
await fs.rm(tempDir, { recursive: true, force: true });
|
||||
} catch (error) {
|
||||
// Ignore cleanup errors
|
||||
}
|
||||
});
|
||||
|
||||
describe('formatSize helper function', () => {
|
||||
// Test the formatSize function logic directly
|
||||
it('should format bytes correctly', () => {
|
||||
// Recreate the formatSize function logic for testing
|
||||
function formatSize(bytes: number): string {
|
||||
const units = ['B', 'KB', 'MB', 'GB', 'TB'];
|
||||
if (bytes === 0) return '0 B';
|
||||
|
||||
const i = Math.floor(Math.log(bytes) / Math.log(1024));
|
||||
if (i === 0) return `${bytes} ${units[i]}`;
|
||||
|
||||
return `${(bytes / Math.pow(1024, i)).toFixed(2)} ${units[i]}`;
|
||||
}
|
||||
|
||||
const testCases = [
|
||||
{ bytes: 0, expected: '0 B' },
|
||||
{ bytes: 500, expected: '500 B' },
|
||||
{ bytes: 1024, expected: '1.00 KB' },
|
||||
{ bytes: 1536, expected: '1.50 KB' },
|
||||
{ bytes: 1048576, expected: '1.00 MB' },
|
||||
{ bytes: 1073741824, expected: '1.00 GB' }
|
||||
];
|
||||
|
||||
testCases.forEach(testCase => {
|
||||
expect(formatSize(testCase.bytes)).toBe(testCase.expected);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('headFile function', () => {
|
||||
// Test the head functionality logic
|
||||
async function simulateHeadFile(filePath: string, numLines: number): Promise<string> {
|
||||
const fileHandle = await fs.open(filePath, 'r');
|
||||
try {
|
||||
const lines: string[] = [];
|
||||
let buffer = '';
|
||||
let bytesRead = 0;
|
||||
const chunk = Buffer.alloc(1024);
|
||||
|
||||
while (lines.length < numLines) {
|
||||
const result = await fileHandle.read(chunk, 0, chunk.length, bytesRead);
|
||||
if (result.bytesRead === 0) break;
|
||||
bytesRead += result.bytesRead;
|
||||
buffer += chunk.slice(0, result.bytesRead).toString('utf-8');
|
||||
|
||||
const newLineIndex = buffer.lastIndexOf('\n');
|
||||
if (newLineIndex !== -1) {
|
||||
const completeLines = buffer.slice(0, newLineIndex).split('\n');
|
||||
buffer = buffer.slice(newLineIndex + 1);
|
||||
for (const line of completeLines) {
|
||||
lines.push(line);
|
||||
if (lines.length >= numLines) break;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if (buffer.length > 0 && lines.length < numLines) {
|
||||
lines.push(buffer);
|
||||
}
|
||||
|
||||
return lines.join('\n');
|
||||
} finally {
|
||||
await fileHandle.close();
|
||||
}
|
||||
}
|
||||
|
||||
it('should read first N lines of a file', async () => {
|
||||
const content = 'Line 1\nLine 2\nLine 3\nLine 4\nLine 5\n';
|
||||
await fs.writeFile(testFile, content);
|
||||
|
||||
const result = await simulateHeadFile(testFile, 3);
|
||||
expect(result).toBe('Line 1\nLine 2\nLine 3');
|
||||
});
|
||||
|
||||
it('should handle empty files', async () => {
|
||||
await fs.writeFile(testFile, '');
|
||||
|
||||
const result = await simulateHeadFile(testFile, 3);
|
||||
expect(result).toBe('');
|
||||
});
|
||||
|
||||
it('should handle files with fewer lines than requested', async () => {
|
||||
const content = 'Line 1\nLine 2';
|
||||
await fs.writeFile(testFile, content);
|
||||
|
||||
const result = await simulateHeadFile(testFile, 5);
|
||||
expect(result).toBe('Line 1\nLine 2');
|
||||
});
|
||||
});
|
||||
|
||||
describe('tailFile function', () => {
|
||||
// Test the tail functionality logic
|
||||
async function simulateTailFile(filePath: string, numLines: number): Promise<string> {
|
||||
function normalizeLineEndings(text: string): string {
|
||||
return text.replace(/\r\n/g, '\n');
|
||||
}
|
||||
|
||||
const CHUNK_SIZE = 1024;
|
||||
const stats = await fs.stat(filePath);
|
||||
const fileSize = stats.size;
|
||||
|
||||
if (fileSize === 0) return '';
|
||||
|
||||
const fileHandle = await fs.open(filePath, 'r');
|
||||
try {
|
||||
const lines: string[] = [];
|
||||
let position = fileSize;
|
||||
let chunk = Buffer.alloc(CHUNK_SIZE);
|
||||
let linesFound = 0;
|
||||
let remainingText = '';
|
||||
|
||||
while (position > 0 && linesFound < numLines) {
|
||||
const size = Math.min(CHUNK_SIZE, position);
|
||||
position -= size;
|
||||
|
||||
const { bytesRead } = await fileHandle.read(chunk, 0, size, position);
|
||||
if (!bytesRead) break;
|
||||
|
||||
const readData = chunk.slice(0, bytesRead).toString('utf-8');
|
||||
const chunkText = readData + remainingText;
|
||||
|
||||
const chunkLines = normalizeLineEndings(chunkText).split('\n');
|
||||
|
||||
if (position > 0) {
|
||||
remainingText = chunkLines[0];
|
||||
chunkLines.shift();
|
||||
}
|
||||
|
||||
for (let i = chunkLines.length - 1; i >= 0 && linesFound < numLines; i--) {
|
||||
lines.unshift(chunkLines[i]);
|
||||
linesFound++;
|
||||
}
|
||||
}
|
||||
|
||||
return lines.join('\n');
|
||||
} finally {
|
||||
await fileHandle.close();
|
||||
}
|
||||
}
|
||||
|
||||
it('should read last N lines of a file', async () => {
|
||||
const content = 'Line 1\nLine 2\nLine 3\nLine 4\nLine 5';
|
||||
await fs.writeFile(testFile, content);
|
||||
|
||||
const result = await simulateTailFile(testFile, 3);
|
||||
expect(result).toBe('Line 3\nLine 4\nLine 5');
|
||||
});
|
||||
|
||||
it('should handle empty files', async () => {
|
||||
await fs.writeFile(testFile, '');
|
||||
|
||||
const result = await simulateTailFile(testFile, 3);
|
||||
expect(result).toBe('');
|
||||
});
|
||||
});
|
||||
|
||||
describe('head and tail parameter validation', () => {
|
||||
it('should reject when both head and tail are specified', () => {
|
||||
// This tests the validation logic that should throw an error
|
||||
// when both parameters are provided
|
||||
const hasHead = true;
|
||||
const hasTail = true;
|
||||
|
||||
if (hasHead && hasTail) {
|
||||
expect(() => {
|
||||
throw new Error("Cannot specify both head and tail parameters simultaneously");
|
||||
}).toThrow("Cannot specify both head and tail parameters simultaneously");
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('list_directory_with_sizes functionality', () => {
|
||||
it('should list directory contents with sizes', async () => {
|
||||
// Create test files with known content
|
||||
await fs.writeFile(path.join(testDir, 'small.txt'), 'hello');
|
||||
await fs.writeFile(path.join(testDir, 'large.txt'), 'x'.repeat(1024));
|
||||
await fs.mkdir(path.join(testDir, 'subdir'));
|
||||
|
||||
// Read the directory and verify structure
|
||||
const entries = await fs.readdir(testDir, { withFileTypes: true });
|
||||
|
||||
expect(entries.length).toBe(3);
|
||||
|
||||
const fileNames = entries.map(e => e.name).sort();
|
||||
expect(fileNames).toEqual(['large.txt', 'small.txt', 'subdir']);
|
||||
|
||||
// Verify we can distinguish files from directories
|
||||
const dirs = entries.filter(e => e.isDirectory()).map(e => e.name);
|
||||
const files = entries.filter(e => e.isFile()).map(e => e.name);
|
||||
|
||||
expect(dirs).toEqual(['subdir']);
|
||||
expect(files.sort()).toEqual(['large.txt', 'small.txt']);
|
||||
});
|
||||
|
||||
it('should handle sorting by name and size', async () => {
|
||||
// Create files with different sizes
|
||||
await fs.writeFile(path.join(testDir, 'big.txt'), 'x'.repeat(2000));
|
||||
await fs.writeFile(path.join(testDir, 'small.txt'), 'hi');
|
||||
await fs.writeFile(path.join(testDir, 'medium.txt'), 'x'.repeat(1000));
|
||||
|
||||
const entries = await fs.readdir(testDir, { withFileTypes: true });
|
||||
|
||||
// Get file sizes
|
||||
const filesWithSizes = await Promise.all(
|
||||
entries.filter(e => e.isFile()).map(async (entry) => {
|
||||
const stats = await fs.stat(path.join(testDir, entry.name));
|
||||
return { name: entry.name, size: stats.size };
|
||||
})
|
||||
);
|
||||
|
||||
// Test name sorting
|
||||
const sortedByName = [...filesWithSizes].sort((a, b) => a.name.localeCompare(b.name));
|
||||
expect(sortedByName.map(f => f.name)).toEqual(['big.txt', 'medium.txt', 'small.txt']);
|
||||
|
||||
// Test size sorting (descending)
|
||||
const sortedBySize = [...filesWithSizes].sort((a, b) => b.size - a.size);
|
||||
expect(sortedBySize.map(f => f.name)).toEqual(['big.txt', 'medium.txt', 'small.txt']);
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -124,6 +124,35 @@ describe('Path Utilities', () => {
|
||||
expect(normalizePath('C:\\Path with @at+plus$dollar%percent'))
|
||||
.toBe('C:\\Path with @at+plus$dollar%percent');
|
||||
});
|
||||
|
||||
it('capitalizes lowercase drive letters for Windows paths', () => {
|
||||
expect(normalizePath('c:/windows/system32'))
|
||||
.toBe('C:\\windows\\system32');
|
||||
expect(normalizePath('/mnt/d/my/folder')) // WSL path with lowercase drive
|
||||
.toBe('D:\\my\\folder');
|
||||
expect(normalizePath('/e/another/folder')) // Unix-style Windows path with lowercase drive
|
||||
.toBe('E:\\another\\folder');
|
||||
});
|
||||
|
||||
it('handles UNC paths correctly', () => {
|
||||
// UNC paths should preserve the leading double backslash
|
||||
const uncPath = '\\\\SERVER\\share\\folder';
|
||||
expect(normalizePath(uncPath)).toBe('\\\\SERVER\\share\\folder');
|
||||
|
||||
// Test UNC path with double backslashes that need normalization
|
||||
const uncPathWithDoubles = '\\\\\\\\SERVER\\\\share\\\\folder';
|
||||
expect(normalizePath(uncPathWithDoubles)).toBe('\\\\SERVER\\share\\folder');
|
||||
});
|
||||
|
||||
it('returns normalized non-Windows/WSL/Unix-style Windows paths as is after basic normalization', () => {
|
||||
// Relative path
|
||||
const relativePath = 'some/relative/path';
|
||||
expect(normalizePath(relativePath)).toBe(relativePath.replace(/\//g, '\\'));
|
||||
|
||||
// A path that looks somewhat absolute but isn't a drive or recognized Unix root for Windows conversion
|
||||
const otherAbsolutePath = '\\someserver\\share\\file';
|
||||
expect(normalizePath(otherAbsolutePath)).toBe(otherAbsolutePath);
|
||||
});
|
||||
});
|
||||
|
||||
describe('expandHome', () => {
|
||||
|
||||
@@ -53,11 +53,28 @@ export function normalizePath(p: string): string {
|
||||
// Convert WSL or Unix-style Windows paths to Windows format
|
||||
p = convertToWindowsPath(p);
|
||||
|
||||
// Handle double backslashes and ensure proper escaping
|
||||
p = p.replace(/\\\\/g, '\\');
|
||||
// Check if this is a UNC path before normalization
|
||||
const isUNCPath = p.startsWith('\\\\');
|
||||
|
||||
// For non-UNC paths, normalize double backslashes first
|
||||
if (!isUNCPath) {
|
||||
p = p.replace(/\\\\/g, '\\');
|
||||
}
|
||||
|
||||
// Use Node's path normalization, which handles . and .. segments
|
||||
const normalized = path.normalize(p);
|
||||
let normalized = path.normalize(p);
|
||||
|
||||
// Handle UNC paths after normalization to preserve the leading \\
|
||||
if (isUNCPath) {
|
||||
// Ensure UNC path starts with exactly two backslashes
|
||||
if (normalized.startsWith('\\') && !normalized.startsWith('\\\\')) {
|
||||
normalized = '\\' + normalized;
|
||||
}
|
||||
// Normalize any remaining double backslashes in the rest of the path
|
||||
normalized = normalized.replace(/^(\\\\)(.*)/, (match, leading, rest) => {
|
||||
return leading + rest.replace(/\\\\/g, '\\');
|
||||
});
|
||||
}
|
||||
|
||||
// Handle Windows paths: convert slashes and ensure drive letter is capitalized
|
||||
if (normalized.match(/^[a-zA-Z]:|^\/mnt\/[a-z]\/|^\/[a-z]\//i)) {
|
||||
|
||||
Reference in New Issue
Block a user