Issue 133: correct the stale RolledOutIntervals skip reason - #134
Open
bryantaustin13 wants to merge 2 commits into
Open
bryantaustin13 wants to merge 2 commits into
bryantaustin13 wants to merge 2 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses #133.
What
Replaces the stale
RolledOutIntervalsskip reason in the four shipped configs that carried it, andfixes a JSON syntax error in one of them.
The old reason cited an identifier the test has not contained since 2024-08:
The new reason is the error the test actually produces today, verified against the reference engine
(HAPI FHIR 8.10.0 / CQF engine 4.1.0,
$cql, CQL 1.5) using the expression exactly as the runnersends it:
Full engine diagnostic:
Files:
conf/development.json,conf/localhost-evaluate.json,conf/localhost.json,conf/smile-cdr-local.json.conf/smile-cdr-local.jsonadditionally had a trailing comma after"cqlEngineVersion", which madeit invalid JSON —
ConfigLoaderthrew on it, so that configuration could not be loaded at all. Fixedhere because the file could not otherwise be edited or verified.
Why it matters
These configurations ship with the runner, so the obsolete message is reproduced in every recorded
run in
cql-tests-results— cqf-java, cqf-javascript, firely and cozeva all report:Anyone investigating that skip chases an identifier that is not in the test.
Relationship to cqframework/cql-tests#146
This PR is not blocked by #146. #133 proposes eventually deleting these skip entries, and that
does depend on #146 ("Fix RolledOutIntervals starting type") merging plus a submodule bump. This PR
does not delete them — it corrects a reason that misdescribes reality, which is independent of #146
and can merge now.
Worth noting for the follow-up: the test still fails to translate both with the current
List<Interval<DateTime>>starting type and with #146'sList<Interval<Date>>, because theDecimal/Integer error above is unrelated to the starting type. So #146 is necessary but not
sufficient for removing these skips — the entries cannot simply be deleted once it merges.
Not included
conf/cql-execution-local.jsonalso skips this test, but for a current and correctly attributedreason linking #80. That entry is left alone; it should be re-evaluated separately now that #129 has
merged
List<...>unwrapping forcqf-cqlType, and that needs an engine this change was notverified against.
Verification
All five configurations parse as JSON and load through
ConfigLoader(previouslysmile-cdr-local.jsondid neither).Full suite against the reference engine: 1639 pass / 170 fail / 14 skip / 0 error, unchanged
from
main. The test remains skipped, now reporting the corrected reason:No occurrence of
MedicationRequestIntervalsremains inconf/orsrc/.Run on Node 26 (
engines: >=26) with a clean dependency install; results identical to the earlierNode 22 run.