healthcare API: update all FHIR v1beta1 samples to v1 #3384
Conversation
* healthcare API: update all v1beta1 samples to v1 * healthcare API: use v1 instead of v1beta1 endpoint * healthcare API: remove FHIR conditional methods from v1 because they are only in v1beta1 * add requirements-test.txt files * Change test response for searching for studies to 204 * fix missing param in execute_bundle.json causing test failures * fix HL7v2 store failure. notificationConfig was deprecated in favor of notificationConfigs * address review comments: use ADC and simplify credentials auth * delete new FHIR files, they are now in #3384 * delete new DICOM files, they are now in #3387 * delete new HL7v2 files, they are now in #3388 * clean up test_dataset fixture * remove try block from the sample * more robust test setup and cleanup Co-authored-by: Takashi Matsuo <tmatsuo@google.com>
|
|
||
|
|
||
| def get_client(): | ||
| """Returns an authorized API client by discovering the Healthcare API and |
There was a problem hiding this comment.
Nit: Usually, we format long docstring like the following:
def get_client():
"""Returns an authorized API client.
It assumes you set GOOGLE_APPLICATION_CREDENTIALS environment
variable.
"""Short summary, then long description.
|
|
||
| # [START healthcare_get_client_fhir] | ||
| def get_client_fhir(): | ||
| """Returns an authorized API client by discovering the Healthcare API and |
|
|
||
| print("Created Encounter resource with ID {}".format(response["id"])) | ||
| print(json.dumps(response, indent=2)) | ||
|
|
There was a problem hiding this comment.
Here and there, I'd remove these 2 empty lines.
There was a problem hiding this comment.
A result of black as we discussed in the other PR. I'll try removing it when I remember to.
| # limitations under the License. | ||
|
|
||
| import os | ||
| import pytest |
There was a problem hiding this comment.
Can you put pytest import in the bottom below?
There was a problem hiding this comment.
I don't see the change. The import statements should be in 3 blocks; 1) standard libs 2) 3rd party libs 3) local libs
So they should be:
import os
import sys
import uuid
import backoff
from googleapiclient.errors import HttpError
import pytest
# Add datasets for bootstrapping datasets for testing
sys.path.append(os.path.join(os.path.dirname(__file__), "..", "datasets")) # noqa
import datasets
import fhir_stores
import fhir_resourcesor something like that
|
|
||
| @pytest.fixture(scope="module") | ||
| def test_dataset(): | ||
| @retry( |
There was a problem hiding this comment.
FYI, I replaced retrying with backoff in #3434.
It's up to you to follow.
| # The API returns 200 whether the resource exists or was | ||
| # successfully deleted or not, so only retry on | ||
| # unathorized exceptions. | ||
| if err.resp.status == 401: |
There was a problem hiding this comment.
This code ignores the exception when the status is 401. Is this intended?
|
|
||
| # [START healthcare_get_client] | ||
| def get_client(): | ||
| """Returns an authorized API client by discovering the Healthcare API and |
There was a problem hiding this comment.
Short summary and long description.
| def test_import_fhir_store_gcs(test_dataset, test_fhir_store, capsys): | ||
| storage_client = storage.Client() | ||
| bucket = storage_client.get_bucket(gcs_uri) | ||
| blob = bucket.blob(source_file_name) |
There was a problem hiding this comment.
I think it is very easy to create a fixture for blob. WDYT?
With the current code, it is not guaranteed to be deleted.
| resource_id, | ||
| ): | ||
| """ | ||
| Deletes a FHIR resource. Regardless of whether the operation succeeds or |
There was a problem hiding this comment.
Can you format the docstring like:
"""Deletes a FHIR resource.
Regardless of whether the operation succeeds or....| base_url, project_id, cloud_region, dataset_id, fhir_store_id | ||
| ): | ||
| """ | ||
| Searches resources in the given FHIR store using the |
There was a problem hiding this comment.
ditto:
"""Searches resources in the given FHIR store.
It uses the ...| # limitations under the License. | ||
|
|
||
| import os | ||
| import pytest |
There was a problem hiding this comment.
I don't see the change. The import statements should be in 3 blocks; 1) standard libs 2) 3rd party libs 3) local libs
So they should be:
import os
import sys
import uuid
import backoff
from googleapiclient.errors import HttpError
import pytest
# Add datasets for bootstrapping datasets for testing
sys.path.append(os.path.join(os.path.dirname(__file__), "..", "datasets")) # noqa
import datasets
import fhir_stores
import fhir_resourcesor something like that
| # The API returns 200 whether the resource exists or was | ||
| # successfully deleted or not, so only retry on | ||
| # unathorized exceptions. | ||
| if err.resp.status == 401: |
|
|
||
| out, _ = capsys.readouterr() | ||
|
|
||
| print(out) |
There was a problem hiding this comment.
Why do you want to print this?
|
|
||
| import backoff | ||
| import pytest | ||
|
|
|
|
||
| out, _ = capsys.readouterr() | ||
|
|
||
| print(out) |
There was a problem hiding this comment.
nit: you don't need this I guess.
There was a problem hiding this comment.
Weird, I don't know where these statements came from.
|
|
||
| out, _ = capsys.readouterr() | ||
|
|
||
| print(out) |
|
|
||
| out, _ = capsys.readouterr() | ||
|
|
||
| print(out) |
|
|
||
| out, _ = capsys.readouterr() | ||
|
|
||
| print(out) |
|
|
||
| out, _ = capsys.readouterr() | ||
|
|
||
| print(out) |
|
|
||
| out, _ = capsys.readouterr() | ||
|
|
||
| print(out) |
Please check again? |
| try: | ||
| datasets.delete_dataset(project_id, cloud_region, dataset_id) | ||
| except HttpError as err: | ||
| # The API returns 403 when the dataset doesn't exist. |
There was a problem hiding this comment.
Can we expand this comment (similar to other PR)?
| project_id, cloud_region, dataset_id, fhir_store_id | ||
| ) | ||
| except HttpError as err: | ||
| # The API returns 403 when the FHIR store doesn't exist. |
Originally from #3300 but split up here