-
Notifications
You must be signed in to change notification settings - Fork 0
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
build: impl mock test utils #314
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## aviv/create_mock_test_leaf #314 +/- ##
===========================================================
Coverage 70.54% 70.54%
===========================================================
Files 38 38
Lines 2091 2091
Branches 2091 2091
===========================================================
Hits 1475 1475
Misses 546 546
Partials 70 70 ☔ View full report in Codecov by Sentry. |
b64dae1
to
2b36ea0
Compare
2b36ea0
to
bd88982
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @AvivYossef-starkware and the rest of your teammates on Graphite |
bd88982
to
d96d5f8
Compare
d96d5f8
to
9de23da
Compare
Benchmark movements: |
64f9a1b
to
9658446
Compare
9de23da
to
1e92f7b
Compare
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.
Reviewed 2 of 2 files at r2, 7 of 7 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @AvivYossef-starkware)
crates/committer/src/patricia_merkle_tree/internal_test_utils.rs
line 26 at r3 (raw file):
use rstest::{fixture, rstest}; #[derive(Debug, PartialEq, Clone, Copy, Default, Eq)] pub(crate) struct MockLeaf(pub(crate) Felt);
newline after use
s
Suggestion:
use rstest::{fixture, rstest};
#[derive(Debug, PartialEq, Clone, Copy, Default, Eq)]
pub(crate) struct MockLeaf(pub(crate) Felt);
crates/committer/src/patricia_merkle_tree/internal_test_utils.rs
line 32 at r3 (raw file):
StorageValue(self.0.to_bytes_be().to_vec()) } fn get_prefix(&self) -> Vec<u8> {
newline between function impls
Suggestion:
}
fn get_prefix(&self) -> Vec<u8> {
crates/committer/src/patricia_merkle_tree/internal_test_utils.rs
line 70 at r3 (raw file):
.0; Ok(Self(leaf_val)) }
how does this differ from the default implementation?
if it doesn't, you should be able to just delete this
Code quote:
fn from_modifications(
index: &NodeIndex,
leaf_modifications: Arc<LeafModifications<Self>>,
) -> LeafResult<Self> {
let leaf_val = leaf_modifications
.get(index)
.expect("Leaf not found in modifications")
.0;
Ok(Self(leaf_val))
}
crates/committer/src/patricia_merkle_tree/internal_test_utils.rs
line 75 at r3 (raw file):
generate_trie_config!(OriginalSkeletonMockTrieConfig, MockLeaf); struct MockHash;
it's not a hash, it's a hash function
Suggestion:
MockHashFunction
9658446
to
0b1ec9d
Compare
build: mock hash
1e92f7b
to
6d9b95d
Compare
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.
Reviewable status: 4 of 9 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware)
crates/committer/src/patricia_merkle_tree/internal_test_utils.rs
line 26 at r3 (raw file):
Previously, dorimedini-starkware wrote…
newline after
use
s
Done.
crates/committer/src/patricia_merkle_tree/internal_test_utils.rs
line 32 at r3 (raw file):
Previously, dorimedini-starkware wrote…
newline between function impls
Done.
crates/committer/src/patricia_merkle_tree/internal_test_utils.rs
line 70 at r3 (raw file):
Previously, dorimedini-starkware wrote…
how does this differ from the default implementation?
if it doesn't, you should be able to just delete this
I haven't noticed that we have one.
crates/committer/src/patricia_merkle_tree/internal_test_utils.rs
line 75 at r3 (raw file):
Previously, dorimedini-starkware wrote…
it's not a hash, it's a hash function
Done.
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.
Reviewed 5 of 5 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware)
This change is