refactor: desrialization structs for regression test output

[aner-starkware]

This change is 

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.65%. Comparing base (ad3112d) to head (8e244a9).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #293   +/-   ##
=======================================
  Coverage   70.65%   70.65%           
=======================================
  Files          38       38           
  Lines        2021     2021           
  Branches     2021     2021           
=======================================
  Hits         1428     1428           
  Misses        524      524           
  Partials       69       69           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.075 ms 34.143 ms 34.231 ms]
change: [-4.1018% -2.6072% -1.3200%] (p = 0.00 < 0.05)
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
3 (3.00%) high mild
2 (2.00%) high severe

[dorimedini-starkware]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @aner-starkware and @TzahiTaub)

---

crates/committer_cli/src/tests/benchmark_tests.rs line 36 at r2 (raw file):

struct StorageObject {
    storage: Value,
}

why do you need a wrapper for a type that's already serde?

Code quote:

#[derive(Deserialize)]
struct StorageObject {
    storage: Value,
}

---

crates/committer_cli/src/tests/benchmark_tests.rs line 42 at r2 (raw file):

    compiled_class_root_hash: Value,
    storage: StorageObject,
}

newlines between multiline structs please

Suggestion:

#[derive(Deserialize)]
struct CommitterRegressionInput {
    committer_input: String,
    contract_states_root: String,
    contract_classes_root: String,
    expected_facts: String,
}

#[derive(Deserialize)]
struct TreeRegressionOutput {
    root_hash: Value,
    storage_changes: Value,
}

#[derive(Deserialize)]
struct StorageObject {
    storage: Value,
}

#[derive(Deserialize)]
struct CommitterRegressionOutput {
    contract_storage_root_hash: Value,
    compiled_class_root_hash: Value,
    storage: StorageObject,
}

---

crates/committer_cli/src/tests/benchmark_tests.rs line 85 at r2 (raw file):

    // Assert correctness of the output of the single tree flow test.
    let regression_output: TreeRegressionOutput = serde_json::from_str(&output).unwrap();

destructure (removes need for regression_output.<field_name>)

Suggestion:

let TreeRegressionOutput { bla } = serde_json::from_str(&output).unwrap();

[aner-starkware]

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @TzahiTaub)

---

crates/committer_cli/src/tests/benchmark_tests.rs line 36 at r2 (raw file):

Previously, dorimedini-starkware wrote…

why do you need a wrapper for a type that's already serde?

That's not the reason - the reason is that the storage is double-nested in the committer output (i.e., {"storage":{"storage":{....}}); I asked about this in the channel, but apparently no one who is available knows.

---

crates/committer_cli/src/tests/benchmark_tests.rs line 42 at r2 (raw file):

Previously, dorimedini-starkware wrote…

newlines between multiline structs please

Done.

---

crates/committer_cli/src/tests/benchmark_tests.rs line 85 at r2 (raw file):

Previously, dorimedini-starkware wrote…

destructure (removes need for regression_output.<field_name>)

Done.

[dorimedini-starkware]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @aner-starkware, @nimrod-starkware, and @TzahiTaub)

---

crates/committer_cli/src/tests/benchmark_tests.rs line 36 at r2 (raw file):

Previously, aner-starkware wrote…

That's not the reason - the reason is that the storage is double-nested in the committer output (i.e., {"storage":{"storage":{....}}); I asked about this in the channel, but apparently no one who is available knows.

@nimrod-starkware ?

---

crates/committer_cli/src/tests/benchmark_tests.rs line 93 at r3 (raw file):

    let TreeRegressionOutput {
        root_hash,
        storage_changes: Value::Object(acutal_storage_changes),

typo

Code quote:

acutal_

---

crates/committer_cli/src/tests/benchmark_tests.rs line 97 at r3 (raw file):

    else {
        panic!("Expected storage changes object to be an object.");
    };

strange that this is needed. if the output of serde_json::from_str(&output) cannot be destructured like you request, won't unwrap() panic now? nono-blocking

Code quote:

    else {
        panic!("Expected storage changes object to be an object.");
    };

[aner-starkware]

crates/committer_cli/src/tests/benchmark_tests.rs line 36 at r2 (raw file):

Previously, dorimedini-starkware wrote…

@nimrod-starkware ?

I think it's probably @yoavGrs

[aner-starkware]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware, @nimrod-starkware, @TzahiTaub, and @yoavGrs)

---

crates/committer_cli/src/tests/benchmark_tests.rs line 97 at r3 (raw file):

Previously, dorimedini-starkware wrote…

strange that this is needed. if the output of serde_json::from_str(&output) cannot be destructured like you request, won't unwrap() panic now? nono-blocking

I didn't put this out of my own free will 😅
p.s. does "nono-blocking" actually mean blocking? 🤔

[dorimedini-starkware]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware, @TzahiTaub, and @yoavGrs)

---

crates/committer_cli/src/tests/benchmark_tests.rs line 36 at r2 (raw file):

Previously, aner-starkware wrote…

I think it's probably @yoavGrs

why yoav? nimrod was in charge of python->rust serde, no?
unblocking anyway, out of scope

---

crates/committer_cli/src/tests/benchmark_tests.rs line 97 at r3 (raw file):

Previously, aner-starkware wrote…

I didn't put this out of my own free will 😅
p.s. does "nono-blocking" actually mean blocking? 🤔

yes, not all conversations are marked with ⛔