Skip to content
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

chore(test): simulation unit tests #357

Merged
merged 7 commits into from
Sep 15, 2023
Merged

chore(test): simulation unit tests #357

merged 7 commits into from
Sep 15, 2023

Conversation

alex-miao
Copy link
Collaborator

@alex-miao alex-miao commented Sep 12, 2023

[Closes/Fixes] #298

Proposed Changes

  • Refactor parts of simulation.rs, tracer.rs, and provider_like.rs to be mockable
  • Add a few basic unit tests to simulation.rs

@alex-miao alex-miao marked this pull request as draft September 12, 2023 17:31
@alex-miao alex-miao force-pushed the alex/test-simulation branch 2 times, most recently from b4e7f18 to 4285ec4 Compare September 12, 2023 17:55
Copy link
Collaborator

@dphilipson dphilipson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! A few notes inline, but these are really good tests.

Address::from_str("0xb856dbd4fa1a79a46d426f537455e7d3e79ab7c4").unwrap(),
Address::from_str("0x8abb13360b87be5eeb1b98647a016add927a136c").unwrap(),
],
associated_slots_by_address: serde_json::from_str(r#"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might consider writing this using the json! macro, which would let you parse the json at compile time, although it doesn't really matter (docs).

.simulate_validation(op, max_validation_gas)
.await?;

let asdf = self
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Nit) Needs a more descriptive name. trace might be good.

src/common/types/provider_like.rs Outdated Show resolved Hide resolved
src/common/simulation.rs Outdated Show resolved Hide resolved

tracer.expect_trace_simulate_validation().returning(move |_, _, _| Ok(SimulationTracerOutput {
accessed_contract_addresses: vec![
Address::from_str("0x5ff137d4b0fdcd49dca30c7cf57e578a026d2789").unwrap(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that you have the option of writing each of these as

"0x5ff137d4b0fdcd49dca30c7cf57e578a026d2789".parse().unwrap()

although writing it with Address::from_str is fine too if you like the clarity.

You also have the option of declaring the test return type as a result:

async fn test_simulate_validation() -> anyhow::Result<()> {

and then you could use ? instead of .unwrap() throughout, but that's just personal preference.

src/common/simulation.rs Outdated Show resolved Hide resolved

tracer.expect_trace_simulate_validation().returning(move |_, _, _| Ok(SimulationTracerOutput {
accessed_contract_addresses: vec![
Address::from_str("0x5ff137d4b0fdcd49dca30c7cf57e578a026d2789").unwrap(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's already written it might not be worth it, but there's enough of these that if you had to do this again I would consider writing some kind of helper function to write these with less boilerplate. Something like

fn address(s: &str) -> Address {
    Address::from_str("0x5ff137d4b0fdcd49dca30c7cf57e578a026d2789").unwrap()
}

and then you could have

accessed_contract_addresses: vec![
    address("0x5ff137d4b0fdcd49dca30c7cf57e578a026d2789"),
    address("0xb856dbd4fa1a79a46d426f537455e7d3e79ab7c4"),
    address("0x8abb13360b87be5eeb1b98647a016add927a136c"),
],

@alex-miao alex-miao marked this pull request as ready for review September 15, 2023 00:26
@alex-miao alex-miao merged commit 4544f4c into main Sep 15, 2023
5 checks passed
@alex-miao alex-miao deleted the alex/test-simulation branch September 15, 2023 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants