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

Add statement-level HTML to statement results #270

Merged
merged 8 commits into from
Aug 23, 2023

Conversation

sarahmcdougall
Copy link
Contributor

Summary

Adds statement-level HTML as a field on each statement result returned from measure calculation.

See #268 for more details on this feature.

New behavior

The user can now specify a new calculation option: buildStatementLevelHTML. When set to true, the statement results returned from the detailed results will contain a new element statementLevelHTML that maps to a string of the HTML markup for the given statement. Thus, the structure of each statementResult will be as follows:

{
   "libraryName": "library name",
   "statementName": "statement name",
   ...
   "statementLevelHTML": "statement-level HTML"
}

For backwards compatibility, the overall html element and corresponding debug output are still available.

Code changes

  • Documentation updates to reflect the new calculation option and updated output
  • Calculator type updates to reflect the new calculation option
  • HTML Builder changes
    • Removed the statementAnnotations array, which stores all the statement annotations and then loops over each one for generating HTML clauses. Instead generates the statement-level HTML when looping over the relevant statements, and appends the statement-level HTML to an overall HTML structure.
  • HTML Builder unit tests

Testing guidance

  • Run unit tests and integration tests (npm run test:plus)
  • Run measure calculation against a measure with the --debug flag enabled. After calculation finishes, compare the logic highlighting html to the statementResults available on the detailedResults.
    • For statements that are not highlighted in the debug html (that have relevance equal to NA), the statementLevelHTML element should not be present on the statement’s statementResults object.
    • For statements that do not have relevance equal to NA, the statementLevelHTML element should be present on the statement’s statementResults object, and the statement-level HTML should match the HTML markup for the given statement in the debug HTML file. You can search by the data-statement-name/data-library-name in both the statementResults and debug html to compare the markup. The statement-level HTML markup should include the <pre> tag with the appropriate styling.

Note - open to thoughts on integration testing for this feature. A backlog task exists for adding more thorough testing for HTML building since the unit tests are very small in scope, but we could incorporate the beginnings of integration testing into this PR if desired. I did some brainstorming but am not 100% sure of a path forward for the integration testing (ex. Do we want to add HTML building tests for each measure type? Do we want to add tests to just the proportion boolean integration test? Do we want to make a whole new directory of integration tests specifically for HTML building?)

@github-actions
Copy link

github-actions bot commented Aug 7, 2023

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements
86.49% (-0% 🔻)
2349/2716
🟡 Branches
73.77% (+0.08% 🔼)
2180/2955
🟢 Functions
89.05% (-0.02% 🔻)
423/475
🟢 Lines
86.84% (-0.01% 🔻)
2270/2614
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / HTMLBuilder.ts
92.97% (-0.05% 🔻)
79.01% (+2.62% 🔼)
94.44% (-0.15% 🔻)
93.81% (-0.05% 🔻)

Test suite run success

446 tests passing in 31 suites.

Report generated by 🧪jest coverage report action from a5f3f85

@elsaperelli elsaperelli self-requested a review August 7, 2023 18:00
@elsaperelli elsaperelli self-assigned this Aug 7, 2023
Copy link
Contributor

@elsaperelli elsaperelli left a comment

Choose a reason for hiding this comment

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

Great job with this!! I went to test out implementing it with the prettyResults in fqm-testify and it took me 5 minutes rather than the 1000 it took me when having to parse through the whole HTML :) Some small stuff, but I did some testing and the html looks great and it is backwards compatible.

In terms of more testing, I agree that we should add more HTML testing but in my opinion that is out of scope for this task and that we can add it whenever we do that integration testing task.

I also ran the regression script which was successful since all the tests failed (they should with the added statementLevelHTML but the html object did not change.

@@ -152,7 +152,8 @@ Statement results are a part of the calculation's `detailedResults` data. Statem
"relevance": <whether the statement impacted the calculation>,
"raw": <raw result of the statement calculation>,
"isFunction": <whether the statement is a function>,
"pretty": <human readable version of the raw result>
"pretty": <human readable version of the raw result>,
"statementLevelHTML": <Generated HTML markup for the CQL statement>
}
```

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we mention the .pretty attribute below here, should we mention the new statementLevelHTML attribute in like one sentence here too? Since the two are related (or at least we intend them to be used together). Or maybe just pull the section you wrote about it up here? Maybe?

"statementName": "SDE Sex",
"final": "TRUE",
"relevance": "TRUE",
"isFunction": false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add what the pretty result would look like here?

buildStatementLevelHTML: true
});

expect(statementResults[0].statementLevelHTML).toBeDefined();
Copy link
Contributor

Choose a reason for hiding this comment

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

The only other case I can think of for now is maybe one that makes sure there is no statementLevelHTML when the relevance is NA?

Copy link
Contributor

@elsaperelli elsaperelli left a comment

Choose a reason for hiding this comment

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

Lgtm!

Copy link
Contributor

@hossenlopp hossenlopp left a comment

Choose a reason for hiding this comment

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

This looks good, one really small request/question, should/is this option be defaulted to false?

Running the CLI with CMS1028 and test-cases generates a 350MB detailed results file. The CLI should minimally default to false to speed up runs.

@sarahmcdougall
Copy link
Contributor Author

This looks good, one really small request/question, should/is this option be defaulted to false?

Running the CLI with CMS1028 and test-cases generates a 350MB detailed results file. The CLI should minimally default to false to speed up runs.

Ah good point. The option is defaulted to true, but I agree it should default to false to speed up runs. I will make that change this morning 👍

Copy link
Contributor

@hossenlopp hossenlopp left a comment

Choose a reason for hiding this comment

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

Now we can see a clean regression run for this.

@hossenlopp hossenlopp merged commit 93dcd29 into master Aug 23, 2023
5 checks passed
@hossenlopp hossenlopp deleted the statement-level-html branch August 23, 2023 17:41
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.

3 participants