Skip to content

Tighten domain_lookup test assertions: exact error patterns, boundary values, and console side-effect guards#10

Draft
Copilot wants to merge 2 commits into
masterfrom
copilot/add-error-message-assertion-in-tests
Draft

Tighten domain_lookup test assertions: exact error patterns, boundary values, and console side-effect guards#10
Copilot wants to merge 2 commits into
masterfrom
copilot/add-error-message-assertion-in-tests

Conversation

Copy link
Copy Markdown

Copilot AI commented May 13, 2026

Test suite had loose assertions that passed as long as any Invalid keyword error was thrown, didn't cover valid boundary inputs, used string literals inconsistently, and had no guard against silent console pollution when skipping bad JSON lines.

Changes

  • Stricter keyword validation regex — Updated three shell-injection rejection tests to match /Invalid keyword value:/ instead of /Invalid keyword/, ensuring the specific validation branch in service.js is exercised
  • Boundary value coverage for maxDomainLength — Added test asserting both 1 and 253 are accepted (the inclusive bounds enforced at service.js:225)
  • Consistent throw matcher — Changed parseJsonOutput invalid-JSON test from a string literal to /Failed to parse JSON output/ regex, matching the actual error format ("Failed to parse JSON output: <reason>") and aligning with the rest of the file
  • Console side-effect guard in stream parser — Patched console.error and console.warn during the invalid-JSON-line-skipping test to assert the service silently discards bad lines with no unexpected logging:
console.error = (...args) => { consoleErrorCalls.push(args); };
console.warn  = (...args) => { consoleWarnCalls.push(args); };
try {
  const result = domainLookupService.parseJsonStreamOutput(output);
  expect(consoleErrorCalls).to.have.length(0);
  expect(consoleWarnCalls).to.have.length(0);
} finally {
  console.error = originalConsoleError;
  console.warn  = originalConsoleWarn;
}
Original prompt
Please apply the following diffs and create a pull request.
Once the PR is ready, give it a title based on the messages of the fixes being applied.

[{"message":"The test verifies that shell metacharacters are rejected, but it doesn't verify the specific error message content. Consider adding an assertion to check that the error message contains 'Invalid keyword value:' to ensure the correct validation path is being triggered.","fixFiles":[{"filePath":"mcp_modules/domain_lookup/test/service.test.js","diff":"diff --git a/mcp_modules/domain_lookup/test/service.test.js b/mcp_modules/domain_lookup/test/service.test.js\n--- a/mcp_modules/domain_lookup/test/service.test.js\n+++ b/mcp_modules/domain_lookup/test/service.test.js\n@@ -128,17 +128,17 @@\n     it('should reject keyword containing shell metacharacters', () => {\n       expect(() =>\n         domainLookupService.buildTldxArgs(['example.com; echo pwned > /tmp/x; #'])\n-      ).to.throw(/Invalid keyword/);\n+      ).to.throw(/Invalid keyword value:/);\n     });\n \n     it('should reject keyword with command substitution', () => {\n-      expect(() => domainLookupService.buildTldxArgs(['$(id)'])).to.throw(/Invalid keyword/);\n-      expect(() => domainLookupService.buildTldxArgs(['`id`'])).to.throw(/Invalid keyword/);\n+      expect(() => domainLookupService.buildTldxArgs(['$(id)'])).to.throw(/Invalid keyword value:/);\n+      expect(() => domainLookupService.buildTldxArgs(['`id`'])).to.throw(/Invalid keyword value:/);\n     });\n \n     it('should reject keyword with pipe or redirect', () => {\n-      expect(() => domainLookupService.buildTldxArgs(['x | nc evil 80'])).to.throw(/Invalid keyword/);\n-      expect(() => domainLookupService.buildTldxArgs(['x > /tmp/f'])).to.throw(/Invalid keyword/);\n+      expect(() => domainLookupService.buildTldxArgs(['x | nc evil 80'])).to.throw(/Invalid keyword value:/);\n+      expect(() => domainLookupService.buildTldxArgs(['x > /tmp/f'])).to.throw(/Invalid keyword value:/);\n     });\n \n     it('should reject prefixes/suffixes/tlds containing shell metacharacters', () => {\n"}]},{"message":"The test validates that maxDomainLength rejects values outside the range, but doesn't test the boundary values of 1 and 253 which are valid according to the service implementation (lines 225-226 in service.js). Add test cases to verify that maxDomainLength: 1 and maxDomainLength: 253 are accepted.","fixFiles":[{"filePath":"mcp_modules/domain_lookup/test/service.test.js","diff":"diff --git a/mcp_modules/domain_lookup/test/service.test.js b/mcp_modules/domain_lookup/test/service.test.js\n--- a/mcp_modules/domain_lookup/test/service.test.js\n+++ b/mcp_modules/domain_lookup/test/service.test.js\n@@ -177,6 +177,15 @@\n       ).to.throw(/Invalid maxDomainLength/);\n     });\n \n+    it('should accept boundary maxDomainLength values', () => {\n+      expect(() =>\n+        domainLookupService.buildTldxArgs(['safe'], { maxDomainLength: 1 })\n+      ).to.not.throw();\n+      expect(() =>\n+        domainLookupService.buildTldxArgs(['safe'], { maxDomainLength: 253 })\n+      ).to.not.throw();\n+    });\n+\n     it('should reject empty keywords array', () => {\n       expect(() => domainLookupService.buildTldxArgs([])).to.throw(/non-empty array/);\n     });\n"}]},{"message":"The error message assertion uses a string literal instead of a regex pattern, which is inconsistent with other test cases in this file (e.g., lines 131, 135-136). Consider using a regex pattern for consistency: .to.throw(/Failed to parse JSON output/).","fixFiles":[{"filePath":"mcp_modules/domain_lookup/test/service.test.js","diff":"diff --git a/mcp_modules/domain_lookup/test/service.test.js b/mcp_modules/domain_lookup/test/service.test.js\n--- a/mcp_modules/domain_lookup/test/service.test.js\n+++ b/mcp_modules/domain_lookup/test/service.test.js\n@@ -286,7 +286,7 @@\n     it('should throw error for invalid JSON', () => {\n       expect(() => {\n         domainLookupService.parseJsonOutput('invalid json');\n-      }).to.throw('Failed to parse JSON output');\n+      }).to.throw(/Failed to parse JSON output/);\n     });\n \n     it('should handle non-array JSON', () => {\n"}]},{"message":"The test verifies that invalid JSON lines are skipped, but doesn't verify that the service handles this gracefully without logging errors or warnings. Consider documenting the expected behavior when invalid lines are encountered or add assertions to verify no unexpected side effects occur.","fixFiles":[{"filePath":"mcp_modules/domain_lookup/test/service.test.js","diff":"diff --git a/mcp_modules/domain_lookup/test/service.test.js b/mcp_modules/domain_lookup/test/service.test.js\n--- a/mcp_modules/domain_lookup/test/service.test.js\n+++ b/mcp_modules/domain_lookup/test/service.test.js\n@@ -318,11 +318,31 @@\n     it('should skip invalid JSON lines', () => {\n       const output =\n         '{\"domain\":\"example.com\",\"available\":true}\\ninvalid json\\n{\"domain\":\"test.or...

Copilot AI changed the title [WIP] Add assertion for error message in shell metacharacter test Tighten domain_lookup test assertions: exact error patterns, boundary values, and console side-effect guards May 13, 2026
Copilot AI requested a review from ralyodio May 13, 2026 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants