From 30b9a9a23664e46b754ac8cb737585b8117a4ec5 Mon Sep 17 00:00:00 2001 From: Bryant Austin Date: Tue, 1 Sep 2026 09:52:30 -0600 Subject: [PATCH] Handle every error kind the test schema's invalid attribute defines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit testSchema.xsd defines five values for InvalidType — false, syntax, semantic, execution and true — four of which say the test expects the engine to fail, differing only in how. The runner recognised only `true` and `semantic`, so a test declaring `invalid="syntax"` fell through to value comparison and was scored a failure even though the engine had produced exactly the syntax error the test asked for. The semantics were duplicated across five call sites, so they now live in one place as two predicates: - expectsError() — all four error kinds. The test passes only if the engine errored, and needs no to compare against. - preventsTranslation() — syntax and semantic only. Those cannot survive CQL-to-ELM translation, so build-cql must leave them out of a generated library; one such define would fail the translation of every define around it. Runtime and execution errors translate fine and are kept. Two further sites needed the same widening and would otherwise have broken: - The results schema enumerated only ["true", "false", "semantic"], so writing invalid="syntax" into a report would have failed results validation. - build-cql excluded only `semantic` from generated libraries, so a syntax-invalid define would have been emitted and broken its whole library. Both currently affected tests are exactly that case. Verified against the reference engine: 1639/170 -> 1641/168, with the two invalid="syntax" tests (CeilingIntegerGreaterThanMaxInteger and CeilingIntegerLessThanMinInteger) passing and no other test changing status. Co-Authored-By: Claude Opus 5 (1M context) --- assets/schema/cql-test-results.schema.json | 3 +- src/commands/build-cql-command.ts | 5 +- src/models/test-types.ts | 10 +-- src/shared/invalid-utils.ts | 40 ++++++++++++ src/shared/results-shared.ts | 12 ++-- src/shared/run-test-core.ts | 11 ++-- src/test-results/cql-test-results.ts | 3 +- test/invalid-utils.test.ts | 72 ++++++++++++++++++++++ 8 files changed, 140 insertions(+), 16 deletions(-) create mode 100644 src/shared/invalid-utils.ts create mode 100644 test/invalid-utils.test.ts diff --git a/assets/schema/cql-test-results.schema.json b/assets/schema/cql-test-results.schema.json index 8f8e0cd..e8943f0 100644 --- a/assets/schema/cql-test-results.schema.json +++ b/assets/schema/cql-test-results.schema.json @@ -71,7 +71,8 @@ }, "invalid": { "type": "string", - "enum": ["true", "false", "semantic"] + "description": "The test's expected outcome, per InvalidType in testSchema.xsd: false to evaluate successfully, or the kind of error expected.", + "enum": ["false", "syntax", "semantic", "execution", "true"] }, "capabilities": { "type": "array", diff --git a/src/commands/build-cql-command.ts b/src/commands/build-cql-command.ts index 57fbea4..c54eac0 100644 --- a/src/commands/build-cql-command.ts +++ b/src/commands/build-cql-command.ts @@ -4,6 +4,7 @@ import * as path from 'node:path'; import { ConfigLoader } from '../conf/config-loader.js'; import { TestLoader } from '../loaders/test-loader.js'; import { generateEmptyResults } from '../shared/results-shared.js'; +import { preventsTranslation } from '../shared/invalid-utils.js'; export class BuildCommand { async execute(options: any): Promise { @@ -50,7 +51,9 @@ export class BuildCommand { continue; } - if (r.invalid !== 'semantic') { + // A define that cannot be translated would fail the whole generated library, so + // expressions expected to produce a syntax or semantic error are left out. + if (!preventsTranslation(r.invalid)) { const defineVal = `define "${r.groupName}.${r.testName}": ${r.expression}`; const key = `${r.testsName}-${r.groupName}-${r.testName}`; let reason = ''; diff --git a/src/models/test-types.ts b/src/models/test-types.ts index 7fc06ed..36e71f3 100644 --- a/src/models/test-types.ts +++ b/src/models/test-types.ts @@ -1,11 +1,13 @@ +import type { InvalidKind, ResultInvalidKind } from '../shared/invalid-utils.js'; + export interface TestExpression { text: string; - invalid: 'false' | 'true' | 'semantic'; + invalid: InvalidKind; } export interface TestLibrary { text: string; - invalid: 'false' | 'true' | 'semantic'; + invalid: InvalidKind; } export interface TestOutput { @@ -89,7 +91,7 @@ export interface InternalTestResult { testName: string; testVersion?: string; testVersionTo?: string; - invalid?: 'false' | 'true' | 'semantic' | 'undefined'; + invalid?: ResultInvalidKind; expression: string; // For library-style tests: the full CQL library source, sent to Library/$evaluate wrapped as // a FHIR Library resource. (For these tests, `expression` holds the name of the define whose @@ -113,7 +115,7 @@ export interface TestResult { testsName: string; groupName: string; testName: string; - invalid?: 'false' | 'true' | 'semantic'; + invalid?: InvalidKind; capabilities?: CapabilityKV[]; expression: string; } diff --git a/src/shared/invalid-utils.ts b/src/shared/invalid-utils.ts new file mode 100644 index 0000000..881fccf --- /dev/null +++ b/src/shared/invalid-utils.ts @@ -0,0 +1,40 @@ +/** + * Semantics of the test schema's `invalid` attribute. + * + * `testSchema.xsd` defines five values for `InvalidType`, four of which say the test expects the + * engine to fail — they differ only in *how* it is expected to fail: + * + * - `false` — the expression is expected to evaluate successfully + * - `syntax` — expected to produce a syntax error (the translator rejects it) + * - `semantic` — expected to produce a semantic error (the translator rejects it) + * - `execution` — expected to produce an execution error + * - `true` — expected to produce a runtime error + * + * Treating only `true` and `semantic` as error-expecting scored a test as failed when the engine + * had in fact produced the syntax error the test asked for. + */ +export type InvalidKind = 'false' | 'syntax' | 'semantic' | 'execution' | 'true'; + +/** `invalid` as recorded on a result, which also covers a test that declares no expression. */ +export type ResultInvalidKind = InvalidKind | 'undefined'; + +const ERROR_EXPECTED: ReadonlySet = new Set(['true', 'semantic', 'syntax', 'execution']); + +/** + * True when the test expects the engine to error, whatever kind of error that is. Such a test + * passes only if the engine actually errored, and needs no `` to compare against. + */ +export function expectsError(invalid: string | undefined): boolean { + return invalid !== undefined && ERROR_EXPECTED.has(invalid); +} + +const TRANSLATION_ERRORS: ReadonlySet = new Set(['semantic', 'syntax']); + +/** + * True when the expression cannot survive CQL-to-ELM translation, so it must be left out of a + * generated library — one such define would fail the translation of every define around it. + * Runtime and execution errors translate fine and are kept. + */ +export function preventsTranslation(invalid: string | undefined): boolean { + return invalid !== undefined && TRANSLATION_ERRORS.has(invalid); +} diff --git a/src/shared/results-shared.ts b/src/shared/results-shared.ts index f41e21d..56b4599 100644 --- a/src/shared/results-shared.ts +++ b/src/shared/results-shared.ts @@ -1,5 +1,7 @@ import type { Tests, Test, InternalTestResult, CapabilityKV } from '../models/test-types.js'; import type { Parameters } from 'fhir/r4'; +import { expectsError } from './invalid-utils.js'; +import type { ResultInvalidKind } from './invalid-utils.js'; /** * The XML parser yields a bare object when an element declares exactly one @@ -31,7 +33,7 @@ export class Result implements InternalTestResult { testName: string; testVersion?: string; testVersionTo?: string; - invalid: 'false' | 'true' | 'semantic' | 'undefined'; + invalid: ResultInvalidKind; expression: string; library?: string; capability: CapabilityKV[] = []; @@ -99,10 +101,10 @@ export class Result implements InternalTestResult { } else { this.expected = test.output as string; } - } else if (this.invalid !== 'true' && this.invalid !== 'semantic') { - // No output is expected only when the expression is marked invalid ("true" - // for a run-time error, "semantic" for a translation error) — the test expects - // an error. Otherwise there is nothing to compare against, so skip. + } else if (!expectsError(this.invalid)) { + // No output is expected only when the expression is marked invalid — any of the four + // error kinds means the test expects the engine to fail. Otherwise there is nothing + // to compare against, so skip. this.testStatus = 'skip'; this.skipMessage = 'No output specified'; } diff --git a/src/shared/run-test-core.ts b/src/shared/run-test-core.ts index c49b4a7..0303ece 100644 --- a/src/shared/run-test-core.ts +++ b/src/shared/run-test-core.ts @@ -12,6 +12,7 @@ import { resultsEqual } from './results-utils.js'; import { formatActualValue } from '../test-results/cql-test-results.js'; import { publishTestLibrary } from './library-publisher.js'; import type { PublishedLibrary } from './library-publisher.js'; +import { expectsError } from './invalid-utils.js'; /** * Shared execution state for a test run: the resolved config, the engine, the CVL parser, @@ -121,8 +122,8 @@ function logSkip(result: InternalTestResult): void { /** * Runs a single test against the engine and records its outcome on `result`. Applies skip * precedence (pre-marked skip → OnlyList → config SkipList → version gating), then POSTs the - * expression, extracts the actual value, and classifies pass/fail/error. Errors expected by - * `invalid="true"/"semantic"` tests pass only when the engine actually erred. + * expression, extracts the actual value, and classifies pass/fail/error. A test whose `invalid` + * attribute names any of the four error kinds passes only when the engine actually erred. * * This is the single implementation shared by the CLI and server runners — both use `fetch` and * identical classification, so a test scores the same regardless of how it is invoked. @@ -206,8 +207,10 @@ export async function runTest( const invalid = result.invalid; const erroredOut = responseIndicatesError(response.status, responseBody); - if (invalid === 'true' || invalid === 'semantic') { - // The expression is expected to error; it passes only if the engine erred. + if (expectsError(invalid)) { + // The expression is expected to error; it passes only if the engine erred. All four + // error kinds (syntax, semantic, execution, true) are treated alike here — the test + // asks for a failure, and the runner does not police which kind the engine produced. result.testStatus = erroredOut ? 'pass' : 'fail'; } else if (!erroredOut) { result.testStatus = resultsEqual(parsedExpected, result.actual) ? 'pass' : 'fail'; diff --git a/src/test-results/cql-test-results.ts b/src/test-results/cql-test-results.ts index bb84124..01b9f59 100644 --- a/src/test-results/cql-test-results.ts +++ b/src/test-results/cql-test-results.ts @@ -5,6 +5,7 @@ import type { TestResult, InternalTestResult } from '../models/test-types.js'; import type { TestResultsSummary, CQLTestResultsData } from '../models/results-types.js'; import { ResultsValidator } from '../conf/results-validator.js'; import { isIntervalShaped } from '../shared/interval-utils.js'; +import type { InvalidKind } from '../shared/invalid-utils.js'; /** * Formats an actual value for report output. Structured CQL values are rendered in @@ -209,7 +210,7 @@ export class CQLTestResults { }), ...(result.invalid && result.invalid !== 'undefined' && { - invalid: result.invalid as 'false' | 'true' | 'semantic', + invalid: result.invalid as InvalidKind, }), ...(result.capability && result.capability.length > 0 && { capabilities: result.capability }), diff --git a/test/invalid-utils.test.ts b/test/invalid-utils.test.ts new file mode 100644 index 0000000..506a473 --- /dev/null +++ b/test/invalid-utils.test.ts @@ -0,0 +1,72 @@ +import { describe, it, expect } from 'vitest'; +import { expectsError, preventsTranslation } from '../src/shared/invalid-utils.js'; +import { Result } from '../src/shared/results-shared.js'; + +/** + * testSchema.xsd defines five InvalidType values. Four of them say the test expects the engine to + * fail; only `false` expects success. + */ +const ERROR_KINDS = ['true', 'semantic', 'syntax', 'execution'] as const; + +describe('expectsError', () => { + it('is true for every error kind the schema defines', () => { + for (const kind of ERROR_KINDS) { + expect(expectsError(kind), kind).toBe(true); + } + }); + + it('is false for a test expected to succeed, or with no invalid attribute', () => { + expect(expectsError('false')).toBe(false); + expect(expectsError(undefined)).toBe(false); + // The runner's own marker for a test that declares no expression at all. + expect(expectsError('undefined')).toBe(false); + }); + + it('is false for an unrecognised value rather than treating it as an error', () => { + expect(expectsError('nonsense')).toBe(false); + expect(expectsError('')).toBe(false); + }); +}); + +describe('preventsTranslation', () => { + it('is true only for errors the translator raises', () => { + // A define that cannot translate would fail the whole generated library. + expect(preventsTranslation('syntax')).toBe(true); + expect(preventsTranslation('semantic')).toBe(true); + }); + + it('is false for run-time and execution errors, which translate fine', () => { + expect(preventsTranslation('true')).toBe(false); + expect(preventsTranslation('execution')).toBe(false); + }); + + it('is false for a valid expression or a missing attribute', () => { + expect(preventsTranslation('false')).toBe(false); + expect(preventsTranslation(undefined)).toBe(false); + }); +}); + +describe('Result treats every error kind as needing no output', () => { + // A test that expects an error carries no ; before, only `true` and `semantic` + // were recognised, so a syntax-invalid test with no output was skipped as + // "No output specified" instead of being run. + for (const kind of ERROR_KINDS) { + it(`does not skip an invalid="${kind}" test that declares no output`, () => { + const result = new Result('T', 'G', { + name: 'ErrorExpected', + expression: { text: 'Ceiling(2147483648)', invalid: kind as any }, + } as any); + expect(result.testStatus).not.toBe('skip'); + expect(result.skipMessage).toBeUndefined(); + }); + } + + it('still skips a valid test that declares no output', () => { + const result = new Result('T', 'G', { + name: 'NoOutput', + expression: '1 + 1', + } as any); + expect(result.testStatus).toBe('skip'); + expect(result.skipMessage).toBe('No output specified'); + }); +});