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.

Sign up for GitHub

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

WFLY-19229 The OpenAPI BasicRuntimeIT is ineffective and passes even … #894

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

WFLY-19229 The OpenAPI BasicRuntimeIT is ineffective and passes even … #894

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions ...le-openapi/src/test/java/org/wildfly/quickstarts/microprofile/openapi/BasicRuntimeIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,17 @@
import java.time.Duration;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

/**
* The very basic runtime integration testing.
* The very basic runtime integration test to verify that /openapi context is registered by the OpenAPI subsystem.
*
* @author emartins
* @author Radoslav Husar
*/
public class BasicRuntimeIT {

private static final String DEFAULT_SERVER_HOST = "http://localhost:8080/microprofile-openapi";
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@rhusar rhusar Jun 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

I will give you a better look and opinion tomorrow, just need to review a pair of PR first in the morning

Copy link
Member Author

Choose a reason for hiding this comment

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

@emmartins Sure, we can do that too. I will close and reopen with a new test.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

@emmartins No update?

private static final String DEFAULT_SERVER_HOST = "http://localhost:8080";

@Test
public void testHTTPEndpointIsAvailable() throws IOException, InterruptedException, URISyntaxException {
Expand All @@ -45,7 +48,9 @@ public void testHTTPEndpointIsAvailable() throws IOException, InterruptedExcepti
serverHost = DEFAULT_SERVER_HOST;
}
final HttpRequest request = HttpRequest.newBuilder()
.uri(new URI(serverHost))
// Check the /openapi context being available (rather than checking dummy /microprofile-openapi response)
// This test would thus correctly fail (404) if run against a server without an OpenAPI subsystem
.uri(new URI(serverHost + "/openapi"))
.GET()
.build();
final HttpClient client = HttpClient.newBuilder()
Expand All @@ -54,5 +59,9 @@ public void testHTTPEndpointIsAvailable() throws IOException, InterruptedExcepti
.build();
final HttpResponse<String> response = client.send(request, HttpResponse.BodyHandlers.ofString());
assertEquals(200, response.statusCode());

// First line are just dashes, so lets check the second line with OpenAPI version key for the prefix
String[] bodyLines = response.body().split("\n");
assertTrue(bodyLines[1].startsWith("openapi:"));
}
}
Loading