Skip to content

FI-3412 | Map client bulk data requirements to existing tests#35

Merged
karlnaden merged 21 commits into
mainfrom
v2-verifies-requirements
Jul 1, 2025
Merged

FI-3412 | Map client bulk data requirements to existing tests#35
karlnaden merged 21 commits into
mainfrom
v2-verifies-requirements

Conversation

@carrils

@carrils carrils commented Dec 10, 2024

Copy link
Copy Markdown
Contributor

Summary

This adds references to the requirements fulfilled by the tests in bulk data v2.
Where each requirement is tested, a reference to the requirement_ID has been made.
The requirements_coverage csv file has also been updated with requirements:generate_coverage.

Testing Guidance

The test list map (excel workbook on sharepoint for bulk data v2) is a spreadsheet that lists the requirement, the location of where it is tested, and whether it has been added to the codebase with the verifies_requirement inferno DSL.

The bulk-data-test-kit_requirements_coverage.csv file has short ID's and Full ID's.

Can pull down branch and try running the rake requirements:generate_coverage command to generate the csv with short and full ID's.

@edeyoung edeyoung left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

not quite finished, but some actionable things for you to take a look at

Comment thread lib/bulk_data_test_kit/v1.0.1/bulk_data_export_cancel_test.rb Outdated
Comment thread lib/bulk_data_test_kit/v1.0.1/bulk_data_export_cancel_test.rb Outdated
Comment thread lib/bulk_data_test_kit/v1.0.1/bulk_data_export_kick_off_test.rb Outdated
Comment thread lib/bulk_data_test_kit/v1.0.1/bulk_data_export_kick_off_test.rb Outdated
Comment thread lib/bulk_data_test_kit/v1.0.1/bulk_data_export_kick_off_test.rb Outdated
Comment thread lib/bulk_data_test_kit/v1.0.1/bulk_data_export_kick_off_test.rb Outdated
Comment thread lib/bulk_data_test_kit/v1.0.1/bulk_data_export_kick_off_test.rb Outdated
@carrils carrils requested a review from edeyoung December 13, 2024 18:49

@edeyoung edeyoung left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

thanks for your updates! similarly wasn't able t finish everything but have some actionable items for you.

Comment thread lib/bulk_data_test_kit/v1.0.1/bulk_data_output_check_test.rb Outdated
Comment thread lib/bulk_data_test_kit/v1.0.1/bulk_data_output_check_test.rb Outdated
Comment thread lib/bulk_data_test_kit/v1.0.1/bulk_data_output_check_test.rb Outdated
Comment thread lib/bulk_data_test_kit/v1.0.1/bulk_data_status_check_test.rb Outdated
Comment thread lib/bulk_data_test_kit/v1.0.1/bulk_data_status_check_test.rb Outdated
Comment thread lib/bulk_data_test_kit/v1.0.1/bulk_data_status_check_test.rb Outdated
Comment thread lib/bulk_data_test_kit/v1.0.1/bulk_data_status_check_test.rb Outdated
Comment thread lib/bulk_data_test_kit/v1.0.1/bulk_data_status_check_test.rb Outdated
Comment thread lib/bulk_data_test_kit/v1.0.1/bulk_data_status_check_test.rb Outdated
Comment thread lib/bulk_data_test_kit/v1.0.1/bulk_data_status_check_test.rb Outdated
Comment thread lib/bulk_data_test_kit/v1.0.1/bulk_data_status_check_test.rb Outdated

@edeyoung edeyoung left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please see the latest comments.

Thoughts: in the excel doc, filter for the conformance verb SHALL and the Actor Server (or Server/Client). That will reduce your requirements pool considerably. Do you have an idea what requirements coverage looks like at this point?

Thank you!

Comment thread lib/bulk_data_test_kit/v1.0.1/bulk_data_valid_resources_test.rb Outdated
Comment thread lib/bulk_data_test_kit/v1.0.1/bulk_data_valid_resources_test.rb Outdated
Comment thread lib/bulk_data_test_kit/v1.0.1/group/bulk_data_group_export_group.rb Outdated
Comment thread lib/bulk_data_test_kit/v2.0.0/bulk_data_export_cancel_test.rb Outdated
Comment thread lib/bulk_data_test_kit/v2.0.0/bulk_data_export_cancel_test.rb Outdated
'hl7.fhir.uv.bulkdata_2.0.0@49',
'hl7.fhir.uv.bulkdata_2.0.0@51',
'hl7.fhir.uv.bulkdata_2.0.0@52',
'hl7.fhir.uv.bulkdata_2.0.0@230',

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

it looks like the 230+ requirements are specific to output types: group / patient / system. Is there a place where these can be moved to?

@carrils carrils Jan 13, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So 230 specifically is _outputFormat: The server SHALL support Newline Delimited JSON. I left it here in v2.0.0/bulk_data_outputFormat_param_test.rb because of its location in the IG.
I have reviewed the operation-specific requirements and have moved these:
223, 224, 225, 229, 249, and 272.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe this is correct but leaving it open for confirmation.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

230 should be deprecated in favor of 49, which is how the rest of the output type specific requirements are.
230 should be removed from here.

Deprecating in the requirements sheet now.

Comment thread lib/bulk_data_test_kit/v2.0.0/bulk_data_since_param_test.rb Outdated
Comment thread lib/bulk_data_test_kit/v2.0.0/bulk_data_smart_backend_services_v200_group.rb Outdated
@elsaperelli elsaperelli self-requested a review May 13, 2025 17:56
@elsaperelli elsaperelli self-assigned this May 13, 2025
Comment thread lib/bulk_data_test_kit/v1.0.1/bulk_data_valid_resources_test.rb Outdated
@edeyoung edeyoung self-requested a review May 17, 2025 18:04

@edeyoung edeyoung left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please

  • regenerate the entire requirements file
  • regenerate the coverage file

then

  • check if your coverage file is showing 47, 49, 51, 52 as uncovered
  • if so, trouble shoot why the coverage file is showing 47,49,51,52 (and maybe others?) as uncovered when they are referenced in lib/bulk_data_test_kit/v2.0.0/bulk_data_outputFormat_param_test.rb

Please also confirm that you reviewed the uncovered requirements and double checked that they couldn't be covered in the existing code.

'hl7.fhir.uv.bulkdata_2.0.0@49',
'hl7.fhir.uv.bulkdata_2.0.0@51',
'hl7.fhir.uv.bulkdata_2.0.0@52',
'hl7.fhir.uv.bulkdata_2.0.0@230',

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

230 should be deprecated in favor of 49, which is how the rest of the output type specific requirements are.
230 should be removed from here.

Deprecating in the requirements sheet now.

Comment thread lib/bulk_data_test_kit/v2.0.0/bulk_data_outputFormat_param_test.rb Outdated
@elsaperelli

Copy link
Copy Markdown
Contributor

Please

  • regenerate the entire requirements file
  • regenerate the coverage file

then

  • check if your coverage file is showing 47, 49, 51, 52 as uncovered
  • if so, trouble shoot why the coverage file is showing 47,49,51,52 (and maybe others?) as uncovered when they are referenced in lib/bulk_data_test_kit/v2.0.0/bulk_data_outputFormat_param_test.rb

Please also confirm that you reviewed the uncovered requirements and double checked that they couldn't be covered in the existing code.

  • regenerated entire requirements file
  • regenerated the coverage file
  • removed req 230
  • figured out why covered reqs were hidden- moved annotations 47, 49, 51, 52, 53
  • Reviewed uncovered requirements- have some notes on ones that may be covered below

Remaining Questions:

  • I think that the _since parameter for Patient-level export should actually be a SHALL requirement instead of a MAY
  • The following are requirements that may be tested but I am not totally sure:
    • 15, 16, 18 - covered by bulk_data_client_wait_test.rb?
    • 32 - perform_export_kickoff_request ?
    • 148 - bulk_status_check_test.rb?
    • 201 - is this test correctly placed?

@elsaperelli elsaperelli requested a review from edeyoung May 19, 2025 20:00
@edeyoung

edeyoung commented Jun 17, 2025

Copy link
Copy Markdown
  • I think that the _since parameter for Patient-level export should actually be a SHALL requirement instead of a MAY

Is this @53 "_since: Optionality for Server: required"?

  • The following are requirements that may be tested but I am not totally sure:

    • 15, 16, 18 - covered by bulk_data_client_wait_test.rb?

bulk_data_client_wait_test.rb doesn't look like it actually tests anything - it just waits for the request to then send the data to other tests.
Maybe bulk_data_client_token_verification_test.rb for 15/16 (need to check the innards of the smart test)
18 would be an attestation - not erroring out when sent bulk data tests.

  • 32 - perform_export_kickoff_request ?

I don't think so - i don't see anything checking comma delimited stuff here

  • 148 - bulk_status_check_test.rb?

this is an optional test, so not here.
i think this needs to be an attestation. the requirement is whether or not the files are accessible to the client, which I don't think inferno does (or can) test.

  • 201 - is this test correctly placed?

I think it's in the right place, but i'm confused about how it's tested when the expected response in ndjson_download_requiresAccessToken_check is assert_response_status([400, 401])

@karlnaden karlnaden merged commit a31ce28 into main Jul 1, 2025
2 checks passed
@karlnaden karlnaden deleted the v2-verifies-requirements branch July 31, 2025 14:14
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