Skip to content

Commit d5b79e5

Browse files
authored
refactor(server): deduplicate test helpers and grpc utilities (#1708)
Remove three groups of copy-pasted code in openshell-server: 1. grpc/mod.rs had a private current_time_ms() wrapper identical to the one already exported from persistence/mod.rs. Remove the duplicate and update the three grpc sub-modules (policy, sandbox, service) to import directly from crate::persistence. 2. test_store() was repeated verbatim in seven #[cfg(test)] blocks. Promote a single canonical version to persistence/mod.rs (cfg-gated) and replace all copies with crate::persistence::test_store() calls or a thin Arc wrapper in supervisor_session. 3. grpc_client_mtls() and build_tls_root() were copy-pasted across edge_tunnel_auth.rs and multiplex_tls_integration.rs. Move both into the existing tests/common/mod.rs shared module and import from there.
1 parent 1c8417c commit d5b79e5

14 files changed

Lines changed: 63 additions & 117 deletions

File tree

crates/openshell-server/src/grpc/mod.rs

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -153,10 +153,6 @@ enum StoredSettingValue {
153153
// Utility
154154
// ---------------------------------------------------------------------------
155155

156-
fn current_time_ms() -> i64 {
157-
openshell_core::time::now_ms()
158-
}
159-
160156
/// Validate that object metadata is present and contains required fields.
161157
///
162158
/// This is a crate-level helper that wraps the validation module's implementation.

crates/openshell-server/src/grpc/policy.rs

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,8 @@ use tracing::{debug, info, warn};
7272
use super::validation::{
7373
level_matches, source_matches, validate_policy_safety, validate_static_fields_unchanged,
7474
};
75-
use super::{MAX_PAGE_SIZE, StoredSettingValue, StoredSettings, clamp_limit, current_time_ms};
75+
use super::{MAX_PAGE_SIZE, StoredSettingValue, StoredSettings, clamp_limit};
76+
use crate::persistence::current_time_ms;
7677

7778
// ---------------------------------------------------------------------------
7879
// Constants
@@ -3853,16 +3854,11 @@ mod tests {
38533854
Principal, SandboxIdentitySource, SandboxPrincipal, UserPrincipal,
38543855
};
38553856
use crate::grpc::test_support::test_server_state;
3857+
use crate::persistence::test_store;
38563858
use std::collections::HashMap;
38573859
use std::sync::Arc;
38583860
use tonic::Code;
38593861

3860-
async fn test_store() -> Store {
3861-
Store::connect("sqlite::memory:?cache=shared")
3862-
.await
3863-
.expect("in-memory SQLite store should connect")
3864-
}
3865-
38663862
/// Wrap a request with a user `Principal` so handler scope guards treat
38673863
/// the test caller as a CLI user. Most handler tests exercise
38683864
/// user-facing behavior and should not trip sandbox equality checks.

crates/openshell-server/src/grpc/provider.rs

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1604,12 +1604,7 @@ mod tests {
16041604
use super::*;
16051605
use crate::grpc::MAX_MAP_KEY_LEN;
16061606
use crate::grpc::test_support::test_server_state;
1607-
1608-
async fn test_store() -> Store {
1609-
Store::connect("sqlite::memory:?cache=shared")
1610-
.await
1611-
.expect("in-memory SQLite store should connect")
1612-
}
1607+
use crate::persistence::test_store;
16131608
use openshell_core::proto::{
16141609
DeleteProviderProfileRequest, GetProviderProfileRequest, ImportProviderProfilesRequest,
16151610
L7Allow, L7Rule, LintProviderProfilesRequest, ListProviderProfilesRequest, NetworkBinary,

crates/openshell-server/src/grpc/sandbox.rs

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,8 @@ use super::validation::{
4949
level_matches, source_matches, validate_exec_request_fields, validate_policy_safety,
5050
validate_sandbox_spec,
5151
};
52-
use super::{MAX_PAGE_SIZE, MAX_PROVIDERS, clamp_limit, current_time_ms};
52+
use super::{MAX_PAGE_SIZE, MAX_PROVIDERS, clamp_limit};
53+
use crate::persistence::current_time_ms;
5354

5455
const TCP_FORWARD_CHUNK_SIZE: usize = 64 * 1024;
5556

@@ -117,8 +118,6 @@ async fn handle_create_sandbox_inner(
117118
state: &Arc<ServerState>,
118119
request: Request<CreateSandboxRequest>,
119120
) -> Result<Response<SandboxResponse>, Status> {
120-
use crate::persistence::current_time_ms;
121-
122121
let request = request.into_inner();
123122
let spec = request
124123
.spec

crates/openshell-server/src/grpc/service.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ pub(super) async fn handle_expose_service(
3939
.map_err(|e| Status::internal(format!("fetch sandbox failed: {e}")))?
4040
.ok_or_else(|| Status::not_found("sandbox not found"))?;
4141

42-
let now = super::current_time_ms();
42+
let now = crate::persistence::current_time_ms();
4343
let key = service_routing::endpoint_key(&req.sandbox, &req.service);
4444

4545
// Fetch existing endpoint to determine create vs. update path

crates/openshell-server/src/inference.rs

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -934,9 +934,7 @@ mod tests {
934934
use wiremock::{Mock, MockServer, ResponseTemplate};
935935

936936
async fn test_store() -> Store {
937-
Store::connect("sqlite::memory:?cache=shared")
938-
.await
939-
.expect("in-memory SQLite store should connect")
937+
crate::persistence::test_store().await
940938
}
941939

942940
fn test_user_principal() -> Principal {

crates/openshell-server/src/persistence/mod.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -687,5 +687,12 @@ impl Store {
687687
}
688688
}
689689

690+
#[cfg(test)]
691+
pub async fn test_store() -> Store {
692+
Store::connect("sqlite::memory:?cache=shared")
693+
.await
694+
.expect("in-memory SQLite store should connect")
695+
}
696+
690697
#[cfg(test)]
691698
mod tests;

crates/openshell-server/src/persistence/tests.rs

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,11 @@
11
// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
22
// SPDX-License-Identifier: Apache-2.0
33

4-
use super::{ObjectType, PersistenceError, Store, generate_name};
4+
use super::{ObjectType, PersistenceError, Store, generate_name, test_store};
55
use crate::policy_store::PolicyStoreExt;
66
use openshell_core::proto::{ObjectForTest, SandboxPolicy};
77
use prost::Message;
88

9-
async fn test_store() -> Store {
10-
Store::connect("sqlite::memory:?cache=shared")
11-
.await
12-
.expect("in-memory SQLite store should connect")
13-
}
14-
159
#[tokio::test]
1610
async fn sqlite_put_get_round_trip() {
1711
let store = test_store().await;

crates/openshell-server/src/provider_refresh.rs

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -776,7 +776,7 @@ mod tests {
776776
refresh_provider_credential, refresh_state_name, refresh_strategy_name,
777777
run_refresh_worker_tick, seconds_until_ms,
778778
};
779-
use crate::persistence::Store;
779+
use crate::persistence::test_store;
780780
use openshell_core::ObjectId;
781781
use openshell_core::proto::datamodel::v1::ObjectMeta;
782782
use openshell_core::proto::{
@@ -786,12 +786,6 @@ mod tests {
786786
use wiremock::matchers::{body_string_contains, method, path};
787787
use wiremock::{Mock, MockServer, ResponseTemplate};
788788

789-
async fn test_store() -> Store {
790-
Store::connect("sqlite::memory:?cache=shared")
791-
.await
792-
.expect("in-memory SQLite store should connect")
793-
}
794-
795789
#[test]
796790
fn refresh_state_name_preserves_distinct_credential_keys() {
797791
let provider_id = "provider-id";

crates/openshell-server/src/ssh_sessions.rs

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -84,9 +84,7 @@ mod tests {
8484
use std::collections::HashMap;
8585

8686
async fn test_store() -> Store {
87-
Store::connect("sqlite::memory:?cache=shared")
88-
.await
89-
.expect("in-memory SQLite store should connect")
87+
crate::persistence::test_store().await
9088
}
9189

9290
fn make_session(id: &str, sandbox_id: &str, expires_at_ms: i64, revoked: bool) -> SshSession {

0 commit comments

Comments
 (0)