feat(#4887): e2e-go Bolt conformance suite (neo4j-go-driver) - #4966
Conversation
Covers auth, transactions (incl. two-writer transient-error race), causal-consistency, multi-database, result-handling, type round-trip, errors, and protocol - plus the CONN-002/005 TLS tests. Six documented gaps use the strict-xfail helper (RESULT-004, TYPE-003/011/012, ERR-002, PROTO-002); CONN-004 and ERR-003 skip per the shared spec.
|
Tick the box to add this pull request to the merge queue (same as
|
Up to standards ✅🟢 Issues
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new standalone Go end-to-end test suite (e2e-go/) to certify the official neo4j-go-driver/v5 against the Bolt protocol conformance spec, complete with container fixtures, TLS testing capabilities, and a PR-gating CI job. The code review identified several critical improvements, including resolving potential container and temporary directory leaks on setup failures, optimizing HTTP payload reading by avoiding string allocations, and adding a timeout to the HTTP client to prevent indefinite hangs.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| return nil, err | ||
| } | ||
| bp, err := c.MappedPort(ctx, boltPort) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &arcadeContainer{container: c, host: host, httpPort: hp.Port(), boltPort: bp.Port()}, nil | ||
| } | ||
|
|
||
| func (c *arcadeContainer) httpBase() string { | ||
| return fmt.Sprintf("http://%s:%s", c.host, c.httpPort) | ||
| } |
There was a problem hiding this comment.
If c.Host, c.MappedPort(ctx, httpPort), or c.MappedPort(ctx, boltPort) fails, the successfully started container c is leaked because it is never terminated. Ensure the container is terminated on any of these errors.
host, err := c.Host(ctx)
if err != nil {
_ = c.Terminate(ctx)
return nil, err
}
hp, err := c.MappedPort(ctx, httpPort)
if err != nil {
_ = c.Terminate(ctx)
return nil, err
}
bp, err := c.MappedPort(ctx, boltPort)
if err != nil {
_ = c.Terminate(ctx)
return nil, err
}There was a problem hiding this comment.
Fixed: startArcade now terminates the container on any Host()/MappedPort() failure after start.
| import ( | ||
| "context" | ||
| "encoding/json" | ||
| "fmt" | ||
| "io" | ||
| "net/http" | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
| "sync" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/neo4j/neo4j-go-driver/v5/neo4j" | ||
| "github.com/testcontainers/testcontainers-go" | ||
| "github.com/testcontainers/testcontainers-go/wait" | ||
| ) |
There was a problem hiding this comment.
Add the "bytes" package to the import block to support using bytes.NewReader instead of strings.NewReader(string(body)) for better performance and fewer allocations.
| import ( | |
| "context" | |
| "encoding/json" | |
| "fmt" | |
| "io" | |
| "net/http" | |
| "os" | |
| "path/filepath" | |
| "strings" | |
| "sync" | |
| "testing" | |
| "time" | |
| "github.com/neo4j/neo4j-go-driver/v5/neo4j" | |
| "github.com/testcontainers/testcontainers-go" | |
| "github.com/testcontainers/testcontainers-go/wait" | |
| ) | |
| import ( | |
| "bytes" | |
| "context" | |
| "encoding/json" | |
| "fmt" | |
| "io" | |
| "net/http" | |
| "os" | |
| "path/filepath" | |
| "strings" | |
| "sync" | |
| "testing" | |
| "time" | |
| "github.com/neo4j/neo4j-go-driver/v5/neo4j" | |
| "github.com/testcontainers/testcontainers-go" | |
| "github.com/testcontainers/testcontainers-go/wait" | |
| ) |
There was a problem hiding this comment.
Fixed: added the bytes import; httpCommand now uses bytes.NewReader(body).
|
|
||
| // httpCommand POSTs a JSON body to an ArcadeDB HTTP endpoint with root auth. | ||
| func httpCommand(c *arcadeContainer, path string, body []byte) error { | ||
| req, err := http.NewRequest(http.MethodPost, c.httpBase()+path, strings.NewReader(string(body))) |
There was a problem hiding this comment.
Converting body from []byte to string and then wrapping it in strings.NewReader causes an unnecessary memory allocation. Use bytes.NewReader(body) directly instead.
| req, err := http.NewRequest(http.MethodPost, c.httpBase()+path, strings.NewReader(string(body))) | |
| req, err := http.NewRequest(http.MethodPost, c.httpBase()+path, bytes.NewReader(body)) |
There was a problem hiding this comment.
Fixed: httpCommand uses bytes.NewReader(body), no []byte->string copy.
| } | ||
| req.SetBasicAuth("root", rootPassword) | ||
| req.Header.Set("Content-Type", "application/json") | ||
| resp, err := http.DefaultClient.Do(req) |
There was a problem hiding this comment.
Using http.DefaultClient without a configured timeout can cause the test suite to hang indefinitely if the server becomes unresponsive. It is highly recommended to use a custom http.Client with a reasonable timeout.
client := &http.Client{Timeout: 30 * time.Second}
resp, err := client.Do(req)There was a problem hiding this comment.
Fixed: added a package-level http.Client with a 30s timeout (httpClient) and use it for seeding/create-db.
| dir, err := os.MkdirTemp("", "bolt-tls-certs") | ||
| if err != nil { | ||
| tlsImageErr = err | ||
| return | ||
| } |
There was a problem hiding this comment.
If any subsequent step in buildTLSImage fails (such as runKeytool or docker build), the temporary directory dir is leaked because the package-level cleanup is only registered at the very end of the function. Use a deferred cleanup function that is disarmed only upon successful registration of the package-level cleanup.
dir, err := os.MkdirTemp("", "bolt-tls-certs")
if err != nil {
tlsImageErr = err
return
}
cleanupDir := true
defer func() {
if cleanupDir {
_ = os.RemoveAll(dir)
}
}There was a problem hiding this comment.
Fixed: buildTLSImage now registers a deferred os.RemoveAll(dir) guarded by cleanupDir, disarmed once the package-level cleanup takes ownership.
| tlsImageTag = tag | ||
| addTLSCleanup(func() { | ||
| _ = exec.Command("docker", "image", "rm", "-f", tag).Run() | ||
| _ = os.RemoveAll(dir) | ||
| }) |
There was a problem hiding this comment.
Disarm the local deferred cleanup once the package-level cleanup has been successfully registered.
tlsImageTag = tag
cleanupDir = false
addTLSCleanup(func() {
_ = exec.Command("docker", "image", "rm", "-f", tag).Run()
_ = os.RemoveAll(dir)
})There was a problem hiding this comment.
Fixed: cleanupDir is set false right before addTLSCleanup so ownership of dir passes to the package-level cleanup.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Meterian found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
PR Review: e2e-go Bolt conformance suite (#4887)Reviewed the full Go module, the new Strengths
Potential issues
Minor / nits
None of these are blockers. Items 1 and 2 are the ones most likely to bite in CI; the rest are polish. 🤖 Generated with Claude Code |
…bumps - startArcade: terminate the container if Host/MappedPort fails after start - buildTLSImage: deferred temp-dir cleanup, disarmed once package-level cleanup takes ownership - httpCommand: shared http.Client with a 30s timeout; bytes.NewReader(body) instead of strings.NewReader(string(body)) - bump vulnerable transitive deps flagged by the dependency scan (testcontainers-go 0.38.0, golang.org/x/net 0.56, x/crypto 0.53, grpc 1.82, protobuf 1.36.11)
- raceUntilConflict: retry the race (distinct marker per attempt) only when no conflict reproduced, up to 5 tries; a surfaced non-transient error still fails the assertion rather than being retried away (TX-005, ERR-004) - go-e2e-tests: run go test with -timeout 20m (plain + 2 TLS containers plus a derived-image build run sequentially, near the 10m default) - document the shared-container isolation assumption (unique markers + no t.Parallel)
|
Review: e2e-go Bolt conformance suite Nicely structured suite. The 1:1 Test_ naming, the strict-xfail assertStillFails helper, and the detailed doc comments explaining the CONN-003/PROTO-002 gaps make this very readable and maintainable, and it mirrors the Python/C# suites well. A few things worth addressing. Go version mismatch (likely to affect CI reproducibility)
Race-based tests may be flaky in CI (TX-005 / ERR-004) Minor
Good practices worth calling out
Overall this is solid; the Go version alignment is the one item I would fix before merge, and I would keep an eye on the two race tests. |
|
Addressed in 48bf6d3 (and 46a057d for #4, which landed before this review ran):
Nits:
Full suite still green locally (36 pass / 2 skip, 6 gaps reproduce, 0 XPASS); CONN-003 is the CI-only/Linux scenario per the note on the test. |
- use go-version-file: e2e-go/go.mod so setup-go tracks the module's go directive (bumped to 1.25 by the dependency updates) instead of a hardcoded version that drifts and forces a GOTOOLCHAIN auto-download - add timeout-minutes: 30 to guard against a hung image pull/build
|
Addressing this review (of 46a057d): Go version mismatch - real issue, caused by the dependency bumps in 46a057d pushing the Race flakiness (TX-005 / ERR-004) - addressed in 48bf6d3 (just before this review): Job timeout - added Shared-DB mutation - added a comment on Dependencies/licensing - confirmed: all three are test-only Go modules (Apache-2.0 / MIT), the module is outside the Maven reactor and not shipped in the distribution, so ATTRIBUTIONS.md is untouched - matching how e2e-python and e2e-csharp were handled (neither updated it). |
|
PR Review: e2e-go Bolt conformance suite Reviewed the full diff. This is a high-quality, well-documented test-only addition that mirrors the existing e2e-python/e2e-csharp suites closely. The Test_ traceability convention, the strict-xfail (assertStillFails) helper for documented gaps, and the sequential shared-container design are all clean and consistent with the sibling suites. No production Java code is touched, so blast radius is limited to CI. Feedback below, ordered by importance. [Should fix] go.mod go directive (1.25.0) does not match the CI toolchain (1.23) e2e-go/go.mod declares [Reliability] two-writer race can still be flaky (TX-005, ERR-004) raceUntilConflict retries up to 5 times only when no error surfaces, a nice hardening over a single attempt. But the test still hard-fails (require.NotEmpty) if the engine serializes both writers cleanly on all 5 attempts. Since the scenario hinges on a 500ms-sleep timing window, there is a residual flake risk on a loaded CI runner. Given the suite is PR-gating, consider whether a clean-serialization outcome should be a t.Skip/soft-pass rather than a failure - "no conflict reproduced" is not itself a conformance violation. At minimum it is worth documenting as a known flake source. [Reliability] external-network dependency at container startup The beer database is imported from a GitHub raw URL (OpenBeer.gz) via defaultDatabases, and readiness is gated only on /api/v1/ready returning 204. Every scenario depends on that fetch succeeding and on the import finishing before the readiness probe passes. This matches the Python/C# suites so it is not a regression, but a GitHub outage turns the whole PR-gating job red. Flagging the coupling, not blocking. Minor notes
Test coverage / conventions All 39 spec.yaml scenarios are represented 1:1, outcomes track current_status, and the 6 documented gaps use the strict-xfail helper that fails loudly on XPASS - correctly forcing a spec.yaml update whenever a gap gets fixed. Apache headers are present on every .go file with no debug/attribution leftover, per the repo conventions. Overall: solid work. The go.mod/CI version mismatch is the one item I would want resolved before merge; the rest are judgment calls. Automated review by Claude (Opus 4.8). Focused on the diff; could not execute the Go suite in this environment. |
assertConflictTransient skips TX-005/ERR-004 when the bounded retries never surface a conflict (a scheduling artifact on a loaded runner, not a conformance violation) instead of hard-failing the PR gate. When a conflict does surface it must still be a retryable Neo.TransientError.* and never a non-retryable ClientError/DatabaseError.
|
Thanks for the thorough pass. This review is of 48bf6d3; addressing it: go.mod (1.25) vs CI (1.23) - already resolved in 6589c74 (pushed just after this review ran): the Two-writer race soft-pass - good call for a PR gate. bb888fd: External-network dependency (OpenBeer.gz import) - agreed it's a coupling; left as-is since it's identical to the Python/C# fixtures (not a regression), but flagging it forward. Minor notes - TYPE-010's Full suite still green locally (36 pass / 2 skip / 6 gaps reproduce / 0 XPASS). |
|
Review: e2e-go Bolt conformance suite Reviewed the full diff. This is a clean, test-only addition (no production Java touched), well-documented, and it faithfully mirrors the merged e2e-python/e2e-csharp suites. The 1:1 Test_ traceability, the strict-xfail helper, and HTTP-only seeding (never over Bolt) are all the right calls. Nice work. Observations below are mostly minor/nits, none blocking. Correctness / potential bugs
Flakiness (CI)
Minor / nits
Test coverage Complete against the 39-scenario spec: 31 passing, 6 documented gaps via strict-xfail, 2 justified skips, all tied to spec.yaml and 4890. The XPASS guard forcing a spec.yaml update when a gap is fixed is a nice forcing function. Overall: LGTM. The items above are refinements, not blockers. Main thing to watch post-merge is CI stability of the two race-based tests. |
Pulls github.com/docker/docker v28.2.2 -> v28.5.2, clearing the two high severity transitive advisories (CVE-2026-34040, CVE-2026-33997) flagged by the dependency scanner. Test-only module, not shipped in the distribution. Full suite re-verified green.
|
On the Meterian scan: it flagged transitive dependencies of The remaining medium/low advisories are inherent to the Docker/OpenTelemetry transitive tree that any |
Review: e2e-go Bolt conformance suite (#4966)Overall this is a high-quality, well-scoped PR. It faithfully mirrors the merged A few observations, all minor: 1. Two-writer race can now silently
|
|
PR Review: e2e-go Bolt conformance suite Reviewed the full diff. This is a clean, well-documented, test-only module that faithfully mirrors the existing e2e-python/e2e-csharp suites. The traceability naming (Test_), the strict-xfail helper, the lazy TLS fixtures, and the CI wiring are all consistent with the sibling suites. No Java/engine code is touched, so there is no compile or runtime risk to the database itself. Nice work overall - the inline comments explaining why (CONN-003 routing, shared-container isolation, downgrade dependency) are genuinely helpful. A few observations, roughly in priority order. 1. TX-005 / ERR-004 can silently certify nothing (main concern). assertConflictTransient calls t.Skip when the two-writer race never surfaces a conflict after the 5 bounded retries, and raceUntilConflict returns early only on an error. On a runner where ArcadeDB serializes the two writers cleanly (no abort), BOTH transient-error scenarios skip and the transient-error contract - the whole point of these two spec scenarios - is never actually exercised, while the PR gate stays green. The soft-skip is a reasonable anti-flake measure, but combined with the fixed 500ms hold it risks these becoming permanent no-ops without anyone noticing. Worth considering: (a) emit a t.Logf (visible even on skip with -v) making it explicit that the conformance assertion was NOT verified this run, so a green run is not mistaken for certified; (b) if the conflict is not reliably reproducible against ArcadeDB locking, a more deterministic construction (hold writer A tx open, then have writer B attempt and assert its error synchronously) may certify the behavior without depending on scheduler timing. 2. Shared beer DB accumulates write side-effects. TX-004 (TX-004-Beer), RESULT-004 (RESULT-004-Beer/Brewery), and each raceUntilConflict attempt (RaceProbe per distinct marker) permanently mutate the shared beer database. This is documented (the fixtures comment says count-based assertions must use a fresh DB) and safe because the container is ephemeral per run. Just flagging that it makes the suite order-and-rerun sensitive against any non-ephemeral target; the current LIMIT-based reads avoid trouble, but future scenarios should keep using boltscratch / a fresh DB rather than beer for anything count-sensitive. 3. MDB-002 leaks the open transaction on a mid-test assertion failure. In Test_MDB_002 the require.Equal(t, int64(0), c) isolation check runs between BeginTransaction and Commit, with no t.Cleanup/defer for tx or the scratch session. If that assertion fails, require calls runtime.Goexit and the open transaction is never committed/rolled back and the session is not closed (the driver Close cleanup eventually reclaims it, so low severity). A defer sess.Close(ctx) / t.Cleanup on the scratch session would make failure teardown clean and match the pattern used elsewhere in the file. 4. TLS scenarios depend on the runner preinstalled keytool. The go-e2e-tests job sets up only Go, not a JDK, so CONN-002/CONN-005 rely on ubuntu-latest shipping a keytool on PATH (README claims they always run in CI). This matches the python/csharp jobs (they do not set up Java either), so it is consistent - but it is a silent-skip failure mode: if the runner image ever drops the default JDK from PATH, these two silently skip instead of failing. An explicit actions/setup-java step (or a CI assertion that keytool exists) would harden the always-run-in-CI guarantee. Minor / optional. Minor notes. repoRoot() panics on failure and is reached from TestMain via seedTypeMatrix; the defer c.container.Terminate in run() still executes during unwind, so no container leak - just noting the asymmetry with the error-returning style of the surrounding helpers. go.mod pins go 1.25.0 (patch-level directive); using go-version-file in CI is the right call to avoid a GOTOOLCHAIN auto-download surprise. Dependency hygiene is good: pinned SHAs for actions, testcontainers-go bumped to clear the docker/docker CVEs, and transitive x/net / x/crypto / grpc bumps. All Apache-2.0/BSD/MIT compatible - no license concern for a test-only module. Verdict. Solid, low-risk addition that extends conformance coverage to the Go driver with strong cross-language traceability. My only substantive ask is item 1 (make the transient-error scenarios visibly signal when they did not actually assert anything); the rest are polish. LGTM once item 1 is at least acknowledged. |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Coverage variation | ✅ -6.53% coverage variation |
| Diff coverage | ✅ ∅ diff coverage |
Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (c0dbdc8) 134228 99739 74.31% Head commit (b22197d) 166031 (+31803) 112525 (+12786) 67.77% (-6.53%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>
Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#4966) 0 0 ∅ (not applicable) Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4966 +/- ##
============================================
+ Coverage 65.32% 66.06% +0.74%
============================================
Files 1682 1682
Lines 134228 134228
Branches 28712 28712
============================================
+ Hits 87685 88679 +994
+ Misses 34526 33379 -1147
- Partials 12017 12170 +153 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… keytool guard - assertConflictTransient logs a loud 'NOT CERTIFIED THIS RUN' line before skipping, so a clean-serialization skip can't be mistaken for a certified green (TX-005/ERR-004) - Test_MDB_002: t.Cleanup on the scratch session so a mid-test isolation assertion failure can't leak the open transaction - go-e2e-tests: assert keytool is on PATH before running, so the TLS scenarios fail loudly instead of silently skipping if the runner ever drops the JDK
|
Addressed in 0ed2544: 1. Transient-error scenarios could silently certify nothing - the real risk introduced by the soft-skip. 3. MDB-002 open-tx leak on mid-test failure - added 4. TLS keytool dependency - added a 2. Shared beer DB accumulation - already documented on Minor notes (repoRoot panic, go 1.25 directive) left as-is - the panic only fires in TestMain setup with the deferred container teardown still running, and |
PR Review: e2e-go Bolt conformance suite (#4966)Reviewed the full diff. This is a clean, unusually well-documented test-only module that mirrors the existing e2e-python/e2e-csharp suites. Scenario coverage checks out (39 functions: CONN×5, AUTH×3, TX×5, CAUSAL×1, MDB×2, RESULT×4, TYPE×12, ERR×4, PROTO×3), the container/cleanup lifecycle is correct ( Correctness / robustness
CI / tooling
Nits
Overall: solid, ready to merge after considering #1 and #2. The rest are optional hardening. |
Weekly gomod ecosystem entry for /e2e-go so the new Go test module's dependencies (notably the testcontainers-go transitive tree that carried the docker/docker CVEs) get automated update PRs. Minor/patch grouped into one PR to limit noise; majors open individually.
|
Review: e2e-go Bolt conformance suite (PR #4966) Reviewed the new Strengths
Things worth addressing
Minor / nits
Test coverage Coverage is the point of this PR and it is comprehensive: every spec scenario is exercised or explicitly, loudly deferred - no gaps silently absent. The keytool-availability CI guard ( Overall: solid, mergeable work. None of the above is blocking; item (1) is the only concrete code change I would suggest. |
2 participants
Part of epic #4882 (Bolt Driver Compatibility Certification), Group B. Closes #4887.
What
New standalone
e2e-go/module that certifies the officialneo4j-go-driver/v5against every scenario in the shared conformance specbolt/conformance/spec.yaml(#4883), mirroring the mergede2e-python(#4885) ande2e-csharp(#4886) suites. Adds a PR-gatinggo-e2e-testsCI job.Design / plan
docs/superpowers/specs/2026-07-04-e2e-go-bolt-conformance-design.mddocs/superpowers/plans/2026-07-04-e2e-go-bolt-conformance.mdCoverage (all 39 scenarios)
One
Test_<AREA>_<NNN>_<slug>function perspec.yamlscenario, so results are 1:1 comparable with the other language suites (traceability convention inbolt/conformance/README.md). Outcomes track each scenario'scurrent_status:assertStillFails, the Go port of C#'sKnownGapAssertions):RESULT-004(empty summary counters),TYPE-003(no native Path),TYPE-011(Duration),TYPE-012(Point),ERR-002(semantic vs syntax error),PROTO-002(no Bolt 5.x). Each passes while the gap reproduces and fails loudly (XPASS) if the gap is ever fixed, forcing aspec.yamlupdate in the same PR. All tracked by Bolt: tracking issue for protocol/type-fidelity gaps surfaced by certification #4890.CONN-004(needs a 3-node HA cluster) andERR-003(needs a raw-socket client - out of scope per the "official drivers only" principle).Infrastructure
testcontainers-go, shared plain container viaTestMain; data seeded over HTTP (never Bolt).CONN-002,CONN-005).ARCADEDB_DOCKER_IMAGE(defaults toarcadedata/arcadedb:latest).go-e2e-testsjob inmvn-test.yml, modeled oncsharp-e2e-tests,needs: build-and-package, PR-gating.Verification
Local run (macOS) against the branch image: 36 PASS / 2 SKIP, all 6 gaps reproduced, zero XPASS. The single local
CONN-003failure is a Docker Desktop artifact - ArcadeDB's ROUTE response advertises the container's bridge IP, which is host-routable on the Linux CI runner (soCONN-003passes in CI, exactly as in the Python/C# suites) but not on Docker Desktop. This is documented on the test and ine2e-go/README.md.Scope
Multi-version driver-band matrix + nightly run (#4891), published compatibility matrix/badge (#4892), and the protocol/type-fidelity fixes themselves (#4890) are explicitly out of scope and deferred to their own issues.