test(rust): move fake integration backend tests to extracted file#1960
Merged
senamakel merged 3 commits intoMay 16, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR introduces comprehensive test infrastructure for OpenHuman integrations: a fake HTTP backend with axum, harness tests for native tool chains, backend validation tests, and integration execution tests verifying tool registry setup and output against the fake backend. ChangesTest Infrastructure and Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Member
Author
|
@coderabbitai reivew |
Contributor
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/tools/ops_tests.rs (1)
349-1217: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftThis test module should be split; it is significantly beyond the repo’s size limit.
The new additions further increase an already very large test file. Please move integration-family suites into dedicated sibling modules/files (e.g., registry baseline vs backend execution suites) to keep test maintenance tractable.
As per coding guidelines: “Keep Rust source files to ≤ ~500 lines; split modules when growing larger.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/tools/ops_tests.rs` around lines 349 - 1217, The test module ops_tests.rs is too large and should be split: extract the long "integration-family" suites (tests named all_tools_executes_apify_family_against_fake_backend, all_tools_executes_google_places_family_against_fake_backend, all_tools_executes_parallel_and_web_search_family_against_fake_backend, all_tools_executes_stock_and_twilio_family_against_fake_backend and any helpers they use such as integration_tools_for_config and integration_test_support usage) into one or more dedicated sibling test modules/files (e.g., ops_integration_apify.rs, ops_integration_parallel.rs, ops_integration_financial.rs) and leave the registry/baseline tests (all_tools_default_registry_contains_expected_baseline_surface, all_tools_default_registry_has_no_duplicate_tool_names, default_tools_* etc.) in ops_tests.rs; preserve #[tokio::test] and #[test] attributes, re-export or import shared helpers with use crate::... or super:: as needed, add mod declarations so the new files are compiled as tests, and ensure any environment setup (TEST_ENV_LOCK, env vars) and store_test_session_token calls are still available to the moved tests.
🧹 Nitpick comments (2)
src/openhuman/agent/harness/test_support_tests.rs (1)
227-297: 🏗️ Heavy liftSplit these new tests into a dedicated sibling test module to stop growing this oversized file.
These additions are solid, but appending them here grows an already very large module further. Please move these two new scenarios into a separate
_test.rssibling (or feature-focused test module) to keep this file maintainable.As per coding guidelines, "Keep Rust source files to ≤ ~500 lines; split modules when growing larger."
Also applies to: 299-353
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/agent/harness/test_support_tests.rs` around lines 227 - 297, Split the two new test functions (including keyword_provider_uses_latest_tool_result_to_drive_the_next_tool_call and the other test around lines 299-353) out of the oversized test module into a new sibling test file (a dedicated _test.rs file) and move all their dependencies/imports into that file; ensure you copy or re-export the required symbols (KeywordScriptedProvider, KeywordRule, ScriptedToolCall, RecordingTool, run_tool_call_loop, ChatMessage, mm, etc.) via use super::* or explicit crate paths so the tests compile, add the new file to the crate (no additional runtime code changes), and run cargo test to verify everything passes.src/openhuman/tools/ops_tests.rs (1)
905-930: ⚡ Quick winMake LSP env-var cleanup panic-safe to avoid cross-test contamination.
If any assertion panics after
set_varand before the explicitremove_var, later tests can observe staleLSP_ENABLED_ENV. Please switch to a small drop guard so cleanup always runs.Suggested fix
+ struct LspEnvGuard; + impl Drop for LspEnvGuard { + fn drop(&mut self) { + unsafe { + std::env::remove_var(crate::openhuman::tools::implementations::LSP_ENABLED_ENV); + } + } + } + let _env_guard = crate::openhuman::config::TEST_ENV_LOCK .lock() .unwrap_or_else(|e| e.into_inner()); + let _lsp_env_guard = LspEnvGuard; unsafe { std::env::set_var( crate::openhuman::tools::implementations::LSP_ENABLED_ENV, "1", ); } @@ - unsafe { - std::env::remove_var(crate::openhuman::tools::implementations::LSP_ENABLED_ENV); - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/tools/ops_tests.rs` around lines 905 - 930, Replace the current unsafe set_var/remove_var pair with a panic-safe drop guard: after acquiring TEST_ENV_LOCK and calling std::env::set_var(crate::openhuman::tools::implementations::LSP_ENABLED_ENV, "1"), immediately create a small RAII guard (e.g., LspEnvGuard) whose Drop implementation calls std::env::remove_var for the same LSP_ENABLED_ENV; keep the rest of the test using all_tools(...) and assertions unchanged so the guard will always run even if assertions panic. Ensure the guard is created in the same scope as the set_var so it lives long enough for the test and is dropped automatically at scope exit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/integrations/test_support.rs`:
- Around line 56-678: spawn_fake_integration_backend currently contains all
route handlers and is too large; split the file by integration family and make
this function compose smaller route-builder helpers. Extract route groups into
separate functions/ modules (e.g., build_apify_routes,
build_google_places_routes, build_parallel_routes, build_financial_apis_routes,
build_twilio_routes) that return Router or IntoRouter, move their handler
closures into those modules (preserving symbols like FakeIntegrationState,
record, as_string_array), then in spawn_fake_integration_backend compose them
(Router::new().merge(build_apify_routes(state.clone())).merge(...)) so
src/openhuman/integrations/test_support.rs becomes a small coordinator while
each integration family lives in its own file/module.
---
Outside diff comments:
In `@src/openhuman/tools/ops_tests.rs`:
- Around line 349-1217: The test module ops_tests.rs is too large and should be
split: extract the long "integration-family" suites (tests named
all_tools_executes_apify_family_against_fake_backend,
all_tools_executes_google_places_family_against_fake_backend,
all_tools_executes_parallel_and_web_search_family_against_fake_backend,
all_tools_executes_stock_and_twilio_family_against_fake_backend and any helpers
they use such as integration_tools_for_config and integration_test_support
usage) into one or more dedicated sibling test modules/files (e.g.,
ops_integration_apify.rs, ops_integration_parallel.rs,
ops_integration_financial.rs) and leave the registry/baseline tests
(all_tools_default_registry_contains_expected_baseline_surface,
all_tools_default_registry_has_no_duplicate_tool_names, default_tools_* etc.) in
ops_tests.rs; preserve #[tokio::test] and #[test] attributes, re-export or
import shared helpers with use crate::... or super:: as needed, add mod
declarations so the new files are compiled as tests, and ensure any environment
setup (TEST_ENV_LOCK, env vars) and store_test_session_token calls are still
available to the moved tests.
---
Nitpick comments:
In `@src/openhuman/agent/harness/test_support_tests.rs`:
- Around line 227-297: Split the two new test functions (including
keyword_provider_uses_latest_tool_result_to_drive_the_next_tool_call and the
other test around lines 299-353) out of the oversized test module into a new
sibling test file (a dedicated _test.rs file) and move all their
dependencies/imports into that file; ensure you copy or re-export the required
symbols (KeywordScriptedProvider, KeywordRule, ScriptedToolCall, RecordingTool,
run_tool_call_loop, ChatMessage, mm, etc.) via use super::* or explicit crate
paths so the tests compile, add the new file to the crate (no additional runtime
code changes), and run cargo test to verify everything passes.
In `@src/openhuman/tools/ops_tests.rs`:
- Around line 905-930: Replace the current unsafe set_var/remove_var pair with a
panic-safe drop guard: after acquiring TEST_ENV_LOCK and calling
std::env::set_var(crate::openhuman::tools::implementations::LSP_ENABLED_ENV,
"1"), immediately create a small RAII guard (e.g., LspEnvGuard) whose Drop
implementation calls std::env::remove_var for the same LSP_ENABLED_ENV; keep the
rest of the test using all_tools(...) and assertions unchanged so the guard will
always run even if assertions panic. Ensure the guard is created in the same
scope as the set_var so it lives long enough for the test and is dropped
automatically at scope exit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5efb8ce5-4263-4ad6-af47-8da707bbabea
📒 Files selected for processing (4)
src/openhuman/agent/harness/test_support_tests.rssrc/openhuman/integrations/test_support.rssrc/openhuman/integrations/test_support_test.rssrc/openhuman/tools/ops_tests.rs
17 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
#[cfg(test)]blocks intosrc/openhuman/integrations/test_support_test.rssrc/openhuman/tools/ops_tests.rsto include the fake backend directly for Rust-only tool integration coveragesrc/openhuman/integrations/mod.rs*_test.rsfile splitProblem
integrations/mod.rsSolution
test_support.rsvia#[path = "test_support_test.rs"] mod tests;../integrations/test_support.rsdirectly insideops_tests.rsfor integration-family execution testsSubmission Checklist
diff-cover) meet the gate enforced by.github/workflows/coverage.yml. Validated locally for the changed Rust files and enforced again by CI.docs/TEST-COVERAGE-MATRIX.mdreflect this change. Behaviour-only Rust test organization change.## Related. No product feature behavior changed.docs/RELEASE-MANUAL-SMOKE.md). Rust test-only change.Closes #NNNin the## Relatedsection. No issue was provided for this change.Impact
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
codex/rust-tool-integration-mocksca127a7aValidation Run
pnpm --filter openhuman-app format:checkpnpm typecheckcargo test --manifest-path Cargo.toml all_tools_executes_apify_family_against_fake_backend -- --nocapturecargo test --manifest-path Cargo.toml fake_backend_records_requests_and_applies_route_defaults -- --nocapturecargo fmt --all --checkcargo check --manifest-path app/src-tauri/Cargo.tomlValidation Blocked
command:pnpm test:coverageerror:not run in this shipping passimpact:CI remains the source of truth for merged diff-coverage gateBehavior Changes
Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit