From caf9383ba1299037ef9a93592eba432da64813f6 Mon Sep 17 00:00:00 2001 From: czlonkowski <56956555+czlonkowski@users.noreply.github.com> Date: Sat, 8 Nov 2025 18:49:59 +0100 Subject: [PATCH] test: Add comprehensive edge case coverage for telemetry quick wins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Added 20 edge case tests based on code review recommendations: **Duplicate ID Validation (4 tests)**: - Multiple duplicate IDs (3+ nodes with same ID) - Duplicate IDs with same node type - Duplicate IDs with empty/null node names - Duplicate IDs with missing node properties **AI Agent Validator (16 tests)**: maxIterations edge cases (7 tests): - Boundary values: 0 (reject), 1 (accept), 51 (warn), MAX_SAFE_INTEGER (warn) - Invalid types: NaN (reject), negative decimal (reject) - Threshold testing: 50 vs 51 promptType validation (4 tests): - Whitespace-only text (reject) - Very long text 3200+ chars (accept) - undefined/null text (reject) System message validation (5 tests): - Empty/whitespace messages (suggest adding) - Very long messages >1000 chars (accept) - Special characters, emojis, unicode (accept) - Multi-line formatting (accept) - Boundary: 19 chars (warn), 20 chars (accept) **Test Quality Improvements**: - Fixed flaky system message test (changed from expect.stringContaining to .some()) - All tests are deterministic - Comprehensive inline comments - Follows existing test patterns All 20 new tests passing. Zero regressions. Concieved by Romuald Członkowski - www.aiadvisors.pl/en --- .../services/node-specific-validators.test.ts | 230 +++++++++++++++++- .../unit/services/workflow-validator.test.ts | 217 +++++++++++++++++ 2 files changed, 445 insertions(+), 2 deletions(-) diff --git a/tests/unit/services/node-specific-validators.test.ts b/tests/unit/services/node-specific-validators.test.ts index 8dd8d9a..e0d8ae9 100644 --- a/tests/unit/services/node-specific-validators.test.ts +++ b/tests/unit/services/node-specific-validators.test.ts @@ -2355,6 +2355,62 @@ return [{"json": {"result": result}}] const textErrors = context.errors.filter(e => e.property === 'text'); expect(textErrors).toHaveLength(0); }); + + it('should reject whitespace-only text with promptType "define"', () => { + // Edge case: Text is only whitespace + context.config.promptType = 'define'; + context.config.text = ' \n\t '; + + NodeSpecificValidators.validateAIAgent(context); + + expect(context.errors).toContainEqual({ + type: 'missing_required', + property: 'text', + message: 'Custom prompt text is required when promptType is "define"', + fix: 'Provide a custom prompt in the text field, or change promptType to "auto"' + }); + }); + + it('should accept very long text with promptType "define"', () => { + // Edge case: Very long prompt text (common for complex AI agents) + context.config.promptType = 'define'; + context.config.text = 'You are a helpful assistant. '.repeat(100); // 3200 characters + + NodeSpecificValidators.validateAIAgent(context); + + const textErrors = context.errors.filter(e => e.property === 'text'); + expect(textErrors).toHaveLength(0); + }); + + it('should handle undefined text with promptType "define"', () => { + // Edge case: Text is undefined + context.config.promptType = 'define'; + context.config.text = undefined; + + NodeSpecificValidators.validateAIAgent(context); + + expect(context.errors).toContainEqual({ + type: 'missing_required', + property: 'text', + message: 'Custom prompt text is required when promptType is "define"', + fix: 'Provide a custom prompt in the text field, or change promptType to "auto"' + }); + }); + + it('should handle null text with promptType "define"', () => { + // Edge case: Text is null + context.config.promptType = 'define'; + context.config.text = null; + + NodeSpecificValidators.validateAIAgent(context); + + expect(context.errors).toContainEqual({ + type: 'missing_required', + property: 'text', + message: 'Custom prompt text is required when promptType is "define"', + fix: 'Provide a custom prompt in the text field, or change promptType to "auto"' + }); + }); }); describe('system message validation', () => { @@ -2363,9 +2419,11 @@ return [{"json": {"result": result}}] NodeSpecificValidators.validateAIAgent(context); - expect(context.suggestions).toContain( - expect.stringContaining('system message') + // Should contain a suggestion about system message + const hasSysMessageSuggestion = context.suggestions.some(s => + s.toLowerCase().includes('system message') ); + expect(hasSysMessageSuggestion).toBe(true); }); it('should warn when system message is too short', () => { @@ -2389,6 +2447,93 @@ return [{"json": {"result": result}}] const systemWarnings = context.warnings.filter(w => w.property === 'systemMessage'); expect(systemWarnings).toHaveLength(0); }); + + it('should suggest adding system message when empty string', () => { + // Edge case: Empty string system message + context.config.systemMessage = ''; + + NodeSpecificValidators.validateAIAgent(context); + + // Should contain a suggestion about system message + const hasSysMessageSuggestion = context.suggestions.some(s => + s.toLowerCase().includes('system message') + ); + expect(hasSysMessageSuggestion).toBe(true); + }); + + it('should suggest adding system message when whitespace only', () => { + // Edge case: Whitespace-only system message + context.config.systemMessage = ' \n\t '; + + NodeSpecificValidators.validateAIAgent(context); + + // Should contain a suggestion about system message + const hasSysMessageSuggestion = context.suggestions.some(s => + s.toLowerCase().includes('system message') + ); + expect(hasSysMessageSuggestion).toBe(true); + }); + + it('should accept very long system messages', () => { + // Edge case: Very long system message (>1000 chars) for complex agents + context.config.systemMessage = 'You are a highly specialized assistant. '.repeat(30); // ~1260 chars + + NodeSpecificValidators.validateAIAgent(context); + + const systemWarnings = context.warnings.filter(w => w.property === 'systemMessage'); + expect(systemWarnings).toHaveLength(0); + }); + + it('should handle system messages with special characters', () => { + // Edge case: System message with special characters, emojis, unicode + context.config.systemMessage = 'You are an assistant 🤖 that handles data with special chars: @#$%^&*(){}[]|\\/<>~`'; + + NodeSpecificValidators.validateAIAgent(context); + + const systemWarnings = context.warnings.filter(w => w.property === 'systemMessage'); + expect(systemWarnings).toHaveLength(0); + }); + + it('should handle system messages with newlines and formatting', () => { + // Edge case: Multi-line system message with formatting + context.config.systemMessage = `You are a helpful assistant. + +Your responsibilities include: +1. Analyzing customer feedback +2. Generating reports +3. Providing insights + +Always be professional and concise.`; + + NodeSpecificValidators.validateAIAgent(context); + + const systemWarnings = context.warnings.filter(w => w.property === 'systemMessage'); + expect(systemWarnings).toHaveLength(0); + }); + + it('should warn about exactly 19 character system message', () => { + // Edge case: Just under the 20 character threshold + context.config.systemMessage = 'Be a good assistant'; // 19 chars + + NodeSpecificValidators.validateAIAgent(context); + + expect(context.warnings).toContainEqual({ + type: 'inefficient', + property: 'systemMessage', + message: 'System message is very short (< 20 characters)', + suggestion: 'Consider a more detailed system message to guide the agent\'s behavior' + }); + }); + + it('should not warn about exactly 20 character system message', () => { + // Edge case: Exactly at the 20 character threshold + context.config.systemMessage = 'Be a great assistant'; // 20 chars + + NodeSpecificValidators.validateAIAgent(context); + + const systemWarnings = context.warnings.filter(w => w.property === 'systemMessage'); + expect(systemWarnings).toHaveLength(0); + }); }); describe('maxIterations validation', () => { @@ -2426,6 +2571,87 @@ return [{"json": {"result": result}}] const maxIterErrors = context.errors.filter(e => e.property === 'maxIterations'); expect(maxIterErrors).toHaveLength(0); }); + + it('should reject maxIterations of 0', () => { + // Edge case: Zero iterations is invalid + context.config.maxIterations = 0; + + NodeSpecificValidators.validateAIAgent(context); + + expect(context.errors).toContainEqual({ + type: 'invalid_value', + property: 'maxIterations', + message: 'maxIterations must be a positive number', + fix: 'Set maxIterations to a value >= 1 (e.g., 10)' + }); + }); + + it('should accept maxIterations of 1', () => { + // Edge case: Minimum valid value + context.config.maxIterations = 1; + + NodeSpecificValidators.validateAIAgent(context); + + const maxIterErrors = context.errors.filter(e => e.property === 'maxIterations'); + expect(maxIterErrors).toHaveLength(0); + }); + + it('should warn about maxIterations of 51', () => { + // Edge case: Just above the threshold (50) + context.config.maxIterations = 51; + + NodeSpecificValidators.validateAIAgent(context); + + expect(context.warnings).toContainEqual( + expect.objectContaining({ + type: 'inefficient', + property: 'maxIterations', + message: expect.stringContaining('51') + }) + ); + }); + + it('should handle extreme maxIterations values', () => { + // Edge case: Very large number + context.config.maxIterations = Number.MAX_SAFE_INTEGER; + + NodeSpecificValidators.validateAIAgent(context); + + expect(context.warnings).toContainEqual( + expect.objectContaining({ + type: 'inefficient', + property: 'maxIterations' + }) + ); + }); + + it('should reject NaN maxIterations', () => { + // Edge case: Not a number + context.config.maxIterations = 'invalid'; + + NodeSpecificValidators.validateAIAgent(context); + + expect(context.errors).toContainEqual({ + type: 'invalid_value', + property: 'maxIterations', + message: 'maxIterations must be a positive number', + fix: 'Set maxIterations to a value >= 1 (e.g., 10)' + }); + }); + + it('should reject negative decimal maxIterations', () => { + // Edge case: Negative decimal + context.config.maxIterations = -0.5; + + NodeSpecificValidators.validateAIAgent(context); + + expect(context.errors).toContainEqual({ + type: 'invalid_value', + property: 'maxIterations', + message: 'maxIterations must be a positive number', + fix: 'Set maxIterations to a value >= 1 (e.g., 10)' + }); + }); }); describe('error handling', () => { diff --git a/tests/unit/services/workflow-validator.test.ts b/tests/unit/services/workflow-validator.test.ts index ef11da7..923b2f0 100644 --- a/tests/unit/services/workflow-validator.test.ts +++ b/tests/unit/services/workflow-validator.test.ts @@ -353,5 +353,222 @@ describe('WorkflowValidator', () => { // This validates that our implementation uses the pattern expect(expectedPattern.test('crypto.randomUUID()')).toBe(true); }); + + it('should detect multiple nodes with the same duplicate ID', () => { + // Edge case: Three or more nodes with the same ID + const workflow = { + name: 'Test Workflow with Multiple Duplicates', + nodes: [ + { + id: 'shared-id', + name: 'First Node', + type: 'n8n-nodes-base.httpRequest', + typeVersion: 3, + position: [250, 300], + parameters: {} + }, + { + id: 'shared-id', // Duplicate 1 + name: 'Second Node', + type: 'n8n-nodes-base.set', + typeVersion: 2, + position: [450, 300], + parameters: {} + }, + { + id: 'shared-id', // Duplicate 2 + name: 'Third Node', + type: 'n8n-nodes-base.code', + typeVersion: 1, + position: [650, 300], + parameters: {} + } + ], + connections: {} + }; + + // Simulate validation logic + const nodeIds = new Set(); + const nodeIdToIndex = new Map(); + const errors: Array<{ message: string }> = []; + + for (let i = 0; i < workflow.nodes.length; i++) { + const node = workflow.nodes[i]; + if (nodeIds.has(node.id)) { + const firstNodeIndex = nodeIdToIndex.get(node.id); + const firstNode = firstNodeIndex !== undefined ? workflow.nodes[firstNodeIndex] : undefined; + + errors.push({ + message: `Duplicate node ID: "${node.id}". Node at index ${i} (name: "${node.name}", type: "${node.type}") conflicts with node at index ${firstNodeIndex} (name: "${firstNode?.name || 'unknown'}", type: "${firstNode?.type || 'unknown'}")` + }); + } else { + nodeIds.add(node.id); + nodeIdToIndex.set(node.id, i); + } + } + + // Should report 2 errors (nodes at index 1 and 2 both conflict with node at index 0) + expect(errors).toHaveLength(2); + expect(errors[0].message).toContain('index 1'); + expect(errors[0].message).toContain('Second Node'); + expect(errors[1].message).toContain('index 2'); + expect(errors[1].message).toContain('Third Node'); + }); + + it('should handle duplicate IDs with same node type', () => { + // Edge case: Both nodes are the same type + const workflow = { + name: 'Test Workflow with Same Type Duplicates', + nodes: [ + { + id: 'duplicate-slack', + name: 'Slack Send 1', + type: 'n8n-nodes-base.slack', + typeVersion: 2, + position: [250, 300], + parameters: {} + }, + { + id: 'duplicate-slack', + name: 'Slack Send 2', + type: 'n8n-nodes-base.slack', + typeVersion: 2, + position: [450, 300], + parameters: {} + } + ], + connections: {} + }; + + // Simulate validation logic + const nodeIds = new Set(); + const nodeIdToIndex = new Map(); + const errors: Array<{ message: string }> = []; + + for (let i = 0; i < workflow.nodes.length; i++) { + const node = workflow.nodes[i]; + if (nodeIds.has(node.id)) { + const firstNodeIndex = nodeIdToIndex.get(node.id); + const firstNode = firstNodeIndex !== undefined ? workflow.nodes[firstNodeIndex] : undefined; + + errors.push({ + message: `Duplicate node ID: "${node.id}". Node at index ${i} (name: "${node.name}", type: "${node.type}") conflicts with node at index ${firstNodeIndex} (name: "${firstNode?.name || 'unknown'}", type: "${firstNode?.type || 'unknown'}")` + }); + } else { + nodeIds.add(node.id); + nodeIdToIndex.set(node.id, i); + } + } + + expect(errors).toHaveLength(1); + expect(errors[0].message).toContain('Duplicate node ID: "duplicate-slack"'); + expect(errors[0].message).toContain('Slack Send 2'); + expect(errors[0].message).toContain('Slack Send 1'); + // Both should show the same type + expect(errors[0].message).toMatch(/n8n-nodes-base\.slack.*n8n-nodes-base\.slack/s); + }); + + it('should handle duplicate IDs with empty node names gracefully', () => { + // Edge case: Empty string node names + const workflow = { + name: 'Test Workflow with Empty Names', + nodes: [ + { + id: 'empty-name-id', + name: '', + type: 'n8n-nodes-base.httpRequest', + typeVersion: 3, + position: [250, 300], + parameters: {} + }, + { + id: 'empty-name-id', + name: '', + type: 'n8n-nodes-base.set', + typeVersion: 2, + position: [450, 300], + parameters: {} + } + ], + connections: {} + }; + + // Simulate validation logic with safe fallback + const nodeIds = new Set(); + const nodeIdToIndex = new Map(); + const errors: Array<{ message: string }> = []; + + for (let i = 0; i < workflow.nodes.length; i++) { + const node = workflow.nodes[i]; + if (nodeIds.has(node.id)) { + const firstNodeIndex = nodeIdToIndex.get(node.id); + const firstNode = firstNodeIndex !== undefined ? workflow.nodes[firstNodeIndex] : undefined; + + errors.push({ + message: `Duplicate node ID: "${node.id}". Node at index ${i} (name: "${node.name}", type: "${node.type}") conflicts with node at index ${firstNodeIndex} (name: "${firstNode?.name || 'unknown'}", type: "${firstNode?.type || 'unknown'}")` + }); + } else { + nodeIds.add(node.id); + nodeIdToIndex.set(node.id, i); + } + } + + // Should not crash and should use empty string in message + expect(errors).toHaveLength(1); + expect(errors[0].message).toContain('Duplicate node ID'); + expect(errors[0].message).toContain('name: ""'); + }); + + it('should handle duplicate IDs with missing node properties', () => { + // Edge case: Node with undefined type or name + const workflow = { + name: 'Test Workflow with Missing Properties', + nodes: [ + { + id: 'missing-props', + name: 'Valid Node', + type: 'n8n-nodes-base.httpRequest', + typeVersion: 3, + position: [250, 300], + parameters: {} + }, + { + id: 'missing-props', + name: undefined as any, + type: undefined as any, + typeVersion: 2, + position: [450, 300], + parameters: {} + } + ], + connections: {} + }; + + // Simulate validation logic with safe fallbacks + const nodeIds = new Set(); + const nodeIdToIndex = new Map(); + const errors: Array<{ message: string }> = []; + + for (let i = 0; i < workflow.nodes.length; i++) { + const node = workflow.nodes[i]; + if (nodeIds.has(node.id)) { + const firstNodeIndex = nodeIdToIndex.get(node.id); + const firstNode = firstNodeIndex !== undefined ? workflow.nodes[firstNodeIndex] : undefined; + + errors.push({ + message: `Duplicate node ID: "${node.id}". Node at index ${i} (name: "${node.name}", type: "${node.type}") conflicts with node at index ${firstNodeIndex} (name: "${firstNode?.name || 'unknown'}", type: "${firstNode?.type || 'unknown'}")` + }); + } else { + nodeIds.add(node.id); + nodeIdToIndex.set(node.id, i); + } + } + + // Should use fallback values without crashing + expect(errors).toHaveLength(1); + expect(errors[0].message).toContain('Duplicate node ID: "missing-props"'); + expect(errors[0].message).toContain('name: "undefined"'); + expect(errors[0].message).toContain('type: "undefined"'); + }); }); }); \ No newline at end of file