Stream: inferno
Topic: Reference Resolution Tests (Inferno 2.0 )
Nathan Loyer (Mar 25 2022 at 19:28):
Hey all,
I'm having an issue with the reference resolution tests for several resources in inferno 2.0. I created an issue on github for this, but wanted to bring it up here for discussion around it and to get some eyes on it.
https://github.com/inferno-framework/us-core-test-kit/issues/20
The first part of the problem is what the github issue is for, which is that the UI isn't showing us the failed requests. This makes it very difficult to determine what resource we're having problems with. So I can't even begin to determine if this is failing because of an issue with our code, or with the inferno code, which is the second part of my problem.
I found a TODO comment in the file saying that the data needs to be persisted, which I am assuming if that is done then I'd be able to debug the information further. Maybe there's a different root cause though.
Robert Scanlon (Mar 25 2022 at 19:45):
Thanks for pointing out this oversight, and doing the legwork to find exactly where the gap is. We'll work on adding this in as soon as we can.
Nathan Loyer (Mar 25 2022 at 20:00):
thanks. We have noticed a related issue in the bulk export tests as well, but I'm working with my team to determine the specifics so we can forward that along. By related issue I mean there was another instance of a test sequence not showing the HTTP request that caused the failure. We looked through the previous few tests to see if there was a related request that cached data, but weren't able to find it.
In general, any time a test references previous data it is hard for us to determine why something failed because we don't have easy access to the HTTP request. this last part was an issue in inferno program edition as well
Robert Scanlon (Mar 27 2022 at 14:46):
Yes, having a test fail due to a http request/response that happened in a previous test is not a great user experience. Thanks for letting us know that this is a real pain point for you, as it helps us prioritize addressing it.
Robert Scanlon (Mar 27 2022 at 14:50):
In the case of these resource resolution tests... we just aren't storing the requests at all right now which is an oversight. But I understand how it is hard for you to even figure that out, since we are forcing the user to comb through earlier tests to find where the data came from already.
Stephen MacVicar (Mar 28 2022 at 12:04):
In general, any time a test references previous data it is hard for us to determine why something failed because we don't have easy access to the HTTP request.
Is there a specific test where this is an issue? One of the improvements we made in this version of Inferno is the ability for tests to show the requests they used from earlier tests. There are certainly instances, however, where processing the request to extract the necessary data is burdensome, so individual pieces of a response are extracted and saved to be used by later tests rather than reusing the whole request.
Nathan Loyer (Mar 28 2022 at 14:13):
I'll have to check with my team on where exactly we last noticed that issue. I believe it was in the multi patient / bulk export test section in the g10 tests. I think it was with DocumentReference maybe? Will get back to you on that.
I'm not sure if the implementation is in the us core repo or the g10 repo or elsewhere. I understand why you broke the test kits up like this, but it does make it hard to poke through dependencies in the code. Do you have a recommended IDE or way to set the files up locally so you get features like intellisense's click through? Last week I checked out all the repos into the same parent directory and opened the parent directory in vscode, then with using an extension there called solargraph I was able to get some of these features, but still seems like it could be better.
Stephen MacVicar (Mar 28 2022 at 16:03):
I think there's only one person on the team who uses an Intellisense IDE, and they're out of the office this week. I use emacs, and switching projects and performing a project-wide search only take a few keystrokes, so that's what I do.
When it comes to dealing with requests in Inferno, methods are provided in the FHIRClient and HTTPClient modules to make FHIR and HTTP requests. These modules both make use of the RequestStorage module to actually store and retrieve the requests. So any requests made using a method from FHIRClient or HTTPClient will automatically be persisted and displayed in the UI. The reference resolution tests don't use a method from those modules, hence the TODO saying that we need to persist those requests.
We also provide a way for a test to specify that it relies on a request from an earlier test. If a test specifies uses_request, that request will be loaded, made available to the test when it runs, and displayed in the UI for that test. There's no way for a test to use a request from an earlier test without doing this, but there are many situations where we pull information out of a request and use that rather than reusing the request itself. For instance, decoding and validating a JWT is a multi-step process which could fail at multiple points, so rather than passing a request with a JWT around and repeating all of those steps, we are likely to extract the relevant information in one place and pass that around instead. Those types of inputs/outputs are not currently displayed in the UI.
@Robert Scanlon I think there are a couple takeaways from this in addition to needing to prioritize the reference resolution request issue:
- When tests use information from an earlier request, we should stick
uses_requeston them even if they don't directly make use of the request in their code so that those requests appear in the UI. A potential downside is that it would prevent the test from running if those requests don't exist. Maybe that's fine, or maybe we want some way to say that the request isn't directly needed for the test, but it should be displayed in the UI if available. - It may be useful to also prioritize displaying inputs/outputs on individual tests.
Nathan Loyer (Mar 28 2022 at 18:59):
sounds like the one line just needs to be updated to use the fhir_read instead of the reference.read method... I was trying to make the change locally and test it, but I'm struggling to figure out how to link my local copy of the g10 kit to my local copy of the us core kit and still build/run via the docker-compose files in the g10 repo. I suppose I can open a PR with what I think the change is, but I am having difficulty testing it so not sure if I should just wait for one of yall to get to it instead
Stephen MacVicar (Mar 28 2022 at 19:10):
The thing that makes it more than a one-liner is ensuring that all types of references (contained, relative, absolute) are handled correctly.
If you want to try things out with a modified version of a test kit, you can do the following:
- remove/comment out the dependency in
onc_certification_g10_test_kit.gemspec - add a corresponding entry in
Gemfile, and point it to a git repo:
gem 'us_core_test_kit', git: 'https://...whatever.git', branch: 'some-branch'
- run
bundle_updateto update that gem, e.g.bundle update us_core_test_kit
Stephen MacVicar (Mar 28 2022 at 19:12):
We are planning to address this quickly, but if you know, for example, that you only need to deal with relative references, you may be able to change that line pretty easily to quickly test against your system.
Nathan Loyer (Mar 28 2022 at 19:13):
Ah I hadn't thought to try linking to a repo. I was trying to link it through my file system and ran into issues because the directory with the files wasn't shared with the docker build daemon. I decided earlier to see if I could replicate in the us core repo alone.
Nathan Loyer (Mar 28 2022 at 19:13):
We're only using relative references in our server.
Stephen MacVicar (Mar 28 2022 at 19:14):
Yeah, you can point it at a path with the path: option, but then you need to put that repo inside the g10 repo to deal with docker directory permissions.
EDIT: The other issue with this is you need to update the Dockerfile to copy in the folder with that repo before it performs bundle install.
Nathan Loyer (Mar 28 2022 at 19:15):
I wasn't sure how long it would take for yall to get a fix out and in a versioned release that we could pick up to validate. So I was still trying to see if I could hack it together to at least get us more info so we can debug the issue on our side
Stephen MacVicar (Mar 28 2022 at 19:21):
As a temporary workaround only supporting relative references, I think you can replace that line with:
resolved_resource = fhir_read(reference.resource_type, reference.reference_id)&.resource
Nathan Loyer (Mar 28 2022 at 19:30):
fhir_read(reference.resource_type, reference.reference_id)
that was my guess, although trying to start up the server locally I hit an error before it even got that far. Starting up on tag v0.1.1 doesn't have this error
worker_1 | E, [2022-03-28T19:29:04.344055 #1] ERROR -- : /usr/local/bundle/gems/inferno_core-0.2.0.rc1/lib/inferno/entities/test.rb:111:in `method_missing': undefined method `find_a_value_at' for #<#<Class:0x0000560d81814ba8>:0x00007f80f1720a78> (NoMethodError)
worker_1 | Did you mean? find_validator
worker_1 | from /opt/inferno/lib/us_core_test_kit/reference_resolution_test.rb:46:in `block (2 levels) in unresolved_references'
worker_1 | from /opt/inferno/lib/us_core_test_kit/reference_resolution_test.rb:45:in `any?'
worker_1 | from /opt/inferno/lib/us_core_test_kit/reference_resolution_test.rb:45:in `block in unresolved_references'
worker_1 | from /opt/inferno/lib/us_core_test_kit/reference_resolution_test.rb:40:in `select'
worker_1 | from /opt/inferno/lib/us_core_test_kit/reference_resolution_test.rb:40:in `unresolved_references'
worker_1 | from /opt/inferno/lib/us_core_test_kit/reference_resolution_test.rb:10:in `perform_reference_resolution_test'
worker_1 | from /opt/inferno/lib/us_core_test_kit/generated/v3.1.1/care_team/care_team_reference_resolution_test.rb:32:in `block in <class:CareTeamReferenceResolutionTest>'
Stephen MacVicar (Mar 28 2022 at 19:32):
Yeah, that's fixed in an open PR. Stick require_relative 'fhir_resource_navigation' at the top of reference_resolution_test.rb.
Stephen MacVicar (Mar 28 2022 at 19:35):
Oh, and include FHIRResourceNavigation in the top of the ReferenceResolutionTest module in that file
Nathan Loyer (Mar 28 2022 at 19:54):
so yeah the fhir_read change is working for me locally, and conveniently/inconveniently the root cause of the issue which was why I am looking at this in the first place is not reproduceable today, so that's something :/
Nathan Loyer (Mar 28 2022 at 19:55):
should I bother making a PR with just this change?
Nathan Loyer (Mar 28 2022 at 21:35):
was able to use the gemfile with the pointer to my github fork to update our g10 deployment in aws. Verified that it let me see the attempted requests there as well, so further confirmation that this approach works for relative references
Stephen MacVicar (Mar 29 2022 at 12:01):
should I bother making a PR with just this change?
No, it'll require more changes than that.
The problem could be that your access token was expiring, and now that you're using fhir_read, it's automatically refreshing.
Nathan Loyer (Mar 29 2022 at 13:55):
No, it'll require more changes than that.
Got it, I'll keep our workaround for now and wait for your fix to remove it. Although our test proctor expects us to use their deployment, so we're going to need this and the PKCE code verifier fix released before we can complete cert, which we were planning to do in april.
The problem could be that your access token was expiring, and now that you're using fhir_read, it's automatically refreshing.
That seems likely, when the tests were failing we saw that it was always 401 response codes (the only detail available to us was 401 response and what field the reference was from), and the resources that failed were inconsistent between test runs. When I got it deployed to our prod deployment of inferno all of the tests passed, so pretty sure you're right on that.
On that note, it appears that every single test is requesting a new token. Is that intentional? I would expect the test code to only request a new token either shortly before it expires or shortly after. This current method works fine, it's just a little spammy on /token requests
Stephen MacVicar (Mar 30 2022 at 12:03):
It shouldn't refresh in each test. I don't see that in runs against our reference server. This is the logic which determines whether a refresh will be performed:
return false if access_token.blank? || refresh_token.blank?
return true if expires_in.blank?
token_retrieval_time.to_i + expires_in - DateTime.now.to_i < 60
Nathan Loyer (Mar 30 2022 at 20:21):
interesting, I wonder if this might be a time zone issue
Nathan Loyer (Mar 30 2022 at 20:22):
where is this code..? got a link or something so I can take a closer look?
Stephen MacVicar (Mar 31 2022 at 12:59):
Sure, all of the fhir requests are wrapped in store_request_and_refresh_token, which checks whether the FHIR client is able to refresh its token and whether it needs to refresh its token. The client just delegates those calls to its OAuthCredentials, which is a class/input type we made so that we could handle automatic refreshes.
Buu Le (Apr 01 2022 at 07:03):
Hi all,
I'm new to inferno and fhir,
I'm trying to run test against SMART Scheduling Links - Slot Publisher Tests
But get Fatal error for SLB-06 and SLB-11
SLB-06: Location ndjson files contain valid FHIR resources that have all required fields.
Fatal Error: incompatible character encodings: UTF-8 and ASCII-8BIT
554 unknown Locations referenced as actor (e.g. Location/c8100f58d380e10f77c5f7123ee8a4ccf4691f89960b190573cf105aed0efe87)
Does someone know why it happens and how can I pass the step ?
Nathan Loyer (Apr 04 2022 at 19:32):
Stephen MacVicar said:
Sure, all of the fhir requests are wrapped in
store_request_and_refresh_token, which checks whether the FHIR client is able to refresh its token and whether it needs to refresh its token. The client just delegates those calls to its OAuthCredentials, which is a class/input type we made so that we could handle automatic refreshes.
Yeah based on what I'm reading, our tests shouldn't be requesting a new token for every inferno test section... but it is... only other thing I can think of is maybe this OAuthCredentials class is re-initialized for every Inferno::Test that is created/run? As mentioned before, I don't know ruby very well so I don't know if this is creating a singleton or if it's initializing it over and over again.
Last updated: Apr 12 2022 at 19:14 UTC
