Functionality to support testing of Time zone offset - #119
Functionality to support testing of Time zone offset #119bryantaustin13 wants to merge 14 commits into
Conversation
Carried over from #77This PR replaces #77, which was raised from a fork. The review discussion there did not @cmoesel (2026-03-24): I'm not sure I understand the premise of the offset timezone policy. In the description of the timezone offset policy in this PR's README, it says:
and...
I don't think there should be any ambiguity here. The CQL specification is pretty clear about this (emphasis mine):
So engines always treate DateTimes as having a timezone offset; it's just a question of which timezone offset (and that's indicated by the evaluation request timestamp). Am I misunderstanding what the README is trying to say? @bryantaustin13 (2026-03-27): Please look at this again and let me know if this works and any changes you would like. @brynrhodes (2026-04-02): Add to the mapping of CQL values to/from FHIR support for the time-hasOffset extension described here: https://jira.hl7.org/browse/FHIR-56046 Open items from the above: @cmoesel's question about the premise of the offset timezone |
This branch carried three test files that duplicate others already present,
adding 415 lines and no coverage. Each was checked for unique tests first;
all three had none.
test/run-DataTransferItemList.test.ts 352 lines, a copy of run-tests.test.ts,
which is also present. The name is the
browser drag-and-drop API, so this looks
like an accidental IDE rename.
test/value-Map.test.ts byte-identical to value-map.test.ts,
differing only in case. Two paths that
differ only by case cannot coexist in a
checkout on macOS or Windows.
test/results-utils.test.js a .js duplicate of results-utils.test.ts.
Also drops `import { response } from 'express'` from cql-engine.ts. It was
unused — the four other mentions of `response` are a local AxiosResponse — and
it pulled express into a module with no need for it.
Test files 11 -> 9 and tests 134 -> 122, all of the reduction duplicates. The
two remaining failures in run-tests.test.ts predate this change; they were
previously reported four times because the duplicate file doubled them.
tsc --noEmit clean.
Ports the timezone-offset policy feature onto the deduped runner and drops the
parts main has since covered.
Dropped, both entirely superseded:
test-types.ts, results-shared.ts this branch's groupCapability work. Nothing
emitted the field, so it collected data that
never reached the results file; #122 reports
group capabilities through the existing
capabilities field instead. Took main's
versions of both files.
test-runner.ts #117 moved the per-test logic into
shared/run-test-core.ts, so the 260 lines
this branch added there are ported rather
than restored. Took main's version.
Ported into run-test-core.ts, so the CLI and server paths share it:
- policy resolution (metadata -> env/config -> probe -> default), run once per
execution context rather than per test
- a skip arm for tests declaring a timezone-offset-policy capability that does
not match the server's
- {{SERVER_OFFSET_ISO}} substitution, applied to result.expression before the
request is built so the expression sent and the expression reported match
Two changes from the original while porting:
Resolution is gated on the loaded suite actually containing a policy-dependent
test. It costs a metadata request and possibly a probe expression, and almost no
suite consults it; doing it unconditionally also broke tests that stub CQLEngine
or sequence fetch mocks.
The required policy is read from result.capability rather than test.capability.
In CQLTimeZoneOffsetTest.xml the policy is declared on the <group>, and one of
the three groups does not restate it on its tests — reading only the test level
silently never gated those. This depends on #122 to see group capabilities;
until that merges, group-level declarations still are not visible.
The probe now uses fetch only, since the dedupe removed the axios path, and
CQLEngine gains a serverMetadata getter so the resolution can read the
CapabilityStatement without reaching into a private field.
The feature had no tests, which matters because of how it merges: it is added to the same region of run-test-core.ts that the library-style test support touches, and resolving that conflict in favour of the other side silently drops the skip arm and the substitution. Verified that mis-resolution leaves tsc clean and every other test passing, so nothing would have caught it. Eight tests: policy mismatch skips before any request is made, a matching policy runs, a test declaring no policy runs whatever the server follows, and a policy inherited from the group gates the same as one on the test itself. Then the placeholder: substituted, substituted at every occurrence, left alone when no offset is configured, and — the point of doing it before the request is built — the expression sent to the server is the one recorded on the result. Removing either the gate or the substitution fails five of the eight.
Pre-resolves the one conflict between the two, so that when #118 lands in main this branch merges clean rather than presenting a resolution whose obvious answer silently deletes a feature. Both PRs add to the same region of runTest, after the version gate. The two sides are not alternatives — keep all of it: - the timezone policy skip arm and the {{SERVER_OFFSET_ISO}} substitution, from this branch - the publishedLibrary declaration, from #118 Dropped only this branch's `const data = generateParametersResource(...)` line: #118 moved that call inside the try, after publishing, so it can pass the published library's canonical url. The substitution still happens before it, which is what keeps the expression sent and the expression reported identical. tsc clean, 214 tests passing, with both features verified present: policy gate, offset substitution, library publish, canonical url, and removal in finally.
| @@ -1,4 +1,4 @@ | |||
| import { TestRunner } from '../services/test-runner.js'; | |||
| import { TestRunner } from '../services/test-runner'; | |||
There was a problem hiding this comment.
Is there a reason for removing the the js extension? What impact does that have on converting the TS file to JS down the road?
Unless there is a good reason to remove the extension I recommend adding it back.
2 participants
This depends on the cql-tests PR cqframework/cql-tests#88 (comment) which contains the CQLTimeZoneOffsetTest.xml.
Adds functionality to test timezone offsets. See the README for more information.
Moved from #77, which was raised from a fork before I had committer access on this repository. Same work, same branch name, now hosted here so CI and reviews run against
cqframeworkdirectly.