* MR-786: merge-pair truth table with exhaustive op-variant matrix
Add crates/omnigraph/tests/merge_truth_table.rs that enumerates every
(left_op, right_op) cell from the operation vocabulary named in the
ticket — {noop, addNode, removeNode, addEdge, removeEdge, setProperty,
dropProperty, addLabel, removeLabel} — and asserts the deterministic
outcome of Omnigraph::branch_merge against a structured oracle.
The matrix is built in a 9x9 match in build_case, so adding a new
OpVariant is a compile-time, fail-on-omission task. Today's mutation
grammar only exposes insert | update set | delete (see
docs/query-language.md), so the 36 cells over the first six ops are
executable and the 45 cells involving dropProperty/addLabel/removeLabel
are recorded as Expected::Unsupported with a note. Each executable cell
spins up a fresh tempdir, applies one mutation per branch, calls
branch_merge, and asserts either:
* MergeOutcome (AlreadyUpToDate / FastForward / Merged) plus a
GraphAssert on the affected entities, or
* an OmniError::MergeConflicts whose entries match the expected
table_key + MergeConflictKind (row_id is optional because edge
ULIDs are generated at runtime).
branch_merge is directional, so the (L, R) and (R, L) cells live in
separate entries in the matrix and are run independently — the
op-pair symmetry encoded in build_case serves as the commutativity
oracle without doubling the runtime. End-to-end the suite runs in
~10s on a fresh build, well under the 30s budget asserted at the
bottom of the test.
Also adds a row to docs/testing.md so the test-coverage map points
future agents at this file.
Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
* Use one Omnigraph handle for both branches
Self-review caught that the runner was opening two Omnigraph handles
on the same temp dataset (one for main, a second via Omnigraph::open
for feature). tests/branching.rs uses one handle and passes the branch
name to mutate_branch — same pattern works here and avoids any
cache-coherency surprises between the two handles. Also drops the
post-merge reopen, which only existed to give the second handle a
fresh snapshot.
Runtime drops ~10s -> ~9s.
Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
* Assert exact conflict count, not subset inclusion
cubic and Devin Review both flagged that check_outcome's
Expected::Conflicts arm only enforces want ⊆ got, so a regression that
produces a spurious extra conflict (e.g. emitting both OrphanEdge and
a stray DivergentInsert) would silently pass the truth-table cell.
For a deterministic oracle that's the wrong direction — the cell pins
the exact conflict-artifact set, not a lower bound. Add an
assert_eq!(got.len(), want.len()) before the existence loop. All 36
executable cells still pass; runtime unchanged.
Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
* Subsume 4 conflict tests in branching.rs into truth table
The four `branch_merge_reports_*_conflict` tests
(DivergentUpdate / DivergentInsert / DeleteVsUpdate / OrphanEdge)
were redundant with the deterministic-oracle cells in the new
`merge_truth_table.rs` and only added drift risk.
To preserve the post-conflict invariant that lived in
`branch_merge_reports_divergent_update_conflict` (target unchanged
after a failed merge), the truth-table runner now generalizes it:
on every `Conflicts` cell, main's state is asserted against
`state_after_apply_only(right_op)`. That gives strictly more
coverage than the deleted tests carried, since the invariant now
applies to *all* seven conflict cells, not just one.
The `UniqueViolation` and `CardinalityViolation` cases stay in
`branching.rs` — they're combinatorial (require >1 op per side
with a non-default schema) and out of scope for the pair-wise
truth table.
Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
* Fix misleading 'Total edges: 0' comment in (AddEdge, RemoveEdge) cell
Devin Review flagged that the comment said 'Total edges: 0' while the
parenthetical math evaluates to 1 (matching `GraphAssert::base()`).
The assertion is correct; only the leading number in the comment was
wrong. Reworded to 'Net edges: … = 1 (matches base)' so the prose
agrees with both the math and the assertion.
Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
---------
Co-authored-by: Ragnor <ragnor@modernrelay.com>
Co-authored-by: Ragnor Comerford <ragnor.comerford@gmail.com>
Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
10 KiB
Testing
This file is the always-on map of the test surface. Consult it before every task so you know what tests already cover the area you're about to change, what helpers to reuse, and where a new test belongs. The architectural invariant "tests at every boundary, not just end-to-end" lives in docs/invariants.md §VIII.47.
Where tests live, per crate
| Crate | Path | Style |
|---|---|---|
omnigraph (engine) |
crates/omnigraph/tests/ |
Integration tests (15 files), fixture-driven, share tests/helpers/mod.rs |
omnigraph-cli |
crates/omnigraph-cli/tests/ |
cli.rs (unit-ish), system_local.rs, system_remote.rs, share tests/support/mod.rs |
omnigraph-server |
crates/omnigraph-server/tests/ |
server.rs (HTTP-level), openapi.rs (OpenAPI drift / regeneration) |
omnigraph-compiler |
mostly in-source #[cfg(test)] mod tests |
Parser, type-checker, IR lowering, lint |
The engine's tests/ is the principal coverage surface; most graph-shaped behavior is exercised there.
Engine integration tests (crates/omnigraph/tests/)
| File | Covers |
|---|---|
end_to_end.rs |
Full init → load → query/mutate flow |
branching.rs |
Branch create / list / delete, lazy fork |
merge_truth_table.rs |
Merge-pair truth table (MR-786): all 9×9 (left_op, right_op) cells from {noop, addNode, removeNode, addEdge, removeEdge, setProperty, dropProperty, addLabel, removeLabel}. Adding a new op to OpVariant forces a compile error in build_case until the new row + column are dispositioned. 36 executable cells run through real branch_merge with a structured oracle (MergeOutcome / MergeConflictKind + graph-state assert); 45 cells involving dropProperty/addLabel/removeLabel are recorded as Unsupported until the mutation grammar grows. |
runs.rs |
Direct-publish writes: cancellation, concurrent-writer CAS, multi-statement atomicity, MR-794 staged-write rewire (D₂ rejection, insert+update coalesce, multi-append coalesce, partial-failure recovery, load RI/cardinality recovery) |
staged_writes.rs |
TableStore staged-write primitives (stage_append, stage_merge_insert, commit_staged, scan_with_staged, count_rows_with_staged) — primitive-level only; engine code uses the in-memory MutationStaging accumulator instead |
lifecycle.rs |
Repo lifecycle, schema state |
point_in_time.rs |
Snapshots, time travel (snapshot_at_version, entity_at) |
changes.rs |
diff_between / diff_commits |
consistency.rs |
Cross-table snapshot isolation, atomic publish |
schema_apply.rs |
Migration plan + apply, schema-apply lock |
search.rs |
FTS / vector / hybrid (bm25, nearest, rrf) |
traversal.rs |
Expand, variable-length hops, anti-join |
aggregation.rs |
count, sum, avg, min, max |
export.rs |
NDJSON streaming export filters |
s3_storage.rs |
S3-backed repo (skipped unless OMNIGRAPH_S3_TEST_BUCKET is set) |
lance_version_columns.rs |
Per-row _row_last_updated_at_version behavior |
failpoints.rs |
Failure-injection coverage (gated on failpoints feature). Includes the four per-writer Phase B → recovery integration tests (recovery_rolls_forward_after_finalize_publisher_failure, schema_apply_phase_b_failure_recovered_on_next_open, branch_merge_phase_b_failure_recovered_on_next_open, ensure_indices_phase_b_failure_recovered_on_next_open). |
recovery.rs |
Open-time recovery sweep — sidecar I/O, classifier dispatch (NoMovement / RolledPastExpected / UnexpectedAtP1 / UnexpectedMultistep / InvariantViolation), all-or-nothing decision, roll-forward via ManifestBatchPublisher::publish, roll-back via Dataset::restore, audit row in _graph_commit_recoveries.lance, OpenMode::ReadOnly skip path |
composite_flow.rs |
Compositional/narrative end-to-end stories — multi-step flows that compose mechanics covered by other test files. Catches integration regressions where individual operations all pass their unit tests but their composition breaks (sequential merges, post-merge main writes, time-travel through merge DAG, reopen consistency over multi-merge histories). |
Fixtures
crates/omnigraph/tests/fixtures/ holds the canonical schema (.pg), seed data (.jsonl), and queries (.gq) shared across tests. Reuse these before inventing new ones — the helpers harness already knows how to load them.
Test helpers
- Engine —
crates/omnigraph/tests/helpers/mod.rs:init_and_load()(bootstrap a temp repo + load standard fixture),snapshot_main(),snapshot_branch(), query/mutation runners, row collection and counting. Use these instead of hand-rolling. - CLI —
crates/omnigraph-cli/tests/support/mod.rs:Command-style wrapper for invokingomnigraph, server-process spawning, fixture resolution, output assertion helpers. - Server — no shared helpers; server tests call the
Omnigraphengine API directly and exercise endpoints over the wire.
Note: there is no
MemStorageor in-memory backend today. Tests usetempfile::tempdir()for local FS. If you find yourself needing one for layer isolation, that's an architectural ask — see docs/invariants.md §VIII.48 (reference impl + test impl per trait).
Failpoints (fault injection)
- Cargo feature:
failpoints = ["dep:fail", "fail/failpoints"](incrates/omnigraph/Cargo.toml). - Wrapper:
crates/omnigraph/src/failpoints.rsexposesmaybe_fail("name")andScopedFailPointfor tests. - Call sites are inserted at sensitive transaction boundaries (branch create, graph publish commit, etc.).
- Activated tests:
crates/omnigraph/tests/failpoints.rs. Run withcargo test -p omnigraph-engine --features failpoints --test failpoints.
RustFS / S3 integration
CI runs three S3-backed tests against a containerized RustFS server (.github/workflows/ci.yml → rustfs_integration job):
cargo test -p omnigraph-engine --test s3_storagecargo test -p omnigraph-server --test server server_opens_s3_repo_directly_and_serves_snapshot_and_readcargo test -p omnigraph-cli --test system_local local_cli_s3_end_to_end_init_load_read_flow
Locally, set OMNIGRAPH_S3_TEST_BUCKET (and the usual AWS_* vars including AWS_ENDPOINT_URL_S3 for non-AWS) before running. Without those, S3 tests skip gracefully.
OpenAPI drift
crates/omnigraph-server/tests/openapi.rs regenerates openapi.json and diffs against the checked-in copy. CI auto-commits the regeneration on same-repo PRs and otherwise runs in strict-check mode (env: OMNIGRAPH_UPDATE_OPENAPI).
Examples & benches
crates/omnigraph/examples/bench_expand.rs— runnable example (not part of CI).- No
benches/directories. The architectural rule docs/invariants.md §VIII.50 requires benchmark motivation before optimization, so addbenches/per crate when you ship a perf-driven change.
Coverage tooling — what's missing
There is no coverage tooling in the repo today: no tarpaulin.toml, no codecov.yml, no coverage CI step. If you want to know whether your change is covered, the answer comes from reading and running the relevant integration tests, not from a tool.
If introducing coverage tooling is in scope for your task, the natural first step is cargo-llvm-cov wired into a separate CI job, and a per-crate threshold rather than a global one.
First principle: check what already covers it
Before writing any new test, check whether an existing test already covers the case. The cost of duplicating coverage is high: more code to read, more places to keep in sync when behavior changes, and more drift when one copy lags. The cost of extending an existing test is usually one extra assertion or one extra fixture row.
How to check:
- Map the change to an area — use the engine integration-test table above (
branching.rs,runs.rs,search.rs, etc.). The filename usually names the area. - Open the file and skim every test fn name. Test fn names are the index — read them all, not just the first few.
- Grep for the symbol or path you're changing.
rg <FunctionName>orrg <enum_variant>across alltests/directories surfaces existing coverage you might miss. - Decide one of three outcomes, in this order of preference:
- Existing test already asserts the new behavior → no new test needed; this PR is a refactor or no-op behaviorally. Confirm by running the existing test against the change.
- Existing test covers the area but not your case → add an assertion or a fixture row to the existing test, don't write a new function with
init_and_load()again. - No existing coverage in any test file → only then write a new test; put it in the file that owns the area, or open a new file only if the area itself is new.
Three duplicated init_and_load() → run_query → assert_eq blocks where one parameterized test would do is the most common form of test rot in this repo. Don't add to it.
Before-every-task checklist
When you pick up any change, walk through this:
- Find existing coverage (per the principle above). Don't just look at the first test file by name — grep for the symbol you're touching across every crate's
tests/. - Run those tests locally before editing.
cargo test --workspace --lockedfor the broad pass;-p <crate> --test <file>for a focused loop. Confirm a clean baseline. - Decide extend-vs-new explicitly. If you can extend an existing test (assertion, fixture row, parameterization), do that. Only add a new test fn or new file if no existing one owns the area.
- Reuse the helpers.
init_and_load(), fixture files, the CLIsupportharness — re-use them. Don't bootstrap a fresh repo by hand if a helper exists. - Mind the boundary. Per docs/invariants.md §VIII.47, test at the layer the change lives at — planner-level changes deserve planner-level tests, not just end-to-end.
- For substrate-touching changes (Lance behavior), reach for
failpointsor fixture-driven scenarios, not stubbed-out mocks. - For server / API changes, confirm the OpenAPI regeneration happens in
openapi.rsand that the diff lands inopenapi.json. - Verify your change makes an existing test fail before it makes the new one pass. If you can break the code without breaking a test, your coverage gap is the problem to fix first.
When in doubt, re-read docs/invariants.md §VIII — quality gates apply to every change.