Skip to content

Commit 4c8f5e1

Browse files
felixweinbergerfelixweinberger
committed
Refactor tool name validation tests using test.each
Reduce test file size by 46% (109 lines) using parameterized tests. This improves readability while maintaining the same test coverage.
1 parent 5cabbe7 commit 4c8f5e1

File tree

1 file changed

+65
-174
lines changed

1 file changed

+65
-174
lines changed

‎src/shared/toolNameValidation.test.ts‎

Lines changed: 65 additions & 174 deletions
Original file line numberDiff line numberDiff line change
@@ -13,142 +13,54 @@ afterEach(() => {
1313

1414
describe('validateToolName', () => {
1515
describe('valid tool names', () => {
16-
test('should accept simple alphanumeric names', () => {
17-
const result = validateToolName('getUser');
18-
expect(result.isValid).toBe(true);
19-
expect(result.warnings).toHaveLength(0);
20-
});
21-
22-
test('should accept names with underscores', () => {
23-
const result = validateToolName('get_user_profile');
24-
expect(result.isValid).toBe(true);
25-
expect(result.warnings).toHaveLength(0);
26-
});
27-
28-
test('should accept names with dashes', () => {
29-
const result = validateToolName('user-profile-update');
30-
expect(result.isValid).toBe(true);
31-
expect(result.warnings).toHaveLength(0);
32-
});
33-
34-
test('should accept names with dots', () => {
35-
const result = validateToolName('admin.tools.list');
36-
expect(result.isValid).toBe(true);
37-
expect(result.warnings).toHaveLength(0);
38-
});
39-
40-
test('should reject names with forward slashes', () => {
41-
const result = validateToolName('user/profile/update');
42-
expect(result.isValid).toBe(false);
43-
expect(result.warnings).toContain('Tool name contains invalid characters: "/"');
44-
});
45-
46-
test('should accept mixed character names', () => {
47-
const result = validateToolName('DATA_EXPORT_v2.1');
48-
expect(result.isValid).toBe(true);
49-
expect(result.warnings).toHaveLength(0);
50-
});
51-
52-
test('should accept single character names', () => {
53-
const result = validateToolName('a');
54-
expect(result.isValid).toBe(true);
55-
expect(result.warnings).toHaveLength(0);
56-
});
57-
58-
test('should accept 128 character names', () => {
59-
const name = 'a'.repeat(128);
60-
const result = validateToolName(name);
16+
test.each`
17+
description | toolName
18+
${'simple alphanumeric names'} | ${'getUser'}
19+
${'names with underscores'} | ${'get_user_profile'}
20+
${'names with dashes'} | ${'user-profile-update'}
21+
${'names with dots'} | ${'admin.tools.list'}
22+
${'mixed character names'} | ${'DATA_EXPORT_v2.1'}
23+
${'single character names'} | ${'a'}
24+
${'128 character names'} | ${'a'.repeat(128)}
25+
`('should accept $description', ({ toolName }) => {
26+
const result = validateToolName(toolName);
6127
expect(result.isValid).toBe(true);
6228
expect(result.warnings).toHaveLength(0);
6329
});
6430
});
6531

6632
describe('invalid tool names', () => {
67-
test('should reject empty names', () => {
68-
const result = validateToolName('');
69-
expect(result.isValid).toBe(false);
70-
expect(result.warnings).toContain('Tool name cannot be empty');
71-
});
72-
73-
test('should reject names longer than 128 characters', () => {
74-
const name = 'a'.repeat(129);
75-
const result = validateToolName(name);
76-
expect(result.isValid).toBe(false);
77-
expect(result.warnings).toContain('Tool name exceeds maximum length of 128 characters (current: 129)');
78-
});
79-
80-
test('should reject names with spaces', () => {
81-
const result = validateToolName('get user profile');
82-
expect(result.isValid).toBe(false);
83-
expect(result.warnings).toContain('Tool name contains invalid characters: " "');
84-
});
85-
86-
test('should reject names with commas', () => {
87-
const result = validateToolName('get,user,profile');
88-
expect(result.isValid).toBe(false);
89-
expect(result.warnings).toContain('Tool name contains invalid characters: ","');
90-
});
91-
92-
test('should reject names with other special characters', () => {
93-
const result = validateToolName('user@domain.com');
94-
expect(result.isValid).toBe(false);
95-
expect(result.warnings).toContain('Tool name contains invalid characters: "@"');
96-
});
97-
98-
test('should reject names with multiple invalid characters', () => {
99-
const result = validateToolName('user name@domain,com');
100-
expect(result.isValid).toBe(false);
101-
expect(result.warnings).toContain('Tool name contains invalid characters: " ", "@", ","');
102-
});
103-
104-
test('should reject names with unicode characters', () => {
105-
const result = validateToolName('user-ñame');
33+
test.each`
34+
description | toolName | expectedWarning
35+
${'empty names'} | ${''} | ${'Tool name cannot be empty'}
36+
${'names longer than 128 characters'} | ${'a'.repeat(129)} | ${'Tool name exceeds maximum length of 128 characters (current: 129)'}
37+
${'names with spaces'} | ${'get user profile'} | ${'Tool name contains invalid characters: " "'}
38+
${'names with commas'} | ${'get,user,profile'} | ${'Tool name contains invalid characters: ","'}
39+
${'names with forward slashes'} | ${'user/profile/update'} | ${'Tool name contains invalid characters: "/"'}
40+
${'names with other special chars'} | ${'user@domain.com'} | ${'Tool name contains invalid characters: "@"'}
41+
${'names with multiple invalid chars'} | ${'user name@domain,com'} | ${'Tool name contains invalid characters: " ", "@", ","'}
42+
${'names with unicode characters'} | ${'user-ñame'} | ${'Tool name contains invalid characters: "ñ"'}
43+
`('should reject $description', ({ toolName, expectedWarning }) => {
44+
const result = validateToolName(toolName);
10645
expect(result.isValid).toBe(false);
107-
expect(result.warnings).toContain('Tool name contains invalid characters: "ñ"');
46+
expect(result.warnings).toContain(expectedWarning);
10847
});
10948
});
11049

11150
describe('warnings for potentially problematic patterns', () => {
112-
test('should warn about names with spaces', () => {
113-
const result = validateToolName('get user profile');
114-
expect(result.isValid).toBe(false);
115-
expect(result.warnings).toContain('Tool name contains spaces, which may cause parsing issues');
116-
});
117-
118-
test('should warn about names with commas', () => {
119-
const result = validateToolName('get,user,profile');
120-
expect(result.isValid).toBe(false);
121-
expect(result.warnings).toContain('Tool name contains commas, which may cause parsing issues');
122-
});
123-
124-
test('should warn about names starting with dash', () => {
125-
const result = validateToolName('-get-user');
126-
expect(result.isValid).toBe(true);
127-
expect(result.warnings).toContain('Tool name starts or ends with a dash, which may cause parsing issues in some contexts');
128-
});
129-
130-
test('should warn about names ending with dash', () => {
131-
const result = validateToolName('get-user-');
132-
expect(result.isValid).toBe(true);
133-
expect(result.warnings).toContain('Tool name starts or ends with a dash, which may cause parsing issues in some contexts');
134-
});
135-
136-
test('should warn about names starting with dot', () => {
137-
const result = validateToolName('.get.user');
138-
expect(result.isValid).toBe(true);
139-
expect(result.warnings).toContain('Tool name starts or ends with a dot, which may cause parsing issues in some contexts');
140-
});
141-
142-
test('should warn about names ending with dot', () => {
143-
const result = validateToolName('get.user.');
144-
expect(result.isValid).toBe(true);
145-
expect(result.warnings).toContain('Tool name starts or ends with a dot, which may cause parsing issues in some contexts');
146-
});
147-
148-
test('should warn about names with both leading and trailing dots', () => {
149-
const result = validateToolName('.get.user.');
150-
expect(result.isValid).toBe(true);
151-
expect(result.warnings).toContain('Tool name starts or ends with a dot, which may cause parsing issues in some contexts');
51+
test.each`
52+
description | toolName | expectedWarning | shouldBeValid
53+
${'names with spaces'} | ${'get user profile'} | ${'Tool name contains spaces, which may cause parsing issues'} | ${false}
54+
${'names with commas'} | ${'get,user,profile'} | ${'Tool name contains commas, which may cause parsing issues'} | ${false}
55+
${'names starting with dash'} | ${'-get-user'} | ${'Tool name starts or ends with a dash, which may cause parsing issues in some contexts'} | ${true}
56+
${'names ending with dash'} | ${'get-user-'} | ${'Tool name starts or ends with a dash, which may cause parsing issues in some contexts'} | ${true}
57+
${'names starting with dot'} | ${'.get.user'} | ${'Tool name starts or ends with a dot, which may cause parsing issues in some contexts'} | ${true}
58+
${'names ending with dot'} | ${'get.user.'} | ${'Tool name starts or ends with a dot, which may cause parsing issues in some contexts'} | ${true}
59+
${'names with leading and trailing dots'} | ${'.get.user.'} | ${'Tool name starts or ends with a dot, which may cause parsing issues in some contexts'} | ${true}
60+
`('should warn about $description', ({ toolName, expectedWarning, shouldBeValid }) => {
61+
const result = validateToolName(toolName);
62+
expect(result.isValid).toBe(shouldBeValid);
63+
expect(result.warnings).toContain(expectedWarning);
15264
});
15365
});
15466
});
@@ -175,62 +87,41 @@ describe('issueToolNameWarning', () => {
17587
});
17688

17789
describe('validateAndWarnToolName', () => {
178-
test('should return true and issue warnings for valid names with warnings', () => {
179-
const result = validateAndWarnToolName('-get-user-');
180-
expect(result).toBe(true);
181-
expect(warnSpy).toHaveBeenCalled();
90+
test.each`
91+
description | toolName | expectedResult | shouldWarn
92+
${'valid names with warnings'} | ${'-get-user-'} | ${true} | ${true}
93+
${'completely valid names'} | ${'get-user-profile'} | ${true} | ${false}
94+
${'invalid names with spaces'} | ${'get user profile'} | ${false} | ${true}
95+
${'empty names'} | ${''} | ${false} | ${true}
96+
${'names exceeding length limit'} | ${'a'.repeat(129)} | ${false} | ${true}
97+
`('should handle $description', ({ toolName, expectedResult, shouldWarn }) => {
98+
const result = validateAndWarnToolName(toolName);
99+
expect(result).toBe(expectedResult);
100+
101+
if (shouldWarn) {
102+
expect(warnSpy).toHaveBeenCalled();
103+
} else {
104+
expect(warnSpy).not.toHaveBeenCalled();
105+
}
182106
});
183107

184-
test('should return true and not issue warnings for completely valid names', () => {
185-
const result = validateAndWarnToolName('get-user-profile');
186-
expect(result).toBe(true);
187-
expect(warnSpy).not.toHaveBeenCalled();
188-
});
189-
190-
test('should return false and issue warnings for invalid names', () => {
191-
const result = validateAndWarnToolName('get user profile');
192-
expect(result).toBe(false);
193-
expect(warnSpy).toHaveBeenCalled(); // Now issues warnings instead of errors
108+
test('should include space warning for invalid names with spaces', () => {
109+
validateAndWarnToolName('get user profile');
194110
const warningCalls = warnSpy.mock.calls.map(call => call.join(' '));
195111
expect(warningCalls.some(call => call.includes('Tool name contains spaces'))).toBe(true);
196112
});
197-
198-
test('should return false for empty names', () => {
199-
const result = validateAndWarnToolName('');
200-
expect(result).toBe(false);
201-
expect(warnSpy).toHaveBeenCalled(); // Now issues warnings instead of errors
202-
});
203-
204-
test('should return false for names exceeding length limit', () => {
205-
const longName = 'a'.repeat(129);
206-
const result = validateAndWarnToolName(longName);
207-
expect(result).toBe(false);
208-
expect(warnSpy).toHaveBeenCalled(); // Now issues warnings instead of errors
209-
});
210113
});
211114

212115
describe('edge cases and robustness', () => {
213-
test('should warn about names with only dots', () => {
214-
const result = validateToolName('...');
215-
expect(result.isValid).toBe(true);
216-
expect(result.warnings).toContain('Tool name starts or ends with a dot, which may cause parsing issues in some contexts');
217-
});
218-
219-
test('should handle names with only dashes', () => {
220-
const result = validateToolName('---');
221-
expect(result.isValid).toBe(true);
222-
expect(result.warnings).toContain('Tool name starts or ends with a dash, which may cause parsing issues in some contexts');
223-
});
224-
225-
test('should reject names with only forward slashes', () => {
226-
const result = validateToolName('///');
227-
expect(result.isValid).toBe(false);
228-
expect(result.warnings).toContain('Tool name contains invalid characters: "/"');
229-
});
230-
231-
test('should handle names with mixed valid and invalid characters', () => {
232-
const result = validateToolName('user@name123');
233-
expect(result.isValid).toBe(false);
234-
expect(result.warnings).toContain('Tool name contains invalid characters: "@"');
116+
test.each`
117+
description | toolName | shouldBeValid | expectedWarning
118+
${'names with only dots'} | ${'...'} | ${true} | ${'Tool name starts or ends with a dot, which may cause parsing issues in some contexts'}
119+
${'names with only dashes'} | ${'---'} | ${true} | ${'Tool name starts or ends with a dash, which may cause parsing issues in some contexts'}
120+
${'names with only forward slashes'} | ${'///'} | ${false} | ${'Tool name contains invalid characters: "/"'}
121+
${'names with mixed valid/invalid chars'} | ${'user@name123'} | ${false} | ${'Tool name contains invalid characters: "@"'}
122+
`('should handle $description', ({ toolName, shouldBeValid, expectedWarning }) => {
123+
const result = validateToolName(toolName);
124+
expect(result.isValid).toBe(shouldBeValid);
125+
expect(result.warnings).toContain(expectedWarning);
235126
});
236127
});

0 commit comments

Comments
 (0)