Repository navigation
test: add coverage across struct, commons, server, pd, store and cluster - #3309
bitflicker64 wants to merge 13 commits into
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.29% -1.35%
- Complexity 2435 2497 +62
============================================
Files 770 759 -11
Lines 67694 66363 -1331
Branches 9172 8932 -240
============================================
- Hits 35631 34038 -1593
- Misses 28520 29080 +560
+ 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
Purpose of the PR
Before 1.8.0 we wanted more executable tests module by module, with attention to the parts that changed recently: the Java 17 migration, the new
hugegraph-structmodule, and the RocksDB and JRaft upgrades. Several test classes in the repository also compiled but never ran, because no suite or Surefire include picked them up.This PR only touches test classes and suite registration. There are no production, dependency, configuration, CI or documentation changes. Each module is a separate commit so it can be reviewed on its own.
Main Changes
test(struct)BytesBufferTest: varint rejection, unsigned limits, length-prefixed bytes, string endings, edge and index id encoding, partition and OLAP id parsing, tagged collections, buffer growth.EdgeIdTest: text form, parse errors, binary id generation.test(commons)TypedOptionTest(option parsing and checkers),HugeConfigValidationTest(URL normalization, input validation),HostNameVerifierTest(negative paths after #3307: SAN matching, wildcard scope, IP vs DNS, no CN fallback), rpcRpcConfigTest(self URL exclusion, invalid options, provider and consumer config, service cleanup on destroy).test(server)RocksDBStdSessionsLifecycleTest(unopened tables, rollback, reopen, lock release after force close and bad options, iterator double close, snapshot path),RocksDBIteratorPoolTest,HstoreTableRoutingTest. RegistersConsumersTestandMemoryManageTest, which existed but never ran.test(pd)KVOperationTest(encoding, null checks, truncated input, whitelist decoding),RaftKVStoreTest(writes go through raft, leader check, raft failure, state machine replay),PeerUtilTest,ZipUtilsTest(snapshot checksums, zip-slip entries). RegistersRaftReflectionUtilTest.test(store)Base58EncoderTest,KeyUtilTest,CRC64Test,ZipUtilsPathSafetyTest(both ZipUtils copies),SessionOperatorTest(writes, ranges, iterator release, snapshots),PartitionMetaCodecTest,HgBufferProxyTest. RegistersHgStoreStateMachineTest,FutureClosureTest,HgAssertTestandHgPairTest. ThereuseForks=falsesetting of the core execution is unchanged.test(cluster)SimpleClusterGraphTest: 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.New RocksDB tests use
TemporaryFolderinstead of fixed paths. No new test binds a fixed port.Verifying these changes
03ffdf0bcafter the review fixes. Base is36bc1c64b. The figures are run/failed/errors/skipped.mvn test -pl hugegraph-structmvn test -pl hugegraph-commons/hugegraph-common -Dtest=UnitTestSuite -DskipCommonsTests=falsemvn test -pl hugegraph-commons/hugegraph-rpc -Dtest=UnitTestSuite -DskipCommonsTests=falsemvn test -pl hugegraph-server/hugegraph-test -P unit-testmvn test -pl hugegraph-pd/hg-pd-test -P pd-core-testmvn test -pl hugegraph-store/hg-store-test -P store-common-test... -P store-rocksdb-test... -P store-raftcore-test... -P store-client-test... -P store-core-testmvn test -pl hugegraph-cluster-test/hugegraph-clustertest-test -P simple-cluster-testRestClientTestHTTPS tests time out reaching an external endpoint, and fourMachineInfoTesttests find no MAC or IP address on that machine.HeartbeatShutdownTest, in a checkout wherehg-store-nodehad not been compiled. With a full build it passes.mvn editorconfig:checkpasses on every touched test module.36bc1c64b. After the branch was synced with master atde392632e, CI at14b1f4136ran the serverUnitTestSuite(jobserver_memory) with 1103/0/0/8; that count includes unit tests that arrived from master.Does this PR potentially affect the following parts?
Documentation Status
Doc - TODO: required documentation is pending; complete it before merging.Doc - Done: documentation is included here or linked below.Doc - No Need: no user-visible documentation is affected.Documentation files in this PR or paired hugegraph-doc PR:
Known limitations
These tests were left out or left unregistered on purpose.
MemoryAllocateTestpasses, but two of its tests force a 1 GiB heap or direct allocation that is never released, so it is a stress test rather than a default unit test. It is left unregistered and unchanged.MemoryConsumerTestpasses on its own but fails insideUnitTestSuite. An earlier test terminates the sharedEventHubexecutor, and itsclearBackend()calls then getRejectedExecutionException. It is left unregistered and unchanged.PDWatchTesthas no assertions and needs a live PD.PDPulseTestis marked@Uselessand waits forever against a live PD without stores. Neither is registered.CoreSuiteTestTODO block pass on their own; they are left for a follow-up.multi-cluster-testwas not run locally.Some new tests exposed behaviour that looks wrong in production code. Those tests are not part of this PR; the findings will be reported separately:
IdUtil, an id part that ends with a backtick does not survive escaping and unescaping.SplicingIdGenerator.split(concat("a", "b"))returns["a>b"]`.BytesBuffer.readVIntandreadVLong, theleading == 0x80check compares a signed byte with 128, so it is never true.wrap({0x80, 1}).readVInt()returns 1.HugeConfig.validateOptionskips the option checker when a value is already of the target Java type. A port of 70000 passed as anIntegeris accepted.RocksDBStdSessions.scan()opens a native iterator before the scan arguments are checked, and never closes it when the check fails.The native cursor capacity-stop fixture now waits for the initial producer handoff before allowing the worker to stop. This preserves the existing capacity, edge-count and resource-release assertions without depending on thread scheduling. Production code is unchanged; the fix passed the relevant native tests, Java 17 reactor compilation and three independent reviews.