Copilot commented on code in PR #3720:
URL: https://github.com/apache/polaris/pull/3720#discussion_r2785784539
##########
runtime/service/src/test/java/org/apache/polaris/service/tracing/RequestIdHeaderTest.java:
##########
@@ -116,4 +111,13 @@ private String sendRequest(Map<String, String> headers) {
return
response.getHeaders().get(REQUEST_ID_HEADER).getFirst().toString();
}
}
+
+ private static int getQuarkusTestPort() {
+ return ConfigProvider.getConfig()
+ .getOptionalValue("quarkus.http.test-port", Integer.class)
+ .orElseThrow(
+ () ->
+ new IllegalStateException(
+ "Quarkus property not set correctly:
quarkus.http.test-port"));
+ }
Review Comment:
`getQuarkusTestPort()` duplicates the same logic now present in
`org.apache.polaris.service.it.ServerManager`. Consider moving this helper into
a shared test utility (e.g., in testFixtures) and reusing it to avoid
divergence in error handling / property keys.
##########
runtime/service/src/testFixtures/java/org/apache/polaris/service/it/ServerManager.java:
##########
@@ -52,8 +52,11 @@ public void close() {
}
private static Integer getQuarkusTestPort() {
- return Objects.requireNonNull(
- Integer.getInteger("quarkus.http.test-port"),
- "System property not set correctly: quarkus.http.test-port");
+ return ConfigProvider.getConfig()
+ .getOptionalValue("quarkus.http.test-port", Integer.class)
+ .orElseThrow(
+ () ->
+ new IllegalStateException(
+ "Quarkus property not set correctly:
quarkus.http.test-port"));
Review Comment:
`getQuarkusTestPort()` returns a boxed `Integer` but callers always use it
as a primitive port number. Returning `int` would avoid unnecessary boxing and
better communicate that this value is required.
##########
runtime/service/src/test/java/org/apache/polaris/service/tracing/RequestIdHeaderTest.java:
##########
@@ -57,12 +57,7 @@ public Map<String, String> getConfigOverrides() {
private static final String CLIENT_ID = "client1";
private static final String CLIENT_SECRET = "secret1";
- private static final URI baseUri =
- URI.create(
- "http://localhost:";
- + Objects.requireNonNull(
- Integer.getInteger("quarkus.http.test-port"),
- "System property not set correctly:
quarkus.http.test-port"));
+ private static final URI baseUri = URI.create("http://localhost:"; +
getQuarkusTestPort());
private Response request(Map<String, String> headers) {
Review Comment:
`baseUri` is initialized in a static field using `getQuarkusTestPort()`. In
JUnit, static initializers run when the test class is loaded, which can happen
before the Quarkus test extension has finished bootstrapping and before Quarkus
has finalized/overridden the effective test port (especially when
`quarkus.http.test-port=0` is used for random port selection). This can lock in
an incorrect port (e.g., 0) for the lifetime of the test class. Prefer
resolving the base URI after Quarkus has started (e.g., via `@TestHTTPResource`
injection or by computing the URI lazily inside the request path).
##########
runtime/service/src/testFixtures/java/org/apache/polaris/service/it/ServerManager.java:
##########
@@ -19,11 +19,11 @@
package org.apache.polaris.service.it;
import java.net.URI;
-import java.util.Objects;
import org.apache.polaris.service.it.env.ClientCredentials;
import org.apache.polaris.service.it.env.ClientPrincipal;
import org.apache.polaris.service.it.env.Server;
import org.apache.polaris.service.it.ext.PolarisServerManager;
Review Comment:
Import order is inconsistent with other files in this module (e.g.,
`RequestIdHeaderTest` keeps `org.apache.*` imports before third-party
`org.eclipse.*`). Consider reordering imports so `org.apache.polaris.*` stays
grouped/sorted before `org.eclipse.microprofile.*` to match the prevailing
style and avoid formatter/linter churn.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
