Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression cql #321

Merged
merged 9 commits into from
Nov 20, 2024
Merged

Regression cql #321

merged 9 commits into from
Nov 20, 2024

Conversation

lmd59
Copy link
Contributor

@lmd59 lmd59 commented Nov 7, 2024

Summary

Updates regression scripts to do an npm install before running so that cql-execution (and other dependencies) can be changed between runs based on the package file.
Adds scripting for pulling from the gitlab coverage-script-bundles and test regression against those formats of packages.

New behavior

No changes to fqm-execution functionality

Code changes

  • Update run-regression.sh file to pull coverage-script-bundles from gitlab and npm install before runs
  • In regression.ts add findPatientBundlePathsInDirectory for pulling test files out of nested folders, pull some functionality into a shared calculateRegression function, and add functionality for handling and testing against the structure of the coverage-script-bundles directories.

Testing guidance

Create a local branch off of this branch and commit any changes, including a change to the cql-execution version ("cql-execution": "3.0.1",), clone the coverage bundles repo into the regression/bundles directory, then run regression scripts using ./regression/run-regression.sh -b regression-cql -p. (It's important to have the same regression.ts file on both branches that are being compared.)
Note that ecqm content and connectathon repositories will be pulled into the default-bundles folder with the -p flag.

Copy link

github-actions bot commented Nov 7, 2024

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements 85.63% 2455/2867
🟡 Branches 73.41% 2308/3144
🟢 Functions 87.95% 438/498
🟢 Lines 85.91% 2371/2760

Test suite run success

459 tests passing in 31 suites.

Report generated by 🧪jest coverage report action from 6b05441

@elsaperelli elsaperelli self-requested a review November 7, 2024 14:11
@elsaperelli elsaperelli self-assigned this Nov 7, 2024
Copy link

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @lmd59! No one asked me to review this, but I needed to run a regression anyway, so I took it for a spin.

Everything seems to be working well! The addition of the coverage script bundles is a nice feature, as it appears to add quite a few additional test cases. Excellent!

The one thing I would suggest you consider is making it optional to include the coverage script bundles (maybe enabled by a command line flag). The reasons:

  1. The coverage script bundles take a long time. Sometimes a quick regression might be preferred.
  2. Anyone using fqm-execution outside of MITRE will not have access to the MITRE GitLab -- so the coverage script bundles will never work for them.

Thanks for this. It will be helpful in the future for feeling more confident about changes to cql-execution.

@elsaperelli elsaperelli requested review from hossenlopp and removed request for elsaperelli November 11, 2024 14:23
@lmd59
Copy link
Contributor Author

lmd59 commented Nov 11, 2024

Thank you, @lmd59! No one asked me to review this, but I needed to run a regression anyway, so I took it for a spin.

Everything seems to be working well! The addition of the coverage script bundles is a nice feature, as it appears to add quite a few additional test cases. Excellent!

The one thing I would suggest you consider is making it optional to include the coverage script bundles (maybe enabled by a command line flag). The reasons:

  1. The coverage script bundles take a long time. Sometimes a quick regression might be preferred.
  2. Anyone using fqm-execution outside of MITRE will not have access to the MITRE GitLab -- so the coverage script bundles will never work for them.

Thanks for this. It will be helpful in the future for feeling more confident about changes to cql-execution.

Thanks for the comments @cmoesel - this is a great idea, and I've updated with a command line option to include the internal testing files (default is to not include them). Let me know if you have any other recommendations!

Copy link

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thank you, @lmd59! That said, I'm new here and wasn't around for discussions about this feature -- so you probably will want a review and approval from someone on the team as well.

@lmd59 lmd59 merged commit 87786bd into master Nov 20, 2024
6 checks passed
@lmd59 lmd59 deleted the regression-cql branch November 20, 2024 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants