[issue-325] - Add tests to verify that HealthCheckResponse has a name when built as recommended on the producer side#326
Conversation
xstefank
left a comment
xstefank
left a comment
There was a problem hiding this comment.
Thanks @fabiobrz for your PR. But I need to request some changes. I don't think generating the name should be our solution to null names. I don't see the point of having random name since it won't (probably) give value to user when the health checks is first invoked. So then when then need to restart the app to add the name, I think it would be better to throw and exception (possibly on startup when we detect null name) to prevent app from running.
Also in every case, this will require adjustment in the specification text. I'm willing to hop on a quick call if you want to discuss this case in more detail. (/cc @pgunapal I would like to hear you opinion too).
Yeah, you might be right in here, thanks, fixing.
Correct, the spec text should be adjusted. I am glad to learn about this, so feel free to get in touch, and thanks for your review! |
fabiobrz
changed the title
|
I agree with @xstefank, It wouldn't make sense to randomly generate a name for a Health check with no-name. +1 on updating the specification, with this detail. |
fabiobrz
changed the title
fabiobrz
force-pushed
the
issue-325
branch
2 times, most recently
from
3c65f11 to
ed14991
Compare
|
Hi @xstefank, @pgunapal - I am back in order to update this PR and the related issue for resolution. The only thing we can do here, as agreed the last time we discussed this, is to add tests to the TCK - which is what I've done - for it to be aligned with the related spec requirements. The spec codebase itself cannot enforce the requirement since it clearly states that application level code is expected to use static methods to build the response and spec implementors are required to provide the In order to verify that the new TCK tests are executed, I've built a snapshot from the PR branch, then I let the WildFly integration test suite run them, and they passed, see a GitHub action execution which is based on a workflow that I've created for this: https://github.com/fabiobrz/wildfly/actions/runs/15956712879/job/45003958341 ✅ Specifically, we can see from the server logs generated by the above linked job execution, that the I think this is ready to go in. Could you please review the changes again? |
| @@ -0,0 +1,65 @@ | |||
| /* | |||
| * Copyright (c) 2021 Contributors to the Eclipse Foundation | |||
There was a problem hiding this comment.
we use a slightly different format for the years - https://github.com/microprofile/microprofile-health/blob/main/api/src/main/java/org/eclipse/microprofile/health/Liveness.java#L2
…HealthCheckResponse name is null or empty string
…st verify that a consistent name is provided when constructing a HealthCheckResponse instance, and fail accordingly if it is not
| It's required that a response defines a name. | ||
| It's required that a response defines a name, i.e. implementations must verify that a consistent name is provided when | ||
| constructing a `HealthCheckResponse` instance, and fail accordingly if it is not. | ||
| Specific tests to verify this behavior are executed when running the TCKs. |
There was a problem hiding this comment.
Now that I'm reading this @fabiobrz, don't you think it would be better to fail the deployment when such scenario is detected? So rather than 503, test here for DeploymentException. Also please point me to where you have an implementation that passes these new tests ;)
There was a problem hiding this comment.
Hi @xstefank. I am not sure about requiring DeploymentException to be thrown.
That would be a major change in the spec, for instance the SmallRye implementation is doing something different now, by throwing an IllegalArgumentException here: https://github.com/smallrye/smallrye-health/blob/main/implementation/src/main/java/io/smallrye/health/ResponseBuilder.java#L59
Failing to deploy would be something that the implementation might not know about (e.g.: when delegated to a component, like in the SMallRye case) and rather it should happen at the application server level (integration).
Or am I missing something?
Regarding your question about having an implementation that passes these new tests, I added the link in my last comment above, but anyway here it is: https://github.com/fabiobrz/wildfly/actions/runs/15956712879/job/45003958341 - but maybe too here I am not getting what you're asking for?
There was a problem hiding this comment.
Now that I'm reading this @fabiobrz, don't you think it would be better to fail the deployment when such scenario is detected? So rather than 503, test here for DeploymentException. Also please point me to where you have an implementation that passes these new tests ;)
Additionally, the new tests are checking something which happens when hitting the health endpoints, rather than when deploying.
And they only verify that a consistent name is provided when building responses via the static methods, rather than checking annotated health checks.
There was a problem hiding this comment.
True, I forgot we don't have such test cases here as of now, but you can see an example here - https://github.com/microprofile/microprofile-lra/blob/main/tck/src/main/java/org/eclipse/microprofile/lra/tck/TckInvalidSignaturesTests.java.
It doesn't make sense to wait for the first health call when we can detect something right away.
3 participants
SSIA.
Fixes #325
TCKs are running fine, see last comment