healthcare API: update all HL7v2 v1beta1 samples to v1 #3388

noerog · 2020-04-13T22:33:21Z

Originally from #3300 but split up here

tmatsuo

I reviewed files other then hl7v2_stores_test.py.

I'm sending this review, but I found an issue on the approach we're taking. We'll need to make some more change to the datasets samples and tests.

tmatsuo · 2020-04-15T17:16:05Z

healthcare/api-client/v1/hl7v2/hl7v2_messages.py

+    api_version = 'v1'
+    service_name = 'healthcare'
+
+    return discovery.build(


This fits in one line.

tmatsuo · 2020-04-15T17:19:54Z

healthcare/api-client/v1/hl7v2/hl7v2_messages_test.py

+import sys
+import uuid
+
+from gcp_devrel.testing import eventually_consistent


Can you remove this line?

tmatsuo · 2020-04-15T17:21:48Z

healthcare/api-client/v1/hl7v2/hl7v2_messages_test.py

+
+
+@pytest.fixture(scope='module')
+def test_dataset():


For these fixtures, can you also add retries like we did in datasets? It will make the tests more robust.

tmatsuo · 2020-04-15T17:22:57Z

healthcare/api-client/v1/hl7v2/hl7v2_messages_test.py

+    assert 'Deleted HL7v2 message' in out
+
+
+def test_ingest_hl7v2_message(test_dataset, test_hl7v2_store, capsys):


I thought this test used to be eventually consistent. Has the situation changed?

I thought the list_messages part was made eventually consistent because you thought the list_messages method was flaky, but it wasn't flaky--it was broken for a short amount of time because the response body from the method changed.

I don't see much reason to keep it eventually consistent but please let me know if you do.

Sounds good :)

FYI, to verify whether the test is flaky or not, you can utilize flaky. You can add a decorator like the following temporarily:

@pytest.mark.flaky(max_runs=100, min_passes=100)

Then you can verify this locally:

$ python -m nox -s py-3.6 -- hl7v2_messages_test.py

tmatsuo · 2020-04-15T17:23:22Z

healthcare/api-client/v1/hl7v2/hl7v2_messages_test.py

+        hl7v2_store_id,
+        hl7v2_message_file)
+
+        hl7v2_messages_list = hl7v2_messages.list_hl7v2_messages(


I think the indent is off here.

tmatsuo · 2020-04-15T17:27:39Z

healthcare/api-client/v1/hl7v2/hl7v2_messages_test.py

+        hl7v2_store_id,
+        hl7v2_message_file)
+
+        hl7v2_messages_list = hl7v2_messages.list_hl7v2_messages(


Indent is off here

tmatsuo · 2020-04-15T17:28:08Z

healthcare/api-client/v1/hl7v2/hl7v2_messages_test.py

+
+        assert len(hl7v2_messages_list) > 0
+        hl7v2_message_name = hl7v2_messages_list[0].get('name')
+        nonlocal hl7v2_message_id


Remove nonlocal line

tmatsuo · 2020-04-15T17:30:44Z

healthcare/api-client/v1/hl7v2/hl7v2_messages_test.py

+        label_key,
+        label_value)
+
+    hl7v2_messages.delete_hl7v2_message(


The same concern about cleaning up.

If the API doesn't allow recursive deletion, one idea is to list the messages and delete them in the clean up code.

Does it matter if we're deleting the HL7v2 store anyway? Deleting the store deletes any messages inside of it.

If the API behaves like that, it's fine.

However, note that it will be not OK once the API changes the behavior (not allowing recursive deletion).

tmatsuo · 2020-04-15T17:31:08Z

healthcare/api-client/v1/hl7v2/hl7v2_stores.py

+    api_version = 'v1'
+    service_name = 'healthcare'
+
+    return discovery.build(


It fits in one line.

tmatsuo · 2020-04-15T17:36:19Z

healthcare/api-client/v1/hl7v2/requirements-test.txt

@@ -0,0 +1,3 @@
+pytest==5.3.2
+gcp-devrel-py-tools==0.0.16


Remove these two lines

You highlited pytest and gcp-devrel-py-tools. Did you mean to highlight gcp-devrel-py-tools and google-cloud-core? We need pytest don't we?

Ah sorry, yes, remove gcp-devrel-py-tools and google-cloud-core.

* 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 <[email protected]>

tmatsuo

Overall, it looks good. Please address my comments.

tmatsuo · 2020-04-16T23:10:58Z

healthcare/api-client/v1/hl7v2/hl7v2_messages_test.py

+    assert 'Deleted HL7v2 message' in out
+
+
+def test_ingest_hl7v2_message(test_dataset, test_hl7v2_store, capsys):


Sounds good :)

FYI, to verify whether the test is flaky or not, you can utilize flaky. You can add a decorator like the following temporarily:

@pytest.mark.flaky(max_runs=100, min_passes=100)

Then you can verify this locally:

$ python -m nox -s py-3.6 -- hl7v2_messages_test.py

tmatsuo · 2020-04-16T23:13:41Z

healthcare/api-client/v1/hl7v2/hl7v2_messages_test.py

+        label_key,
+        label_value)
+
+    hl7v2_messages.delete_hl7v2_message(


If the API behaves like that, it's fine.

However, note that it will be not OK once the API changes the behavior (not allowing recursive deletion).

tmatsuo · 2020-04-16T23:15:35Z

healthcare/api-client/v1/hl7v2/hl7v2_stores_test.py

+        retry_on_exception=retry_if_server_exception)
+    def clean_up():
+        try:
+            hl7v2_stores.delete_hl7v2_store_id(


s/delete_hl7v2_store_id/delete_hl7v2_store/ ?

tmatsuo · 2020-04-16T23:16:28Z

healthcare/api-client/v1/hl7v2/hl7v2_stores_test.py

+
+
+def test_get_set_hl7v2_store_iam_policy(test_dataset, capsys):
+    hl7v2_stores.create_hl7v2_store(


Can you move creation/deletion to the fixture?

tmatsuo · 2020-04-16T23:16:45Z

healthcare/api-client/v1/hl7v2/hl7v2_stores_test.py

+
+
+def test_patch_hl7v2_store(test_dataset, capsys):
+    hl7v2_stores.create_hl7v2_store(


Can you use the fixture?

tmatsuo

Looks great, thanks!

healthcare/api-client/v1/hl7v2/hl7v2_messages.py

healthcare/api-client/v1/hl7v2/hl7v2_messages_test.py

kurtisvg · 2020-04-17T16:59:44Z

healthcare/api-client/v1/hl7v2/hl7v2_messages_test.py

+sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'datasets'))  # noqa
+import datasets


Does this not work?

Suggested change

sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'datasets')) # noqa

import datasets

from ... import datasets

healthcare/api-client/v1/hl7v2/hl7v2_stores.py

healthcare/api-client/v1/hl7v2/hl7v2_stores_test.py

kurtisvg · 2020-04-17T17:03:28Z

healthcare/api-client/v1/hl7v2/hl7v2_stores_test.py

+sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'datasets'))  # noqa
+import datasets


Suggested change

sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'datasets')) # noqa

import datasets

from ... import datasets

kurtisvg · 2020-04-17T17:05:58Z

healthcare/api-client/v1/hl7v2/hl7v2_stores_test.py

+    assert 'Created HL7v2 store' in out
+    assert 'Name' in out
+    assert 'hl7V2Stores' in out
+    assert 'Deleted HL7v2 store' in out


These asserts should probably happen after each function is run to avoid one function from accidentally testing positive for all 3

kurtisvg · 2020-04-17T17:06:47Z

healthcare/api-client/v1/hl7v2/hl7v2_stores.py

+    request = client.projects().locations().datasets().hl7V2Stores(
+    ).getIamPolicy(resource=hl7v2_store_name)


Suggested change

request = client.projects().locations().datasets().hl7V2Stores(

).getIamPolicy(resource=hl7v2_store_name)

request = client.projects().locations().datasets().hl7V2Stores().getIamPolicy(

resource=hl7v2_store_name)

kurtisvg · 2020-04-17T17:16:19Z

Also if you have the noxfile-template copied locally, you can use it to run black on your code:

nox -S blacken

To auto format the code.

googlebot · 2020-04-17T17:29:17Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to determine that you authored the commits in this PR. Maybe you used a different email address in the git commits than was used to sign the CLA? If someone else authored these commits, then please add them to this pull request and have them confirm that they're okay with them being contributed to Google. If there are co-authors, make sure they're formatted properly.

In order to pass this check, please resolve this problem and then comment@googlebot I fixed it... If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2020-04-17T18:08:39Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Co-Authored-By: Kurtis Van Gent <[email protected]>

healthcare/api-client/v1/hl7v2/hl7v2_messages.py

tmatsuo · 2020-04-19T00:47:04Z

@noerog

It seems like there is a flake:
https://source.cloud.google.com/results/invocations/b1295e23-bc1e-49e7-9bd0-a10b1eb975d2/targets/github%2Fpython-docs-samples%2Fhealthcare%2Fapi-client%2Fv1%2Fhl7v2/tests

noerog requested a review from a team as a code owner April 13, 2020 22:33

blunderbuss-gcf bot assigned crwilcox Apr 13, 2020

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 13, 2020

noerog mentioned this pull request Apr 13, 2020

healthcare API: update v1beta1 datasets samples to v1 #3300

Merged

noerog assigned tmatsuo and unassigned crwilcox Apr 13, 2020

tmatsuo reviewed Apr 15, 2020

View reviewed changes

tmatsuo suggested changes Apr 16, 2020

View reviewed changes

tmatsuo approved these changes Apr 17, 2020

View reviewed changes

kurtisvg suggested changes Apr 17, 2020

View reviewed changes

noerog added 4 commits April 17, 2020 13:22

healthcare API: update all HL7v2 v1beta1 samples to v1

67a7238

address review comments

a638c3d

fix some test things I missed

2e9be4e

address review comments, fix failing test

03a9694

noerog requested review from bradmiro, busunkim96, crwilcox, davidcavazos, djmailhot, gguuss, grant, leahecole, nnegrey, shollyman, sirtorry and telpirion as code owners April 17, 2020 17:28

googlebot removed the cla: yes This human has signed the Contributor License Agreement. label Apr 17, 2020

googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Apr 17, 2020

noerog removed request for crwilcox, djmailhot, grant, shollyman, davidcavazos, gguuss, leahecole, telpirion, nnegrey, sirtorry, busunkim96 and bradmiro April 17, 2020 17:36

googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Apr 17, 2020

noerog and others added 3 commits April 17, 2020 14:10

Update healthcare/api-client/v1/hl7v2/hl7v2_messages.py

208585a

Co-Authored-By: Kurtis Van Gent <[email protected]>

Update healthcare/api-client/v1/hl7v2/hl7v2_stores.py

8ced227

Co-Authored-By: Kurtis Van Gent <[email protected]>

address review comments, run nox blacken

45562f4

kurtisvg approved these changes Apr 17, 2020

View reviewed changes

healthcare/api-client/v1/hl7v2/hl7v2_messages.py Show resolved Hide resolved

noerog merged commit d6bf6c0 into GoogleCloudPlatform:master Apr 17, 2020

noerog deleted the chc-api-hl7v2 branch April 17, 2020 20:26

		assert 'Deleted HL7v2 message' in out


		def test_ingest_hl7v2_message(test_dataset, test_hl7v2_store, capsys):



		def test_get_set_hl7v2_store_iam_policy(test_dataset, capsys):
		hl7v2_stores.create_hl7v2_store(



		def test_patch_hl7v2_store(test_dataset, capsys):
		hl7v2_stores.create_hl7v2_store(

		sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'datasets')) # noqa
		import datasets

	sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'datasets')) # noqa
	import datasets
	from ... import datasets

		request = client.projects().locations().datasets().hl7V2Stores(
		).getIamPolicy(resource=hl7v2_store_name)

		@@ -0,0 +1,3 @@
		pytest==5.3.2
		gcp-devrel-py-tools==0.0.16

healthcare API: update all HL7v2 v1beta1 samples to v1 #3388

healthcare API: update all HL7v2 v1beta1 samples to v1 #3388

Uh oh!

Conversation

noerog commented Apr 13, 2020

Uh oh!

tmatsuo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmatsuo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmatsuo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!