[dehall]

This PR introduces some improvements to the build:spec-test-data process to reduce some manual processes, and automatically skip tests that are not relevant to the library.

This PR depends on an update to cql-to-elm-cli to return a nonzero process exit code when there is a file that fails to translate. This causes the build script to terminate and should hopefully be obvious to the user. Currently, if the cli fails to translate a file, nothing detects that and we can get stale/out-of-sync files. That's happened a couple times in the past 4 months alone.
This PR is marked WIP as it depends on cqframework/clinical_quality_language#1813 to be merged and a new release of cql-to-elm-cli. Until then, if we want to test this out, a local installation is necessary. Steps below.

Modifications here:

1. Bump the referenced cql-to-elm-cli to that updated version ^
2. Implicitly skip tests that are marked invalid: "semantic" or invalid: "syntax" as these fail at the translator level. Remove these from skip-list.txt
3. Implicitly skip tests that target different versions of CQL than this library does, see for instance Update tests for Decimal successor/predecessor for 2.0.0 cql-tests#147 . Remove these from skip-list.txt.
   • To do this required some way of indicating what the target version of the repo is, so I've added ./specification.version
4. Remove the included cql-tests xml files and replace them by making cql-tests-runner a git submodule. Update scripts accordingly to point to where the relevant files are, and update tool settings (mocha, lint, etc) to ignore those nested directories.
5. Regenerate all spec-tests based on the above.
6. Add a new script test/spec-tests/create-runner-skip-list.js to generate an updated cql-tests-runner config file based on the latest skip list. This file is heavily based on check-skip-list.js and does similar iteration over the skip-list and cql-test XMLs. (For now I didn't touch check-skip-list.js to keep the diff smaller, but we could probably refactor some common logic if we wanted.) Because the sets of tests we want to skip are different between our unit tests and the tests-runner, this script depends on a new file test/spec-tests/runner-skip-list.txt to add further skips.
7. Break skip-list.txt out into 2 files, common-skip-list and unit-tests-skip-list . This is primarily for the benefit of the create-runner-skip-list script, where we don't want to skip the "unit-test" skips related to known issues in this repo. Also moved a few tests between categories such as adding a new category for "Numeric Type Mismatch" and moving a couple tests based on review.
8. Add a .gitattributes that marks generated ELM files and spec-test CQLs as generated, which makes GitHub collapse them by default in the code view. This is by file path, not by file type or extension, so there shouldn't be false positives that hide real code changes.

Pull requests into cql-execution require the following.
Submitter and reviewer should ✔ when done.
For items that are not-applicable, mark "N/A" and ✔.

Submitter:

• This pull request describes why these changes were made
• Code diff has been done and been reviewed (it does not contain: additional white space, not applicable code changes, debug statements, etc.)
• Tests are included and test edge cases
• Tests have been run locally and pass
• Code coverage has not gone down and all code touched or added is covered.
• Code passes lint and prettier (hint: use npm run check to run tests, lint, and prettier)
• All dependent libraries are appropriately updated or have a corresponding PR related to this change
  Reviewer:

Name:

• Code is maintainable and reusable, reuses existing code and infrastructure where appropriate, and accomplishes the task’s purpose
• The tests appropriately test the new code, including edge cases
• You have tried to break the code

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.29%. Comparing base (e87e3fa) to head (967631a).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #383      +/-   ##
==========================================
+ Coverage   89.21%   89.29%   +0.07%     
==========================================
  Files          60       60              
  Lines        5081     5081              
  Branches     1451     1451              
==========================================
+ Hits         4533     4537       +4     
+ Misses        317      315       -2     
+ Partials      231      229       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[cmoesel]

I like the idea of investing in this.

One other thing I'd like to see is some way to keep the skiplist here synchronized with the skiplist in cql-tests-runner/conf/cql-execution-local.json. Note that the skiplist in cql-execution is a superset of the one in the cql-tests-runner config because the cql-tests-runner config only skips tests we consider to be invalid (whereas cql-execution also skips tests that we fail due to our own bugs/limitations).

I think the best way to do this is to have the skiplist here be the sole source of truth with a script to export the cql-tests-runner config from it. One thing we'd need to manage as well is keeping the set of tests that are used here and in the cql-tests-runner in sync. We could potentially use a submodule to cql-tests-runner for that since git supports recursive submodules.

[dehall]

@cmoesel Status update: I've added cql-tests-runner as a submodule here, and so that lets us build the spec-test CQL and ELM from its nested cql-tests submodule, and remove the cql-tests xml files from here.

The skip-lists are a little more complicated than they might seem though, there are:

• some tests that are skipped in both (tests we know have an incorrect expected result),
• some that are skipped in the tests-runner but not here (issues with how the tests-runner compares results), and
• some that are skipped here but not in the runner (eg, tests with numeric type discrepancies don't have to be skipped there since the tests-runner compares all numeric results as plain js number).

It's obviously not impossible to represent all this but I don't think it works well with the current skip-list.txt file format. And everything clean seems like a bigger refactor than I was hoping to do. But I'm exploring some options on that now

[dehall]

After exploring a few options, the one that I've implemented here is to have a separate test/spec-tests/runner-skip-overrides.txt file that defines the specific tests that we want to skip in the cql-tests-runner config, with the same format as our existing skip-list.txt. Then a new script test/spec-tests/create-runner-skip-list.js uses the two skip files to assemble the set of skipped tests, and it can exclude entire categories from the skip-list file, or specific tests. Then it reads the previous config file and replaces its SkipList field

I think it's still not a totally perfect option, but it's a much smaller change than rewriting everything to work with, say, a YAML skip file instead

[cmoesel]

Thanks for this @dehall. I didn't do a super thorough review, but there are a few things that I think we should consider:

• Excluding tests from the runner based on categories seems brittle and kind of opaque (when reviewing the skip-list.txt). Did you consider having three files: common-skip-list.txt, runner-skip-list.txt, and unit-skip-list.txt? That would be explicit and probably simpler (at least in the code).
• Now that we're bringing the runner in as a submodule, it might make sense for developers to actually use it in conjunction with the test-server (rather than needing to clone the runner somewhere else). That being the case should we move the sub-module up to the root and update the test-server README to call out the submodule location?
• ... which also makes me wonder if we might be able to eventually migrate to invoking the runner at part of our automated tests instead of using our homegrown test runner for the unit tests. The one thing holding me back is I don't like the idea of sparking up a real server (the test-server) listening on real ports as part of the automated tests. But if we could find a way around that...

[dehall]

Excluding tests from the runner based on categories seems brittle and kind of opaque (when reviewing the skip-list.txt). Did you consider having three files: common-skip-list.txt, runner-skip-list.txt, and unit-skip-list.txt? That would be explicit and probably simpler (at least in the code).

✅ I think conceptually the categories are pretty good, but yeah basing anything on the text of the category name is flaky. I had been hesitant to do much that would change things like check-skip-list.js, but breaking skip-list into multiple files isn't a big change there. Just call the main function in a loop.

Now that we're bringing the runner in as a submodule, it might make sense for developers to actually use it in conjunction with the test-server (rather than needing to clone the runner somewhere else). That being the case should we move the sub-module up to the root and update the test-server README to call out the submodule location?

✅

... which also makes me wonder if we might be able to eventually migrate to invoking the runner at part of our automated tests instead of using our homegrown test runner for the unit tests. The one thing holding me back is I don't like the idea of sparking up a real server (the test-server) listening on real ports as part of the automated tests. But if we could find a way around that...

I haven't implemented this yet. It is possible, Codex showed me a couple options, one with a replaced global fetch function, and one that reuses as many pieces from the runner as possible to implement our own "entrypoint", but neither is as clean as I would like. I think the idea overall makes sense but it's a bigger change, and we might want to propose some light refactoring to the runner itself to make this easier.

[cmoesel]

... which also makes me wonder if we might be able to eventually migrate to invoking the runner at part of our automated tests instead of using our homegrown test runner for the unit tests. The one thing holding me back is I don't like the idea of sparking up a real server (the test-server) listening on real ports as part of the automated tests. But if we could find a way around that...

I haven't implemented this yet. It is possible, Codex showed me a couple options, one with a replaced global fetch function, and one that reuses as many pieces from the runner as possible to implement our own "entrypoint", but neither is as clean as I would like. I think the idea overall makes sense but it's a bigger change, and we might want to propose some light refactoring to the runner itself to make this easier.

Yeah, that was definitely a longer term thought. You've already made great improvements to how this works, so I'm happy as it is right now.

[dehall]

@cmoesel The cql-to-elm-cli PR that this depended on is now merged and included in a release, so this is now ready for a full review