Repository navigation
test: add coverage across struct, commons, server, pd, store and cluster - #3309
Conversation
Add struct-local tests for BytesBuffer varint rejection, unsigned limits, length-prefixed bytes, string endings, edge and index id encoding, partition and OLAP id parsing, tagged collections and buffer growth, plus EdgeId text form, parse errors and binary id generation.
Add common tests for typed option parsing and checkers, HugeConfig URL normalization and input validation, and HostNameVerifier negative paths (SAN matching, wildcard scope, IP vs DNS, no CN fallback). Add an RPC test for self URL exclusion, invalid RpcOptions, provider and consumer config, and service cleanup on destroy. Register all in UnitTestSuite.
Add unit tests for RocksDBStdSessions lifecycle and error paths (unopened tables, rollback, reopen, lock release on force close and bad options, iterator double close, snapshot path), the RocksDB iterator pool, and HStore table partition routing. Register them in UnitTestSuite together with the existing ConsumersTest, MemoryAllocateTest and MemoryManageTest, which compiled but never ran.
Add PD core tests for KVOperation encoding and whitelist decoding, RaftKVStore write routing through raft (leader check, raft failure, state machine replay), PeerUtil config parsing and ZipUtils snapshot checksums and zip-slip handling. Register them, plus the existing RaftReflectionUtilTest, in PDCoreSuiteTest.
Add Store tests for Base58 encoding, edge owner keys, CRC64, both ZipUtils copies (round trip and zip-slip entries), SessionOperator writes, ranges, iterator release and snapshots, partition and shard group metadata, and HgBufferProxy flow control. Register them, plus the existing HgStoreStateMachineTest, FutureClosureTest, HgAssertTest and HgPairTest, in the matching suites.
Add SimpleClusterGraphTest, which creates schema, vertices and an edge through the server, reads them back by id and by adjacency through the HStore backend, and checks that invalid vertex requests and unknown ids are rejected. Register it in SimpleClusterSuiteTest.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3309 +/- ##
============================================
- Coverage 52.63% 51.33% -1.30%
- Complexity 2435 2497 +62
============================================
Files 770 759 -11
Lines 67694 66363 -1331
Branches 9172 8932 -240
============================================
- Hits 35631 34068 -1563
- Misses 28520 29050 +530
+ Partials 3543 3245 -298 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: Test-only change, and every new or newly registered class runs in CI at this head with the counts the description gives. One minor point on a mock that would turn a regression into an endless loop instead of a failed assertion. Evidence: Read the full diff at a46e822 against base 36bc1c6 (31 files, +3912/-6) and the production code under test. CI logs at this head show struct 129, commons 389 and 31, server UnitTestSuite 1031, PDCoreSuiteTest 149, store suites 14/19/27/90/100 and SimpleClusterSuiteTest 8, all without failures. The two red jobs are outside this diff: server_hbase stopped on a 504 while downloading jacococli.jar after its tests passed, and macos-15-intel failed MultiNodeShortestPathApiTest.testPost in ApiTestSuite on a query timeout.
Stop the mocked raft iterator after one entry in testFollowerReplayStopsOnStoreFailure, so a missing early return fails the never() check instead of looping forever. Unregister MemoryAllocateTest from UnitTestSuite: two of its tests force an unreleased 1 GiB heap or direct allocation, which is a stress test rather than a unit test for the default suite.
There was a problem hiding this comment.
🟡 Changes recommended
The advertised MemoryAllocateTest registration is absent, leaving a core verification claim inaccurate.
1 open finding
What changed in this PR
Expands executable regression coverage across HugeGraph modules for Java 17, serialization, RocksDB/JRaft, HStore, RPC, and cluster behavior.
Changes:
- Adds targeted unit tests across Struct, Commons, Server, PD, and Store.
- Registers new and previously dormant tests in module suites.
- Adds an end-to-end HStore cluster graph test.
| File | Description |
|---|---|
hugegraph-struct/.../BytesBufferTest.java |
Tests binary encoding and validation. |
hugegraph-struct/.../EdgeIdTest.java |
Tests edge ID parsing and encoding. |
hugegraph-store/.../ZipUtilsPathSafetyTest.java |
Tests archive safety and round trips. |
hugegraph-store/.../SessionOperatorTest.java |
Tests RocksDB session operations. |
hugegraph-store/.../RocksDbSuiteTest.java |
Registers RocksDB tests. |
hugegraph-store/.../CRC64Test.java |
Tests CRC64 behavior. |
hugegraph-store/.../RaftSuiteTest.java |
Registers Raft tests. |
hugegraph-store/.../PartitionMetaCodecTest.java |
Tests partition metadata conversion. |
hugegraph-store/.../CoreSuiteTest.java |
Registers metadata tests. |
hugegraph-store/.../KeyUtilTest.java |
Tests key ownership routing. |
hugegraph-store/.../CommonSuiteTest.java |
Registers common Store tests. |
hugegraph-store/.../Base58EncoderTest.java |
Tests Base58 encoding. |
hugegraph-store/.../HgBufferProxyTest.java |
Tests buffered streaming behavior. |
hugegraph-store/.../ClientSuiteTest.java |
Registers client tests. |
hugegraph-server/.../UnitTestSuite.java |
Expands server suite registration. |
hugegraph-server/.../RocksDBStdSessionsLifecycleTest.java |
Tests RocksDB lifecycle paths. |
hugegraph-server/.../HstoreTableRoutingTest.java |
Tests HStore routing. |
hugegraph-server/.../RocksDBIteratorPoolTest.java |
Tests iterator ownership and closure. |
hugegraph-pd/.../ZipUtilsTest.java |
Tests PD snapshot archives. |
hugegraph-pd/.../PeerUtilTest.java |
Tests Raft peer parsing. |
hugegraph-pd/.../KVOperationTest.java |
Tests Raft operation serialization. |
hugegraph-pd/.../RaftKVStoreTest.java |
Tests Raft-backed KV dispatch. |
hugegraph-pd/.../PDCoreSuiteTest.java |
Registers PD tests. |
hugegraph-commons/.../UnitTestSuite.java |
Registers RPC configuration tests. |
hugegraph-commons/.../RpcConfigTest.java |
Tests RPC configuration and cleanup. |
hugegraph-commons/.../UnitTestSuite.java |
Registers Commons tests. |
hugegraph-commons/.../HostNameVerifierTest.java |
Tests certificate hostname matching. |
hugegraph-commons/.../TypedOptionTest.java |
Tests typed option parsing. |
hugegraph-commons/.../HugeConfigValidationTest.java |
Tests configuration validation. |
hugegraph-cluster-test/.../SimpleClusterSuiteTest.java |
Registers cluster graph tests. |
hugegraph-cluster-test/.../SimpleClusterGraphTest.java |
Tests graph operations through HStore. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The 21 new and 7 newly registered test classes are all picked up by a CI test run and pass at this head, but SessionOperatorTest.tearDown() deadlocks on the CF write lock whenever a test fails while the test thread still holds the read lock from prepare(), so a failure in that class turns into a hung store job with no test named. Evidence: read the full diff 36bc1c6..03ffdf0 (31 files) and the production classes each test targets; checked every new class against its suite and the surefire includes in the hg-store-test, hg-pd-test, hugegraph-clustertest-test and server hugegraph-test poms (struct tests run through the default surefire includes, commons through -Dtest=UnitTestSuite); confirmed from the job logs of run 38136372195 that the suites ran with the new counts (commons 389 and 31, server UnitTestSuite 1027 in server_memory, PDCoreSuiteTest 149, CommonSuiteTest 14, ClientSuiteTest 90, RocksDbSuiteTest 19, RaftSuiteTest 27, CoreSuiteTest 41, SimpleClusterSuiteTest 8, struct 129); reproduced the hang on Linux, JDK 17.0.20, with one assertion changed to fail and a jstack of the surefire fork. The red pd_store / store check at this head is ScanLifecycleTest.testReceivePreservesExternalInterrupt in ServerSuiteTest, which this PR does not touch; affected-module-tests is red only because it reports that job.
A test that fails between prepare() and commit() leaves the CF read lock held by the test thread, and RocksDBSession.close() then parks forever on the CF write lock. Release the thread's remaining read holds before closing, and add a Timeout rule so a stuck lock fails the test instead of hanging the store job.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
5 open findings
teardown()assumes all native resources were created successfully insetup(). Ifsetup()… · New\"lock\"is brittle: RocksDB error strings can vary by… · NewString#getBytes()/new String(byte[])), which… · Newzip.delete()can be flaky on some platforms/filesystems (notably Windows) where… · New
🧠 Review effort: Lite
Null-check each native resource before closing it, so a failure in setup() is reported as itself instead of a NullPointerException from teardown().
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
9 open findings
OptionSpace.register(...)is called unconditionally. If other tests (or re-runs in the same JVM)… · NewString.getBytes()/new String(bytes)use the platform default charset, which can make these… · NewFile#getAbsolutePath(), which is OS-dependent… · Newzip.delete()can be flaky on some platforms/filesystems (notably Windows) where…String#getBytes()/new String(byte[])), which…\"lock\"is brittle: RocksDB error strings can vary by…teardown()assumes all native resources were created successfully insetup(). Ifsetup()…remoteUrls) via reflection, which makes the test… · New
🧠 Review effort: Lite
Check the direct URL the SOFA consumer is created with instead of the private remoteUrls field, so the test follows what the client uses.
- encode lifecycle fixture keys and values as UTF-8 - assert native lock status without localized messages - normalize malicious ZIP entries and isolate archives - preserve containment and live-owner assertions
- merge the approved ASF server baseline - inherit current startup and required CI fixtures - preserve existing coverage portability fixes
- wait for the initial producer handoff before capacity stop - preserve native cursor and producer cleanup assertions - retain the existing capacity and edge-count expectations
- exercise sparse map growth and value removal - propagate writer and reader failures through futures - close owned executors after checking stored values
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Independent source cross-review of the two-file map-fixture delta, 14b1f41 to 4204567: 9/10. Joining all writer/reader futures makes asynchronous assertion failures observable, and the sparse-key case checks resizing, replacement and removal through public APIs. No new blocking issue was found in that delta. A separate P3 remains in the current PR's suite registration, noted inline; it predates this map commit. This is a bounded source assessment, not a new whole-PR acceptance or runtime pass. Current CI's HTTP-channel fixture failure is separate and its cause was not independently reproduced by these reviewers.
- wait for the asynchronous close future in the fixture - retain channel, request release and output assertions - bound the wait and report active or pending state
- retain the existing ConsumersTest suite entry - remove duplicate execution without losing test methods - keep consumer and Gremlin test implementations unchanged
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
TODO: we need reduce the log print in Tinkerpop-Test case to avoid CI failure
Purpose of the PR
Expand executable test coverage for the 1.8.0 release work in #3242. Previously, several existing tests compiled without being selected by a suite, and important encoding, configuration and resource-lifecycle paths had little coverage.
Main changes
Production code, dependencies, configuration and workflows are unchanged.
Validation
Java 17 reactor compilation, formatting and the relevant unit tests passed. Controlled interleavings reproduce the original fixture failures and pass with the fixes; a connection that never closes still fails within the deadline. The latest fixture changes passed independent review.
Current org and ASF CI is being checked after this update. Earlier successful runs apply to their corresponding revisions, and incomplete coverage uploads do not establish the final project coverage result.
Documentation status
Doc - No Need: this PR changes tests and suite registration only.Live-cluster failure scenarios and the intentionally excluded allocation stress tests remain outside this PR.
Implementation overview
Conceptual source-change overview of six-module test registration and reliable fixtures. This diagram does not claim that tests have passed.