apunkt/omnigraph
1
0
Fork 0
mirror of https://github.com/ModernRelay/omnigraph.git synced 2026-06-12 01:45:14 +02:00
Code Issues Projects Releases Packages Wiki Activity Actions 5

mr-668: close coverage gaps surfaced by the test-coverage audit

Browse source 
The bot-review pass and the subsequent coverage audit surfaced two
material gaps in PR #119's test surface — both easy to close, both
worth closing before merge.

* **Gap 1 — cluster-route sweep.** The Bug-1 path-extractor
  regression slipped through because
  `cluster_routes_dispatch_per_graph_handle` only exercised
  `/snapshot`. The other six protected cluster routes (`/read`,
  `/change`, `/export`, `/schema`, `/schema/apply`, `/ingest`,
  `/branches/merge`) were implicitly trusted to work without any
  multi-mode integration test.

  Add `all_protected_cluster_routes_resolve_to_their_handler`
  (`tests/server.rs`) that hits each protected cluster route with
  a minimal request and asserts the response is consistent with
  the handler being reached — no 404 (router didn't match), no 500
  with "Wrong number of path arguments" (Bug-1 class), no 500 with
  "missing extension" (routing middleware didn't inject the handle).
  Status code is a negative assertion because each handler's
  happy-path inputs differ; what matters is "the request reached
  the handler," not "the handler returned 200" — that's already
  pinned by the single-mode tests.

* **Gap 2 — `--force` happy path.** The strict re-init regression
  test (`init_on_existing_graph_uri_does_not_destroy_existing_schema`)
  pins the error path; nothing pinned the `force: true` escape
  hatch actually doing what its docstring claims.

  Add `init_with_force_recovers_from_orphan_schema_files`
  (`tests/lifecycle.rs`). Writes a bare `_schema.pg` to simulate
  orphan files from a failed prior init, confirms strict mode
  bails as expected, then confirms `init_with_options(force: true)`
  succeeds and produces a functional graph.

  Note: the test follows the documented semantics — force skips
  the preflight only, it does NOT purge existing Lance state. An
  earlier draft of the test (against full overwrite of an existing
  populated graph) failed because `GraphCoordinator::init` errored
  on the existing `__manifest`, which is exactly the limitation
  the `InitOptions::force` docstring already calls out. Recursive
  purge needs `StorageAdapter::delete_prefix` (tracked separately).

Coverage is now fully aligned with the PR's claims.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Ragnor Comerford 2026-05-27 18:32:30 +02:00
parent 4120448431
commit ae4121b7c1
No known key found for this signature in database
2 changed files with 133 additions and 4 deletions
Download patch file Download diff file Expand all files Collapse all files

82
crates/omnigraph-server/tests/server.rs
View file

@ -4482,6 +4482,81 @@ mod multi_graph_startup {
assert_eq!(resp.status(), StatusCode::NOT_FOUND);
}
/// Coverage net for cluster-route regressions across every
/// protected handler — not just the few that have inner path
/// params. Bug-1 surfaced because only `/snapshot` was being
/// exercised in cluster mode, leaving the other six protected
/// routes implicitly untested. This sweep hits each one and
/// asserts the response shows the handler was reached: no 404
/// (router didn't match), no 500 with "Wrong number of path
/// arguments" (path extractor broke), no 500 with "missing
/// extension" (routing middleware didn't inject the handle).
///
/// Status codes are negative assertions because each handler's
/// happy-path inputs differ — what matters is "the request
/// reached the handler," not "the handler returned 200." The
/// individual handlers' logic is already tested in single mode.
#[tokio::test(flavor = "multi_thread")]
async fn all_protected_cluster_routes_resolve_to_their_handler() {
let (_dirs, app) = build_multi_mode_app(&["alpha"]).await;
// (method, path, body) — one minimal request per protected
// cluster route. Bodies are valid enough that the router and
// extractors succeed; whether the engine ultimately returns
// 200 or 4xx is per-handler and not what this test pins.
let cases: &[(Method, &str, Option<&str>)] = &[
(Method::GET, "/graphs/alpha/snapshot?branch=main", None),
(Method::GET, "/graphs/alpha/schema", None),
(Method::GET, "/graphs/alpha/branches", None),
(Method::GET, "/graphs/alpha/commits", None),
(Method::POST, "/graphs/alpha/read", Some(r#"{"query_source":"query q() { return {} }"}"#)),
(Method::POST, "/graphs/alpha/change", Some(r#"{"query_source":"query q() { return {} }"}"#)),
(Method::POST, "/graphs/alpha/export", Some(r#"{"branch":"main"}"#)),
(Method::POST, "/graphs/alpha/schema/apply", Some(r#"{"schema_source":"","allow_data_loss":false}"#)),
(Method::POST, "/graphs/alpha/ingest", Some(r#"{"data":""}"#)),
(Method::POST, "/graphs/alpha/branches/merge", Some(r#"{"source":"main","target":"main"}"#)),
];
for (method, path, body) in cases {
let req_body = body.map(|s| Body::from(s.to_string())).unwrap_or_else(Body::empty);
let req = Request::builder()
.method(method.clone())
.uri(*path)
.header("content-type", "application/json")
.body(req_body)
.unwrap();
let resp = app.clone().oneshot(req).await.unwrap();
let status = resp.status();
let bytes = to_bytes(resp.into_body(), usize::MAX).await.unwrap();
let body_str = String::from_utf8_lossy(&bytes);
assert_ne!(
status,
StatusCode::NOT_FOUND,
"{} {} — router didn't match (cluster-route mounting regression). Body: {}",
method,
path,
body_str,
);
assert!(
!(status == StatusCode::INTERNAL_SERVER_ERROR
&& body_str.contains("Wrong number of path arguments")),
"{} {} — path extractor broke (Bug-1 class regression). Body: {}",
method,
path,
body_str,
);
assert!(
!(status == StatusCode::INTERNAL_SERVER_ERROR
&& body_str.to_lowercase().contains("missing extension")),
"{} {} — routing middleware didn't inject GraphHandle. Body: {}",
method,
path,
body_str,
);
}
}
/// Regression for the bot-surfaced path-extractor bug: cluster
/// routes whose inner path also captures a parameter
/// (`/graphs/{graph_id}/branches/{branch}`,
@ -4493,9 +4568,10 @@ mod multi_graph_startup {
/// GET commit-by-id break in multi-mode because their handlers
/// use bare `Path<String>` — this test pins the fix.
///
/// `cluster_routes_dispatch_per_graph_handle` only exercises
/// `/snapshot` (no Path extractor), so the regression slipped
/// through. This test closes that gap structurally.
/// The broader `all_protected_cluster_routes_resolve_to_their_handler`
/// test sweeps the full route surface; this one stays narrowly
/// targeted at the inner-path-param shape because that's the
/// specific regression class.
#[tokio::test(flavor = "multi_thread")]
async fn cluster_routes_with_inner_path_params_deserialize_correctly() {
let (_dirs, app) = build_multi_mode_app(&["alpha"]).await;

55
crates/omnigraph/tests/lifecycle.rs
View file

@ -2,7 +2,7 @@ mod helpers;
use std::fs;
use omnigraph::db::{Omnigraph, ReadTarget};
use omnigraph::db::{InitOptions, Omnigraph, ReadTarget};
use omnigraph_compiler::schema::parser::parse_schema;
use omnigraph_compiler::{build_schema_ir, schema_ir_pretty_json};
@ -251,3 +251,56 @@ async fn init_on_existing_graph_uri_does_not_destroy_existing_schema() {
"__schema_state.json contents must be preserved when re-init is rejected"
);
}
/// Happy-path sibling to the strict re-init regression above:
/// `InitOptions { force: true }` must skip the schema-file preflight
/// when the operator deliberately wants to recover from orphan
/// schema artifacts (e.g. files left behind by a failed prior init).
///
/// Documented semantics per `InitOptions::force`: skips the preflight
/// only. Force does NOT purge existing Lance datasets or `__manifest/`
/// — that needs `StorageAdapter::delete_prefix`, which is tracked
/// separately. The realistic recovery scenario is "schema files
/// exist but Lance state doesn't," which this test reproduces.
///
/// Without this test, a future refactor could invert the `if !force`
/// branch and silently break the operator-facing escape hatch.
#[tokio::test]
async fn init_with_force_recovers_from_orphan_schema_files() {
let dir = tempfile::tempdir().unwrap();
let uri = dir.path().to_str().unwrap();
// Simulate orphan schema files: write `_schema.pg` to disk
// without running a full init. The preflight will see it and
// bail in strict mode.
fs::write(dir.path().join("_schema.pg"), TEST_SCHEMA).unwrap();
// Strict mode refuses because `_schema.pg` exists.
let strict_err = match Omnigraph::init(uri, TEST_SCHEMA).await {
Ok(_) => panic!("strict init must refuse when orphan _schema.pg exists"),
Err(e) => e,
};
assert!(
strict_err.to_string().contains("already initialized"),
"strict init must surface AlreadyInitialized (sanity check); got: {strict_err}"
);
// Force init succeeds: it skips the preflight, overwrites the
// orphan file, and proceeds to initialize Lance state (which
// didn't exist, so `GraphCoordinator::init` is unblocked).
let db = Omnigraph::init_with_options(uri, TEST_SCHEMA, InitOptions { force: true })
.await
.expect("force init must succeed when only orphan schema files block strict init");
// Confirm the catalog is populated as expected — proves the
// graph is functional after force-recovery, not just that the
// call returned Ok.
assert!(
db.catalog().node_types.contains_key("Person"),
"force-recovered graph must have the new catalog installed"
);
assert!(
dir.path().join("__schema_state.json").exists(),
"force-recovered graph must have full schema state written"
);
}