- 
                Notifications
    
You must be signed in to change notification settings  - Fork 239
 
feat: mock_call with dynamic return data #2904
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
0a08540    to
    d75e50b      
    Compare
  
    Add `mock_call_when`, `start_mock_call_when` and `stop_mock_call_when`
…:TargetCalls is 0
89e963b    to
    02a7dcd      
    Compare
  
    | let key_zero = (call.entry_point_selector, Felt::zero()); | ||
| 
               | 
          ||
| match contract_functions.get(&key) { | ||
| Some(CheatStatus::Cheated(_, CheatSpan::TargetCalls(0))) => { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After calling get_mocked_function_cheat_status, we call .decrement_cheat_span on the returned value. It changes the CheatSpan to Uncheated after it decreases to 0.
I don't think there can be a case where TargetCalls is equal to 0 unless explicitly set to this value somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this logic match arm was added to handle the case where entrypoint for specific calldata is no longer cheated but for any calldata still is, but it should already be handled by _ case anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out technically it could be set to 0 but it shouldn't be allowed.
Created #2927
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure to understand what should I do here: remove my workaround?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't have to match for the case where cheat span is CheatSpan::TargetCalls(0). Technically, user could have somehow created it like this (as explained in #2927), but during normal operation it will either be CheatSpan::TargetCalls(some value > 0) or CheatStatus::Uncheated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See this
starknet-foundry/crates/cheatnet/src/state.rs
Line 141 in f2e7d0d
| pub fn decrement_cheat_span(&mut self) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Workaround removed
        
          
                crates/cheatnet/src/runtime_extensions/forge_runtime_extension/cheatcodes/mock_call.rs
              
                Outdated
          
            Show resolved
            Hide resolved
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also have test cases where mock_call_when is mixed with mock_call, these are the ones most likely to break/have some weird edge cases.
- rename cairo MockCallData enum into MockCalldata - use Serde derivation for MockCalldata instead of custom serializer.
| } | ||
| 
               | 
          ||
| #[test] | ||
| fn mock_calls_when_mixed() { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some cases where the same tests uses both mock_call and mock_call_when?
E.g. in case where you do start_mock_call_when(MockCalldata::Any, ...) and then stop_mock_call(...) and similar?
| 
           Hi! This pull request hasn't had any activity for a while, so I am  | 
    
| 
           Hey @ptisserand 👋 , are you going to continue working on this PR?  | 
    
          
 Hi @franciszekjob, sorry I completely forgot about this PR when working on other projects :(  | 
    
| 
           @ptisserand sure thing, no hurry!  | 
    
| 
           Hi @ptisserand 👋 , kindly asking if there's any update on this?  | 
    
| 
           Hey @ptisserand is this ready for re-review?  | 
    
          
 Not yet, I have only managed conflicts with master branch.  | 
    
| 
           Hi! This pull request hasn't had any activity for a while, so I am  | 
    
| 
           Hi @ptisserand 👋 , any update on this?  | 
    
| 
           Hi @franciszekjob, I have added some tests cases.  | 
    
| 
           Hi @cptartur @franciszekjob, could you please review this PR ?  | 
    
| ### `stop_mock_call_when` | ||
| 
               | 
          ||
| > `fn stop_mock_call_when(contract_address: ContractAddress, function_selector: felt252, calldata: MockCalldata)` | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ### `stop_mock_call_when` | |
| > `fn stop_mock_call_when(contract_address: ContractAddress, function_selector: felt252, calldata: MockCalldata)` | |
| ## `stop_mock_call_when` | |
| > `fn stop_mock_call_when(contract_address: ContractAddress, function_selector: felt252, calldata: MockCalldata)` | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
        
          
                CHANGELOG.md
              
                Outdated
          
        
      | 
               | 
          ||
| - Rust is no longer required to use `snforge` if using Scarb >= 2.10.0 on supported platforms - precompiled `snforge_scarb_plugin` plugin binaries are now published to [package registry](https://scarbs.xyz) for new versions. | ||
| - Added a suggestion for using the `--max-n-steps` flag when the Cairo VM returns the error: `Could not reach the end of the program. RunResources has no remaining steps`. | ||
| - `mock_call_when`, `start_mock_call_when`, `stop_mock_call_when` cheatcodes. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be under unreleased version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| @@ -0,0 +1,43 @@ | |||
| # `mock_call_when` | |||
| 
               | 
          |||
| Cheatcodes mocking contract entry point calls: | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the same as description of mock_call. Maybe let's emphasize the difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated with Cheatcodes mocking contract entry point calls based on calldata:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be really nice to add an example of usage, similarly how it's done here 😄
You can create and issue for that, link it here and if you have time open - a separate PR.
Closes #2861
Introduced changes
mock_call_when,start_mock_call_whenandstop_mock_call_whento support "dynamic" return value when mocking a contract entry point.Checklist
CHANGELOG.md