chore(minikupo): Add comprehensive tests#939
Merged
Conversation
|
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 due to trivial changes (1)
📝 WalkthroughWalkthroughAdds extensive test suites and test-support utilities across minibf and minikupo, extends synthetic test vectors with datum/script fixtures, exposes TestApp/TestDomainBuilder helpers for HTTP testing and fault injection, and makes a single config field public. No runtime API logic was changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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)
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/minibf/src/routes/scripts.rs (2)
172-182: Add a same-length non-hex case for the parser decode branch.
invalid_script_hash()andinvalid_datum_hash()are only invalid because of length, so the new*_invalid_hashtests never exercise thehex::decodefailure path inparse_script_hash/parse_datum_hash. Please add a 56/64-character non-hex case as well, otherwise a regression there can slip through while these tests still pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/minibf/src/routes/scripts.rs` around lines 172 - 182, Tests only cover length-invalid hashes; add same-length non-hex variants so the hex::decode failure path in parse_script_hash and parse_datum_hash is exercised: add functions (e.g. invalid_script_hash_nonhex returning a 56-char non-hex string like repeated 'g' and invalid_datum_hash_nonhex returning a 64-char non-hex string) and update the *_invalid_hash tests to include/replace a case using these new functions so parse_script_hash and parse_datum_hash hit the hex decode error branch.
198-286: Plutus script responses are still untested.Line 214 locks the shared fixture to
ScriptType::Timelock, so the happy-path coverage here only exercises the native-script branch. That leaves the materially different Plutus behavior unverified for/scripts/{hash},/scripts/{hash}/json, and/scripts/{hash}/cbor(serialised_size = Some(_),json = None,cbor = Some(_)). Adding one Plutus fixture would close that gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/minibf/src/routes/scripts.rs` around lines 198 - 286, Add a new test that mirrors scripts_by_hash_happy_path / scripts_by_hash_json_happy_path / scripts_by_hash_cbor_happy_path but uses a Plutus-backed fixture (e.g. fixture_app_plutus() or create a TestApp whose vectors contain a Plutus script) and assert the Plutus-specific expectations: for scripts_by_hash assert item.r#type == ScriptType::Plutus and item.serialised_size.is_some(); for /json assert ScriptJson.json.is_none(); for /cbor assert ScriptCbor.cbor.is_some(); reuse the same path formatting and serde parsing as the existing tests (Script, ScriptJson, ScriptCbor) so the Plutus branch is covered.crates/testing/src/toy_domain.rs (1)
266-271: Consider preserving the original CardanoConfig during reinitialization.When reinitializing
CardanoLogic,CardanoConfig::default()is used instead of the config that was originally passed toToyDomain::new_with_genesis_and_config. If tests use non-default configurations, this could lead to subtle inconsistencies.♻️ Suggested fix to preserve config
You could store the
CardanoConfiginToyDomainand use it here:let new_chain = dolos_cardano::CardanoLogic::initialize::<ToyDomain>( - CardanoConfig::default(), + domain.cardano_config().clone(), domain.state(), domain.genesis.as_ref(), )This would require adding a
cardano_configfield and accessor toToyDomain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/testing/src/toy_domain.rs` around lines 266 - 271, The reinitialization call uses CardanoConfig::default() which discards any non-default config passed to ToyDomain::new_with_genesis_and_config; update ToyDomain to store the original CardanoConfig (add a cardano_config field and a getter, e.g., cardano_config()) and change the CardanoLogic::initialize call to pass domain.cardano_config() (or the accessor) instead of CardanoConfig::default() so the original configuration is preserved during reinitialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/minibf/src/routes/scripts.rs`:
- Around line 172-182: Tests only cover length-invalid hashes; add same-length
non-hex variants so the hex::decode failure path in parse_script_hash and
parse_datum_hash is exercised: add functions (e.g. invalid_script_hash_nonhex
returning a 56-char non-hex string like repeated 'g' and
invalid_datum_hash_nonhex returning a 64-char non-hex string) and update the
*_invalid_hash tests to include/replace a case using these new functions so
parse_script_hash and parse_datum_hash hit the hex decode error branch.
- Around line 198-286: Add a new test that mirrors scripts_by_hash_happy_path /
scripts_by_hash_json_happy_path / scripts_by_hash_cbor_happy_path but uses a
Plutus-backed fixture (e.g. fixture_app_plutus() or create a TestApp whose
vectors contain a Plutus script) and assert the Plutus-specific expectations:
for scripts_by_hash assert item.r#type == ScriptType::Plutus and
item.serialised_size.is_some(); for /json assert ScriptJson.json.is_none(); for
/cbor assert ScriptCbor.cbor.is_some(); reuse the same path formatting and serde
parsing as the existing tests (Script, ScriptJson, ScriptCbor) so the Plutus
branch is covered.
In `@crates/testing/src/toy_domain.rs`:
- Around line 266-271: The reinitialization call uses CardanoConfig::default()
which discards any non-default config passed to
ToyDomain::new_with_genesis_and_config; update ToyDomain to store the original
CardanoConfig (add a cardano_config field and a getter, e.g., cardano_config())
and change the CardanoLogic::initialize call to pass domain.cardano_config() (or
the accessor) instead of CardanoConfig::default() so the original configuration
is preserved during reinitialization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f91bc41-ce3d-4ac0-8006-176692eb71a0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
crates/minibf/src/routes/scripts.rscrates/minibf/src/test_support.rscrates/minikupo/Cargo.tomlcrates/minikupo/src/lib.rscrates/minikupo/src/routes/datums.rscrates/minikupo/src/routes/health.rscrates/minikupo/src/routes/matches.rscrates/minikupo/src/routes/metadata.rscrates/minikupo/src/routes/scripts.rscrates/minikupo/src/test_support.rscrates/testing/src/synthetic.rscrates/testing/src/toy_domain.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/minikupo/src/test_support.rs (1)
126-144: Consider simplifying the fault handling logic.The
matchstatement can be simplified since both branches constructFaultyToyDomain::newwith different fault values. Usingunwrap_or_default()leverages the#[default]derive onTestFault::None.♻️ Suggested simplification
pub fn new_with_cfg_and_fault(cfg: SyntheticBlockConfig, fault: Option<TestFault>) -> Self { let (domain, vectors) = TestDomainBuilder::new_with_synthetic(cfg).finish(); - let domain = match fault { - Some(fault) => dolos_testing::faults::FaultyToyDomain::new(domain, fault), - None => dolos_testing::faults::FaultyToyDomain::new(domain, TestFault::None), - }; + let domain = dolos_testing::faults::FaultyToyDomain::new( + domain, + fault.unwrap_or_default(), + ); let router = build_router_with_facade(Facade {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/minikupo/src/test_support.rs` around lines 126 - 144, The match in new_with_cfg_and_fault can be simplified: replace the explicit match on fault with using fault.unwrap_or_default() (leveraging TestFault's Default/#[default] None) and pass that to dolos_testing::faults::FaultyToyDomain::new(domain, ...); update references in that function (new_with_cfg_and_fault, TestFault, and FaultyToyDomain::new) accordingly so there is a single call to FaultyToyDomain::new with the unwrapped/defaulted TestFault.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/minikupo/src/test_support.rs`:
- Around line 126-144: The match in new_with_cfg_and_fault can be simplified:
replace the explicit match on fault with using fault.unwrap_or_default()
(leveraging TestFault's Default/#[default] None) and pass that to
dolos_testing::faults::FaultyToyDomain::new(domain, ...); update references in
that function (new_with_cfg_and_fault, TestFault, and FaultyToyDomain::new)
accordingly so there is a single call to FaultyToyDomain::new with the
unwrapped/defaulted TestFault.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98d74874-2268-487b-8433-011eff5299c5
📒 Files selected for processing (1)
crates/minikupo/src/test_support.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/core/src/config.rs (1)
756-757: Avoid wideningMinikupoConfigAPI just for tests.Line 757 exposes
permissive_corspublicly, but test setup can use the existing constructor without opening this field to external mutation.Suggested change
diff --git a/crates/core/src/config.rs b/crates/core/src/config.rs @@ pub struct MinikupoConfig { pub listen_address: SocketAddr, #[serde(default, skip_serializing_if = "Option::is_none")] - pub permissive_cors: Option<bool>, + permissive_cors: Option<bool>, }diff --git a/crates/minikupo/src/test_support.rs b/crates/minikupo/src/test_support.rs @@ fn test_config() -> MinikupoConfig { - MinikupoConfig { - listen_address: "[::]:0".parse().expect("invalid listen address"), - permissive_cors: None, - } + MinikupoConfig::new("[::]:0".parse().expect("invalid listen address")) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/core/src/config.rs` around lines 756 - 757, The field permissive_cors on MinikupoConfig is unnecessarily public for tests; make it private by removing the pub qualifier (change `pub permissive_cors: Option<bool>` to `permissive_cors: Option<bool>`) so the API surface isn't widened, and keep the existing serde attributes; ensure tests use the existing MinikupoConfig constructor or factory instead of accessing this field directly and update any internal references to access the field via the constructor or existing methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/core/src/config.rs`:
- Around line 756-757: The field permissive_cors on MinikupoConfig is
unnecessarily public for tests; make it private by removing the pub qualifier
(change `pub permissive_cors: Option<bool>` to `permissive_cors: Option<bool>`)
so the API surface isn't widened, and keep the existing serde attributes; ensure
tests use the existing MinikupoConfig constructor or factory instead of
accessing this field directly and update any internal references to access the
field via the constructor or existing methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31c29d4b-dacc-447d-93ae-0f4c94e2c63f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/cardano/src/model/epochs.rscrates/core/src/config.rscrates/minikupo/src/lib.rscrates/minikupo/src/test_support.rscrates/testing/src/synthetic.rs
✅ Files skipped from review due to trivial changes (2)
- crates/minikupo/src/lib.rs
- crates/cardano/src/model/epochs.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/testing/src/synthetic.rs
scarmuega
approved these changes
Apr 30, 2026
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
2 participants
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 by CodeRabbit
Tests
Chores
Refactor