Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix intermittent error with CDS test #753

Merged
merged 2 commits into from
Oct 31, 2024

Conversation

XcrigX
Copy link
Contributor

@XcrigX XcrigX commented Oct 29, 2024

I was getting an error running the CdsHooksServletIT.testCdsHooks() test on Windows. The test is blocking while it waits for the CDS Services to be "ready" by looking at the response from /cds-services. I was seeing an empty response from it initially like:

{
  "services": []
}

This empty response is 22 characters (on Windows), which is the minimum size that hasCdsServices() was looking for. So the test was continuing to run thinking the CDS services were "ready", but they were not.

This led to:

2024-10-29T12:58:34.199-05:00  INFO 19928 --- [           main] c.u.h.f.c.svc.CdsServiceRegistryImpl     : Unregistered active service opioidcds-10-order-sign
2024-10-29T12:58:34.436-05:00  INFO 19928 --- [o-auto-1-exec-2] fhirtest.access                          : Path[/fhir] Source[] Operation[transaction  ] UA[HAPI-FHIR/7.4.0 (FHIR Client; FHIR 4.0.1/R4; apache)] Params[] ResponseEncoding[JSON] Operation[transaction  ] UA[HAPI-FHIR/7.4.0 (FHIR Client; FHIR 4.0.1/R4; apache)] Params[] ResponseEncoding[JSON]
2024-10-29T12:58:34.542-05:00  INFO 19928 --- [o-auto-1-exec-4] c.u.f.j.s.cdshooks.CdsHooksServlet       : /cds-services
2024-10-29T12:58:34.546-05:00  INFO 19928 --- [o-auto-1-exec-6] c.u.f.j.s.cdshooks.CdsHooksServlet       : /cds-services/hello-world
2024-10-29T12:58:34.549-05:00  INFO 19928 --- [o-auto-1-exec-6] c.u.f.j.s.cdshooks.CdsHooksServlet       : {  "hookInstance": "12345",  "hook": "patient-view",  "context": {    "userId": "Practitioner/example",    "patientId": "Patient/example-hello-world"  },  "prefetch": {    "item1": {      "resourceType": "Patient",      "id": "example-hello-world",      "gender": "male",      "birthDate": "2000-01-01"    }  }}
2024-10-29T12:58:34.549-05:00  INFO 19928 --- [o-auto-1-exec-6] c.u.f.j.s.cdshooks.CdsHooksServlet       : cds-hooks hook instance: 12345
2024-10-29T12:58:34.549-05:00  INFO 19928 --- [o-auto-1-exec-6] c.u.f.j.s.cdshooks.CdsHooksServlet       : cds-hooks local server address: null
2024-10-29T12:58:34.549-05:00  INFO 19928 --- [o-auto-1-exec-6] c.u.f.j.s.cdshooks.CdsHooksServlet       : cds-hooks fhir server address: null
2024-10-29T12:58:34.550-05:00  INFO 19928 --- [o-auto-1-exec-6] c.u.f.j.s.cdshooks.CdsHooksServlet       : cds-hooks cql_logging_enabled: false
2024-10-29T12:58:34.553-05:00 ERROR 19928 --- [o-auto-1-exec-6] c.u.f.j.s.cdshooks.CdsHooksServlet       : ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException: HAPI-2391: No service with id hello-world is registered on this server

com.google.gson.JsonSyntaxException: Expected a com.google.gson.JsonObject but was com.google.gson.JsonPrimitive

	at com.google.gson.internal.bind.TypeAdapters$33$1.read(TypeAdapters.java:869)
	at com.google.gson.Gson.fromJson(Gson.java:963)
	at com.google.gson.Gson.fromJson(Gson.java:928)
	at com.google.gson.Gson.fromJson(Gson.java:877)
	at com.google.gson.Gson.fromJson(Gson.java:848)
	at ca.uhn.fhir.jpa.starter.CdsHooksServletIT.testCdsHooks(CdsHooksServletIT.java:139)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

You can see just before the exception it is saying: "HAPI-2391: No service with id hello-world is registered on this server"
Changing the test response size to >22 causes it to wait for a non-empty response from /cds-services and the test then passes.

NOTE: This original test passed for me on a linux VM. It obviously runs here in github okay. But it was failing consistently on my Windows machine. Perhaps it's because on Linux the empty response is shorter than 22 chars due to line endings being different sizes?

Copy link
Contributor

@dotasek dotasek left a comment

Choose a reason for hiding this comment

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

@XcrigX out of curiosity, I put this through the debugger. The test passes, locally, and on Mac, but I'd prefer that it looks for something more explicit than 'length' > 25.

Your fix is no worse than what's there, but technically, a valid response could be:

{
   "errorMessage": "CDS service doesn't exist"
}

Mind you, this is in a test, not production code.

I tried pulling a String out of the response to investigate, but almost always run into closed socket exceptions, and other weird stream related errors when trying this, and the test fails due to those exceptions.

@XcrigX
Copy link
Contributor Author

XcrigX commented Oct 31, 2024

Hi @dotasek - I don't disagree with you that the test could be designed better, I just know that it was failing on Windows (for me anyway), so I updated it to pass.
My theory is that linux/mac use a 1-character line ending, so testing for >22 worked to indicate a non-empty response.
Windows uses a 2-character line ending so a non-empty response is slightly longer.

@XcrigX
Copy link
Contributor Author

XcrigX commented Oct 31, 2024

.. and to your point, if you try to actually look at the string it consumes the steam which then causes the test to fail. I didn't dig in enough to determine how to get the string out and not then fail the test later - so that it could test for specific contents in the string rather than just the length.

@dotasek
Copy link
Contributor

dotasek commented Oct 31, 2024

Yeah, it sounds like we both ran into the same issues. I'm just being nitpicky here. If this fixes things for Windows, and doesn't impact other systems, this is fine.

Maybe a comment in the code would be appropriate?

@XcrigX
Copy link
Contributor Author

XcrigX commented Oct 31, 2024

I'll add some comments there and push

@XcrigX
Copy link
Contributor Author

XcrigX commented Oct 31, 2024

comments added

@dotasek dotasek merged commit 02e4de7 into hapifhir:master Oct 31, 2024
3 checks passed
@XcrigX XcrigX deleted the fix-cds-test-race branch October 31, 2024 20:51
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.

2 participants