fix(test): schedule one-shot mocks by call index - #7098
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMock one-time implementations now use absolute call indices, support optional indexed scheduling with replacement, select implementations by invocation count, and preserve scheduled state during restoration. Mock APIs also accept accessor options and preserve function metadata. ChangesNode test mock behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TestContext
participant MockExport
participant MockHandler
participant MockState
participant MockInvocation
TestContext->>MockExport: decorate test export
MockExport->>MockHandler: pass implementation and options
MockHandler->>MockState: configure mock
MockInvocation->>MockState: read current call index
MockState-->>MockInvocation: return matching implementation
MockInvocation->>MockState: consume scheduled implementation
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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. 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-runtime/src/node_submodules/test.rs`:
- Around line 645-652: The mock dispatch flow around take_mock_implementation
and mock_state_call_count must track invocation order before implementation
execution, separately from completed state.calls records. Reserve and increment
an internal invocation index before dispatch, reset it wherever call history is
reset, and use it for onCall/default scheduling while preserving completed-call
recording. Add a regression test covering a recursively invoked mock and
verifying the scheduled implementations run on the correct invocation.
🪄 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 Plus
Run ID: ccf4dce5-918a-4c35-88d2-44b3fed5a20d
📒 Files selected for processing (3)
changelog.d/7098-node-test-mock-once-indices.mdcrates/perry-runtime/src/node_submodules/test.rscrates/perry-runtime/src/node_submodules/test_once_unit_tests.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/perry-runtime/src/node_submodules/test_once_unit_tests.rs`:
- Around line 69-87: Update
reentrant_dispatch_uses_completed_call_indices_like_node to exercise nested
dispatch through an outer mock implementation that recursively invokes the same
mock, rather than directly calling take_mock_implementation and manually setting
the count. Assert the onCall=1 entry remains pending after the outer and nested
invocations, then schedule another implementation using the actual completed
count and verify it is selected on the next invocation.
🪄 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 Plus
Run ID: bd0c1769-a635-416d-8240-61a0285e14ca
📒 Files selected for processing (1)
crates/perry-runtime/src/node_submodules/test_once_unit_tests.rs
5471c66 to
ed19d74
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/perry-runtime/src/node_submodules/test.rs (2)
676-699: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRoot the restored closure before any GC-capable call.
The only metadata helpers used here are static lookups, but
set_bound_native_closure_nameallocates aString, so reusingclosure_ptrafterwards is a stale native-stack pointer across GC candidates. Reloadclosure_ptrfromfunction.get_nanbox_f64()beforeset_builtin_closure_length,set_builtin_property_attrs, andclosure_set_dynamic_prop.🤖 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 `@crates/perry-runtime/src/node_submodules/test.rs` around lines 676 - 699, Update the closure setup in the mock-function initialization flow so the restored closure remains rooted across the allocation in set_bound_native_closure_name. Reload closure_ptr from function.get_nanbox_f64() after set_bound_native_closure_name and use that refreshed pointer for set_builtin_closure_length, set_builtin_property_attrs, and closure_set_dynamic_prop.Source: Learnings
545-551: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon’t normalize accessor args the same as methods.
normalize_mock_method_args()currently swaps wheneverimplementationis a non-callable pointer. Formock.getter()/mock.setter(), keep the second argument as options even when the implementation is an object/array/string; otherwisemock.getter(obj, 'p', {}, { getter: true })discards the explicit options and passes the non-function implementation blindly toassert_callable_arg("implementation", implementation).🤖 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 `@crates/perry-runtime/src/node_submodules/test.rs` around lines 545 - 551, The argument normalization in normalize_mock_method_args must distinguish accessor calls from method calls: preserve the explicit options argument for mock.getter/mock.setter even when implementation is an object, array, or string, while retaining the existing swap behavior for regular methods. Update the relevant caller or normalization flow so accessor implementations still reach assert_callable_arg("implementation", implementation) correctly without discarding accessor options.
🧹 Nitpick comments (1)
crates/perry-runtime/src/node_submodules/test.rs (1)
849-851: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
metadata_testsdeclaration with the other test modules.
test_metadata_unit_tests.rsexists, so this doesn’t block compilation; keep it grouped with the other#[cfg(test)] #[path = ...]declarations outside production code.🤖 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 `@crates/perry-runtime/src/node_submodules/test.rs` around lines 849 - 851, Move the metadata_tests module declaration, including its #[cfg(test)] and #[path = "test_metadata_unit_tests.rs"] attributes, out of the production-code section and group it with the other test module declarations in the file. Preserve the module name and path unchanged.
🤖 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.
Outside diff comments:
In `@crates/perry-runtime/src/node_submodules/test.rs`:
- Around line 676-699: Update the closure setup in the mock-function
initialization flow so the restored closure remains rooted across the allocation
in set_bound_native_closure_name. Reload closure_ptr from
function.get_nanbox_f64() after set_bound_native_closure_name and use that
refreshed pointer for set_builtin_closure_length, set_builtin_property_attrs,
and closure_set_dynamic_prop.
- Around line 545-551: The argument normalization in normalize_mock_method_args
must distinguish accessor calls from method calls: preserve the explicit options
argument for mock.getter/mock.setter even when implementation is an object,
array, or string, while retaining the existing swap behavior for regular
methods. Update the relevant caller or normalization flow so accessor
implementations still reach assert_callable_arg("implementation",
implementation) correctly without discarding accessor options.
---
Nitpick comments:
In `@crates/perry-runtime/src/node_submodules/test.rs`:
- Around line 849-851: Move the metadata_tests module declaration, including its
#[cfg(test)] and #[path = "test_metadata_unit_tests.rs"] attributes, out of the
production-code section and group it with the other test module declarations in
the file. Preserve the module name and path unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e97ba8ea-12bf-42fb-8f71-3dc5bde54553
📒 Files selected for processing (3)
changelog.d/7098-node-test-mock-once-indices.mdcrates/perry-runtime/src/node_submodules/test.rscrates/perry-runtime/src/node_submodules/test_once_unit_tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- changelog.d/7098-node-test-mock-once-indices.md
1 participant
Summary
mockImplementationOncereplacements by absolute call index instead of FIFO orderonCallargument, with duplicate indices replacing the prior entry like Node'sMap.mock.restore()restores the underlying implementationThis clears two more failures in #6767:
mock-fn/implementationsandmock-fn/once-indices.Validation
cargo fmt --all -- --checkcargo check -p perry-runtimecargo test -p perry-runtime default_once_scheduling_overwrites_the_current_call_index -- --nocapturecargo test -p perry-runtime indexed_once_scheduling_uses_absolute_call_indices -- --nocapturecargo test -p perry-runtime restoring_a_mock_preserves_calls_and_scheduled_implementations -- --nocapture./run_parity_tests.sh --suite node-suite --module test --filter node-suite/test/mock-fn/implementations(1/1 pass, 100%)./run_parity_tests.sh --suite node-suite --module test --filter node-suite/test/mock-fn/once-indices(1/1 pass, 100%)git diff --checkRefs #6767
Summary by CodeRabbit
New Features
testaliases.Bug Fixes
Tests