refactor: Replace wiremock for mockito#436
Conversation
WalkthroughMigrated test suites from WireMock to Mockito across multiple modules, updated async mock server creation and mock lifetimes, adapted request/response/header/json matching, and replaced usages of Changes
Sequence Diagram(s)(omitted — changes are test harness/mocking migrations and do not alter runtime control flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/services/jupiter/mod.rs (1)
274-286: Bug: wrong slippage basis points calculation (loss of precision).
(request.slippage as u32) * 100truncates before multiplying (e.g., 0.5 → 0). Convert first, then cast.- let slippage_bps: u32 = request.slippage as u32 * 100; + // Convert percent (e.g., 0.5%) to bps (50) without truncation + let slippage_bps: u32 = (request.slippage * 100.0).round() as u32;src/services/turnkey/mod.rs (1)
481-505: Critical: infinite recursion and potential ambiguity in trait impl methods.
sign_evm_transactioncalls itself, causing infinite recursion.address_solana/address_evmmay resolve ambiguously; use fully-qualified calls to the inherent methods.Fix by explicitly calling inherent methods.
impl TurnkeyServiceTrait for TurnkeyService { fn address_solana(&self) -> Result<Address, TurnkeyError> { - self.address_solana() + TurnkeyService::address_solana(self) } fn address_evm(&self) -> Result<Address, TurnkeyError> { - self.address_evm() + TurnkeyService::address_evm(self) } @@ async fn sign_evm_transaction(&self, message: &[u8]) -> Result<Vec<u8>, TurnkeyError> { - let signature_bytes = self.sign_evm_transaction(message).await?; - Ok(signature_bytes) + TurnkeyService::sign_evm_transaction(self, message).await }src/services/notification/mod.rs (1)
36-52: Fix infinite recursion in trait impl by calling inherent methods explicitlyBoth trait methods call themselves recursively, leading to stack overflow at runtime.
impl WebhookNotificationServiceTrait for WebhookNotificationService { async fn send_notification( &self, notification: WebhookNotification, ) -> Result<WebhookResponse, WebhookNotificationError> { - self.send_notification(notification).await + WebhookNotificationService::send_notification(self, notification).await } fn sign_payload( &self, payload: &str, secret_key: &SecretString, ) -> Result<String, WebhookNotificationError> { - self.sign_payload(payload, secret_key) + WebhookNotificationService::sign_payload(self, payload, secret_key) } }
🧹 Nitpick comments (25)
src/services/provider/mod.rs (4)
278-279: Remove unuseduse mockito;or import specific items.You already refer to items as
mockito::...; thisuseis redundant and may trigger an unused import warning. Prefer importing the symbols you use (e.g.,Matcher) or drop this line.- use mockito; + // use mockito::{Matcher}; // If you want shorter paths
382-389: Mockito migration for 429 case is correct; consider asserting call count.The matcher and status mapping are aligned with
categorize_reqwest_error. Add.expect(1)to ensure the mock was hit exactly once.- let _mock = mock_server - .mock("GET", mockito::Matcher::Any) + let _mock = mock_server + .mock("GET", mockito::Matcher::Any) + .expect(1) .with_status(429)Also applies to: 392-396
410-417: Mockito migration for 502 case is correct; add.expect(1)for robustness.Same rationale as the 429 test to avoid false positives.
- let _mock = mock_server - .mock("GET", mockito::Matcher::Any) + let _mock = mock_server + .mock("GET", mockito::Matcher::Any) + .expect(1) .with_status(502)Also applies to: 420-424
364-379: Unify async test runtime to Tokio for consistency.These tests don’t use Actix runtime features. Consider switching
#[actix_rt::test]to#[tokio::test](as in other modules) to keep a single runtime in tests.- #[actix_rt::test] + #[tokio::test]src/services/signer/evm/google_cloud_kms_signer.rs (3)
171-175: Base URL wiring is good; ensure OAuth token endpoint isn’t hit in tests.If
GoogleCloudKmsServicefetches OAuth tokens viatoken_uri, your tests may perform external network calls. Recommend stubbing the token endpoint and pointingtoken_uriat the mock server.- use mockito::{Mock, ServerGuard}; + use mockito::{Mock, ServerGuard}; @@ - async fn setup_mock_gcp_signer(mock_server: &ServerGuard) -> GoogleCloudKmsSigner { - let base_url = mock_server.url(); + async fn setup_mock_gcp_signer(mock_server: &mut ServerGuard) -> GoogleCloudKmsSigner { + let base_url = mock_server.url(); + // Stub OAuth token exchange if the service requests it + let _token_mock = mock_server + .mock("POST", "/token") + .expect(1) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"access_token":"test","expires_in":3600,"token_type":"Bearer"}"#) + .create_async() + .await; @@ - token_uri: "https://oauth2.googleapis.com/token".to_string(), + token_uri: format!("{}/token", base_url),Update callers:
- let signer = setup_mock_gcp_signer(&mock_server).await; + let signer = setup_mock_gcp_signer(&mut mock_server).await;
206-226: Good WireMock → Mockito migration; add call count assertion.Add
.expect(1)to catch accidental extra requests.- let _mock = mock_server + let _mock = mock_server .mock( "GET", mockito::Matcher::Regex( r"/v1/projects/.*/locations/.*/keyRings/.*/cryptoKeys/.*/cryptoKeyVersions/.*" .to_string(), ), ) .match_header("authorization", mockito::Matcher::Any) + .expect(1) .with_status(200)
258-277: Mocks look correct; add.expect(1)to each for stronger guarantees.Ensures exactly one call to public-key and sign endpoints.
- let _mock_pubkey = mock_server + let _mock_pubkey = mock_server .mock( "GET", mockito::Matcher::Regex( r"/v1/projects/.*/locations/.*/keyRings/.*/cryptoKeys/.*/cryptoKeyVersions/.*" .to_string(), ), ) .match_header("authorization", mockito::Matcher::Any) + .expect(1) .with_status(200) @@ - let _mock_sign = mock_server + let _mock_sign = mock_server .mock( "POST", mockito::Matcher::Regex( r"/v1/projects/.*/locations/.*/keyRings/.*/cryptoKeys/.*:asymmetricSign" .to_string(), ), ) .match_header("authorization", mockito::Matcher::Any) + .expect(1) .with_status(200)Also applies to: 293-311
src/services/jupiter/mod.rs (4)
732-783: Query matchers migrated correctly; consider asserting call counts.Optional: add
.expect(1)to ensure single invocation.- let _mock = mock_server + let _mock = mock_server .mock("GET", "/ultra/v1/order") .match_query(mockito::Matcher::AllOf(vec![ /* ... */ ])) + .expect(1) .with_status(200)
824-835: Execute endpoint mock OK; add.expect(1)for determinism.- let _mock = mock_server + let _mock = mock_server .mock("POST", "/ultra/v1/execute") + .expect(1) .with_status(200)
851-866: Minor: fix test failure message to match expectation.The test expects
HttpRequestErrorbut the panic says "Expected ApiError...".- _ => panic!("Expected ApiError but got different error type"), + _ => panic!("Expected HttpRequestError but got different error type"),Also applies to: 879-885
586-631: Optional: avoid external calls in quote tests.These tests hit real endpoints. Consider switching to Mockito like other tests or mark them
#[ignore]to avoid flakiness.src/services/turnkey/mod.rs (3)
635-657: Raw payload mock migration looks good; assert call count.- mock_server + mock_server .mock("POST", "/public/v1/submit/sign_raw_payload") + .expect(1) .match_header("Content-Type", "application/json")
660-685: EVM transaction mock migration looks good; add.expect(1).- mock_server + mock_server .mock("POST", "/public/v1/submit/sign_transaction") + .expect(1) .match_header("Content-Type", "application/json")
688-706: Error response mock is correct; add.expect(1)for strictness.- mock_server + mock_server .mock("POST", "/public/v1/submit/sign_raw_payload") + .expect(1) .match_header("Content-Type", "application/json")src/services/notification/mod.rs (4)
165-179: Strengthen the assertion: verify exact HMAC, not just header presenceUse an exact match for X-Signature to validate signing logic end-to-end. Compute the expected signature from the same payload used in the request.
- .match_header("X-Signature", mockito::Matcher::Any) + .match_header("X-Signature", expected_signature.as_str())Add before creating the mock (outside this hunk):
let secret_key = SecretString::new("test_secret"); let notification = WebhookNotification { /* ...same as below... */ }; let payload = serde_json::to_string(¬ification).unwrap(); let expected_signature = WebhookNotificationService::new("".into(), None) .sign_payload(&payload, &secret_key) .unwrap();
182-182: Avoid unnecessary clone of SecretString- let service = WebhookNotificationService::new(mock_server.url(), Some(secret_key.clone())); + let service = WebhookNotificationService::new(mock_server.url(), Some(secret_key));
197-213: Test name is misleading; it succeeds without signatureRename to reflect expected outcome.
- async fn test_failed_notification_without_signature() { + async fn test_successful_notification_without_signature() {
227-241: Assert error message content to tighten failure checkSince send_notification returns WebhookError(response_text), assert it contains "Internal Server Error".
- assert!(result.is_err()); + assert!(result.is_err()); + let err = format!("{}", result.err().unwrap()); + assert!(err.contains("Internal Server Error"));src/services/google_cloud_kms/mod.rs (3)
328-337: Replace println!/print! with log macrosUse debug! for non-essential output to keep production logs clean.
- println!("PEM solana: {}", pem_str); + debug!("PEM solana: {}", pem_str);
336-337: Replace remaining println!/print! with debug!- println!("PEM evm: {}", pem_str); + debug!("PEM evm: {}", pem_str); ... - print!("KMS asymmetricSign body: {}", body); + debug!("KMS asymmetricSign body: {}", body); ... - println!("KMS asymmetricSign response: {}", resp); + debug!("KMS asymmetricSign response: {}", resp); ... - print!("KMS asymmetricSign body: {}", body); + debug!("KMS asymmetricSign body: {}", body); ... - println!("KMS asymmetricSign response: {}", resp); + debug!("KMS asymmetricSign response: {}", resp); - print!("Signature b64 decoded: {:?}", signature_b64); + debug!("Signature b64 decoded: {:?}", signature_b64);Also applies to: 355-356, 363-364, 395-399
586-596: Optional: assert mocks were hit to prevent false positivesCall _mock.assert() at the end of each test to ensure the expected request occurred.
Also applies to: 662-673, 740-751, 753-767, 769-781, 783-798, 858-869, 883-896
src/bootstrap/config_processor.rs (1)
528-571: Good helper; consider deduplicating across test modulesThis AppRole login mock appears in multiple modules (also in src/services/vault/mod.rs). Move to a shared test util to reduce duplication.
src/services/vault/mod.rs (3)
368-411: Consolidate duplicated AppRole mock helperSame helper exists here and in config_processor tests. Centralize in a test utils module to avoid divergence.
415-455: Remove unused secret mock in auth-failure testLogin is expected to fail; the secret GET mock is unnecessary and adds noise.
- let _secret_mock = mock_server - .mock("GET", "/v1/test-mount/data/my-secret") - .match_header("X-Vault-Token", "test-token") - .with_status(200) - .with_header("content-type", "application/json") - .with_body(serde_json::to_string(&json!({ /* ... */ })).unwrap(),) - .create_async() - .await;
479-537: Optionally assert mock expectations to tighten testsCall .assert() on created mocks to ensure requests were actually issued.
Also applies to: 541-595, 597-645, 648-696
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.toml(1 hunks)src/bootstrap/config_processor.rs(3 hunks)src/services/google_cloud_kms/mod.rs(16 hunks)src/services/jupiter/mod.rs(8 hunks)src/services/notification/mod.rs(4 hunks)src/services/provider/mod.rs(3 hunks)src/services/signer/evm/google_cloud_kms_signer.rs(5 hunks)src/services/turnkey/mod.rs(11 hunks)src/services/vault/mod.rs(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/bootstrap/config_processor.rs (1)
src/services/vault/mod.rs (1)
setup_mock_approle_login(369-411)
src/services/vault/mod.rs (2)
src/bootstrap/config_processor.rs (1)
setup_mock_approle_login(529-571)src/utils/base64.rs (1)
base64_encode(5-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test
- GitHub Check: clippy
- GitHub Check: msrv
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: Redirect rules - openzeppelin-relayer
- GitHub Check: Header rules - openzeppelin-relayer
- GitHub Check: Pages changed - openzeppelin-relayer
- GitHub Check: Analyze (rust)
🔇 Additional comments (5)
Cargo.toml (1)
91-99: No remaining WireMock references — code changes are safe to merge
- Ran
rg -nP '\bwiremock\b'across the repository (excludingtarget/) and within allCargo.*files; no occurrences were found.src/services/jupiter/mod.rs (1)
662-705: Mockito migration for swap endpoint looks solid.Stubbing, headers, and base_url injection are correct.
Also applies to: 706-710
src/services/turnkey/mod.rs (1)
718-732: End-to-end tests: base_url injection and client setup look good.Nice isolation via Mockito and custom client (no redirects). No issues spotted.
Also applies to: 741-753, 763-775, 787-801
src/services/google_cloud_kms/mod.rs (1)
494-507: Regex path matcher usage is supported in mockito v1.7.0We’ve confirmed that the project is on mockito 1.7.0 (Cargo.lock shows version “1.7.0”) (lambdatest.com), and the
Server::mock<P: Into<Matcher>>API explicitly acceptsMatcher::Regexfor the path parameter (see the docs.rs example usingmockito::Matcher::Regexins.mock("GET", mockito::Matcher::Regex(...))) (docs.rs). No changes are required here.src/bootstrap/config_processor.rs (1)
575-612: Mockito migration looks correct; nice use of mock_server.url()Mocks are kept alive via bindings; address uses server.url(); headers and bodies match expected vaultrs calls.
Also applies to: 613-627
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #436 +/- ##
=======================================
+ Coverage 92.7% 93.0% +0.2%
=======================================
Files 217 217
Lines 73295 74164 +869
=======================================
+ Hits 67992 68994 +1002
+ Misses 5303 5170 -133
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/services/provider/mod.rs (1)
381-387: Tighten matchers and assert mocks were hitUsing Matcher::Any for the path is broad; prefer a precise path to avoid accidental matches. Also assert the mock was exercised to catch false positives.
Apply:
- let _mock = mock_server - .mock("GET", mockito::Matcher::Any) + let mock = mock_server + .mock("GET", "/") .with_status(429) .create_async() .await; ... - let _mock = mock_server - .mock("GET", mockito::Matcher::Any) + let mock = mock_server + .mock("GET", "/") .with_status(502) .create_async() .await;And after exercising the request in each test:
assert!(matches!(provider_error, ProviderError::RateLimited)); + mock.assert();assert!(matches!(provider_error, ProviderError::BadGateway)); + mock.assert();Also applies to: 409-415
src/services/google_cloud_kms/mod.rs (5)
329-329: Avoid logging full PEM material; log a fingerprintReduce log verbosity and risk by logging a stable fingerprint instead of the entire PEM.
- debug!("PEM solana: {}", pem_str); + debug!("PEM solana fingerprint (sha256): {}", hex::encode(Sha256::digest(pem_str.as_bytes())));- debug!("PEM evm: {}", pem_str); + debug!("PEM evm fingerprint (sha256): {}", hex::encode(Sha256::digest(pem_str.as_bytes())));Also applies to: 336-336
364-364: Unify logging; avoid print! in library codeYou switched several println! to debug!, but print! calls remain for request bodies. Use debug!/trace! consistently.
Within the same methods (change outside this hunk):
- print!("KMS asymmetricSign body: {}", body); + debug!("KMS asymmetricSign body: {}", body);- print!("KMS asymmetricSign body: {}", body); + debug!("KMS asymmetricSign body: {}", body);Also applies to: 395-399
495-507: Strengthen header matching in mocksMatch the Authorization header format, not Any, to catch regressions in test-only auth header wiring.
- .match_header("Authorization", mockito::Matcher::Any) + .match_header("Authorization", mockito::Matcher::Regex(r"^Bearer .+$".to_string()))Apply to all helper setups that currently use Matcher::Any for Authorization.
Also applies to: 509-521, 553-568, 570-582
523-534: Validate request body shape in sign mocksEnsure tests fail if the client sends an unexpected payload.
For sign success:
mock_server .mock("POST", mockito::Matcher::Regex(r"/v1/projects/.*/locations/global/keyRings/.*/cryptoKeys/.*/cryptoKeyVersions/.*:asymmetricSign".to_string())) .match_header("Authorization", mockito::Matcher::Any) + .match_body(mockito::Matcher::Regex(r#""(digest|data)":"#.to_string())) .with_status(200)For sign error:
mock_server .mock("POST", mockito::Matcher::Regex(r"/v1/projects/.*/locations/global/keyRings/.*/cryptoKeys/.*/cryptoKeyVersions/.*:asymmetricSign".to_string())) .match_header("Authorization", mockito::Matcher::Any) + .match_body(mockito::Matcher::Regex(r#""(digest|data)":"#.to_string())) .with_status(400)Also applies to: 536-551
587-591: Assert mocks were exercisedRename _mock to mock and assert after the call. Prevents tests from passing when no HTTP call is made.
Example (replicate across tests):
- let _mock = setup_mock_solana_public_key(&mut mock_server).await; + let mock = setup_mock_solana_public_key(&mut mock_server).await; ... assert_eq!( result.unwrap(), "6s7RsvzcdXFJi1tXeDoGfSKZWjCDNJLiu74rd72zLy6J" ); + mock.assert();Also applies to: 603-607, 619-623, 635-639, 649-653, 665-669, 695-699, 712-716, 742-746, 755-759, 771-775, 786-790, 859-871, 883-897
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/services/google_cloud_kms/mod.rs(19 hunks)src/services/jupiter/mod.rs(9 hunks)src/services/notification/mod.rs(4 hunks)src/services/provider/mod.rs(2 hunks)src/services/signer/evm/google_cloud_kms_signer.rs(5 hunks)src/services/turnkey/mod.rs(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/services/turnkey/mod.rs
- src/services/jupiter/mod.rs
- src/services/signer/evm/google_cloud_kms_signer.rs
- src/services/notification/mod.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/services/google_cloud_kms/mod.rs (2)
src/utils/address_derivation.rs (1)
derive_solana_address_from_pem(40-60)src/utils/base64.rs (1)
base64_decode(8-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: test
- GitHub Check: msrv
- GitHub Check: clippy
- GitHub Check: Redirect rules - openzeppelin-relayer
- GitHub Check: Header rules - openzeppelin-relayer
- GitHub Check: Pages changed - openzeppelin-relayer
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (rust)
🔇 Additional comments (1)
src/services/provider/mod.rs (1)
391-395: LGTM: mockito base URL usageget(mock_server.url()) is correct with ServerGuard. No issues.
Also applies to: 419-423
2 participants
Summary
Since
wiremockMSRV is1.88.0LukeMathWalker/wiremock-rs#173, I've replaced it withmockitothat is already in our deps.Summary by CodeRabbit
Tests
Chores