Repository navigation
test: improve coverage across toolchain modules before 1.8.0 - #805
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #805 +/- ##
============================================
+ Coverage 47.04% 49.79% +2.75%
- Complexity 4517 4921 +404
============================================
Files 602 603 +1
Lines 29448 30174 +726
Branches 3454 3636 +182
============================================
+ Hits 13853 15025 +1172
+ Misses 14312 13783 -529
- Partials 1283 1366 +83 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: the diff is test-only and every new or newly registered test is wired into a suite that CI runs, but the head conflicts with master in the client JsonUtilTest.java and UnitTestSuite.java since #577 merged, so it cannot be merged as it is. Evidence: read the full diff at 1b96264 against merge base 039153c (30 files, +3311/-4, all under test paths or *_test.go / *.test.js); checked suite wiring against client-ci.yml (-Dtest=UnitTestSuite, ApiTestSuite, FuncTestSuite), tools-ci.yml (-Dtest=FuncTestSuite), the loader unit profile and hubble-be unit-test profile (**/UnitTestSuite.java), spark-connector-ci.yml (mvn test), hubble-ci.yml (yarn test) and client-go-ci.yml (make test over all packages); read BatchInsertTask, InsertTask, LoadContext and HubbleUtil against the new assertions; check runs on this head are 28 success, 4 skipped, 1 neutral; git merge-tree --write-tree origin/master 1b962641 reports the two conflicts with master at 8e6ed84.
Register unit tests that already compiled but never ran (HugeClientBuilderTest, GremlinManagerTest), the Cypher and Adamic-Adar API tests and GraphModeTest, all of which pass against Server 1.8. Add unit tests for ServerException parsing of JSON, alternative-key, non-JSON and empty error bodies, for graph element property validation and for JsonUtil error paths. HugeClientFakeServerTest drives the client against a local fake server that returns 5xx HTML, non-JSON bodies, an unsupported API version, slow responses and dropped connections, and checks that legacy servers get requests without the graphspace prefix.
BackupFailureTest runs the backup manager against a local fake server. Server errors on shard queries are retried and then fail with the original ServerException, non-JSON shards fail backup, schema errors reach the caller, and invalid split sizes and label/type combinations are rejected. Add LocalDirectoryTest for plain and zip streams, append versus override, corrupt, empty and missing archives, and directory checks. Extend RetryManagerTest with retry success, exhaustion and default behavior, and add JsonGraphTest for the dump graph and formatters.
1b96264 to
b05b19d
Compare
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: the diff is test-only (30 files, +3282/-4, all under src/test, *_test.go or *.test.js) and every new or newly registered test runs and passes in CI at b05b19d; one Minor maintainability issue in the loader's BatchInsertRetryTest. Evidence: read the full diff against merge base 8e6ed84 and the production code each test exercises (RetryManager, BackupManager, LocalDirectory, BatchInsertTask, LoadContext, HugeClientHolder, DataTypeUtil, HubbleUtil, spark DateUtils, Go hgtransport and vertex API); confirmed from the exact-head job logs that client UnitTestSuite (124), ApiTestSuite (382, 3 skipped) and FuncTestSuite (104), loader UnitTestSuite (148), tools FuncTestSuite (55), spark DateUtilsTest and DataTypeUtilsTest, hubble-be EntityUtilTest and HubbleUtilTest, the three hubble-fe Jest files and the Go vertex, hgtransport and v1 tests all ran with 0 failures; gh pr checks 805 reports every check passing or skipped.
| // No sleep between retries | ||
| options.retryInterval = 0; | ||
| options.file = this.folder.newFile("mapping.json").getAbsolutePath(); | ||
| LoadContext context = (LoadContext) allocateInstance(LoadContext.class); |
There was a problem hiding this comment.
Minor: context() builds LoadContext with sun.misc.Unsafe.allocateInstance and then sets 13 private fields by name, so the test bypasses the constructor and silently diverges from a real context. LoadContext has 15 instance fields; loadingModeSet and failure are left at defaults, and schemaCache and parseGroup are set to null although the constructor always makes them non-null. A later change that renames a field fails here with NoSuchFieldException, and one that adds a constructor-initialised field or reads context.schemaCache() on the insert path fails with an NPE that has nothing to do with retries. None of this is needed: the public LoadContext(LoadOptions) constructor only calls HugeClientHolder.create(options), which builds the client and requests /versions, the one endpoint the fake server already serves besides /graph/edges/batch. Please set options.host = "127.0.0.1", options.port to the fake server's port and options.timeout = 1, use new LoadContext(options), close context.client() in teardown(), and drop allocateInstance and setField.
Add unit tests for DataTypeUtil conversion of numbers, booleans, UUIDs, text, dates with extra formats and time zones, JDBC temporal values and list/set values, with the matching invalid inputs. Cover the text, CSV and JSON line parsers, including quoted CSV fields and column count mismatches, and LoadMapping parsing of valid and invalid struct files. BatchInsertRetryTest runs BatchInsertTask against a local fake server: 5xx responses are retried until success or until the retry limit, while IllegalArgumentException and unacceptable messages fail immediately.
Add DateUtilsTest for time-zone aware parsing and invalid dates, and extend DataTypeUtilsTest with invalid numbers, text conversion, string dates in the default format, null values and unsupported list/set cardinalities.
b05b19d to
a45f790
Compare
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: the diff is still test-only and every new or newly registered test runs in CI at a45f790, and the BatchInsertRetryTest change since b05b19d builds a real LoadContext as requested. One new hubble-be assertion depends on the JVM time zone and fails mvn test -P unit-test for anyone west of UTC-4. Evidence: read the full diff against merge base 8e6ed84 (30 files, +3240/-4, all under src/test, *_test.go or *.test.js) and the interdiff b05b19d..a45f790 (BatchInsertRetryTest only); checked suite wiring in the client, tools, loader and hubble-be suites against the module validation run 38667385564, where client UnitTestSuite (128), ApiTestSuite (382, 3 skipped), FuncTestSuite (104), tools FuncTestSuite (55), loader UnitTestSuite (148), spark DateUtilsTest and DataTypeUtilsTest, hubble-be EntityUtilTest and HubbleUtilTest, the three Jest files and the Go packages all passed. The only red job, loader-ci, fails in KafkaLoadTest.testNumberToStringInKafkaSource (expected 7 vertices, got 2), a profile and class this PR does not touch, so it needs a rerun rather than a code change. The time zone result comes from running the same SimpleDateFormat call under several zones (output inline).
| @Test | ||
| public void testDateFormatDay() { | ||
| Assert.assertEquals("2024-01-15", HubbleUtil.dateFormatDay("20240115")); | ||
| Assert.assertEquals("20240115", HubbleUtil.dateFormatDay(new Date(1705291200000L))); |
There was a problem hiding this comment.
Important: this assertion only passes in time zones at UTC-4 or further east. HubbleUtil.dateFormatDay(Date) is new SimpleDateFormat("yyyyMMdd").format(date), so it formats in the JVM default zone, and 1705291200000L is 2024-01-15T04:00:00Z. That instant is still 14 January in New York, Los Angeles or Honolulu, so mvn test -P unit-test in hubble-be fails for a contributor there. CI passes only because the GitHub runners use UTC, and the hubble-be surefire config does not pin user.timezone.
Evidence: the same call, run with java -Duser.timezone=<zone>:
UTC day=20240115 month=202401
Asia/Shanghai day=20240115 month=202401
America/Sao_Paulo day=20240115 month=202401
America/New_York day=20240114 month=202401
America/Los_Angeles day=20240114 month=202401
Pacific/Honolulu day=20240114 month=202401
testTimestampsBefore24Hours in the same class already handles this by computing the offset from TimeZone.getDefault().
Requested change: build the date from local calendar fields so it is 15 January in every zone, for example Date day = new GregorianCalendar(2024, Calendar.JANUARY, 15, 12, 0).getTime();, and use it for both the dateFormatDay and dateFormatMonth assertions on lines 73 and 74.
imbajin
left a comment
There was a problem hiding this comment.
Fresh independent cross/adversarial review of the reviewed revision: 7/10 overall; needs a fix. The scope is test-only, suites are correctly wired, and the retry test now uses a real LoadContext instead of Unsafe and closes its client.
The existing timezone finding remains. An independent bounded control of the exact timestamp/formatter returns 20240114 in New York, Los Angeles and Honolulu, versus 20240115 in UTC and Shanghai. This confirms a JVM-default-timezone dependency; it is not repeated as another inline finding.
The latest Loader CI job passes 148 Loader unit tests, then fails the unchanged KafkaLoadTest with expected 7/actual 2. That does not prove a regression introduced by the new tests, and the failure is not waived. No full multi-module rerun was performed independently.
Add backend tests for EntityUtil.merge, which applies entity updates according to @MergeProperty, and for HubbleUtil collection equality, date helpers, digests, the host pattern and the size and duration JSON serializers. Add frontend tests for storage and duration formatting, request payload sanitizing in removeNilKeys and convertStringToJSON.
Add hgtransport tests for URL prefixes, basic auth sources, explicit Authorization headers, user agent and content type, and for server errors, dropped connections, timeouts and request/response logging. Cover api.Response helpers, config validation in NewCommonClient and /versions against non-JSON bodies, JSON errors and dropped connections. The vertex package had no executing test: its only test is commented out. Add httptest coverage for vertex create, batch create and property updates, with and without a graphspace, plus error bodies and cancelled contexts.
a45f790 to
d43da07
Compare
imbajin
left a comment
There was a problem hiding this comment.
Fresh independent cross/adversarial review of the reviewed revision: 8.5/10; recommended for merge within this test-only scope. No new blocking defect was confirmed. All 30 changed files are tests or suite registrations; no production, dependency or workflow changes. The broad module coverage is additive and does not introduce a new testing framework.
The current date fixture uses local calendar fields and passes focused checks across six zones (UTC, Shanghai, New York, Los Angeles, Honolulu and Kiritimati). A correction to our earlier timezone assessment: HubbleUtil itself sets the default timezone to GMT+08 during class initialization, so the earlier isolated SimpleDateFormat control overstated a failure under western JVM startup zones. Using the actual HubbleUtil, the old fixed instant passes at fresh startup; it fails in western zones only when the default timezone is changed after class initialization, while the new local-calendar fixture remains correct. The current change improves robustness, and the previous finding should no longer block this PR. These were focused Java 21 controls with fail-fast stubs for unrelated dependencies, not a full Java 17/Maven run. The retry fixture also uses a real LoadContext and closes its client.
Independent frontend checks executed 35 assertions from the new format and JSON conversion tests against current production source through a lightweight Node harness, not Jest. Incorrect storage-unit mapping and scalar-JSON acceptance negative controls caused 7 and 3 assertion failures respectively. No full independent Go, Maven or live-server rerun is claimed.
The current-head PR merge CI is green for Client, Loader, Tools, Spark, Hubble and Go. Its checkout is the PR merge revision, not standalone head execution. Logs confirm the added tests run; HDFS and same-RC acceptance are not established by this result. The prior Kafka failure is not present in this run. All executed module matrices in this run are candidate-only; it does not establish Server 1.7 compatibility. The PR description's statement that the Server 1.7 baseline is exercised by CI should be qualified to the actual run.
One non-blocking test-strengthening suggestion: TestPerformTimeout currently accepts any error within three seconds. Check that the error implements net.Error with Timeout() true, and that the handler was entered, so an unrelated fast connection failure cannot satisfy the intended timeout test. This is an assertion-specificity gap, not a confirmed production regression.
The production limitations listed in the PR description remain separate follow-up issues. These additive tests neither repair those issues nor establish runtime performance, full concurrent safety or release-candidate acceptance.
Purpose of the PR
More test coverage across the toolchain modules before the 1.8.0 release, as requested for the Java 17 migration. This PR only adds or registers tests. It changes no production code, dependencies, CI workflows or documentation.
Each module is a separate commit, so the PR can be reviewed one commit at a time.
Main Changes
hugegraph-client
HugeClientBuilderTestandGremlinManagerTest(unit),CypherApiTestandAdamicAdarAPITest(API) andGraphModeTest(functional). All pass against the locally built Server 1.8; the Server 1.7 baseline is exercised by CI only.SameNeighborsBatchApiTestis left to fix(client): Add SameNeighborsBatchApiTest to test suite #673.ServerExceptionTest: error bodies with the standard keys, the alternative keys, non-JSON bodies, empty bodies and empty causes.HugeClientFakeServerTest: the client against a localHttpServerthat returns 5xx HTML, non-JSON bodies or an unsupported API version, answers slowly or drops the connection. It also checks that requests to a legacy server leave out thegraphspaces/DEFAULTprefix.GraphElementTestandJsonUtilTest: property value validation and serialization error paths.hugegraph-tools (all registered in
FuncTestSuite)BackupFailureTest: backup against a fake server. A 5xx on the shard query is retried and then fails with the originalServerException; non-JSON shards and schema errors fail the backup; an invalid split size or label/type combination is rejected.LocalDirectoryTest: plain and zip streams, append and override, corrupt, empty and missing archives, directory checks.RetryManagerTest: retry success, exhaustion and the default of no retries.JsonGraphTest: the dump graph,JsonFormatterand formatter loading.hugegraph-loader (all registered in the
unitprofile)DataTypeUtilTest: conversion of numbers, booleans, UUIDs, text, dates (extra formats, time zones, timestamps, JDBC temporal types) and list/set values, each with invalid inputs.LineParserTest: text, CSV (quotes, escaped quotes) and JSON line parsing, missing and extra columns.LoadMappingTest: valid and invalid struct files (version, empty or duplicate structs, a missing input type, an unknown source type).BatchInsertRetryTest:BatchInsertTaskagainst a fake server. A 5xx is retried until it succeeds or the retry limit is reached.IllegalArgumentExceptionand the unacceptable messages fail at once.hugegraph-spark-connector
DateUtilsTestand moreDataTypeUtilsTestcases: invalid numbers, text conversion, string dates, null values, unsupported list/set cardinality.hugegraph-hubble
EntityUtilTestcovers@MergePropertyupdate semantics.HubbleUtilTestcovers collection equality, date helpers, digests, the host pattern and the size and duration serializers.removeNilKeys(algorithm request payloads) andconvertStringToJSON.hugegraph-client-go (
httptestonly, no live server)hgtransport: URL prefix, basic auth from config or URL, explicitAuthorization, user agent, content type, 5xx pass-through, dropped connections, timeouts, logger body handling.api.Responsehelpers,NewCommonClientconfig validation,/versionswith non-JSON, JSON error and dropped-connection responses.TestVertex, has its whole body commented out, so the vertex package showed 0.0% coverage with the CI test flags; it now shows 85.5%.Verifying these changes
install-candidate-sdk.shinto a separate Maven repository. Authentication was on, with the batch limits that CI sets.mvn test -Dtest=UnitTestSuitemvn test -Dtest=ApiTestSuitemvn test -Dtest=FuncTestSuiteHugeClientHttpsTest)HugeClientHttpsTestmvn test -Dtest=FuncTestSuite -Dtools.test.disposable=truemvn test -P unitmvn test -P file(unchanged)mvn testmvn test -P unit-testyarn lint,yarn i18n:check,yarn testgo test -race -count=1 ./...Notes:
HugeClientHttpsTestand the loader HTTPS tests fail for that reason in both runs. CI starts its own HTTPS server.hdfs,jdbcandkafkaprofiles (no HDFS, MySQL or Kafka started), the Hubble packaged API/browser test, and the opt-inHugeGraphIdentityIntegrationTest, which needs a server without authentication.Production issues found (tests not included)
These tests fail on the current code, so they are not part of this PR:
JsonLineParser.parsecatchesSerializeException, butJsonUtil.convertMapthrows the loader'sDeserializeException, so noReadExceptionis raised and--max-read-errorshas no effect. Reproduced withFileLoadTestresources (reload_json_failure_files): one valid edge line plus{"source_name":, with--max-read-errors 10. The load threwDeserializeException, and the valid edge was not loaded either.BatchInsertTaskretries onClientException, but the client now throws a rawIOExceptionfor timeouts and dropped connections. WithretryTimes=3, a fake server that answers after 2.5 s (client timeout 1 s) got one request beforeSocketTimeoutExceptionreached the caller. A dropped connection got two requests: the first try and one OkHttp retry, none from the loader. 5xx responses are retried correctly;BatchInsertRetryTestcovers that case.Long.MAX_VALUEwrap around.DataTypeUtil.parseLongusesLong.parseUnsignedLongfor non-negative input, so"9223372036854775808"becomes-9223372036854775808and"18446744073709551615"becomes-1instead of being rejected. The same helper parses numeric vertex IDs. The Spark connector'sDataTypeUtils.parseLonghas the same code (checked by reading, not run).DataTypeUtil.parseBlobhas its body commented out and returnsnullfor any input, for example"0x01"or"AQI=".client.Vertex.GetByID()and the other unassigned function fields cause a nil pointer panic instead of returning an error.DateUtils. It shares oneSafeDateFormatper pattern and callssetTimeZonebeforeparse, the code the loader fixed in fix(loader): fix thread-safety issue in DateUtil with concurrent date parsing #734. A port of the loader's concurrent test returned 1 wrong instant out of 1600. The connector currently always passes GMT+8, so the sink path does not hit it.Does this PR potentially affect the following parts?
Documentation Status
Doc - TODODoc - DoneDoc - No NeedImplementation overview
Conceptual source-change coverage across six Toolchain modules; this diagram does not claim that tests have passed.