feat: unit test external call mock #14
Conversation
qalisander
changed the title
qalisander
changed the title
qalisander
force-pushed
the
unit-tests/multiple-contract-deployment
branch
from
3838020 to
feb635a
Compare
qalisander
force-pushed
the
unit-tests/multiple-contract-deployment
branch
from
85ce803 to
1ffba32
Compare
…ontract-deployment # Conflicts: # Cargo.lock # Cargo.toml # crates/motsu/README.md
|
Update documentation for the new test case structure |
qalisander
requested review from
bidzyyys,
0xNeshi and
ggonzalez94
as code owners
| @@ -15,57 +15,45 @@ pub(crate) fn test(_attr: &TokenStream, input: TokenStream) -> TokenStream { | |||
| let fn_block = &item_fn.block; | |||
There was a problem hiding this comment.
I left my feedback on motsu::test macro in our group chat, but the gist of it is that with the new context design in this PR, contracts/EOAs can be safely manually instantiated with little effort, and fuzzing crates like proptest work with motsu out-of-the-box (no need for special macro wrappers).
However, there are arguments to be made that we need to keep this attribute:
- Backward compatibility.
- Enables us to easily add additional features in the future.
- A/B testing - we document ways for devs to instantiate contracts/EOAs (injected as is now, and manual), and after some time we investigate how the devs actually prefer to use the crate - if either of the approaches gains drastically more adoption, we can always drop the other.
| /// The key is the test [`Context`], an id of the test thread. | ||
| /// | ||
| /// The value is the [`MockStorage`], a storage of the test case. | ||
| static STORAGE: Lazy<DashMap<Context, MockStorage>> = Lazy::new(DashMap::new); |
There was a problem hiding this comment.
I really like the way parallelism is supported. Did you test the speed execution difference during tests?
| /// The key is the test [`Context`], an id of the test thread. | ||
| /// | ||
| /// The value is the [`MockStorage`], a storage of the test case. | ||
| static STORAGE: Lazy<DashMap<Context, MockStorage>> = Lazy::new(DashMap::new); |
There was a problem hiding this comment.
Just to check if I understood correctly, our context logic will have to be changed after the Stylus team implements their host mocking logic, right?
| pub fn random() -> Self { | ||
| Self::new(Address::random()) | ||
| } |
There was a problem hiding this comment.
Although I understand the reason for having this function, I think the name random is a bit misleading - when I saw it in the context of motsu::test, I thought it generates a contract with its storage fields instantiated with random values.
I think keeping random() is fine, so it can be used in #[motsu::test], but it's also worth having additional functions with clearer names.
I'd suggest any combination of the below, where the first function just calls random() internally:
Contract::new()andContract::new_at(Address)Contract::mock()andContract::mock_at(Address)Contract::deploy()andContract::deploy_at(Address)
P.S. the same argument applies to Account::random.
| let msg_sender = | ||
| Context::current().get_msg_sender().expect("msg_sender should be set"); |
There was a problem hiding this comment.
With the current design, it's no longer necessary for other devs to redefine their own pub unsafe extern "C" fn msg_sender as was the case initially (if I remember correctly)?
They can just use our library by annotating their tests with #[motsu::test]?
Based on this pr (OpenZeppelin/rust-contracts-stylus#423)
Resolves #8 , Resolves #12
PR Checklist