Repository navigation
test: add integration test suite for app layer, registry, and CLI - #12
Merged
Merged
Conversation
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExplicitly register three integration test binaries, expose selected modules from the crate root, switch main to reference the crate namespace, validate provider API env vars as non-empty, consolidate ReqwestHttpClient initialization via Default, and add multiple integration tests plus shared CLI test utilities. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.cargo/config.toml:
- Around line 1-4: The .cargo/config.toml currently sets global,
architecture-specific OpenSSL env vars (OPENSSL_DIR, OPENSSL_INCLUDE_DIR,
OPENSSL_LIB_DIR) which break non-x86_64 and non-Linux builds; remove these
hardcoded entries and instead document optional per-user configuration or add
conditional logic: delete the three env keys from .cargo/config.toml and
instruct maintainers to use platform-specific environment variables (or a
CI/tooling script) to set OPENSSL_DIR/OPENSSL_INCLUDE_DIR/OPENSSL_LIB_DIR when
needed, or replace with platform-agnostic fallbacks (e.g., only set when a
platform-detection step confirms x86_64 Linux) so builds on arm64/macOS/other
distros are not forced to use /usr/lib/x86_64-linux-gnu.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63928a72-dc9b-41c3-976e-b5e0006b2781
📒 Files selected for processing (8)
.cargo/config.tomlCargo.tomlsrc/lib.rssrc/main.rssrc/transport/http.rstests/integration/cli_test.rstests/integration/provider_registry_test.rstests/integration/search_service_test.rs
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/providers/brave/config.rs`:
- Around line 9-16: The code currently rejects whitespace-only BRAVE_API_KEY but
stores leading/trailing whitespace; fetch the env var into api_key, trim it for
emptiness check and then persist the trimmed value (e.g., let api_key =
api_key.trim().to_string()) before constructing Self { api_key, base_url: ... }
so the stored api_key is normalized; update the api_key variable usage where the
struct is built to use the trimmed string.
In `@src/providers/exa/config.rs`:
- Around line 9-16: The API key is validated with api_key.trim() but the raw
value is stored, so trailing whitespace can cause auth failures; update the code
that constructs the config (the block returning Ok(Self { api_key, ... })) to
store a normalized key by trimming (and optionally canonicalizing) the
environment value before assignment, e.g., replace usage of the original api_key
variable in the returned Self with the trimmed/normalized value so the stored
field is clean.
In `@tests/common/cli.rs`:
- Around line 5-7: The test helper run_cli should neutralize the RUST_LOG
environment variable so tracing logs don't leak into stderr; change run_cli to
remove RUST_LOG from the spawned process before executing by calling
env_remove("RUST_LOG") on the Command returned by cli_command(args) (i.e., use
cli_command(args).env_remove("RUST_LOG").output().expect(...)) so
stderr/assertions remain deterministic.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b251e0b-4f39-458a-bfdb-b29c6b1991eb
📒 Files selected for processing (8)
src/lib.rssrc/providers/brave/config.rssrc/providers/exa/config.rstests/common/cli.rstests/fanout_cli_test.rstests/integration/cli_test.rstests/integration/provider_registry_test.rstests/integration/search_service_test.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib.rs
- tests/integration/search_service_test.rs
Comment on lines
9
to
16
| let api_key = std::env::var("BRAVE_API_KEY")?; | ||
| if api_key.trim().is_empty() { | ||
| return Err(std::env::VarError::NotPresent); | ||
| } | ||
|
|
||
| Ok(Self { | ||
| api_key, | ||
| base_url: "https://api.search.brave.com/res/v1".to_string(), |
There was a problem hiding this comment.
Apply the same key normalization before persisting.
Like Exa config, whitespace-only is rejected, but leading/trailing whitespace on otherwise non-empty keys is still kept.
Proposed fix
pub fn from_env() -> Result<Self, std::env::VarError> {
- let api_key = std::env::var("BRAVE_API_KEY")?;
- if api_key.trim().is_empty() {
+ let api_key = std::env::var("BRAVE_API_KEY")?;
+ let api_key = api_key.trim();
+ if api_key.is_empty() {
return Err(std::env::VarError::NotPresent);
}
Ok(Self {
- api_key,
+ api_key: api_key.to_string(),
base_url: "https://api.search.brave.com/res/v1".to_string(),
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let api_key = std::env::var("BRAVE_API_KEY")?; | |
| if api_key.trim().is_empty() { | |
| return Err(std::env::VarError::NotPresent); | |
| } | |
| Ok(Self { | |
| api_key, | |
| base_url: "https://api.search.brave.com/res/v1".to_string(), | |
| let api_key = std::env::var("BRAVE_API_KEY")?; | |
| let api_key = api_key.trim(); | |
| if api_key.is_empty() { | |
| return Err(std::env::VarError::NotPresent); | |
| } | |
| Ok(Self { | |
| api_key: api_key.to_string(), | |
| base_url: "https://api.search.brave.com/res/v1".to_string(), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/providers/brave/config.rs` around lines 9 - 16, The code currently
rejects whitespace-only BRAVE_API_KEY but stores leading/trailing whitespace;
fetch the env var into api_key, trim it for emptiness check and then persist the
trimmed value (e.g., let api_key = api_key.trim().to_string()) before
constructing Self { api_key, base_url: ... } so the stored api_key is
normalized; update the api_key variable usage where the struct is built to use
the trimmed string.
Comment on lines
9
to
16
| let api_key = std::env::var("EXA_API_KEY")?; | ||
| if api_key.trim().is_empty() { | ||
| return Err(std::env::VarError::NotPresent); | ||
| } | ||
|
|
||
| Ok(Self { | ||
| api_key, | ||
| base_url: "https://api.exa.ai".to_string(), |
There was a problem hiding this comment.
Normalize API key before storing to avoid accidental auth failures.
You validate with trim() but persist the raw value. A key like "abc123\n" passes validation and will be sent with whitespace.
Proposed fix
pub fn from_env() -> Result<Self, std::env::VarError> {
- let api_key = std::env::var("EXA_API_KEY")?;
- if api_key.trim().is_empty() {
+ let api_key = std::env::var("EXA_API_KEY")?;
+ let api_key = api_key.trim();
+ if api_key.is_empty() {
return Err(std::env::VarError::NotPresent);
}
Ok(Self {
- api_key,
+ api_key: api_key.to_string(),
base_url: "https://api.exa.ai".to_string(),
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let api_key = std::env::var("EXA_API_KEY")?; | |
| if api_key.trim().is_empty() { | |
| return Err(std::env::VarError::NotPresent); | |
| } | |
| Ok(Self { | |
| api_key, | |
| base_url: "https://api.exa.ai".to_string(), | |
| let api_key = std::env::var("EXA_API_KEY")?; | |
| let api_key = api_key.trim(); | |
| if api_key.is_empty() { | |
| return Err(std::env::VarError::NotPresent); | |
| } | |
| Ok(Self { | |
| api_key: api_key.to_string(), | |
| base_url: "https://api.exa.ai".to_string(), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/providers/exa/config.rs` around lines 9 - 16, The API key is validated
with api_key.trim() but the raw value is stored, so trailing whitespace can
cause auth failures; update the code that constructs the config (the block
returning Ok(Self { api_key, ... })) to store a normalized key by trimming (and
optionally canonicalizing) the environment value before assignment, e.g.,
replace usage of the original api_key variable in the returned Self with the
trimmed/normalized value so the stored field is clean.
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
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
Adds a comprehensive integration test suite to address the missing "Integration Tests Exist" Agent Readiness signal.
Changes
Verification
running 27 tests
test bootstrap::provider_registry::tests::empty_registry_reports_provider_unavailable ... ok
test bootstrap::provider_registry::tests::available_providers_are_returned_in_stable_order ... ok
test bootstrap::provider_registry::tests::production_registry_only_includes_configured_providers ... ok
test bootstrap::provider_registry::tests::build_all_enabled_uses_stable_order_and_rejects_empty_registry ... ok
test app::search_service::tests::test_search_service_delegates ... ok
test app::fanout_search_service::tests::search_all_preserves_provider_order_and_records_failures ... ok
test bootstrap::provider_registry::tests::registered_provider_is_available_and_builds_service ... ok
test domain::result::tests::search_batch_response_can_hold_success_and_failure ... ok
test cli::output::tests::test_render_text_mixed_results ... ok
test providers::brave::mapper::tests::test_map_images_response ... ok
test cli::output::tests::test_render_fanout_text_includes_successes_and_failures ... ok
test providers::brave::mapper::tests::test_map_videos_response ... ok
test providers::brave::mapper::tests::test_map_news_response ... ok
test providers::brave::mapper::tests::test_map_web_response ... ok
test cli::args::tests::test_cli_provider_parses_exa_and_defaults_to_brave ... ok
test providers::exa::dto::tests::test_exa_search_response_deserializes_highlights_and_summary ... ok
test providers::exa::mapper::tests::test_map_news_response_prefers_summary_and_preserves_author ... ok
test cli::args::tests::test_cli_provider_parses_all_and_defaults_to_brave ... ok
test providers::exa::dto::tests::test_exa_search_response_deserializes_minimal_payload ... ok
test providers::exa::mapper::tests::test_map_no_concise_fields_returns_none_despite_huge_text ... ok
test providers::brave::client::tests::test_brave_provider_web_search ... ok
test providers::exa::mapper::tests::test_map_prefers_summary_over_highlights ... ok
test providers::exa::client::tests::test_exa_provider_news_search_posts_expected_payload ... ok
test providers::exa::mapper::tests::test_map_highlights_joined_and_capped ... ok
test providers::exa::mapper::tests::test_map_whitespace_summary_falls_through_to_highlights ... ok
test providers::exa::client::tests::test_exa_provider_rejects_unsupported_query_fields ... ok
test transport::http::tests::test_post_json_decodes_success_response ... ok
test result: ok. 27 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.11s
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 6 tests
test test_bootstrap_does_not_import_cli ... ok
test test_app_does_not_import_cli ... ok
test test_domain_does_not_import_outer_layers ... ok
test test_transport_does_not_import_higher_layers ... ok
test test_providers_do_not_import_cli_or_app ... ok
test test_render_text_only_called_from_cli ... ok
test result: ok. 6 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 6 tests
test cli_exa_provider_without_key_exits_with_unavailable_error ... ok
test cli_missing_query_exits_with_error ... ok
test cli_help_flag_prints_usage ... ok
test cli_with_explicit_arguments_parses_correctly ... ok
test cli_about_flag_prints_description ... ok
test cli_brave_provider_without_key_exits_with_unavailable_error ... ok
test result: ok. 6 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s
running 1 test
test provider_all_without_config_exits_nonzero_with_no_provider_error ... ok
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s
running 6 tests
test available_providers_returned_in_stable_order ... ok
test empty_registry_build_fails_with_provider_unavailable ... ok
test registered_provider_builds_service_that_searches ... ok
test registered_provider_is_available ... ok
test empty_registry_build_all_enabled_fails_with_no_providers ... ok
test build_all_enabled_uses_stable_order ... ok
test result: ok. 6 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s
running 5 tests
test fanout_service_records_failures_without_short_circuiting ... ok
test fanout_service_preserves_stable_order ... ok
test fanout_service_aggregates_multiple_providers ... ok
test search_service_routes_query_to_provider ... ok
test search_service_preserves_result_count ... ok
test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s passes (fmt, clippy, tests, docs build)
Agent Readiness Impact
Summary by CodeRabbit
Bug Fixes
Tests
Chores
Refactor