build: consolidate arcadedb-test-utils into server test-jar + fix HA bootstrap-fingerprint replay race#4259
Conversation
![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 consolidates test scaffolding by moving common test utilities from the arcadedb-test-utils module into the arcadedb-server test-jar and deleting the test-utils module. Key changes include updating Maven dependencies across multiple modules to use the test-jar type, migrating Java imports, and enhancing test helpers with improved resource management and logging. Feedback identifies that the arcadedb-server dependency in the grpc-client module was added with provided scope instead of test scope, which could lead to dependency leakage.
| <dependency> | ||
| <groupId>com.arcadedb</groupId> | ||
| <artifactId>arcadedb-test-utils</artifactId> | ||
| <artifactId>arcadedb-server</artifactId> | ||
| <version>${project.parent.version}</version> | ||
| <scope>provided</scope> | ||
| </dependency> |
There was a problem hiding this comment.
The dependency on arcadedb-server is added with provided scope, which deviates from the implementation plan (Task 4, Step 1) and potentially exposes server-side classes to the client module's main source code at compile time. Since this dependency is required for integration tests (supporting BaseGraphServerTest), it should be restricted to test scope to maintain proper modularity and avoid dependency leakage.
| <dependency> | |
| <groupId>com.arcadedb</groupId> | |
| <artifactId>arcadedb-test-utils</artifactId> | |
| <artifactId>arcadedb-server</artifactId> | |
| <version>${project.parent.version}</version> | |
| <scope>provided</scope> | |
| </dependency> | |
| <dependency> | |
| <groupId>com.arcadedb</groupId> | |
| <artifactId>arcadedb-server</artifactId> | |
| <version>${project.parent.version}</version> | |
| <scope>test</scope> | |
| </dependency> |
There was a problem hiding this comment.
Fixed in commit 0107cb3. Changed <scope>provided</scope> to <scope>test</scope>. Verified RemoteGrpcServerIT still passes — grpc-client main code only imports the protobuf-generated com.arcadedb.server.grpc.* classes (which ship in the arcadedb-grpc module, not arcadedb-server), so test scope is the right fit.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
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.
Code ReviewOverviewThis PR consolidates the duplicate test scaffolding ( Positives
Issues and Concerns1.
|
📜 License Compliance Check✅ License check passed. See artifacts for full report. License Summary (first 50 lines) |
|
Walking the 5 items from the review. 1. 2. 3. AI planning documents. Pushing back. The repo already collects design specs and implementation plans under 4. PR description. Filled in — summary, approach, verification (17 sentinel tests pass), net diff, rollback. Both checklist items remain unchecked because the project's PR template predates this kind of refactor; they don't apply cleanly. 5.
Items 1, 2, 4 are concrete fixes; 3 and 5 addressed with reasoning above. |
📜 License Compliance Check✅ License check passed. See artifacts for full report. License Summary (first 50 lines) |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4259 +/- ##
==========================================
+ Coverage 64.63% 64.65% +0.02%
==========================================
Files 1644 1644
Lines 127254 127277 +23
Branches 27264 27267 +3
==========================================
+ Hits 82255 82297 +42
+ Misses 33509 33479 -30
- Partials 11490 11501 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Code ReviewOverviewThis PR consolidates duplicate test scaffolding ( Positives
Issues to Address1. Docs files should not be checked in
Recommendation: delete both files before merging. 2. The diff adds two separate <!-- plain server jar - no <type> -->
<dependency>
<artifactId>arcadedb-server</artifactId>
<scope>test</scope>
</dependency>
<!-- test-jar -->
<dependency>
<artifactId>arcadedb-server</artifactId>
<scope>test</scope>
<type>test-jar</type>
</dependency>The PR description explains this is intentional ("the test classpath needs 3. This PR bundles a meaningful HA/Raft behavior change alongside the test scaffolding consolidation. The guard added to Two questions:
Minor / Nits
SummaryThe consolidation itself is well-executed and the 403 bug fix is correct. Two action items before merge:
The |
📜 License Compliance Check✅ License check passed. See artifacts for full report. License Summary (first 50 lines) |
|
Pushing back on all three items - mostly verification, not implementation. 1. This is established repo convention, not a one-off. Keeping them. 2. The plain test-scope entry is required, not redundant. Reasoning:
Concretely, grpc-client tests import This mirrors 3.
Yes.
Yes -
This is exactly the path the new guard short-circuits. Without the guard, the re-applied entry hits the install path on the StateMachineUpdater thread, which blocks Ratis leader-info notifications and burns the snapshot-retry budget on null leader URLs (and ultimately halts via the critical-error path). The IT was failing for that reason before the guard. Agreed the PR title undersells the production fix - I'll update it to reflect the HA change alongside the test-utils consolidation. |
robfrank
changed the title
📜 License Compliance Check✅ License check passed. See artifacts for full report. License Summary (first 50 lines) |
Code ReviewOverall this is a solid refactor that pays down real technical debt. The core consolidation is well-executed and the HA race fix addresses a genuine production issue. Some notes below. What works well
Concerns1. <dependency>
<artifactId>arcadedb-server</artifactId>
<scope>test</scope>
<!-- no type - pulls in production jar -->
</dependency>
<dependency>
<artifactId>arcadedb-server</artifactId>
<scope>test</scope>
<type>test-jar</type>
</dependency>The first entry (no 2. A backup statement writes data to disk (the backup archive) and may overwrite or create new files depending on the URL. In the general case, running it twice produces a different side-effect each time (two backups, or an overwrite). Without context on where If The test added covers parsing and the return value, but does not document the intended semantics of the flag. 3. Two files are added:
These look like AI agent working documents used during development - they contain task checklists, step-by-step implementation notes, and internal planning context. They add no value to contributors reading the repository and will confuse future readers. Recommend removing them before merge. 4. PR scope mixes several independent concerns The title covers two topics (test-utils consolidation + HA race fix), but the diff also includes:
Each of these is fine on its own, but bundling them makes the PR harder to review and bisect if a regression appears. Not a blocker, but worth noting for future PRs. Minor notes
|
📜 License Compliance Check✅ License check passed. See artifacts for full report. License Summary (first 50 lines) |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
🟢 Coverage 70.27% diff coverage · -7.45% coverage variation
Metric Results Coverage variation ✅ -7.45% coverage variation Diff coverage ✅ 70.27% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (3591e93) 127254 93773 73.69% Head commit (713e0d7) 158955 (+31701) 105289 (+11516) 66.24% (-7.45%) 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 (#4259) 37 26 70.27% 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.
Code ReviewOverviewThis PR consolidates the duplicated test scaffolding (TestServerHelper, StaticBaseServerTest, BaseGraphServerTest, WebSocketClientHelper) from the arcadedb-test-utils module into server's test-jar, then deletes the now-redundant module. It also bundles several unrelated but valuable fixes: an HA bootstrap-fingerprint replay race, gRPC LocalDate serialization, static HttpClient thread-leak prevention in AI handlers, and a BackupDatabaseStatement.isIdempotent() change. What works well
Issues and suggestions1. grpc-client/pom.xml ends up with two arcadedb-server test entries<!-- entry 1: main jar in test scope -->
<dependency>
<groupId>com.arcadedb</groupId>
<artifactId>arcadedb-server</artifactId>
<version>${project.parent.version}</version>
<scope>test</scope>
</dependency>
<!-- entry 2: test-jar in test scope -->
<dependency>
<groupId>com.arcadedb</groupId>
<artifactId>arcadedb-server</artifactId>
<version>${project.parent.version}</version>
<scope>test</scope>
<type>test-jar</type>
</dependency>The PR description explains the reason (test classpath needs ArcadeDBServer at class-load time), but a short XML comment would help future readers. Also worth confirming: other modules like bolt have arcadedb-server in provided scope (which is visible on the test classpath), so they get the main jar without an extra test-scope entry. If grpc-client genuinely has no transitive path to the main server jar at test time, both entries are necessary - confirming that closes this question. 2. BackupDatabaseStatement.isIdempotent() returning true - semantics look wrong@Override
public boolean isIdempotent() {
return true;
}In ArcadeDB's SQL layer, isIdempotent() returning true typically means the statement is safe for GET/HEAD HTTP dispatch and may be treated as read-only for replication decisions. A backup writes data to storage or the network - it is not read-only. Returning true could allow the statement through GET endpoints meant only for reads, or affect HA routing. The added test verifies the return value but does not explain why it should be true. Please clarify: if backup is intentionally considered idempotent here (e.g. re-running it safely overwrites the same target and callers should be free to retry), the reasoning belongs in a comment. If it is a mistake, false is the correct value, or the override can be dropped to inherit the base-class default. 3. Three separate static HttpClient instances in AI handlersAiActivateHandler, AiAnalyzeProfilerHandler, and AiChatHandler each declare their own static HttpClient with identical configuration. A single shared constant in a utility class or AiConfiguration would be cleaner and marginally cheaper to initialize. Not a blocker, worth a follow-up. 4. Hardcoded local path in plan docdocs/superpowers/plans/2026-05-18-test-utils-consolidation.md contains a reference to /Users/frank/projects/arcade/arcadedb/pom.xml - a local filesystem path from the authoring machine. A repo-relative path or just "the root pom.xml" is enough. Test coverage
SummaryThe test-utils consolidation is clean, well-motivated, and follows project conventions. The HA race fix is solid. The main items worth addressing before merge are: clarifying the isIdempotent() semantics on BackupDatabaseStatement (issue 2) and adding a comment to the dual grpc-client dep (issue 1). |
Captures the design for collapsing the duplicate test scaffolding (TestServerHelper, StaticBaseServerTest, BaseGraphServerTest, WebSocketClientHelper) into a single canonical location at server/src/test/java/com/arcadedb/server/. Downstream modules switch to depend on arcadedb-server:test-jar:test and arcadedb-test-utils is deleted.
Seven tasks producing six commits: three reconciliation commits (one per helper pair that drifted), two migration commits (downstream pom swap, then import rename), and one cleanup commit (delete test-utils module + remove misleading server/pom test-jar override).
…Folders RaftHTTP2ServersCreateReplicatedDatabaseIT and other Raft ITs were failing with HTTP 403 on basic-auth login because stale target/config/server-users.jsonl from a previous test run held a password hash created with a different SERVER_ROOT_PASSWORD. The matching cleanup was already in test-utils/src/main/java/com/arcadedb/test/TestServerHelper - this brings the server-module copy in line. The duplicate helpers will be consolidated in a follow-up branch.
Adopt try-with-resources for PrintWriter, structured async-callback logging, and refreshed Javadoc from the test-utils copy. Server-copy error-body capture in initialConnection retained.
The InterruptedException catch was swallowing the interrupt status, which can cause downstream blocking calls to hang. Mirror the test-utils copy by re-asserting the interrupt flag.
- TestServerHelper: informative fail() message and utility-class form (final + private ctor) - StaticBaseServerTest: explicit Level import instead of wildcard
Step 1 of 2 for the test-utils consolidation. Tests in these modules will still fail to compile until imports are migrated in the next commit.
Step 2 of 2 for the test-utils consolidation. All test classes now import the canonical helpers from server's test-jar.
The four helpers (TestServerHelper, StaticBaseServerTest, BaseGraphServerTest, WebSocketClientHelper) now live solely in server's test-jar. Removes the structural duplication that caused the recent 403 test failures (test-utils' deleteDatabaseFolders had target/config cleanup; server's did not). Also drops the misleading maven-jar-plugin phase=none override in server/pom.xml -- it did not actually disable test-jar emission and would break the test-jar consumers if it ever started working.
- grpc-client/pom.xml: arcadedb-server scope provided -> test. The grpc-client main code only references com.arcadedb.server.grpc.* classes that ship in the arcadedb-grpc protobuf module, not the arcadedb-server module. Test-scope is sufficient for the tests (RemoteGrpcServerIT etc.) that need ArcadeDBServer at class load. - graphql/pom.xml: drop arcadedb-server:test-jar:test. The graphql test sources do not reference any com.arcadedb.server.* helper, so the dependency was dead weight. Aligns the pom with the spec's original intent to drop (not swap) graphql's test-utils dep.
RaftHAComprehensiveIT.test14_writesDuringSlowFollower failed because the
restarted follower's StateMachine replayed the BOOTSTRAP_FINGERPRINT_ENTRY
at index 1 and detected a mismatch (the local database had been
forward-replicated past the original baseline). The mismatch path triggered
installFromLeaderForBootstrap inside applyTransaction. Because that runs
on the StateMachineUpdater thread - the same thread that would service
Ratis leader-info notifications - the snapshot install's 4 retries all
saw a null leader HTTP address, exhausted the budget, and tripped the
critical-error halt. After the halt, every subsequent entry was refused
("State machine halted after critical error at earlier index") and the
follower could never catch up.
Fix: short-circuit applyBootstrapFingerprintEntry when the persisted
applied index is at or beyond this entry's index. In that case the
verification ran in a prior session and the local database has been
replicated forward by Ratis AppendEntries; re-running install is both
unnecessary and dangerous. Recording the baseline in bootstrapBaselines
is preserved so status export and tests still see the correct value.
The genuine initial-bootstrap path is untouched: a brand-new follower
has no persisted applied index file, readPersistedAppliedIndex() returns
-1, and the check fails so the full verification + install runs.
Verified:
- RaftHAComprehensiveIT#test14_writesDuringSlowFollower: was failing,
now passes
- RaftHAComprehensiveIT full suite: 13/13 pass
- SnapshotInstallerRetryTest + SnapshotInstallerIntegrationIT: 6/6 pass
(confirms genuine bootstrap-install path still works)
- PromQLHttpHandlerIT.series: percent-encode the match[] query parameter so Undertow accepts the request (literal [] is rejected per RFC 3986 and the handler was never reached). - RemoteDateIT.dateTimeMicros1: connect to the actual bound HTTP port via arcadeDBServer.getHttpServer().getPort() instead of hard-coding 2480, so the test still authenticates against its own server when another process holds 2480.
…snapshot installation logic
AiActivateHandler, AiChatHandler, and AiAnalyzeProfilerHandler each held the HttpClient as a per-instance final field built in the constructor. The JDK HttpClient spawns a SelectorManager NIO daemon thread that lives until the client is garbage-collected, so every ArcadeDBServer start leaked one SelectorManager per handler (3 per server). Under the ha-raft IT suite (reuseForks=true, 2-5 servers per test, dozens of ITs), a thread dump showed hundreds of leaked SelectorManager threads partway through the run, which slowed leader election enough on later tests to exceed their 30s budget. Make the HttpClient private static final, matching the existing pattern in PostServerCommandHandler and PostBatchHandler. All three handlers used identical configuration (10s connect timeout), so a single shared client is correct.
robfrank
force-pushed
the
fix/failing-it-tests-2
branch
from
cbefa76 to
6a357fd
Compare
Code ReviewOverviewThis PR consolidates duplicate test scaffolding ( Positive aspects
Issues and suggestions1. Planning/spec documents checked into source (medium)
2.
|
…adInsideTransactionIT
📜 License Compliance Check✅ License check passed. See artifacts for full report. License Summary (first 50 lines) |
Code ReviewOverviewThis PR does two things: (1) consolidates duplicate test scaffolding into a single canonical home by deleting The motivation is solid. The duplication was structural (Maven reactor cycle), and the 403 failures were a direct consequence of the helpers diverging between copies. Good approach. Issues1. After the migration, 2.
3. The PR adds two large markdown files:
These are AI agent workflow documents with step-by-step task checklists, commit-message templates, and implementation instructions. They are not user-facing documentation and do not belong in the project's git history. They should be excluded from this PR. If there is a desire to keep design rationale, a brief comment in the deleted module's commit message or a short ADR is enough. 4. AI handlers - three separate static Each of Positives
SummaryThe consolidation approach is correct and the immediate bug fixes are well-executed. Three actionable items before merging: add a POM comment in |
1 participant
Summary
Consolidates the duplicate test scaffolding (
TestServerHelper,StaticBaseServerTest,BaseGraphServerTest,WebSocketClientHelper) into a single canonical home atserver/src/test/java/com/arcadedb/server/, then deletes thearcadedb-test-utilsmodule entirely.Background: two near-identical copies of these helpers existed (one in
arcadedb-test-utils, one inserver/src/test/java). The duplication was structural (a Maven reactor cycle blocked the server module from depending onarcadedb-test-utils) and caused real test failures — thetarget/config/cleanup was added to one copy but not the other, leading to HTTP 403 failures inRaftHTTP2ServersCreateReplicatedDatabaseITandFollowerSessionTokenQueryITwhen staleserver-users.jsonlcarried a different password hash.Approach
After reconciling the four helper pairs (taking the better-quality version from each, keeping the server-copy's error-body capture in
BaseGraphServerTest), all 10 downstream modules switch fromarcadedb-test-utilstoarcadedb-server:test-jar:test. Thetest-utilsmodule is deleted. ~50 test imports rewritten.Spec:
docs/superpowers/specs/2026-05-18-test-utils-consolidation-design.mdPlan:
docs/superpowers/plans/2026-05-18-test-utils-consolidation.mdVerification
mvn clean install -DskipTestsBUILD SUCCESSRaftHTTP2ServersCreateReplicatedDatabaseIT,FollowerSessionTokenQueryITPASSBoltProtocolIT,ConsoleTest,GremlinServerTest,PostgresProtocolIT,MongoDBServerTest,RedisWTest,GrpcServerIT,RemoteGrpcServerIT,PrometheusMetricsPluginAuthenticatedTest,BoltTlsIT— all PASS (17 tests total, 0 failures)phase=noneoverride inserver/pom.xml(verified by inspecting~/.m2/repository/com/arcadedb/arcadedb-server/26.6.1-SNAPSHOT/arcadedb-server-26.6.1-SNAPSHOT-tests.jartimestamp post-build)Net diff
arcadedb-test-utilsmodule (1158 lines)grpc-client/pom.xml(test-scopearcadedb-serveradded because the test classpath needsArcadeDBServerat class load viaBaseGraphServerTest)Rollback
Single
git revertof the consolidation range restores everything (helpers, module, poms, imports). No data, schema, or config impact.