Stream: fhirpath.js
Topic: r4 tests
Paul Lynch (Sep 07 2018 at 14:46):
@Nikolay Artamonov I was about to work on adding the r4 tests, and Nicola said the two of you had discussed something like that. I was going to write a script to convert the patient xml resource into JSON, and then another script to convert https://github.com/hl7-fhir/fhir-svn/blob/master/tests/resources/tests-fhir-r4.xml into yaml, and add some code to test/fhirpath.test.js to read the yaml and run its tests. I would add support for an "exclude" option on the test groups and also the individual tests, so we can turn off the tests for the parts we don't (yet) support.
However, if you were already planning to do this, then I will happily let you handle that and I will and move on to another part of the FHIRPath spec.
Nikolay Artamonov (Sep 07 2018 at 14:52):
@Paul Lynch Today I start writing script to convert fhir-r4.xml into yaml, so I can do this part.
nicola (RIO/SS) (Sep 07 2018 at 14:53):
I think, we can push this code into repo in a separate folder
Paul Lynch (Sep 07 2018 at 14:55):
Yes, I was thinking of test/bin for test-related utility scripts, and maybe test/r4 for r4 tests.
@Nikolay Artamonov Sounds good-- I will wait for your changes.
Nikolay Artamonov (Sep 07 2018 at 14:56):
@nicola (RIO/SS) converter code?
nicola (RIO/SS) (Sep 07 2018 at 14:57):
Yes - so we can re-run it later on next version of FHIR xml :)
Paul Lynch (Sep 11 2018 at 19:23):
I think I had found an npm package for reading XML. I can't remember which one.
nicola (RIO/SS) (Sep 11 2018 at 19:24):
:+1: This is one time job - please add it to devDeps
Paul Lynch (Sep 11 2018 at 21:46):
@Nikolay Artamonov I think it was https://www.npmjs.com/package/xml2js, which seems popular.
Nikolay Artamonov (Sep 12 2018 at 12:15):
Thx @Paul Lynch , I will try it
nicola (RIO/SS) (Sep 18 2018 at 02:59):
I hate babel - do we really need it - it blows the project :( Can we do it without babel?
Nikolay Artamonov (Sep 18 2018 at 14:11):
Oh, ok ( I will remove it
Nikolay Artamonov (Sep 18 2018 at 15:20):
@Paul Lynch I push some changes, can U review ?
Paul Lynch (Sep 18 2018 at 15:33):
Thanks-- will do.
Nikolay Artamonov (Sep 18 2018 at 16:21):
I can't find inputfile - patient-example.xml in HL7 repos, it's this Patient-example file https://www.hl7.org/fhir/patient-example.json.html ?
Paul Lynch (Sep 18 2018 at 16:49):
It is here: https://github.com/FHIR/fluentpath/blob/master/spec/tests.zip
Paul Lynch (Sep 18 2018 at 18:15):
@Nikolay Artamonov PR approved; now we are just waiting on the second review. I think the converter you've written is enough for one PR. The work to actually integrate the converted tests can be a separate PR.
@nicola (RIO/SS) Will you be able to review Nikolay's PR soon? (https://github.com/lhncbc/fhirpath.js/pull/58)
Nikolay Artamonov (Sep 18 2018 at 18:22):
Thx @Paul Lynch ! Generated yaml test file contains 2k lines, think we should split it? For patient-example.xml - need another converter?
Paul Lynch (Sep 18 2018 at 19:35):
I don't think we should split it up. However, I just "unapproved" your pull request by requesting another change. We need to capture the value of inputfile for each test. If search the test XML file, you will see some tests that use something other than patient-example.xml.
I am hoping we don't have to convert patient-example.xml. I have posted a question on the fhirpath.js stream.
Paul Lynch (Sep 18 2018 at 22:02):
@Nikolay Artamonov Following Bryn's example, I was able to get the patient example loaded here: http://hapi.fhir.org/baseDstu3/Patient/example although I had to change the organization ID to match one actually on the FHIR server. I guess it would probably be better to change it back to one after you copy that.
Nikolay Artamonov (Sep 19 2018 at 13:03):
@Paul Lynch How we should update test with value of input file ? I our tests data-structure saved in test file or we can connect it from another file?
Paul Lynch (Sep 19 2018 at 13:35):
The tests should just have "inputfile: [name of file]". I guess for "[name of file]", you could replace ".xml" with ".json". Then, add the JSON resources the tests need as separate files (maybe in a subdirectory). See Bryn's response at: https://chat.fhir.org/#narrow/stream/115-fhirpath/subject/JSON.20version.20of.20FHIRPath.20tests.3F for where to find the files (note the "raw" link).
Nikolay Artamonov (Sep 19 2018 at 16:56):
- desc: '** test'
inputfile: patient-example.json
expression: 1 | 1 is Integer
result:
- true
looks ok?
Nikolay Artamonov (Sep 19 2018 at 16:58):
or
- desc: '** test'
- inputfile: patient-example.json
expression: 1 | 1 is Integer
result:
- true
Nikolay Artamonov (Sep 19 2018 at 17:28):
Also thinks we should convert json into yaml, isn't?
Paul Lynch (Sep 19 2018 at 18:24):
I don't think we should bother converting the JSON resources into YAML. It is better to stick with the format we get from the FHIR build.
Regarding the test formats above, I think the first is correct, because for each test there is a description and an input file.
Nikolay Artamonov (Sep 19 2018 at 19:38):
@Paul Lynch how we connect json resouce files with yaml test cases? Convert and manually past in? Can test file include more than one "subject" ?
Paul Lynch (Sep 19 2018 at 19:42):
Yes, more than one subject (potentially a different one for each test). The test processing code will have to be modified to handle the new "inputfile" property. When it finds a new inputfile, it will load it and use it as the subject for the test. (It can cache the loaded files, because they won't change.) But, this change should be done in a separate pull request. All was I hoping for in this one was the inclusion of the "inputfile" parameter in the test definition file, and perhaps the JSON resources. Since your changes in this PR have already been reviewed, I don't want to add a bunch of new code changes for the processing in the same PR.
Paul Lynch (Sep 20 2018 at 17:27):
@Nikolay Artamonov Now that that is merged, are you planning to revise our test running code to work with the converted fhir-r4.xml?
I was thinking the converted test file could be checked in as fhir-r4.json, and then we could disable groups or individual tests with some additional property (e.g., disable: false) for things we do not yet support.
Nikolay Artamonov (Sep 20 2018 at 19:41):
Why json? We convert into yaml, and how we will include input files data?
Paul Lynch (Sep 20 2018 at 19:50):
Right, yaml for the tests. When the test processor reads a test with an "inputfile" property, it will load that file (if not already loaded) and use it as the subject for the test.
Nikolay Artamonov (Sep 20 2018 at 20:11):
Input load file already done? So we should convert json inputfile data to yaml, right?
Paul Lynch (Sep 20 2018 at 20:44):
The inputfile values are the filenames you added under test/resources. I think you should leave those as JSON and have fhirpath.test.js
read them in as needed to execute the expressions in the converted yaml from the standard r4 tests.
Nikolay Artamonov (Sep 24 2018 at 08:30):
I add converted r4 test with disabled not implemented functions
Paul Lynch (Sep 24 2018 at 15:45):
Thanks-- I took a quick look just now and it looks good, but I will go through it in more detail. I added @nicola (RIO/SS) as a reviewer.
Nikolay Artamonov (Sep 27 2018 at 16:10):
@Paul Lynch @nicola (RIO/SS) What are my next steps?
Paul Lynch (Sep 27 2018 at 16:51):
@Nikolay Artamonov On the pull request, did you see the comment about preserving the test structure (groups of tests)?
Nikolay Artamonov (Sep 29 2018 at 17:14):
Yes, ok I will do it =)
Paul Lynch (Sep 29 2018 at 20:00):
Great-- thanks!
Nikolay Artamonov (Oct 02 2018 at 13:44):
What mean "focus" option in tests?
Nikolay Artamonov (Oct 02 2018 at 18:19):
I implement grops of tests.
https://www.dropbox.com/s/nkz1ol1psg6ef64/Screenshot%202018-10-02%2021.19.10.png?dl=0
Paul Lynch (Oct 03 2018 at 15:01):
The screenshot looks good! "focus" means "run this test (or group)". There is a flag in the the test runner that tells it to only run the focused tests (or groups).
Nikolay Artamonov (Oct 08 2018 at 08:16):
@Paul Lynch can you check my last changes in pr ?
Paul Lynch (Oct 09 2018 at 13:54):
Yes-- sorry for the delay on that. I will do it today.
I suppose it would best to disable the tests that are failing for now (since it is from new tests rather than new code) so we can get it merged.
Nikolay Artamonov (Oct 09 2018 at 15:44):
I disable tests which are failed with not implemented parts.
Nikolay Artamonov (Oct 09 2018 at 16:08):
Another failed tests - looks like we have bugs in the implementations
Paul Lynch (Oct 09 2018 at 16:35):
Yes, clearly we do have some bugs, and your tests are showing that. What I am suggesting is that we disable those too, so we can go ahead and merge your tests in. Then, as we have time, we can re-enable those tests and fix the issues. It will be easier for me to run your tests if they are merged and part of the branch I am working on.
Paul Lynch (Oct 09 2018 at 17:09):
Reviewed commits.
Last updated: Apr 12 2022 at 19:14 UTC
