WFLY-19229 The OpenAPI BasicRuntimeIT is ineffective and passes even … #894
Conversation
|
The whole BasicRuntimeIT is a mess as well... it's being overriden (http://localhost:8080/) but in the templates this forms URIs without the context path - and since it only chcecks for 200, it would pass anwyay... |
Will open a Jira when Jira is back online. |
|
FYI the server host is overriden on server provisioning/bootable jar
environment, and on those the app is deployed at root context, i.e. thus no
context path
…On Thu, 11 Apr 2024 at 16:50, Radoslav Husar ***@***.***> wrote:
The whole BasicRuntimeIT is a mess as well... it's being overriden (
http://localhost:8080/) but in the templates this forms URIs without the
context path - and since it only chcecks for 200, it would pass anwyay...
Will open a Jira when Jira is back online.
—
Reply to this email directly, view it on GitHub
<#894 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEDY6CUDHIBTLR7MA57SVLY42WLXAVCNFSM6AAAAABGCM7BKOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJQGAYDONZUHA>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
|
Wrt just checking 200, some QS have opted also to test the content, and for
sure I think that’s a good decision ;-)
…On Thu, 11 Apr 2024 at 16:52, Eduardo Martins ***@***.***> wrote:
FYI the server host is overriden on server provisioning/bootable jar
environment, and on those the app is deployed at root context, i.e. thus no
context path
On Thu, 11 Apr 2024 at 16:50, Radoslav Husar ***@***.***>
wrote:
> The whole BasicRuntimeIT is a mess as well... it's being overriden (
> http://localhost:8080/) but in the templates this forms URIs without the
> context path - and since it only chcecks for 200, it would pass anwyay...
>
> Will open a Jira when Jira is back online.
>
> —
> Reply to this email directly, view it on GitHub
> <#894 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAEDY6CUDHIBTLR7MA57SVLY42WLXAVCNFSM6AAAAABGCM7BKOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJQGAYDONZUHA>
> .
> You are receiving this because your review was requested.Message ID:
> ***@***.***>
>
|
Gotcha, good point. So only affecting this specific scenario. |
| */ | ||
| public class BasicRuntimeIT { | ||
|
|
||
| private static final String DEFAULT_SERVER_HOST = "http://localhost:8080/microprofile-openapi"; |
There was a problem hiding this comment.
I will need to double "later" check but I believe this fails the standard distribution testing, where app is not app web root and testing command doesn't override -Dserver.host
that is (for now) the default environment, thus all basic tests have the deployment path in the default const.
There was a problem hiding this comment.
OK please go ahead and check (rebased now only just to see what tests think now)
But the /openapi is always at the same location...
There was a problem hiding this comment.
You mean that is something not from the deployment itself?
Ok I see you change the basic test logic, honestly the idea with those basics were to be the same for all QS and just to check the deployment is running (by checking it's default webpage is there and at most it's content is expected), so maybe keep the old test (yet checking the content) and add this a additional functional one?
There was a problem hiding this comment.
I will give you a better look and opinion tomorrow, just need to review a pair of PR first in the morning
There was a problem hiding this comment.
@emmartins Sure, we can do that too. I will close and reopen with a new test.
There was a problem hiding this comment.
I will give you a better look and opinion tomorrow, just need to review a pair of PR first in the morning
OK i ll wait till tomorrow, then close and reopen with a new test instead.
There was a problem hiding this comment.
I will give you a better look and opinion tomorrow, just need to review a pair of PR first in the morning
@emmartins btw did you make a final decision on this yet?
…on non-OpenAPI compliant runtime Fix BasicRuntimeIT does not add /context-path when SERVER_HOST/server.host is overriden.
|
@rhusar: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
hi @rhusar, doing project housekeeping and found this one abandoned. I did check it out and I think what we should do here is:
|
|
@emmartins Sounds good to me - I ll update this tomorrow then. |
|
@emmartins OK then, opening a PR where this is split the code into a separate test case. Superseded by #1077 |
2 participants
…on non-OpenAPI compliant runtime
Fixes
https://issues.redhat.com/browse/WFLY-19229