diff --git a/docs/mcp.md b/docs/mcp.md index 33d23421..272af0df 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -50,14 +50,15 @@ The Provar DX CLI ships with a built-in **Model Context Protocol (MCP) server** - [provar_testplan_add-instance](#provar_testplan_add-instance) - [provar_testplan_create-suite](#provar_testplan_create-suite) - [provar_testplan_remove-instance](#provar_testplan_remove-instance) - - [NitroX — Hybrid Model page objects](#nitrox--hybrid-model-page-objects) - - [provar_nitrox_discover](#provar_nitrox_discover) - - [provar_nitrox_read](#provar_nitrox_read) - - [provar_nitrox_validate](#provar_nitrox_validate) - - [provar_nitrox_generate](#provar_nitrox_generate) - - [provar_nitrox_patch](#provar_nitrox_patch) - - [Quality Hub API tools](#quality-hub-api-tools) - - [provar_qualityhub_examples_retrieve](#provar_qualityhub_examples_retrieve) +- [Data-driven execution](#data-driven-execution) +- [NitroX — Hybrid Model page objects](#nitrox--hybrid-model-page-objects) + - [provar_nitrox_discover](#provar_nitrox_discover) + - [provar_nitrox_read](#provar_nitrox_read) + - [provar_nitrox_validate](#provar_nitrox_validate) + - [provar_nitrox_generate](#provar_nitrox_generate) + - [provar_nitrox_patch](#provar_nitrox_patch) +- [Quality Hub API tools](#quality-hub-api-tools) + - [provar_qualityhub_examples_retrieve](#provar_qualityhub_examples_retrieve) - [Org metadata via Salesforce Hosted MCP](#org-metadata-via-salesforce-hosted-mcp) - [MCP Prompts](#mcp-prompts) - [Migration prompts](#migration-prompts) @@ -78,6 +79,7 @@ The Provar DX CLI ships with a built-in **Model Context Protocol (MCP) server** - [Quality scores explained](#quality-scores-explained) - [API compatibility — `xml` vs `xml_content`](#api-compatibility--xml-vs-xml_content) - [Performance Tuning](#performance-tuning) +- [Warning codes](#warning-codes) --- @@ -531,6 +533,23 @@ On **Windows**, path comparisons are performed case-insensitively to account for --- +## Warning codes + +Cross-cutting warning codes surfaced by validation, configuration, and run tooling. These complement the per-tool `rule_id` codes (e.g. `TC_001`, `VAR-REF-001`) documented under [Available tools](#available-tools). Subsequent revisions will refine the meanings as the relevant tool surfaces stabilise. + +| Code | Surfaced by | Meaning | +| ---------------- | --------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------- | +| `PROVARHOME-001` | properties / automation tooling | `provarHome` is missing, blank, or does not point to a Provar install | +| `DATA-001` | `provar_testcase_validate` | `` iteration is silently ignored when a test case runs in direct `testCase`-mode (see [Data-driven execution](#data-driven-execution)) | +| `PARALLEL-001` | automation / run tooling | Parallel-mode cache mismatch between properties and active runtime config | +| `SCHEMA-001` | strict properties / config validators | Unknown or misspelled key in a JSON / properties schema (typo guard) | +| `RUN-001` | `provar_automation_testrun` and friends | Test run produced no executable results — check input selection | +| `JUNIT-001` | report / RCA tooling | JUnit results file is missing, empty, or not parseable | + +Warnings emitted programmatically follow the shape `WARNING []: ` — and when a typo is detected, the message is suffixed with `Did you mean ''?`. See `src/mcp/utils/warningCodes.ts` for the canonical enum. + +--- + ## Available tools ### `provardx_ping` @@ -825,7 +844,7 @@ Validates an XML test case for schema correctness (validity score) and best prac **Warning rules:** -- **DATA-001** — `testCase` declares a `` element. CLI standalone execution does not bind CSV column variables; steps using variable references will resolve to null. Use `SetValues` (Test scope) steps instead, or add the test to a test plan. +- **DATA-001** — `testCase` declares a `` element. When the validator is called with `file_path` and the project's `provardx-properties.json` references that test case directly via top-level `testCase` or `testCases` (rather than via a `.testinstance` inside a plan), the warning carries the `WARNING [DATA-001]:` prefix and recommends wiring the test into a plan via `provar_testplan_add-instance`. When `file_path` is not supplied (or the project context cannot be resolved), the warning falls back to a structural advisory recommending `SetValues` (Test scope) steps. The warning is suppressed entirely when a `.testinstance` references the test case, because data-driven iteration works in that mode. See also [Data-driven execution](#data-driven-execution). - **ASSERT-001** — An `AssertValues` step uses the `argument id="values"` (namedValues) format, which is designed for UI element attribute assertions. For Apex/SOQL result or variable comparisons this silently passes as `null=null`. Use separate `expectedValue`, `actualValue`, and `comparisonType` arguments instead. - **UI-TARGET-001** — A UiWithScreen or UiWithRow `target` argument uses the wrong XML class (e.g. `class="value"`). Must be `class="uiTarget"` or the screen binding is silently ignored at runtime. - **UI-LOCATOR-001** — A UiDoAction or UiAssert `locator` argument uses the wrong XML class. Must be `class="uiLocator"` or Provar cannot resolve the element. @@ -1684,6 +1703,38 @@ Remove a `.testinstance` file from a plan suite. Path is validated to stay withi --- +## Data-driven execution + +Provar's data-driven execution relies on the `` element inside a `.testcase` XML. The runtime only **iterates rows** when the test case is launched through a test-plan instance (a `.testinstance` file under `plans/`). When the same test case is launched directly via the top-level `testCase` or `testCases` property in `provardx-properties.json`, the runtime ignores the data table entirely — every step referencing a `` resolves to `null`, and the test typically completes "successfully" against an empty row set. + +This produces silent-pass behaviour that is hard to spot from a log: the run exits 0, JUnit shows one test case, and the data-driven assertions never fire. The MCP server detects this configuration mismatch and surfaces a **`DATA-001`** warning so an AI agent can recover before the next run. + +**When does `DATA-001` fire?** + +| Condition (validator called with `file_path`) | DATA-001 emitted? | Severity | +| -------------------------------------------------------------------------------------------- | ----------------- | -------- | +| `` present **and** referenced from a `.testinstance` inside `plans/` | No | — | +| `` present **and** referenced via top-level `testCase` / `testCases` array | Yes | WARNING | +| `` present **and** project context cannot be resolved (no active properties file) | Yes (structural) | WARNING | +| No `` element | No | — | + +The plan-mode resolver consults the properties file registered by [`provar_automation_config_load`](#provar_automation_config_load) (`PROVARDX_PROPERTIES_FILE_PATH` in `~/.sf/config.json`), reads `projectPath`, then: + +1. Walks `/plans/**/*.testinstance` for any `testCasePath="..."` referencing the test under validation. If found → `plan` mode → DATA-001 suppressed. +2. Otherwise checks `testCase` / `testCases` for a direct reference. If found → `direct` mode → DATA-001 with the PDX-489 advisory. +3. Falls back to `unknown` mode when no project context is resolvable — DATA-001 still fires (structural fallback) so authors editing a test case in isolation are still warned. + +**Recommended workaround** + +When `DATA-001` fires in direct mode, wire the test case into a plan via [`provar_testplan_add-instance`](#provar_testplan_add-instance) and run via the `testPlan` property in `provardx-properties.json` instead of `testCase` / `testCases`. The pattern is: + +1. Use [`provar_testplan_create-suite`](#provar_testplan_create-suite) to add a suite under an existing plan if needed. +2. Use [`provar_testplan_add-instance`](#provar_testplan_add-instance) to create the `.testinstance` linking the test case to the suite. +3. Update `provardx-properties.json` to reference the plan (and remove the direct `testCase` entry if it no longer applies) before invoking [`provar_automation_testrun`](#provar_automation_testrun). +4. Re-run [`provar_testcase_validate`](#provar_testcase_validate) on the test case file — DATA-001 should no longer appear. + +The constraint is also referenced in the [`provar_testcase_generate`](#provar_testcase_generate) tool description so an agent constructing a new data-driven test case sees the limitation up front, and in [`provar_automation_testrun`](#provar_automation_testrun) so an agent triggering a run is reminded that direct-mode execution will not iterate. + --- ## NitroX — Hybrid Model page objects diff --git a/src/mcp/tools/testCaseValidate.ts b/src/mcp/tools/testCaseValidate.ts index b374db21..15e0e233 100644 --- a/src/mcp/tools/testCaseValidate.ts +++ b/src/mcp/tools/testCaseValidate.ts @@ -36,6 +36,8 @@ import { resolveValidationDir, type DiffableViolation, } from '../utils/validationDiff.js'; +import { resolveTestCasePlanMode, type TestCasePlanMode } from '../utils/testCasePlanMode.js'; +import { WARNING_CODES, formatWarning } from '../utils/warningCodes.js'; import { runBestPractices } from './bestPracticesEngine.js'; import { desc } from './descHelper.js'; @@ -75,15 +77,20 @@ function tcStorageDir(): string { async function resolveBaseResult( source: string, apiKey: string | null, - requestId: string + requestId: string, + planMode: TestCasePlanMode = 'unknown' ): Promise { if (!apiKey) { - return { ...validateTestCase(source), validation_source: 'local', validation_warning: ONBOARDING_MESSAGE }; + return { + ...validateTestCase(source, undefined, { planMode }), + validation_source: 'local', + validation_warning: ONBOARDING_MESSAGE, + }; } const baseUrl = getQualityHubBaseUrl(); try { const apiResult = await qualityHubClient.validateTestCaseViaApi(source, apiKey, baseUrl); - const localMeta = validateTestCase(source); + const localMeta = validateTestCase(source, undefined, { planMode }); log('info', 'provar_testcase_validate: quality_hub', { requestId }); return { ...apiResult, @@ -107,7 +114,11 @@ async function resolveBaseResult( warning = UNREACHABLE_WARNING; log('warn', 'provar_testcase_validate: api unreachable, falling back', { requestId }); } - return { ...validateTestCase(source), validation_source: 'local_fallback', validation_warning: warning }; + return { + ...validateTestCase(source, undefined, { planMode }), + validation_source: 'local_fallback', + validation_warning: warning, + }; } } @@ -123,7 +134,7 @@ export function registerTestCaseValidate(server: McpServer, config: ServerConfig { title: 'Validate Test Case', description: desc( - 'Validate a Provar XML test case for structural correctness and quality. Checks XML declaration, root element, required attributes (guid UUID v4, testItemId integer), presence, and applies best-practice rules. When a Provar API key is configured (via sf provar auth login or PROVAR_API_KEY env var), calls the Quality Hub API for full 170-rule scoring. Falls back to local validation if no key is set or the API is unavailable. Returns validity_score (schema compliance), quality_score (best practices, 0–100), and validation_source indicating which ruleset was applied. Every response includes run_id — pass it as baseline_run_id in the next call to receive only new/resolved issues. When structural errors are returned, consult the provar://docs/step-reference MCP resource for correct step attribute schemas.', + "Validate a Provar XML test case for structural correctness and quality. Checks XML declaration, root element, required attributes (guid UUID v4, testItemId integer), presence, and applies best-practice rules. When a Provar API key is configured (via sf provar auth login or PROVAR_API_KEY env var), calls the Quality Hub API for full 170-rule scoring. Falls back to local validation if no key is set or the API is unavailable. Returns validity_score (schema compliance), quality_score (best practices, 0–100), and validation_source indicating which ruleset was applied. Every response includes run_id — pass it as baseline_run_id in the next call to receive only new/resolved issues. Data-driven note (DATA-001): when file_path is supplied and the project's provardx-properties.json references the test case directly via top-level `testCase` / `testCases` rather than via a `.testinstance` inside a plan, the validator emits DATA-001 warning a declaration will resolve all variables to null in direct testCase-mode — wire the test into a plan via provar_testplan_add-instance to enable data-driven iteration. When structural errors are returned, consult the provar://docs/step-reference MCP resource for correct step attribute schemas.", 'Validate a Provar XML test case: structure, UUIDs, steps, quality scoring; run_id for baseline diff.' ), inputSchema: { @@ -181,7 +192,10 @@ export function registerTestCaseValidate(server: McpServer, config: ServerConfig } const apiKey = resolveApiKey(); - const baseResult = await resolveBaseResult(source, apiKey, requestId); + const planMode: TestCasePlanMode = file_path + ? resolveTestCasePlanMode({ testCaseFilePath: file_path, allowedPaths: config.allowedPaths }).mode + : 'unknown'; + const baseResult = await resolveBaseResult(source, apiKey, requestId, planMode); const storageDir = tcStorageDir(); const context = tcRunContext(file_path, source); @@ -306,7 +320,11 @@ export function validateTestCaseXml(filePath: string, config: ServerConfig): Tes if (!fs.existsSync(resolved)) { throw Object.assign(new Error(`File not found: ${resolved}`), { code: 'TESTCASE_FILE_NOT_FOUND' }); } - return validateTestCase(fs.readFileSync(resolved, 'utf-8')); + const planMode = resolveTestCasePlanMode({ + testCaseFilePath: resolved, + allowedPaths: config.allowedPaths, + }).mode; + return validateTestCase(fs.readFileSync(resolved, 'utf-8'), undefined, { planMode }); } /** TC_010/TC_011: validate testCase id and guid attributes. */ @@ -348,8 +366,69 @@ function checkTestCaseIdAndGuid(tcId: string | null, tcGuid: string | undefined, } } +/** + * Emit the DATA-001 warning when the test case declares a ``. + * + * PDX-489: when the validator handler resolves the project's + * `provardx-properties.json` and finds the test case is referenced from a + * `.testinstance` (plan mode), suppress the warning — data-driven execution + * works there. When the project references the test case directly via + * top-level `testCase` / `testCases`, emit the PDX-489 advisory. When no + * project context is available (plan mode unknown — pure function called + * without file resolution), keep the structural advisory so the warning + * surface stays backward-compatible. + */ +function maybeEmitDataTableWarning( + tc: Record, + planMode: TestCasePlanMode, + issues: ValidationIssue[] +): void { + const hasDataTable = 'dataTable' in tc && tc['dataTable'] != null; + if (!hasDataTable || planMode === 'plan') return; + if (planMode === 'direct') { + issues.push({ + rule_id: 'DATA-001', + severity: 'WARNING', + message: formatWarning( + WARNING_CODES.DATA_001, + ' only iterates when run through a test plan instance. Direct testCase-mode execution will resolve all data-driven variables as null. Add this test to a plan via provar_testplan_add-instance.' + ), + applies_to: 'testCase', + suggestion: + 'Move this test case under a test plan and reference it through a .testinstance — use provar_testplan_add-instance to wire it up.', + }); + return; + } + issues.push({ + rule_id: 'DATA-001', + severity: 'WARNING', + message: + 'testCase declares a but CLI standalone execution does not bind CSV column variables — steps using references will resolve to null.', + applies_to: 'testCase', + suggestion: + 'Use SetValues (Test scope) steps to bind data for standalone CLI execution, or add this test case to a test plan.', + }); +} + +/** + * Validator options threaded through from the MCP handler. `planMode` controls + * how DATA-001 (the data-driven `` warning) is surfaced. + * + * `direct` emits the PDX-489 warning recommending a plan instance. `plan` + * suppresses DATA-001 entirely because data-driven execution works under plan + * mode. `unknown` (default) emits the structural DATA-001 warning so callers + * without project context still receive the original advisory. + */ +export interface ValidateTestCaseOptions { + planMode?: TestCasePlanMode; +} + /** Pure function — exported for unit testing */ -export function validateTestCase(xmlContent: string, testName?: string): TestCaseValidationResult { +export function validateTestCase( + xmlContent: string, + testName?: string, + options: ValidateTestCaseOptions = {} +): TestCaseValidationResult { const issues: ValidationIssue[] = []; // TC_001: XML declaration @@ -423,18 +502,7 @@ export function validateTestCase(xmlContent: string, testName?: string): TestCas return finalize(issues, tcId, tcName, 0, xmlContent, testName); } - // DATA-001: binding is silently ignored in standalone CLI execution - if ('dataTable' in tc && tc['dataTable'] != null) { - issues.push({ - rule_id: 'DATA-001', - severity: 'WARNING', - message: - 'testCase declares a but CLI standalone execution does not bind CSV column variables — steps using references will resolve to null.', - applies_to: 'testCase', - suggestion: - 'Use SetValues (Test scope) steps to bind data for standalone CLI execution, or add this test case to a test plan.', - }); - } + maybeEmitDataTableWarning(tc, options.planMode ?? 'unknown', issues); // Same self-closing guard for → fast-xml-parser yields '' const rawSteps = tc['steps']; diff --git a/src/mcp/utils/testCasePlanMode.ts b/src/mcp/utils/testCasePlanMode.ts new file mode 100644 index 00000000..0f98ac74 --- /dev/null +++ b/src/mcp/utils/testCasePlanMode.ts @@ -0,0 +1,238 @@ +/* + * Copyright (c) 2024 Provar Limited. + * All rights reserved. + * Licensed under the BSD 3-Clause license. + * For full license text, see LICENSE.md file in the repo root or https://opensource.org/licenses/BSD-3-Clause + */ + +/* eslint-disable camelcase */ +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { assertPathAllowed } from '../security/pathPolicy.js'; + +/** + * How the project's provardx-properties.json runs this test case. + * + * `direct` means the test case path appears in `testCase` or `testCases` and + * no `.testinstance` references it — data-driven `` rows will not + * iterate. `plan` means the test case is referenced by at least one + * `.testinstance` under `plans/`, so data-driven execution works. `unknown` + * means the properties file could not be resolved or parsed (no + * `~/.sf/config.json`, file outside allowed paths, missing project root, etc.) + * — callers should default to the safe behaviour (emit the structural + * warning) when the mode is unknown. + */ +export type TestCasePlanMode = 'direct' | 'plan' | 'unknown'; + +export interface ResolvedTestCaseMode { + mode: TestCasePlanMode; + /** The properties-file path consulted (when resolved). */ + propertiesFilePath?: string; + /** The project root resolved from the properties file (when present). */ + projectPath?: string; +} + +interface ResolveOptions { + /** Path to the test case file under validation. */ + testCaseFilePath: string; + /** Allowed-paths policy from the MCP server config. */ + allowedPaths: string[]; + /** Override of ~/.sf/config.json location for testing. */ + sfConfigPathOverride?: string; + /** Override of provardx-properties.json location for testing. */ + propertiesFilePathOverride?: string; +} + +/** + * Resolve whether a given test case file is referenced directly via + * `testCase`/`testCases` or via a `.testinstance` inside a plan. + * + * The resolution flow is intentionally best-effort and silent: any file + * read failure, JSON parse error, or path-policy violation collapses to + * `mode: 'unknown'` so the caller falls back to default behaviour rather + * than surfacing a confusing error from the validator. + */ +export function resolveTestCasePlanMode(opts: ResolveOptions): ResolvedTestCaseMode { + const propsPath = readPropertiesFilePath(opts); + if (!propsPath) return { mode: 'unknown' }; + + let propsObj: Record; + try { + propsObj = JSON.parse(fs.readFileSync(propsPath, 'utf-8')) as Record; + } catch { + return { mode: 'unknown', propertiesFilePath: propsPath }; + } + + const projectPath = typeof propsObj['projectPath'] === 'string' ? propsObj['projectPath'] : null; + if (!projectPath) { + // Without a project root we cannot resolve relative `testCase` entries or + // walk `plans/`. The mode is unknown. + return { mode: 'unknown', propertiesFilePath: propsPath }; + } + + let resolvedProjectPath: string; + try { + resolvedProjectPath = path.resolve(projectPath); + assertPathAllowed(resolvedProjectPath, opts.allowedPaths); + } catch { + return { mode: 'unknown', propertiesFilePath: propsPath }; + } + + const resolvedTestCasePath = path.resolve(opts.testCaseFilePath); + + // A `.testinstance` reference inside any plan wins — plan mode supports + // data-driven iteration. + if (isReferencedFromPlanInstance(resolvedProjectPath, resolvedTestCasePath)) { + return { mode: 'plan', propertiesFilePath: propsPath, projectPath: resolvedProjectPath }; + } + + if (isReferencedDirectly(propsObj, resolvedProjectPath, resolvedTestCasePath)) { + return { mode: 'direct', propertiesFilePath: propsPath, projectPath: resolvedProjectPath }; + } + + return { mode: 'unknown', propertiesFilePath: propsPath, projectPath: resolvedProjectPath }; +} + +/** + * Resolve the active properties file path. Prefers an explicit override + * (used by tests), then `~/.sf/config.json`'s `PROVARDX_PROPERTIES_FILE_PATH`. + * + * Both the override and the value read from `~/.sf/config.json` are funnelled + * through `assertPathAllowed` so a caller cannot trick the resolver into + * reading a properties file outside the configured `allowedPaths`. Any policy + * violation collapses to `null` to preserve the helper's best-effort contract. + */ +function readPropertiesFilePath(opts: ResolveOptions): string | null { + if (opts.propertiesFilePathOverride) { + return enforceAllowed(opts.propertiesFilePathOverride, opts.allowedPaths); + } + try { + const sfConfigPath = opts.sfConfigPathOverride ?? path.join(os.homedir(), '.sf', 'config.json'); + if (!fs.existsSync(sfConfigPath)) return null; + const sfConfig = JSON.parse(fs.readFileSync(sfConfigPath, 'utf-8')) as Record; + const propsPath = sfConfig['PROVARDX_PROPERTIES_FILE_PATH'] as string | undefined; + if (!propsPath) return null; + return enforceAllowed(propsPath, opts.allowedPaths); + } catch { + return null; + } +} + +/** + * Resolve a candidate properties-file path through `assertPathAllowed`, then + * canonicalize via `fs.realpathSync` when the file exists so a symlink + * inside an allowed directory cannot redirect the read outside it. Returns + * `null` on any policy violation, missing file, or unexpected error — the + * caller treats `null` as "mode unknown" and falls back to default behaviour. + */ +function enforceAllowed(candidate: string, allowedPaths: string[]): string | null { + try { + const resolved = path.resolve(candidate); + assertPathAllowed(resolved, allowedPaths); + if (!fs.existsSync(resolved)) return null; + try { + return fs.realpathSync(resolved); + } catch { + return resolved; + } + } catch { + return null; + } +} + +/** + * Does the properties file's top-level `testCase` or `testCases` array + * reference this test case file? Entries are interpreted relative to + * `/tests/` (per the Provar runtime convention). + */ +function isReferencedDirectly(props: Record, projectPath: string, testCaseFilePath: string): boolean { + const entries: string[] = []; + const tc = props['testCase']; + const tcs = props['testCases']; + for (const candidate of [tc, tcs]) { + if (typeof candidate === 'string') { + entries.push(candidate); + } else if (Array.isArray(candidate)) { + for (const e of candidate as unknown[]) { + if (typeof e === 'string') entries.push(e); + } + } + } + if (entries.length === 0) return false; + + const testsDir = path.join(projectPath, 'tests'); + const targetNorm = path.resolve(testCaseFilePath).toLowerCase(); + + for (const entry of entries) { + // Provar accepts both bare names ("MyTest") and relative paths + // ("Module/MyTest.testcase"). Allow either; match with and without the + // `.testcase` extension. + const variants: string[] = []; + const trimmed = entry.replace(/^[/\\]+/, ''); + variants.push(path.resolve(testsDir, trimmed)); + if (!/\.testcase$/i.test(trimmed)) { + variants.push(path.resolve(testsDir, `${trimmed}.testcase`)); + } + for (const v of variants) { + if (v.toLowerCase() === targetNorm) return true; + } + } + return false; +} + +/** + * Walk `/plans/` for `.testinstance` files referencing this + * test case via `testCasePath="..."`. Best-effort — any read error skips + * the offending file. + */ +function isReferencedFromPlanInstance(projectPath: string, testCaseFilePath: string): boolean { + const plansDir = path.join(projectPath, 'plans'); + if (!fs.existsSync(plansDir)) return false; + + const testsDir = path.join(projectPath, 'tests'); + const targetNorm = path.resolve(testCaseFilePath).toLowerCase(); + let found = false; + + const walk = (dir: string): void => { + if (found) return; + let entries: fs.Dirent[]; + try { + entries = fs.readdirSync(dir, { withFileTypes: true }); + } catch { + return; + } + for (const entry of entries) { + if (found) return; + if (entry.name.startsWith('.') && !entry.name.endsWith('.testinstance')) continue; + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + walk(full); + continue; + } + if (!entry.name.endsWith('.testinstance')) continue; + let content: string; + try { + content = fs.readFileSync(full, 'utf-8'); + } catch { + continue; + } + const matches = content.matchAll(/testCasePath=["']([^"']+)["']/g); + for (const m of matches) { + const rel = m[1].replace(/\\/g, '/'); + // testCasePath in Provar testinstances is conventionally relative to + // `/tests/`. Also tolerate paths relative to project root. + const candidates = [path.resolve(testsDir, rel), path.resolve(projectPath, rel)]; + for (const c of candidates) { + if (c.toLowerCase() === targetNorm) { + found = true; + return; + } + } + } + } + }; + + walk(plansDir); + return found; +} diff --git a/src/mcp/utils/warningCodes.ts b/src/mcp/utils/warningCodes.ts new file mode 100644 index 00000000..9da5d9a3 --- /dev/null +++ b/src/mcp/utils/warningCodes.ts @@ -0,0 +1,22 @@ +/* + * Copyright (c) 2024 Provar Limited. + * All rights reserved. + * Licensed under the BSD 3-Clause license. + * For full license text, see LICENSE.md file in the repo root or https://opensource.org/licenses/BSD-3-Clause + */ + +export const WARNING_CODES = { + PROVARHOME_001: 'PROVARHOME-001', + DATA_001: 'DATA-001', + PARALLEL_001: 'PARALLEL-001', + SCHEMA_001: 'SCHEMA-001', + RUN_001: 'RUN-001', + JUNIT_001: 'JUNIT-001', +} as const; + +export type WarningCode = (typeof WARNING_CODES)[keyof typeof WARNING_CODES]; + +export function formatWarning(code: WarningCode, message: string, suggestion?: string): string { + const base = `WARNING [${code}]: ${message}`; + return suggestion ? `${base} Did you mean '${suggestion}'?` : base; +} diff --git a/test/unit/mcp/testCasePlanMode.test.ts b/test/unit/mcp/testCasePlanMode.test.ts new file mode 100644 index 00000000..1dcdc328 --- /dev/null +++ b/test/unit/mcp/testCasePlanMode.test.ts @@ -0,0 +1,231 @@ +/* + * Copyright (c) 2024 Provar Limited. + * All rights reserved. + * Licensed under the BSD 3-Clause license. + * For full license text, see LICENSE.md file in the repo root or https://opensource.org/licenses/BSD-3-Clause + */ + +/* eslint-disable camelcase */ +import { strict as assert } from 'node:assert'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { describe, it, beforeEach, afterEach } from 'mocha'; +import { resolveTestCasePlanMode } from '../../../src/mcp/utils/testCasePlanMode.js'; + +/** + * Build a minimal Provar project on disk for plan-mode resolution tests. + * + * Layout: `/tests/Module/MyTest.testcase`, + * `/plans/Plan1/Suite1/MyTest.testinstance` (only when + * `wirePlan=true`), and `/provardx-properties.json`. + * + * Returns the test-case path and the properties-file path so callers can + * point the resolver at the right inputs. + */ +function buildProject(opts: { + root: string; + references: 'direct-testCase' | 'direct-testCases' | 'none'; + wirePlan: boolean; +}): { testCasePath: string; propertiesPath: string; projectPath: string } { + const projectPath = path.join(opts.root, 'project'); + fs.mkdirSync(path.join(projectPath, 'tests', 'Module'), { recursive: true }); + const testCasePath = path.join(projectPath, 'tests', 'Module', 'MyTest.testcase'); + fs.writeFileSync(testCasePath, ''); + + if (opts.wirePlan) { + fs.mkdirSync(path.join(projectPath, 'plans', 'Plan1', 'Suite1'), { recursive: true }); + const inst = path.join(projectPath, 'plans', 'Plan1', 'Suite1', 'MyTest.testinstance'); + fs.writeFileSync(inst, ''); + } + + const props: Record = { + projectPath, + provarHome: '/tmp/provarHome', + resultsPath: 'ANT/Results', + }; + if (opts.references === 'direct-testCase') { + props.testCase = ['Module/MyTest.testcase']; + } else if (opts.references === 'direct-testCases') { + props.testCases = ['Module/MyTest.testcase']; + } + + const propertiesPath = path.join(projectPath, 'provardx-properties.json'); + fs.writeFileSync(propertiesPath, JSON.stringify(props, null, 2)); + return { testCasePath, propertiesPath, projectPath }; +} + +describe('resolveTestCasePlanMode', () => { + let tmp: string; + + beforeEach(() => { + // Realpath the tmp root immediately so every derived path uses the canonical form. + // Required on macOS where os.tmpdir() returns /var/folders/... but realpathSync + // canonicalises through the /var → /private/var symlink. The resolver under test + // now calls fs.realpathSync internally, so comparisons against constructed paths + // would otherwise diverge from the resolved result on Mac. + tmp = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'pdx489-mode-'))); + }); + + afterEach(() => { + fs.rmSync(tmp, { recursive: true, force: true }); + }); + + it('returns "direct" when properties references via top-level testCase', () => { + const { testCasePath, propertiesPath, projectPath } = buildProject({ + root: tmp, + references: 'direct-testCase', + wirePlan: false, + }); + const result = resolveTestCasePlanMode({ + testCaseFilePath: testCasePath, + allowedPaths: [tmp], + propertiesFilePathOverride: propertiesPath, + }); + assert.equal(result.mode, 'direct'); + assert.equal(result.propertiesFilePath, propertiesPath); + assert.equal(result.projectPath, projectPath); + }); + + it('returns "direct" when properties references via testCases (plural)', () => { + const { testCasePath, propertiesPath } = buildProject({ + root: tmp, + references: 'direct-testCases', + wirePlan: false, + }); + const result = resolveTestCasePlanMode({ + testCaseFilePath: testCasePath, + allowedPaths: [tmp], + propertiesFilePathOverride: propertiesPath, + }); + assert.equal(result.mode, 'direct'); + }); + + it('returns "plan" when a .testinstance references the test case (even if testCase array also lists it)', () => { + const { testCasePath, propertiesPath } = buildProject({ + root: tmp, + references: 'direct-testCase', + wirePlan: true, + }); + const result = resolveTestCasePlanMode({ + testCaseFilePath: testCasePath, + allowedPaths: [tmp], + propertiesFilePathOverride: propertiesPath, + }); + assert.equal(result.mode, 'plan', 'Plan-instance reference must win over direct testCase array'); + }); + + it('returns "plan" when only a .testinstance references it', () => { + const { testCasePath, propertiesPath } = buildProject({ + root: tmp, + references: 'none', + wirePlan: true, + }); + const result = resolveTestCasePlanMode({ + testCaseFilePath: testCasePath, + allowedPaths: [tmp], + propertiesFilePathOverride: propertiesPath, + }); + assert.equal(result.mode, 'plan'); + }); + + it('returns "unknown" when neither testCase nor a .testinstance references it', () => { + const { testCasePath, propertiesPath } = buildProject({ + root: tmp, + references: 'none', + wirePlan: false, + }); + const result = resolveTestCasePlanMode({ + testCaseFilePath: testCasePath, + allowedPaths: [tmp], + propertiesFilePathOverride: propertiesPath, + }); + assert.equal(result.mode, 'unknown'); + }); + + it('returns "unknown" when the properties file does not exist', () => { + const result = resolveTestCasePlanMode({ + testCaseFilePath: path.join(tmp, 'nope.testcase'), + allowedPaths: [tmp], + propertiesFilePathOverride: path.join(tmp, 'missing.json'), + }); + // Override is honoured but fs read fails → 'unknown' + assert.equal(result.mode, 'unknown'); + }); + + it('returns "unknown" when sf config has no PROVARDX_PROPERTIES_FILE_PATH entry', () => { + const sfDir = path.join(tmp, 'sf'); + fs.mkdirSync(sfDir); + fs.writeFileSync(path.join(sfDir, 'config.json'), JSON.stringify({})); + const result = resolveTestCasePlanMode({ + testCaseFilePath: path.join(tmp, 'nope.testcase'), + allowedPaths: [tmp], + sfConfigPathOverride: path.join(sfDir, 'config.json'), + }); + assert.equal(result.mode, 'unknown'); + }); + + it('returns "unknown" when properties file is outside allowed paths', () => { + const { testCasePath, propertiesPath } = buildProject({ + root: tmp, + references: 'direct-testCase', + wirePlan: false, + }); + // Allowed paths intentionally does NOT include the properties path. + const result = resolveTestCasePlanMode({ + testCaseFilePath: testCasePath, + allowedPaths: [path.join(tmp, 'unrelated-dir')], + propertiesFilePathOverride: propertiesPath, + }); + assert.equal(result.mode, 'unknown'); + }); + + it('honours assertPathAllowed on propertiesFilePathOverride: override inside allowed paths is consulted', () => { + // Security regression guard: a properties-file override path must pass the + // path-policy check before the resolver opens it. When the override sits + // inside the allowed-paths tree the helper should consult it normally and + // return the resolved mode (here: "direct"). + const { testCasePath, propertiesPath, projectPath } = buildProject({ + root: tmp, + references: 'direct-testCase', + wirePlan: false, + }); + const result = resolveTestCasePlanMode({ + testCaseFilePath: testCasePath, + allowedPaths: [tmp], + propertiesFilePathOverride: propertiesPath, + }); + assert.equal(result.mode, 'direct'); + assert.equal(result.projectPath, projectPath); + }); + + it('honours assertPathAllowed on propertiesFilePathOverride: override outside allowed paths is ignored without throwing', () => { + // Security regression guard for the Copilot-flagged path-policy bypass: + // the override must be funnelled through assertPathAllowed and a + // violation must collapse to 'unknown' rather than reading the file or + // throwing — the helper's contract is best-effort silent fallback. + const outsideRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'pdx489-outside-')); + try { + const { testCasePath } = buildProject({ + root: tmp, + references: 'direct-testCase', + wirePlan: false, + }); + // Write a properties file OUTSIDE the allowed-paths tree. + const outsideProps = path.join(outsideRoot, 'provardx-properties.json'); + fs.writeFileSync( + outsideProps, + JSON.stringify({ projectPath: outsideRoot, provarHome: '/tmp', resultsPath: 'r' }) + ); + const result = resolveTestCasePlanMode({ + testCaseFilePath: testCasePath, + allowedPaths: [tmp], + propertiesFilePathOverride: outsideProps, + }); + assert.equal(result.mode, 'unknown'); + assert.equal(result.propertiesFilePath, undefined, 'must not expose a path it rejected'); + } finally { + fs.rmSync(outsideRoot, { recursive: true, force: true }); + } + }); +}); diff --git a/test/unit/mcp/testCaseValidate.test.ts b/test/unit/mcp/testCaseValidate.test.ts index 9ff5bd2e..ad7a1391 100644 --- a/test/unit/mcp/testCaseValidate.test.ts +++ b/test/unit/mcp/testCaseValidate.test.ts @@ -226,16 +226,16 @@ describe('validateTestCase', () => { }); describe('DATA-001', () => { - it('warns when testCase has a child element', () => { - const r = validateTestCase( - ` + const DATA_TABLE_TC = ` mydata.csv -` - ); +`; + + it('warns (structural) when testCase has a child element and mode is unknown', () => { + const r = validateTestCase(DATA_TABLE_TC); assert.ok( r.issues.some((i) => i.rule_id === 'DATA-001'), 'Expected DATA-001' @@ -248,6 +248,42 @@ describe('validateTestCase', () => { const r = validateTestCase(VALID_TC); assert.ok(!r.issues.some((i) => i.rule_id === 'DATA-001'), 'DATA-001 should not fire for valid test case'); }); + + it('PDX-489: emits DATA-001 with formatWarning text when planMode is direct', () => { + const r = validateTestCase(DATA_TABLE_TC, undefined, { planMode: 'direct' }); + const issue = r.issues.find((i) => i.rule_id === 'DATA-001'); + assert.ok(issue, 'Expected DATA-001 to fire in direct mode'); + assert.ok( + issue.message.startsWith('WARNING [DATA-001]:'), + `Message must use formatWarning prefix, got: ${issue.message}` + ); + assert.ok( + issue.message.includes('only iterates when run through a test plan instance'), + `Message must reference the plan-instance fix: ${issue.message}` + ); + assert.ok( + issue.message.includes('provar_testplan_add-instance'), + `Message must reference provar_testplan_add-instance: ${issue.message}` + ); + }); + + it('PDX-489: suppresses DATA-001 when planMode is plan', () => { + const r = validateTestCase(DATA_TABLE_TC, undefined, { planMode: 'plan' }); + assert.ok( + !r.issues.some((i) => i.rule_id === 'DATA-001'), + 'DATA-001 must not fire when the test case is referenced from a .testinstance' + ); + }); + + it('PDX-489: keeps structural DATA-001 when planMode is unknown', () => { + const r = validateTestCase(DATA_TABLE_TC, undefined, { planMode: 'unknown' }); + const issue = r.issues.find((i) => i.rule_id === 'DATA-001'); + assert.ok(issue, 'Expected DATA-001 to fire in unknown mode'); + assert.ok( + !issue.message.startsWith('WARNING [DATA-001]:'), + 'Unknown-mode message should NOT use the formatWarning prefix (preserves prior behaviour)' + ); + }); }); describe('ASSERT-001', () => { @@ -1140,6 +1176,117 @@ describe('validateTestCaseXml', () => { }); }); +// ── PDX-489 handler-level DATA-001 integration ──────────────────────────────── + +/** + * Build a project that wires a dataTable test case directly (via testCase / + * testCases array) OR through a .testinstance — and writes ~/.sf/config.json + * pointing at the project's provardx-properties.json so the handler can + * resolve plan mode without explicit overrides. + */ +function buildDataTableProject( + tmpRoot: string, + references: 'direct-testCase' | 'direct-testCases' | 'plan' +): { testCasePath: string; allowedPaths: string[] } { + const projectPath = path.join(tmpRoot, 'project'); + fs.mkdirSync(path.join(projectPath, 'tests', 'Module'), { recursive: true }); + const testCasePath = path.join(projectPath, 'tests', 'Module', 'DataTest.testcase'); + fs.writeFileSync( + testCasePath, + ` + + mydata.csv + + + +` + ); + + const props: Record = { + projectPath, + provarHome: '/tmp/provarHome', + resultsPath: 'ANT/Results', + }; + + if (references === 'direct-testCase') { + props.testCase = ['Module/DataTest.testcase']; + } else if (references === 'direct-testCases') { + props.testCases = ['Module/DataTest.testcase']; + } else { + fs.mkdirSync(path.join(projectPath, 'plans', 'Plan1', 'Suite1'), { recursive: true }); + fs.writeFileSync( + path.join(projectPath, 'plans', 'Plan1', 'Suite1', 'DataTest.testinstance'), + '' + ); + } + + const propertiesPath = path.join(projectPath, 'provardx-properties.json'); + fs.writeFileSync(propertiesPath, JSON.stringify(props, null, 2)); + + // Wire ~/.sf/config.json so the resolver picks up the properties file. + const sfDir = path.join(tmpRoot, '.sf'); + fs.mkdirSync(sfDir, { recursive: true }); + fs.writeFileSync(path.join(sfDir, 'config.json'), JSON.stringify({ PROVARDX_PROPERTIES_FILE_PATH: propertiesPath })); + + return { testCasePath, allowedPaths: [tmpRoot] }; +} + +describe('PDX-489: DATA-001 handler integration via validateTestCaseXml', () => { + let tmpRoot: string; + let origHomedir: () => string; + + beforeEach(() => { + tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'pdx489-handler-')); + origHomedir = os.homedir; + (os as unknown as { homedir: () => string }).homedir = (): string => tmpRoot; + }); + + afterEach(() => { + (os as unknown as { homedir: () => string }).homedir = origHomedir; + fs.rmSync(tmpRoot, { recursive: true, force: true }); + }); + + it('fires DATA-001 with formatWarning prefix when properties uses top-level testCase', () => { + const { testCasePath, allowedPaths } = buildDataTableProject(tmpRoot, 'direct-testCase'); + const result = validateTestCaseXml(testCasePath, { allowedPaths } as unknown as ServerConfig); + const issue = result.issues.find((i) => i.rule_id === 'DATA-001'); + assert.ok(issue, 'Expected DATA-001 in direct testCase mode'); + assert.ok(issue.message.startsWith('WARNING [DATA-001]:'), `Expected formatWarning prefix: ${issue.message}`); + assert.ok( + issue.message.includes('provar_testplan_add-instance'), + `Expected guidance to plan-instance: ${issue.message}` + ); + }); + + it('fires DATA-001 when properties uses testCases (plural) — typo-tolerant', () => { + const { testCasePath, allowedPaths } = buildDataTableProject(tmpRoot, 'direct-testCases'); + const result = validateTestCaseXml(testCasePath, { allowedPaths } as unknown as ServerConfig); + const issue = result.issues.find((i) => i.rule_id === 'DATA-001'); + assert.ok(issue, 'Expected DATA-001 when testCases array references the test'); + assert.ok(issue.message.startsWith('WARNING [DATA-001]:')); + }); + + it('does NOT fire DATA-001 when test case is referenced from a .testinstance', () => { + const { testCasePath, allowedPaths } = buildDataTableProject(tmpRoot, 'plan'); + const result = validateTestCaseXml(testCasePath, { allowedPaths } as unknown as ServerConfig); + assert.ok( + !result.issues.some((i) => i.rule_id === 'DATA-001'), + 'DATA-001 must not fire when the test case is wired via a plan instance' + ); + }); + + it('does NOT fire DATA-001 when test case has no (direct mode)', () => { + // Reuse the direct project but rewrite the test case content without a . + const { testCasePath, allowedPaths } = buildDataTableProject(tmpRoot, 'direct-testCase'); + fs.writeFileSync(testCasePath, VALID_TC); + const result = validateTestCaseXml(testCasePath, { allowedPaths } as unknown as ServerConfig); + assert.ok( + !result.issues.some((i) => i.rule_id === 'DATA-001'), + 'DATA-001 must not fire when no is present' + ); + }); +}); + // ── tool description ────────────────────────────────────────────────────────── describe('provar_testcase_validate description', () => { diff --git a/test/unit/mcp/utils/warningCodes.test.ts b/test/unit/mcp/utils/warningCodes.test.ts new file mode 100644 index 00000000..3afa5b01 --- /dev/null +++ b/test/unit/mcp/utils/warningCodes.test.ts @@ -0,0 +1,53 @@ +/* + * Copyright (c) 2024 Provar Limited. + * All rights reserved. + * Licensed under the BSD 3-Clause license. + * For full license text, see LICENSE.md file in the repo root or https://opensource.org/licenses/BSD-3-Clause + */ + +import { strict as assert } from 'node:assert'; +import { describe, it } from 'mocha'; +import { WARNING_CODES, formatWarning } from '../../../../src/mcp/utils/warningCodes.js'; + +describe('WARNING_CODES', () => { + it('maps each key to its expected wire string', () => { + const expected: Record = { + PROVARHOME_001: 'PROVARHOME-001', + DATA_001: 'DATA-001', + PARALLEL_001: 'PARALLEL-001', + SCHEMA_001: 'SCHEMA-001', + RUN_001: 'RUN-001', + JUNIT_001: 'JUNIT-001', + }; + for (const [key, value] of Object.entries(expected)) { + assert.equal( + (WARNING_CODES as Record)[key], + value, + `WARNING_CODES.${key} should equal '${value}'` + ); + } + }); +}); + +describe('formatWarning', () => { + it('returns the prefixed message exactly when no suggestion is provided', () => { + assert.equal( + formatWarning(WARNING_CODES.PROVARHOME_001, 'provarHome is missing'), + 'WARNING [PROVARHOME-001]: provarHome is missing' + ); + }); + + it('appends the "Did you mean" suffix exactly when a suggestion is provided', () => { + assert.equal( + formatWarning(WARNING_CODES.SCHEMA_001, "unknown key 'parralelMode'", 'parallelMode'), + "WARNING [SCHEMA-001]: unknown key 'parralelMode' Did you mean 'parallelMode'?" + ); + }); + + it('omits the suffix when suggestion is an empty string', () => { + assert.equal( + formatWarning(WARNING_CODES.DATA_001, 'data-table iteration not bound', ''), + 'WARNING [DATA-001]: data-table iteration not bound' + ); + }); +});