Copilot commented on code in PR #718:
URL:
https://github.com/apache/hugegraph-toolchain/pull/718#discussion_r2964635787
##########
hugegraph-client/src/test/java/org/apache/hugegraph/unit/HugeClientBuilderTest.java:
##########
@@ -0,0 +1,51 @@
+package org.apache.hugegraph.unit;
+
+import org.apache.hugegraph.driver.HugeClient;
+import org.apache.hugegraph.driver.HugeClientBuilder;
+import org.apache.hugegraph.rest.ClientException;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class HugeClientBuilderTest {
+
+ @Test
+ public void testBuilderWithSkipRequiredChecks() {
+ // Should not throw IllegalArgumentException when skipRequiredChecks
is true and graph is null
+ HugeClientBuilder builder = new
HugeClientBuilder("http://127.0.0.1:8080";, "DEFAULT", null, true);
+ try {
+ builder.build();
+ } catch (IllegalArgumentException e) {
+ Assert.fail("Should not throw IllegalArgumentException when
skipRequiredChecks is true, but got: " + e.getMessage());
+ } catch (Exception e) {
+ // Expected since there is probably no server running at
localhost:8080
+ // The fact we reach here means the bypass of graph/url check was
successful.
+ }
Review Comment:
These tests call `builder.build()`, which constructs `HugeClient` and
triggers server API version checks over HTTP. If no server is running, this can
block up to the default 20s timeout per test, making the suite slow/flaky.
Consider avoiding the network path (e.g., assert only that constructor/build
validation doesn’t throw) or explicitly set very small connect/read timeouts
before `build()` so failures return quickly.
##########
hugegraph-client/src/test/java/org/apache/hugegraph/unit/HugeClientBuilderTest.java:
##########
@@ -0,0 +1,51 @@
+package org.apache.hugegraph.unit;
+
+import org.apache.hugegraph.driver.HugeClient;
+import org.apache.hugegraph.driver.HugeClientBuilder;
+import org.apache.hugegraph.rest.ClientException;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class HugeClientBuilderTest {
+
+ @Test
+ public void testBuilderWithSkipRequiredChecks() {
+ // Should not throw IllegalArgumentException when skipRequiredChecks
is true and graph is null
+ HugeClientBuilder builder = new
HugeClientBuilder("http://127.0.0.1:8080";, "DEFAULT", null, true);
+ try {
+ builder.build();
+ } catch (IllegalArgumentException e) {
+ Assert.fail("Should not throw IllegalArgumentException when
skipRequiredChecks is true, but got: " + e.getMessage());
+ } catch (Exception e) {
+ // Expected since there is probably no server running at
localhost:8080
+ // The fact we reach here means the bypass of graph/url check was
successful.
+ }
+ }
+
+ @Test(expected = IllegalArgumentException.class)
+ public void testBuilderWithoutSkipRequiredChecks() {
+ // Should throw exception when skipRequiredChecks is false and graph
is null
+ HugeClientBuilder builder = new
HugeClientBuilder("http://127.0.0.1:8080";, "DEFAULT", null, false);
+ builder.build();
+ }
Review Comment:
`@Test(expected = IllegalArgumentException.class)` here is satisfied by the
exception thrown in the constructor (because `graph` is null) before `build()`
is reached, so it doesn’t actually validate the new conditional checks inside
`build()`. To cover the `build()` path, construct with a non-null graph, then
set `configGraph(null)` (and/or `configUrl(null)`) before calling `build()` and
asserting the expected behavior with/without `skipRequiredChecks`.
##########
hugegraph-client/src/main/java/org/apache/hugegraph/structure/auth/User.java:
##########
@@ -155,9 +155,11 @@ public static class UserRole {
// Mapping of: graphSpace -> graph -> permission -> resourceType ->
resources
@JsonProperty("roles")
- private Map<String, Map<String, Map<HugePermission, Map<String,
List<HugeResource>>>>> roles;
+ private Map<String, Map<String, Map<HugePermission, Map<String,
+ List<HugeResource>>>>> roles;
- public Map<String, Map<String, Map<HugePermission, Map<String,
List<HugeResource>>>>> roles() {
+ public Map<String, Map<String, Map<HugePermission, Map<String,
+ List<HugeResource>>>>> roles() {
Review Comment:
These wrapped generic type lines include trailing whitespace before the
newline (after the comma). This is easy to miss in reviews and can create noisy
diffs or fail whitespace-sensitive tooling; please remove the trailing spaces
while keeping the line wrap.
##########
hugegraph-client/src/test/java/org/apache/hugegraph/unit/HugeClientBuilderTest.java:
##########
@@ -0,0 +1,51 @@
+package org.apache.hugegraph.unit;
+
+import org.apache.hugegraph.driver.HugeClient;
+import org.apache.hugegraph.driver.HugeClientBuilder;
+import org.apache.hugegraph.rest.ClientException;
Review Comment:
Unused import `ClientException` should be removed to keep the test file
clean and avoid failing builds in environments that treat unused imports as
errors (e.g., stricter compiler/lint settings).
```suggestion
```
##########
hugegraph-client/src/main/java/org/apache/hugegraph/driver/HugeClient.java:
##########
@@ -122,6 +122,11 @@ public static HugeClientBuilder builder(String url, String
graphSpace, String gr
return new HugeClientBuilder(url, graphSpace, graph);
}
+ public static HugeClientBuilder builder(String url, String graphSpace,
String graph,
+ boolean skipRequiredChecks) {
+ return new HugeClientBuilder(url, graphSpace, graph,
skipRequiredChecks);
+ }
Review Comment:
PR description indicates this does not affect the public API, but this adds
a new public overload `HugeClient.builder(..., boolean skipRequiredChecks)`
(and a new public `HugeClientBuilder` constructor signature). The PR
metadata/checkboxes should be updated to reflect that this is a public API
change.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
